Community
Participate
Working Groups
Given the following Xtend2 snippet (where '|' marks the cursor position): def foo() { new Object().toString|; } If I now invoke the rename refactoring I get an error dialog (see bug 347273). I would have expected to start renaming the Object#toString() method, as in the JDT when the cursor is placed at the end of an identifier. The error log shows a different error to that of bug 347273. The root cause is: Caused by: java.lang.NullPointerException at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eDerivedStructuralFeatureID(BasicEObjectImpl.java:1495) at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eGet(BasicEObjectImpl.java:1010) at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eGet(BasicEObjectImpl.java:1005) at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eGet(BasicEObjectImpl.java:1000) at org.eclipse.xtext.ui.refactoring.impl.AbstractRenameStrategy.<init>(AbstractRenameStrategy.java:32) at org.eclipse.xtext.ui.refactoring.impl.DefaultRenameStrategy.<init>(DefaultRenameStrategy.java:45) at org.eclipse.xtext.xbase.ui.jvmmodel.refactoring.AbstractJvmModelRenameStrategy.<init>(AbstractJvmModelRenameStrategy.java:25) at org.eclipse.xtext.xtend2.ui.refactoring.Xtend2RenameStrategy.<init>(Xtend2RenameStrategy.java:54) at org.eclipse.xtext.xtend2.ui.refactoring.Xtend2RenameStrategy$Provider.get(Xtend2RenameStrategy.java:48) at org.eclipse.xtext.ui.refactoring.ui.SimpleLinkedPositionGroupCalculator.getLinkedPositionGroup(SimpleLinkedPositionGroupCalculator.java:86) at org.eclipse.xtext.ui.refactoring.ui.RenameLinkedMode.start(RenameLinkedMode.java:67) at org.eclipse.xtext.ui.refactoring.ui.RenameRefactoringController$1.run(RenameRefactoringController.java:111) at org.eclipse.jface.operation.ModalContext.runInCurrentThread(ModalContext.java:464) ... 63 more The problem is how the target element is determined in RenameElementHandler#execute() using EObjectAtOffsetHelper. The EObjectAtOffsetHelper will return the parent object as the ';' (from Xbase::XBlockExpression) doesn't have a direct semantic element. I think the RenameElementHandler should be stricter than the EObjectAtOffsetHelper and not allow this kind of recursion up the containment hierarchy. In fact I think it would make sense to adjust the EObjectAtOffsetHelper to deal with this specific case: If the offset is right behind the end of a non-hidden token, it should attempt to resolve against that token before working up the node containment hierarchy. It seems like HyperlinkHelper#createHyperlinksByOffset() already implements a corresponding workaround: public void createHyperlinksByOffset(XtextResource resource, int offset, IHyperlinkAcceptor acceptor) { EObject crossLinkedEObject = eObjectAtOffsetHelper.resolveCrossReferencedElementAt(resource, offset); if (crossLinkedEObject != null && !crossLinkedEObject.eIsProxy()) { INode leafNode = NodeModelUtils.findLeafNodeAtOffset(resource.getParseResult().getRootNode(), offset); if (leafNode instanceof ILeafNode && ((ILeafNode) leafNode).isHidden() && leafNode.getOffset() == offset) { leafNode = NodeModelUtils.findLeafNodeAtOffset(resource.getParseResult().getRootNode(), offset-1); } INode crossRefNode = getParentNodeWithCrossReference(leafNode); if(crossRefNode!=null) { Region region = new Region(crossRefNode.getOffset(), crossRefNode.getLength()); createHyperlinksTo(resource, region, crossLinkedEObject, acceptor); } } } Also I think the occurrence markers would be more correct if we adjusted the EObjectAtOffsetHelper as outlined.
The EObjectAtOffsetHelper already contains the offset correction code for the case that the cursor is in front of a hidden node. Unfortunately, this does not solve the resolved problem, as the ';' is not hidden. BTW, the same effect can be observed if the cursor is placed between the functions name and the () in a member feature call, e.g. ''.toString|() The ';' as well as the '(' are non-hidden keywords. But you cannot in general ignore these nodes in the EOAOH, as the user would expect the element to be selected if the keyword is an acutal word, e.g. |entity Person {... in the domainmodel. Apparently, we want some 'symbols' to be treated differently from real 'keywords'. It appears to me we should rather implement an EObjectAtOffsetHelper specific to Xtend. Alternatively, we could implement a generic strategy that looks for something like "the nearest object with a name". The latter will change semantics significantly and should not be done in an RC phase. I'm inclined to postpone this one to SR1. Any objections?
+1 for postponing.
I agree with postponing this one. I would however like to propose the strategy of EOAOH to be changed when dealing with offsets *in between* two tokens. In addition to using the previous token (token ending at offset) if the following token (token beginning at offset) is a hidden token I think the strategy should *also* try to use the previous token (unless that is a hidden token) if the following token did not resolve to any object (i.e. was not a cross reference and had no direct semantic element). If that also fails, then we could start walking up the containment hierarchy. But I think this last step should be made optional as I don't think it makes sense in the case of rename refactoring (again based on comparing with JDT). I think this should solve the problem in Xtend2 and I think it would also make sense for other languages (I first encountered this in one of our own DSLs). Further I think this would better match the behavior of the JDT in the areas of hyperlinking and occurrence marking. Other thoughts?
Sounds good to me. Other use cases like the bi-directional synchronization of the outline still benefit from walking up the containers.
pushed to master
Closing all bugs that were set to RESOLVED before Neon.0
