Bug 428913 - AbstractScope#canBeFoundByName uses getSingleLocalElementByName
Summary: AbstractScope#canBeFoundByName uses getSingleLocalElementByName
Status: NEW
Alias: None
Product: TMF
Classification: Modeling
Component: Xtext (show other bugs)
Version: 2.5.1   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-24 09:29 EST by Oliver Libutzki CLA
Modified: 2014-08-20 08:38 EDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Libutzki CLA 2014-02-24 09:29:55 EST
In the global scope are two EObjectDecriptions having the same qualified name "x.y".

One EObject is of type A, the other one is of type B. A and B extend type C.

I have a cross reference to an element of type C. The scope provider delegates to the global scope and filters the parent scope in order to remove EObject of type A.

After applying a quick fix the serializer tries to serialize the cross-reference and fails, because AbstractScope#getLocalElementsByEObject#canBeFoundByName relies on #getSingleLocalElementByName.

#getSingleLocalElementByName just selects the first element with the given name. In my case this is the EObject(-Description) of type A. For that reson #canBeFoundByName returns false and the expected EObjectDescription is "out of scope".

I wonder why #canBeFoundByName doesn't check all local elements. After some "change value" debugger hacking I investigated this to work.
Comment 1 Sven Efftinge CLA 2014-02-24 14:28:16 EST
I think the real issue here is, that the CrossReferenceSerializer is calling  
  scope.getElements(eObj) 
instead of
  scope.getSingleElement(eObj)

which would take care of keeping the symmetry with   
  scope.getSingleElement(QualifiedName) 
which is used during linking.

There is a symmetric contract between 
  scope.getSingleElement(EObject) 
and
  scope.getSingleElement(QualifiedName)

The serializer should produce text, which when parsed creates the same AST (incl. crossrefs) again. AFAIU this contract wouldn't hold in your case?

Also you are right that getElements(eObj) shouldn't use getSingleElement for filtering out elements. as getElements(QualifiedName) doesn't filter as well (that's the other pair of symmetric methods). That said, I'm afraid that we'll break many existing users if we 'fix' this. The bit with the serializer could be fixed, though.

Still, if I understood correctly, your use case is broken, as the serializer would produce text that when parsed again won't result in an equivalent model.
Comment 2 Oliver Libutzki CLA 2014-02-24 15:09:44 EST
(In reply to Sven Efftinge from comment #1)
> Still, if I understood correctly, your use case is broken, as the serializer
> would produce text that when parsed again won't result in an equivalent
> model.

Applying the semantic modification fails completely as I get an exception, because the referenced element is not in scope.
Comment 3 Sven Efftinge CLA 2014-02-24 15:16:46 EST
(In reply to Oliver Libutzki from comment #2)
> (In reply to Sven Efftinge from comment #1)
> > Still, if I understood correctly, your use case is broken, as the serializer
> > would produce text that when parsed again won't result in an equivalent
> > model.
> 
> Applying the semantic modification fails completely as I get an exception,
> because the referenced element is not in scope.

The modification is invalid as there is no way to serialize that state properly. 
You should use the ConcreteSyntaxValidator to check whether your model is serializable.
Comment 4 Oliver Libutzki CLA 2014-02-25 02:26:26 EST
(In reply to Sven Efftinge from comment #1)
> I think the real issue here is, that the CrossReferenceSerializer is calling
> 
>   scope.getElements(eObj) 
> instead of
>   scope.getSingleElement(eObj)
> 
> which would take care of keeping the symmetry with   
>   scope.getSingleElement(QualifiedName) 
> which is used during linking.
> 
Yes, you are right, but that wouldn't help me, too. I have a FilteringScope and its #getSingleElement method delegates to the parent scope's #getElements method. In my case I have duplicate qualified names there which are filtered in the (inner) FilteringScope. Unfortunately the inner scope never gets to know about one of the elements as it's already filtered out by the outer scope.

> There is a symmetric contract between 
>   scope.getSingleElement(EObject) 
> and
>   scope.getSingleElement(QualifiedName)
> 
> The serializer should produce text, which when parsed creates the same AST
> (incl. crossrefs) again. AFAIU this contract wouldn't hold in your case?

Currently the contract is broken, yes. AbstractScope#getLocalElementsByEObject only considers the first IEObjectDescription. Although there is an IEObjectDecrption in scope which represents the given EObject this method might return an empty iterable. AbstractScope#getLocalElementsByName returns every IEObjectDescription with the given qualified name.

> Also you are right that getElements(eObj) shouldn't use getSingleElement for
> filtering out elements. as getElements(QualifiedName) doesn't filter as well
> (that's the other pair of symmetric methods). That said, I'm afraid that
> we'll break many existing users if we 'fix' this. The bit with the
> serializer could be fixed, though.

It only breaks existing users who have duplicate qualified names in their scope which is incorrect anyway. 

(In reply to Sven Efftinge from comment #3)
> The modification is invalid as there is no way to serialize that state
> properly. 
> You should use the ConcreteSyntaxValidator to check whether your model is
> serializable.

For the moment I'm fine with a text-based modification, but I would like to migrate some data from another system by doing a m2m transformation and I worry that I will run into the same problem.
Comment 5 Sebastian Zarnekow CLA 2014-02-25 02:49:46 EST
(In reply to Sven Efftinge from comment #1)
> I think the real issue here is, that the CrossReferenceSerializer is calling
> 
>   scope.getElements(eObj) 
> instead of
>   scope.getSingleElement(eObj)
> 

The serializer has to use getElements(EObject) since an object may be available in a scope with different names. The serializer has to decide which of these are valid (e.g. can be used with the proper IValueConverter) or which one was the name that was used in the original document. This information is strongly related to the node model which the scope provider should never care about.
Comment 6 Sebastian Zarnekow CLA 2014-02-25 04:26:06 EST
Oliver, instead of filtering the global scope, you could try to create it for type B in the first place. See org.eclipse.xtext.scoping.impl.AbstractGlobalScopeProvider.getScope(Resource, EReference, Predicate<IEObjectDescription>)
Comment 7 Oliver Libutzki CLA 2014-02-25 04:57:54 EST
(In reply to Sebastian Zarnekow from comment #6)
> Oliver, instead of filtering the global scope, you could try to create it
> for type B in the first place. See
> org.eclipse.xtext.scoping.impl.AbstractGlobalScopeProvider.getScope(Resource,
> EReference, Predicate<IEObjectDescription>)

Thanks. That might help, yes. Are you going to fix this soon? I prefer not to refactor the code and wait for a fix.
Comment 8 Sebastian Zarnekow CLA 2014-02-25 05:00:19 EST
I'm not sure what you refer to. The situation is unfortunate but works as designed. Also note that filtering scopes is almost always bogus since the scopes (may) filter by name internally and return candidates that do not match your own filter criteria but that caused to remove other candidates that match your filter criteria.
Comment 9 Oliver Libutzki CLA 2014-02-25 05:14:29 EST
(In reply to Sebastian Zarnekow from comment #8)
> I'm not sure what you refer to. The situation is unfortunate but works as
> designed.

Your explanation why the serializer has to use getElements(EObject) is plausible.

I still refer to the headline "AbstractScope#canBeFoundByName uses getSingleLocalElementByName"

This really works as designed? I expect getLocalElementsByName(QualifiedName) instead of getSingleLocalElementByName(QualifiedName) to be called.

Obviously I miss an impact such a change might have...
Comment 10 Sebastian Zarnekow CLA 2014-02-28 05:00:10 EST
Preliminary scheduled for v2.6 to make sure that someone will investigate further