Community
Participate
Working Groups
When you have a template which creates a short text snippet and also imports something at the same time... and you try to use that template in front of a 'fresh' class (i.e. it has no imports yet) then the thing expands into garbage text. I am guessing that the edits to create the imports and the edit to insert the template are overlapping somehow and damage eachother. Here's an example template definition: <template id="org.springframework.ide.eclipse.boot.templates.junit.runwithmockito" name="jurwm" description="@RunWith(MockitoJUnitRunner.class)" context="java"> ${:import(org.junit.runner.RunWith,org.mockito.runners.MockitoJUnitRunner)} @RunWith(MockitoJUnitRunner.class) </template> Now let's say I created a new class 'FooTest' with the following text: --- FooTest.java ---- package com.example; class FooTest { } -------------------- Place cursor on the line before "class FooTest" and invoke the template. The result is this garbage: --- FooTest.java -- package com.example; @RunWith(MockitoJUnitRunner.class) rt org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; class FooTest { } ------------------- Very annoying. I have several templates that are meant to expand into some kind of class-level annotation. All of them are afected by this bug, rendering all these templates as good as useless.
Are you sure you are using 4.4.2? For me it works using <= 4.4.2 but is broken >= 4.5.
ACtually, I'm using 4.5.2. I'll update the ticket.
In org.eclipse.jdt.internal.corext.template.java.JavaContext.rewriteImports(), JavaModelUtil.applyEdit(ICompilationUnit cu, TextEdit edit, boolean save, IProgressMonitor monitor) is invoked with the 'edit' received from org.eclipse.jdt.core.dom.rewrite.ImportRewrite.rewriteImports(IProgressMonitor monitor). For the same given example in comment #0: In 4.4.2, the edit returned by #rewriteImports is: {MultiTextEdit} [22,0] [undefined] {InsertEdit} [22,0] << {InsertEdit} [22,0] <<import org.junit.runner.RunWith; {InsertEdit} [22,0] <<import org.mockito.runners.MockitoJUnitRunner; In 4.6, the returned edit is: {MultiTextEdit} [20,11] [undefined] {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.junit.runner.RunWith; {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.mockito.runners.MockitoJUnitRunner; {InsertEdit} [20,0] << {DeleteEdit} [20,11] This edit is applied in JavaModelUtil#applyEdit via org.eclipse.jdt.core.ICompilationUnit.applyTextEdit(TextEdit edit, IProgressMonitor monitor) and results in the bug in 4.6 by creating incorrect offsets. Moving to JDT/Core to check the difference in result from ImportRewrite#rewriteImports.
*** Bug 472720 has been marked as a duplicate of this bug. ***
re-targeting to 4.7
(In reply to Noopur Gupta from comment #3) > Moving to JDT/Core to check the difference in result from > ImportRewrite#rewriteImports. There was an import rewrite overhaul for 4.5 - a bunch of changes went under the umbrella bug 430303 - initial investigations point to those code modifications
(In reply to Noopur Gupta from comment #3) > In > org.eclipse.jdt.internal.corext.template.java.JavaContext.rewriteImports(), > JavaModelUtil.applyEdit(ICompilationUnit cu, TextEdit edit, boolean save, > IProgressMonitor monitor) is invoked with the 'edit' received from > org.eclipse.jdt.core.dom.rewrite.ImportRewrite. > rewriteImports(IProgressMonitor monitor). > > For the same given example in comment #0: > > In 4.4.2, the edit returned by #rewriteImports is: > {MultiTextEdit} [22,0] [undefined] > {InsertEdit} [22,0] << > > {InsertEdit} [22,0] <<import org.junit.runner.RunWith; > > {InsertEdit} [22,0] <<import org.mockito.runners.MockitoJUnitRunner; > > > In 4.6, the returned edit is: > {MultiTextEdit} [20,11] [undefined] > {InsertEdit} [20,0] << > > > {InsertEdit} [20,0] <<import org.junit.runner.RunWith; > {InsertEdit} [20,0] << > > {InsertEdit} [20,0] <<import org.mockito.runners.MockitoJUnitRunner; > {InsertEdit} [20,0] << > > > {DeleteEdit} [20,11] > > This edit is applied in JavaModelUtil#applyEdit via > org.eclipse.jdt.core.ICompilationUnit.applyTextEdit(TextEdit edit, > IProgressMonitor monitor) and results in the bug in 4.6 by creating > incorrect offsets. > > Moving to JDT/Core to check the difference in result from > ImportRewrite#rewriteImports. From the jdt.ui, it is found that the application of template happens in two stages - first the edit of the imports is applied called at: JavaContext.rewriteImports() line: 650 which in turn calls the JMU#applyEdit and the second one at TemplateProposal.apply(ITextViewer, char, int, int) line: 237 which adds the @RunWith(MockitoJUnitRunner.class). The first set also sets the position of the next insert. One difference from 4.4.2 and current version is the new DeleteEdit which was not there in 4.4.2 - this came as a result of the changes described in comment 6 - I don't see any issue with this new DeleteEdit which is in concurrence with the expected change. However, the removal of this DeleteEdit makes the behavior correct. The reason for that change is that the position for the second insert gets updated correctly without the DeleteEdit. With the DeleteEdit the position remains at the same place and consequently the template is written at the start of the import instead of at a position after the import - the overwrite also happens (to the extent of number of characters in the template proposal prefix). The major part of the stack trace is given at the end of the comment - the stacktrace from the following method MultiTextEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, IDocument) line: 908 is supposed to update the positions. All the line numbers are wrt current master branch tot. Moving to platform.text for further analysis Eclipse.1 [Eclipse Application] org.eclipse.equinox.launcher.Main at localhost:54692 Thread [main] (Suspended) owns: Object (id=479) DefaultPositionUpdater.adaptToInsert() line: 111 DefaultPositionUpdater.adaptToReplace() line: 175 DefaultPositionUpdater.update(DocumentEvent) line: 222 SynchronizableDocument(AbstractDocument).updatePositions(DocumentEvent) line: 1157 SynchronizableDocument(AbstractDocument).updateDocumentStructures(DocumentEvent) line: 682 SynchronizableDocument(AbstractDocument).fireDocumentChanged(DocumentEvent) line: 767 SynchronizableDocument(AbstractDocument).replace(int, int, String, long) line: 1101 SynchronizableDocument.replace(int, int, String, long) line: 173 SynchronizableDocument(AbstractDocument).replace(int, int, String) line: 1119 SynchronizableDocument.replace(int, int, String) line: 161 InsertEdit.performDocumentUpdating(IDocument) line: 84 InsertEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, IDocument) line: 915 MultiTextEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, IDocument) line: 908 RewriteSessionEditProcessor(TextEditProcessor).executeDo() line: 194 MultiTextEdit(TextEdit).dispatchPerformEdits(TextEditProcessor) line: 737 RewriteSessionEditProcessor(TextEditProcessor).performEdits() line: 156 RewriteSessionEditProcessor.performEdits() line: 96 DocumentAdapter$ApplyTextEditCommand.run() line: 199 DocumentAdapter.run(Runnable) line: 132 DocumentAdapter.access$1(Runnable) line: 129 DocumentAdapter$ApplyTextEditCommand.applyTextEdit(TextEdit) line: 209 DocumentAdapter.applyTextEdit(TextEdit, IProgressMonitor) line: 634 CompilationUnit.applyTextEdit(TextEdit, IProgressMonitor) line: 73 JavaModelUtil.applyEdit(ICompilationUnit, TextEdit, boolean, IProgressMonitor) line: 912 JavaContext.rewriteImports() line: 650 JavaContext.evaluate(Template) line: 192 TemplateProposal.apply(ITextViewer, char, int, int) line: 223 CompletionProposalPopup.insertProposal(ICompletionProposal, char, int, int) line: 978 CompletionProposalPopup.insertSelectedProposalWithMask(int) line: 927 CompletionProposalPopup.verifyKey(VerifyEvent) line: 1352 ContentAssistant$InternalListener.verifyKey(VerifyEvent) line: 803
(In reply to Manoj Palat from comment #7) > One difference from 4.4.2 and > current version is the new DeleteEdit which was not there in 4.4.2 - this > came as a result of the changes described in comment 6 - I don't see any > issue with this new DeleteEdit which is in concurrence with the expected > change. However, the removal of this DeleteEdit makes the behavior correct. > The reason for that change is that the position for the second insert gets > updated correctly without the DeleteEdit. With this, I understand that the extra DeleteEdit which is now generated due to changes from bug 430303 causes this issue and its removal fixes the issue. Could you please explain why this DeleteEdit wasn't required earlier and is expected now? > The major part of the stack trace is given at the end of the comment - the > stacktrace from the following method > MultiTextEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, > IDocument) line: 908 is supposed to update the positions. > > Moving to platform.text for further analysis TextEdit#traverseDocumentUpdating just updates the document based on the provided text edits. Also, this code was last modified in 2003 so I doubt that it causes the issue now. Based on my current understanding, the text edits from ImportRewrite#rewriteImports (with/without DeleteEdit) should be generated with correct offsets so that no overwrite happens while applying them. If you feel that TextEdit#traverseDocumentUpdating should be fixed here then please provide more details on what is expected.
(In reply to Noopur Gupta from comment #8) > > With this, I understand that the extra DeleteEdit which is now generated due > to changes from bug 430303 causes this issue and its removal fixes the issue. Not exactly - if you remove DeleteEdit - the position is updated correctly by the MultiTextEdit - However, with the current change from 430303 DeleteEdit is a valid legal edit. > Could you please explain why this DeleteEdit wasn't required earlier and is > expected now? DeleteEdit was introduced to remove the extra whitespace sorrounding the import - This was not part of the policy earlier and hence was not required earlier - this policy got introduced with this umbrella bug. For a live example of how this effects the import, we can comment the deleteRemainingTextEdits at ImportEditor:534 and then run the ImportRewriteTest - without this we can see extra white space(s) at the failing tests. > > The major part of the stack trace is given at the end of the comment - the > > stacktrace from the following method > > MultiTextEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, > > IDocument) line: 908 is supposed to update the positions. > > > > Moving to platform.text for further analysis > > If you feel that TextEdit#traverseDocumentUpdating should be fixed here then > please provide more details on what is expected. So you may want to check whether this DeleteEdit is handled properly with the TextEdit#traDU and the position is updated with the lengths of cumulative insertEdits.
(In reply to Manoj Palat from comment #9) > (In reply to Noopur Gupta from comment #8) > > > > With this, I understand that the extra DeleteEdit which is now generated due > > to changes from bug 430303 causes this issue and its removal fixes the issue. > > Not exactly - if you remove DeleteEdit - the position is updated correctly > by the MultiTextEdit - However, with the current change from 430303 > DeleteEdit is a valid legal edit. > > > Could you please explain why this DeleteEdit wasn't required earlier and is > > expected now? > > DeleteEdit was introduced to remove the extra whitespace sorrounding the > import - This was not part of the policy earlier and hence was not required > earlier - this policy got introduced with this umbrella bug. > For a live example of how this effects the import, we can comment the > deleteRemainingTextEdits at ImportEditor:534 and then run the > ImportRewriteTest - without this we can see extra white space(s) at the > failing tests. So, the extra whitespace was expected earlier but is removed now? This sounds like a change in the expected result from ImportRewrite#rewriteImports which should be handled differently (and I am not sure where and based on what should it be handled). In that case, this bug may not be the only one that is affected by the change. Also, while debugging the simpler scenario from the duplicate bug 472720, I can see that the length in the DeleteEdit is same as the number of characters that I type to insert the proposal. If it is invoked without typing any char (just by selecting it from the proposals list), then the DeleteEdit is missing and the text is inserted without any issue. This indicates that the DeleteEdit here tries to delete the inserted characters and not the whitespace as mentioned above. This again is a change in behavior which should not happen in my view as deleting the invocation chars could already be handled by the clients separately. > > > The major part of the stack trace is given at the end of the comment - the > > > stacktrace from the following method > > > MultiTextEdit(TextEdit).traverseDocumentUpdating(TextEditProcessor, > > > IDocument) line: 908 is supposed to update the positions. > > > > > > Moving to platform.text for further analysis > > > > If you feel that TextEdit#traverseDocumentUpdating should be fixed here then > > please provide more details on what is expected. > > So you may want to check whether this DeleteEdit is handled properly with > the TextEdit#traDU and the position is updated with the lengths of > cumulative insertEdits. I am not sure what should be particularly handled about *this* DeleteEdit in TextEdit#traverseDocumentUpdating as debugging it in 4.4.2 and 4.5 shows the same execution flow based on the children edits.
@Noopur – Maybe the solution lies in TemplateProposals#apply() in jdt.ui – Let us expand comment 7 further Documenting the behavior when <Ctrl>+Space is pressed at position ‘|’: Scenario 1 package com.example; | class FooTest { } gives a list of options – select jurwm (the name for the template) and then the result is: package com.example; @RunWith(MockitoJUnitRunner.class) import org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; class FooTest { } Scenario 2: package com.example; ju| class FooTest { } Result is: package com.example; @RunWith(MockitoJUnitRunner.class) port org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; class FooTest { } Scenario 2: package com.example; jurw| class FooTest { } Result is: package com.example; @RunWith(MockitoJUnitRunner.class) rt org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; class FooTest { } This seems to be the scenario encountered in description. In each of these scenarios, the text edits emitted by JDT Core are given below: Scenario 1: {MultiTextEdit} [20,4] [undefined] {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.junit.runner.RunWith; {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.mockito.runners.MockitoJUnitRunner; {InsertEdit} [20,0] << {DeleteEdit} [20,4] Scenario 2: {MultiTextEdit} [20,6] [undefined] {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.junit.runner.RunWith; {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.mockito.runners.MockitoJUnitRunner; {InsertEdit} [20,0] << {DeleteEdit} [20,6] Scenario 3: {MultiTextEdit} [20,8] [undefined] {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.junit.runner.RunWith; {InsertEdit} [20,0] << {InsertEdit} [20,0] <<import org.mockito.runners.MockitoJUnitRunner; {InsertEdit} [20,0] << {DeleteEdit} [20,8] The common stack trace for these scenarios is: TemplateProposal.apply(ITextViewer, char, int, int) line: 238 CompletionProposalPopup.insertProposal(ICompletionProposal, char, int, int) line: 978 CompletionProposalPopup.access$21(CompletionProposalPopup, ICompletionProposal, char, int, int) line: 940 CompletionProposalPopup$2.run() line: 494 BusyIndicator.showWhile(Display, Runnable) line: 70 CompletionProposalPopup.showProposals(boolean) line: 480 ContentAssistant.showPossibleCompletions() line: 1747 CompilationUnitEditor$AdaptedSourceViewer.doOperation(int) line: 184 ContentAssistAction$1.run() line: 84 BusyIndicator.showWhile(Display, Runnable) line: 70 There are two issues here : One) the erasure of characters and two) the wrong placement of @RunWith(MockitoJUnitRunner.class) With each addition scenarios, the DeleteEdit rightfully adds two additional characters each. Now, moving over to the stack trace in org.eclipse.jdt.internal.ui.text.template.contentassist.TemplateProposal.apply() line 237, as mentioned in comment 7, @RunWith(MockitoJUnitRunner.class) is added – but only after replacing this additional template characters. This is an issue since this delete is already taken care with the DeleteEdit ending up in double deletions which result in the removal of those n characters in the resultant code which is the first issue. The second issue is that the offset to be replaced has to be adjusted for this as well. Solution may be to implement this in org.eclipse.jdt.ui, TemplateProposal.apply [yes, I see that this has not been modified for sometime – but still] and/its callees to make this calculations for the rightful offset and replacement.
As mentioned earlier, there are two problems. Noting down the findings here: 1) Double deletion of characters used to invoke the template. The code in TemplateProposal#apply in jdt.ui is generic which is currently responsible for deleting the invocation characters while applying any template. If the chars deletion is now already done in import rewrite, then we will have to find a way in jdt.ui to detect and ignore it only for the import var case. 2) Incorrect offset to insert @RunWith(MockitoJUnitRunner.class). In TemplateProposal#apply in jdt.ui, the 'start' offset which is received to insert @RunWith... is the same as the original template invocation offset while the expectation is that it should be the offset after the inserted imports. This offset is calculated while applying the text edits received from jdt.core at: JavaElementUtil.applyEdit(ICompilationUnit, TextEdit, boolean, IProgressMonitor) line: 283 JavaContext.rewriteImports() line: 651 ... We think there are two possibilities here: Either the text edits which are generated by the import rewrite are wrong or the application (which happens in the platform.text framework) has some issues.
Thanks Noopur and Manoj for laying out the issues. The bug is that ImportRewrite deletes too much text. ImportRewrite has no business deleting text that is adjacent to the import container, even if that text causes a syntax error. These are the perils of working with a recovering DOM AST that makes it look like everything is nice and shiny in the source document :-) To reproduce without the application of templates, run Organize Imports here: ------------------------------------ package com.example; import java.util.*; jurwm class FooTest { } ------------------------------------ => The text "jurwm" gets deleted. ImportRewriteAnalyzer#determineSurroundingRegion(..) makes a wrong assumption: That no top-level AST node covers the region "jurwm" does not mean there's no text there that's worth preserving. The problem with the rest of the template insertion is a consequence of that bug.
New Gerrit change created: https://git.eclipse.org/r/90767
Gerrit change https://git.eclipse.org/r/90767 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ddc31f9a30a6c50ebba45923165bb0ef193e3f32
Manoj, I think we should backport this fix to 4.6.3. If you agree, please review and cherry-pick the commit for RC2.
+1 to backport.
New Gerrit change created: https://git.eclipse.org/r/90914
+1 for backport
Gerrit change https://git.eclipse.org/r/90914 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=90ccad423c857f1832cbed409cc6c5881df7f284
(In reply to Markus Keller from comment #16) > Manoj, I think we should backport this fix to 4.6.3. If you agree, please > review and cherry-pick the commit for RC2. Thanks Markus. Backport done as mentioned in comment 20
Verified for 4.6.3 RC2 with build M20170215-0400
Setting to VERIFIED as per previous comment.
