Community
Participate
Working Groups
After inserting new nodes to the ListRewrite using ListRewrite.insert...(...) methods, newly inserted nodes can not be removed by ListRewrite.remove(...) method. Here is an example from the attached test case: ListRewrite lrw = rewriter.getListRewrite(td, TypeDeclaration.BODY_DECLARATIONS_PROPERTY); assertFalse(lrw.getOriginalList().contains(newField)); assertFalse(lrw.getRewrittenList().contains(newField)); lrw.insertLast(newField, null); assertFalse(lrw.getOriginalList().contains(newField)); assertTrue(lrw.getRewrittenList().contains(newField)); lrw.remove(newField, null); assertFalse(lrw.getOriginalList().contains(newField)); // the next assert will fail - it is impossible to remove newField once added assertFalse(lrw.getRewrittenList().contains(newField)); Last assert fails since the node remains in the list. After a call to ASTRewrite.rewriteAST(...) the node will stay in the rewritten code. I have noticed that internally insert methods insert null as originalValue and new node as newValue to the event store (org.eclipse.jdt.internal.core.dom.rewrite.ListRewriteEvent:173). On the other hand, remove() methods look for matching originalValue, and never find it (ListRewriteEvent:128, 140). Temporary and very undesired workaround that I found is to subclass ASTRewrite and add another remove method that will revert the change in the RewriteEventStore of adding a new node: public void remove(ASTNode parent, ChildListPropertyDescriptor childProperty, ASTNode node) { ListRewrite lrw = getListRewrite(parent, childProperty); if (lrw.getRewrittenList().contains(node) && !lrw.getOriginalList().contains(node)) { ListRewriteEvent listEvent = super.getRewriteEventStore().getListEvent(parent, childProperty, true); int index = listEvent.getIndex(node, ListRewriteEvent.NEW); if (index >= 0) { listEvent.revertChange((NodeRewriteEvent)listEvent.getChildren()[index]); } else { lrw.remove(node, null); } } else { lrw.remove(node, null); } } }
Created attachment 54011 [details] Test case illustrating the problem
Although the workaround has fixed the problem we were having, I would really appreciate if this issue could be addressed as soon as possible. The reason for the rush is that we really don't like to be using a class from an internal package (ListRewriteEvent) in EMF. Probably you guys will come up with a better solution ;-) but I have 2 suggestions for possible fixes: 1. ListRewriteEvent.replaceEntry(ASTNode originalEntry, ASTNode newEntry) could also check if curr.getNewValue() == originalValue 2. ASTRewrite could provide a method similar to what Dmitry wrote. The former changes the behavior slightly and the latter introduces a new API... I can see benefits and disadvantages on both.
The request makes sense, I'll try to add support for this. Whould it be enough to fix ListRewriter.remove(node) to work on new and old nodes? How would you use ListRewriteEvent.replaceEntry as suggested in comment 2? Would a method revert(newNodeOrOldNode) make sense? Or would that be too complicated to use? Or revertAllListChanges() ?
Great news Martin ;-) IMHO, I think one of the important issues you need to consider when choosing one of the solutions is if you can afford the behavior change on ListRewriter.remove(node). If you do, users wouldn't need to change one single line of code to benefit from being able to remove new nodes. If you can't afford the behavior change, then you could provide the ASTRewriter.revert(ASTNode) method you've mentioned which I am assuming will revert all the changes made to the specified node. In this case ASTRewriter.revertAll() is a nice addition but we do need the capability to revert the changes on single nodes thought. If you choose to go for the latter approach, you may have to consider the following points: - Both methods should probably return a boolean indicating whether the rewriter state has changed - Besides these 2 methods, you may have to (somehow) expose the ListRewriteEvent constants and provide 2 extra methods to allow users to do things like rewriter.revert(node, NEW) // removes the events that are adding "node" and rewriter.revertAll(NEW) // removes all the additions
Marcelo, I think when you said ASTRewriter.revert..(..) in comment 4 you meant ListRewrite. ASTRewrite.remove(ASTNode) method does not know which ListRewrite ASTNode is in. It assumes that parent and locationInParent properties are set, and uses them to get ListRewrite and call remove() method on it. Here is a part of ASTRewrite.remove(ASTNode) method: StructuralPropertyDescriptor property= node.getLocationInParent(); if (property.isChildListProperty()) { getListRewrite(node.getParent(), (ChildListPropertyDescriptor) property).remove(node, editGroup); However, nodes added to ListRewrite may be created by ASTRewrite.createStringPlaceholder(..) or created by AST.copySubtree(..). These nodes may not be part of the actual tree, and may not have parent and locationInParent set. Hence, I think it would be a good idea to leave ASTRewrite class and the limited behaviour of ASTRewrite.remove() as it is now. Regarding comment 3, I think the best and most logical approach is to fix the ListRewrite.remove(ASTNode) method, and that would sufficient for our purpose as well. Regarding replaceEntry, I believe Marcelo meant this as a possible fix inside ListRewrite.remove(ASTNode) method. We prefer to not use ListRewriteEvent as it is in an internal package. As for revert(..) methods, I think it is a less obvious API method than remove() because ListRewrite can only add & remove nodes. Therefore, the only event you can revert is adding a node by removing it, or removing a node by adding it. If somebody (including us :) wants to remove the node no matter if it is original or added node, they will need something as follows: if (lrw.getRewrittenList().contains(node) && !lrw.getOriginalList().contains(node)) lrw.revert(node); else lrw.remove(node); This piece of code is not very obvious. It also seems to be an overhead to check the existence of the node in 2 lists (we could also call revert..(), then check if was not successful, if not, call remove(..)). If this approach is taken, there needs to be a javadoc for remove() and revert() methods explaining their usage including the above scenario. Regarding revertAll..(..) method, I do not see an immediate use of this method in our application, and I can't really say if it is a useful addition to the API.
Definitely my bad... I really meant ListRewrite. I should known by now that I can't think before 9am ;-) Regarding the difference between revert and remove... In our particular scenario, all we really want to do is to remove the node so I agree with Dmitry. But for a greater audience, an API to "undo" changes made to an existing tree may make a lot of sense. You can think of it as a way to properly expose the ListRewriteEvent.revertChange that we ended up using in our hac... err... workaround ;-)
sorry guys, I forgot this bug and now we're too close on 3.3. Will fix early 3.4
For the next time, what might we have done differently to ensure it wasn't forgotten for five months?
by me: adding a target milestone. by you: adding a comment that this bug is really important.
Since comment #2 says Although the workaround has fixed the problem we were having, I would really appreciate if this issue could be addressed as soon as possible. SO I think we did add such a comment. Perhaps setting the severity higher would have been appropriate along with more frequent reminders? (Don't get me wrong, I can understand how things slip through the cracks. I'm just wondering what's best from our end to try to avoid that without being annoying.)
I personally don't mind getting reminders. There's just too many bugs in our inboxes to keep a full overview. And a reminder shows us that this is not just a nice-to-have enhancement but rather pain-in-the-neck problem. So, sorry again that this slipped through, I'll fix it early 3.4.
feature added > 20071214 ASTRewrite.remove, ASTRewrite.replace, ListRewrite.remove and ListRewrite.replace now accept as input: Nodes already inserted as new nodes Nodes already used as replacing nodes
Verified for 3.4M5 using I20080204-0010
Marcelo, Should we now have a bugzilla open to remove our workaround?
