osate / osate2

Open Source AADL2 Tool Environment
http://osate.org
Eclipse Public License 2.0
36 stars 8 forks source link

Types that are allows in global index are too restrictive. #256

Closed philip-alldredge closed 11 years ago

philip-alldredge commented 11 years ago

I am working on a plugin that uses the global EMF index to retrieve objects in the EMF index. Unfortunately, the index only contains a limited selection of model elements. It would be useful for me if Features, Connections, Flows, Subcomponents were included in the index. The use case is for plugins to be able to easily access an AADL element using the qualified name.

It appears that the issue is mostly one of design and not difficulty. I noticed that there was a conscious effort to restrict the elements in the index in org.ostate.xtext.aadl2.naming.Aadl2QualifiedNameProvider.getFullyQualifiedName. Is that done purely for performance reasons? Would you consider allowing more model elements into the index?

juli1 commented 11 years ago

I have no idea about restrictions on id. I will discuss that with Peter and make a follow-up. Please comment if we do not come back to you quickly.

reteprelief commented 11 years ago

There two uses of the EMF index: namespace focused, and simply to identify a global path reference to an objec tin the model. I used the EMF index for namespace purposes. Only classifiers in the public section of a package (or definitions in property set) are in the global name space. Other items are resolved relative to the context. The context for a feature can be classifiers or subcomponents, e.g., as endpoints of connections the context is either a subcomponent or the enclosing classifier. To make things more interesting, a feature like a port could be refined in a type extension (do we point to e refined feature in the EMF index?) or a feature could be inherited by a type extension (do we point to the original feature location or with respect to the extended type?). To identify the correct object in the right context, the name resolver uses helper methods, which themselves use the global EMF index for the global part and then deals with the various forms of inheritance in AADL. See Aadl2LinkingService and PropertiesLinkingService.

If we extend the EMF index to deal with all named objects in a model, then we also have modes, mode transitions, all the objects from the various annexes. If to only rely on the EMF index you will only get the locally defined entities in a classifier.

philip-alldredge commented 11 years ago

Peter, Yes, my use case is more for using it to lookup an object via a global path reference(fully qualified name). For use as that purpose it is necessary to objects that are both public and private in the index. Ideally, all named elements would be in the index.

I don't think type extensions and refined features should cause a problem with ambigious naming. Atleast for my purposes. The fully qualified name of the feature should be different from that of the refined feature correct? I think one would have the name of the base type and one would have the name of the extended type.

During my quick tests, the features were added as expected. Unfortunately, there were some xtext validation errors whenever I started using the refined to keyword. Although it wasn't immediately clear what was causing the error because the error occurs whether I refine a valid feature or not. In any case, the errors are triggered by my code change.

If the EMF index is to be used solely for the first use case(namespace focus) then I will look into alternatives for referencing objects in the model. Referencing objects by URI fragment may be a more reliable way of referencing objects.

reteprelief commented 11 years ago

So you really want all the named objects recorded in the index including objects that represent refinements. One reason for the refined validator error is that the feature declaration of the refinement does not have a name attribute, but a reference to the previous feature.

At this time we use it only for the global reference index. However, adding the other items should not affect our name scoping implementation. So you should be able to add them in – except for these extra validator checks that come from Xtext.

Peter

