Community
Participate
Working Groups
Created attachment 254149 [details] JUnit-Plugin-Test project I believe that org.eclipse.jdt.internal.core.DeltaProcessor.mergeDeltas(Collection) improperly discards CHANGE deltas. Consider a simple refactoring where a compilation unit is renamed from A to B and rename participant additional changes the same compilation unit in some way (e.g. adds a field, method or annotation). After the refactoring I only get the following delta, which says nothing about the modification of the compilation unit. Java Model[*]: {CHILDREN} DeltaProcessorTest[*]: {CHILDREN} src[*]: {CHILDREN} test.deltaprocessor1[*]: {CHILDREN} ClassBeingMoved1.java[-]: {MOVED_TO(ClassBeingMoved2.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} ClassBeingMoved2.java[+]: {MOVED_FROM(ClassBeingMoved1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} But that information was actually available in mergeDeltas: [DeltaProcessorTest[*]: {CHILDREN} src[*]: {CHILDREN} test.deltaprocessor1[*]: {CHILDREN} ClassBeingMoved1.java[-]: {MOVED_TO(ClassBeingMoved2.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} ClassBeingMoved2.java[+]: {MOVED_FROM(ClassBeingMoved1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])}, Java Model[*]: {CHILDREN} DeltaProcessorTest[*]: {CHILDREN} src[*]: {CHILDREN} test.deltaprocessor1[*]: {CHILDREN} ClassBeingMoved2.java[*]: {CHILDREN | FINE GRAINED} ClassBeingMoved2[*]: {CHILDREN | FINE GRAINED} participantField[+]: {}, Java Model[*]: {CHILDREN} <------- this field was added by the participant DeltaProcessorTest[*]: {CHILDREN} src[*]: {CHILDREN} test.deltaprocessor1[*]: {CHILDREN} ClassBeingMoved1.java[-]: {MOVED_TO(ClassBeingMoved2.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} ClassBeingMoved2.java[+]: {MOVED_FROM(ClassBeingMoved1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])}] So the merging of the deltas discards valuable information which is essential for properly handling the change. A testcase is attached.
FWIW, this bug only occurs when the compilation unit being refactored is *not open* in a java editor. When you open the compilation unit in the testcase like this: @Test public void testDeltaProcessor() throws Exception { Assert.assertNotNull(cu); Assert.assertTrue(cu.exists()); DeltaListener tempListener = registerDeltaListener(); // ---> when open in an editor, the test succeeds IEditorPart tempEditor = JavaUI.openInEditor(cu); Assert.assertNotNull(tempEditor); [...] the testcase succeeeds. Any idea what could cause this?
Hi Carsten, AFAIK, FINE_GRAINED deltas are only available for working copies. So in general you cannot expect them to be reported for compilation units not open in the Java editor. HTH, Vladimir
Hi Vladimir, thanks for your comment. Indeed, when I change the testcase to create a working copy in the beginning like this: cu.becomeWorkingCopy(null); I do get the expected delta. Now I'm not sure how to proceed and integrate this into our tooling... Can anyone confirm that this is intended behavior? FWIW, the code discarding the fine grained delta is at JavaElementDelta.insertDeltaTree() and the line says: if (!equalsAndSameParent(element, getElement())) { // handle case of two jars that can be equals but not in the same project addAffectedChild(childDelta); }
I will take a look.
At some point (In reply to Carsten Pfeiffer from comment #3) > cu.becomeWorkingCopy(null); > > I do get the expected delta. What exactly are you expecting in the delta? I think the testcase passes when the delta has the following: [[Working copy] ClassBeingRenamed1.java[*]: {AST AFFECTED}, Java Model[*]: {CHILDREN} Which I believe is added by reconciler but perhaps not the one you are interested in?
Thanks for taking a look at this, Jay. Indeed, when I have a closer look at what I get with having a working copy at hand, the problem is still there. When invoking becomeWorkingCopy(), I get a CHANGE delta before even performing the refactoring, and the testcase happily accepts that change. So I move the registering of the delta listener after the becomeWorkingCopy() call, like this: cu.becomeWorkingCopy(null); DeltaListener tempListener = registerDeltaListener(); and then the testcase fails again because no CHANGE delta is being reported. I only get this, which says nothing about the field being added. Java Model[*]: {CHILDREN} DeltaProcessorTest[*]: {CHILDREN} src[*]: {CHILDREN} test.deltaprocessor1[*]: {CHILDREN} [Working copy] ClassBeingRenamed1.java[-]: {MOVED_TO(ClassBeingRenamed2.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} ClassBeingRenamed2.java[+]: {MOVED_FROM([Working copy] ClassBeingRenamed1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} What I would additionally expect is this: ClassBeingMoved2.java[*]: {CHILDREN | FINE GRAINED} ClassBeingMoved2[*]: {CHILDREN | FINE GRAINED} participantField[+]: {}, Java Model[*]: {CHILDREN} <------- this field was added by the participant This delta is actually produced by the reconciler, but it is discarded by DeltaProcessor.mergeDeltas(), or more concretely, JavaElementDelta# insertDeltaTree(IJavaElement element, JavaElementDelta delta).
I see the problem. As you said, we take no action when we find out that there is an existing child delta on the same element (in this case, the CU). Perhaps we should check for fine grained changes and add it too. I will take a look.
On closer look, this seems like a well thought out strategy of not reporting finer changes when an element is either added or removed. I wonder if it makes sense in most cases. In this particular case, though, it might, because even though the delta kind is ADDED, it was actually moved from another element. ClassBeingRenamed2.java[+]: {CHILDREN | MOVED_FROM([Working copy] ClassBeingRenamed1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]]) | FINE GRAINED}] But I am still wondering whether we also want the children (participantField) to be part of the delta tree.
The Gerrit patch is here with some test changes. https://git.eclipse.org/r/#/c/51634/ While it works, the following delta makes me bit uncomfortable: test.deltaprocessor1[*]: {CHILDREN} [Working copy] ClassBeingRenamed1.java[-]: {MOVED_TO(ClassBeingRenamed2.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]])} ClassBeingRenamed2.java[+]: {CHILDREN | MOVED_FROM([Working copy] ClassBeingRenamed1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]]) | FINE GRAINED} ClassBeingRenamed2[+]: {CHILDREN | MOVED_FROM(ClassBeingRenamed1 [in [Working copy] ClassBeingRenamed1.java [in test.deltaprocessor1 [in src [in DeltaProcessorTest]]]]) | FINE GRAINED} participantField[+]: {}] Will see how the test goes.
(In reply to Carsten Pfeiffer from comment #0) > Created attachment 254149 [details] > JUnit-Plugin-Test project Hi Carsten, can you please update the patch to match the detla I posted in comment #9? Thanks!
Thanks a lot Jay, I will check it out tomorrow.
(In reply to Jay Arthanareeswaran from comment #9) > The Gerrit patch is here with some test changes. > > https://git.eclipse.org/r/#/c/51634/ All JDT Core and UI tests pass with this patch. But ... this violates the following documentation on IJavaElementDelta#ADDED: /** * Status constant indicating that the element has been added. * Note that an added java element delta has no children, as they are all implicitly added. */ Now I am not entirely sure if we will be breaking any clients. May be we can make exceptions for 'move' deltas? Copying Markus on behalf of JDT UI who will be the foremost client.
Created attachment 255211 [details] JUnit-Plugin-Test project v2 The updated testcase, now looking explicitly for the added field. Since there is no modification delta, I removed the code looking for that. The testcase succeeds with your changes in https://git.eclipse.org/r/#/c/51634/
(In reply to Jay Arthanareeswaran Away till 20 July from comment #12) > Now I am not entirely sure if we will be breaking any clients. May be we can > make exceptions for 'move' deltas? Copying Markus on behalf of JDT UI who > will be the foremost client. Would there be an alternative like adding a MODIFIED delta after the ADDED delta? Thanks again for looking into this!
No progress yet and unlikely to get time during 4.6. Moving out.
Bulk move out of 4.8
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