From: philip-alldredge [mailto:notifications@github.com] Sent: Wednesday, July 10, 2013 4:06 PM To: osate/osate2-core Cc: Peter Feiler Subject: Re: [osate2-core] Types that are allows in global index are too restrictive. (#256)

Peter, Yes, my use case is more for using it to lookup an object via a global path reference(fully qualified name). For use as that purpose it is necessary to objects that are both public and private in the index. Ideally, all named elements would be in the index.

I don't think type extensions and refined features should cause a problem with ambigious naming. Atleast for my purposes. The fully qualified name of the feature should be different from that of the refined feature correct? I think one would have the name of the base type and one would have the name of the extended type.

During my quick tests, the features were added as expected. Unfortunately, there were some xtext validation errors whenever I started using the refined to keyword. Although it wasn't immediately clear what was causing the error because the error occurs whether I refine a valid feature or not. In any case, the errors are triggered by my code change.

If the EMF index is to be used solely for the first use case(namespace focus) then I will look into alternatives for referencing objects in the model. Referencing objects by URI fragment may be a more reliable way of referencing objects.

— Reply to this email directly or view it on GitHubhttps://github.com/osate/osate2-core/issues/256#issuecomment-20769229.

philip-alldredge commented 11 years ago

That is correct.

What would you suggest as a path forward as far as adding the other objects in and fixing the validation issues? (What would I need to do if you are open to adding the others in)

reteprelief commented 11 years ago

Why don’t you go ahead and add in what you think you want on a development branch and see how hard the validation issue are to address. (Some probably have to do with assumptions XText makes about the Meta model and about scopes and inheritance). Once we load up the index with this additional data we can see what the performance implications are.

Maybe we can have it parameterized as to whether the index has the global or full set of objects.

The alternative is to maintain a two level index scheme. The global indices via the EMF index, and a second index for the local ones.

If you use it in the context of the graphical editor you work on one package at a time , thus, the index could even be built up as hashtable whenever the editor opens up.

From: philip-alldredge [mailto:notifications@github.com] Sent: Thursday, July 11, 2013 11:00 AM To: osate/osate2-core Cc: Peter Feiler Subject: Re: [osate2-core] Types that are allows in global index are too restrictive. (#256)

That is correct.

What would you suggest as a path forward as far as adding the other objects in and fixing the validation issues? (What would I need to do if you are open to adding the others in)

— Reply to this email directly or view it on GitHubhttps://github.com/osate/osate2-core/issues/256#issuecomment-20817450.

philip-alldredge commented 11 years ago

Below is a link to the patch that adds the items to the index. It also removes the check that prevents elements in the private section from being in the index. I'm unsure of the implications of the latter. I am also unsure of how to resolve the validation issues. I am not familiar enough with xtext or the the validation process at this point to know how to resolve the issue. You are right in that it is related to refined features not having a name attribute. The error occurs when it tries to resolve the refined element.

https://gist.github.com/philip-alldredge/d632980cef22a80c1ab7

In the context to the graphical editor, there are scenarios in the graphical editor where multiple packages appear in a diagram. For example in a package view of a model if a type extends a type in another package it may be advantageous to show the base type to show the extends relationship and to allow for easy navigation between them. If you are talking about building a map manually, it could still be built up when the editor is opened and rebuilt whenever changes are made. I'm just trying to avoid duplicating efforts, especially if this is something that could be useful to other plugins.

Unfortunately I'll be out of the office and email contain for a couple weeks starting tomorrow. Any further discussions after today will have to wait until I return.

juli1 commented 11 years ago

Hi Phil,

Let's discuss that when you are back. Then, we can integrate your patch and check if it introduces any regression. Please ping us when you are back and ready to work on that.

philip-alldredge commented 11 years ago

Hi Julien, I am back in the office. How would you like to proceed?

philip-alldredge commented 11 years ago

Julien, I have developed a work around for myself that involves using the qualified name to look up the object manually. For my use case, it fulfills my need. If this capability is something that more plugin developers need, then I still believe the EMF index would be a better approach.

My approach hasn't been optimized, etc but it should work in my case if there are reasons for not expanding the EMF index. It will also allow me to develop my plugin while discussion of the EMF index issue takes place.

juli1 commented 11 years ago

Hi Philip,

I will commit your patch. This is the only thing we need to integrate ?

philip-alldredge commented 11 years ago

Well, the problem mentioned in my post above is that although the patch adds things I want to the index, it caused xtext validation errors when a feature is refined. I have no idea how to solve the issues involved. Like Peter mentioned, it has something to do with how names work for refined features.

juli1 commented 11 years ago

I integrated your changes, I will discuss with Peter about the refinement issue and other potential impacts. In case, I comment out the code related to your changes.

Thanks for your contribution.

juli1 commented 11 years ago

Hi Philip,

The current patch breaks OSATE validation. I see two solutions :

I will disable the patch temporary because we cannot break the testing release for now.

philip-alldredge commented 11 years ago

Okay. Like I said, I have a work around so if it looks like it's a very difficult problem to solve or is a niche use then I can still implement my plugin.

juli1 commented 11 years ago

Ok, so, as you seem to solve the issue on your side, I assume we can close this ticket now, right ?

philip-alldredge commented 11 years ago

Sure.