Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 331138

Summary: ASTRewrite#replace(..) does not consider the TargetSourceRangeComputer
Product: [Eclipse Project] JDT Reporter: Markus Keller <markus.kell.r>
Component: CoreAssignee: Ayushman Jain <amj87.iitr>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: deepakazad, Olivier_Thomann
Version: 3.7Flags: Olivier_Thomann: review+
Target Milestone: 3.7 M6   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Bug Depends on:    
Bug Blocks: 331116    
Attachments:
Description Flags
proposed fix v1.0 + regression tests
none
proposed fix v1.1 + regression tests none

Description Markus Keller CLA 2010-11-25 11:30:13 EST 
HEAD

ASTRewrite#replace(..) does not consider the TargetSourceRangeComputer. My fix for bug 331116 only resolved that bug partially because of this core bug.

Steps (with QuickAssistProcessor from HEAD):

- have:
    void foo() {
        String message;

        // this comment gets removed

        message = "";        
    }

- set caret into second "message"
- apply the "Join variable declaration" quick assist
=> comment wrongly gets removed

At the end of QuickAssistProcessor#getJoinVariableProposals(..), we have the line:

    rewrite.replace(assignParent, rewrite.createMoveTarget(statement), null);

When I replace that line with the following, (using #remove() instead of #replace()), then it works as expected:

    ListRewrite listRewrite= rewrite.getListRewrite(assignParent.getParent(),
	(ChildListPropertyDescriptor)assignParent.getLocationInParent());
    listRewrite.insertAfter(rewrite.createMoveTarget(statement), assignParent,
	null);
    rewrite.remove(assignParent, null);
Comment 1 Olivier Thomann CLA 2010-11-26 10:55:01 EST 
Ayushman, if you get some time, please take a look.
Comment 2 Ayushman Jain CLA 2011-02-02 09:15:28 EST 
Created attachment 188141 [details]
proposed fix v1.0 + regression tests

This patch makes sure that whenever a replace operation takes place in the ASTRewriteAnalyzer, we check for comments, and place the starting point beyond the end of comments so that they dont get removed.
Comment 3 Ayushman Jain CLA 2011-02-03 01:11:28 EST 
Created attachment 188210 [details]
proposed fix v1.1 + regression tests

Modified the fix a bit to handle multiple comments and to make sure the comments in the extended source range of a node are also replaced with the node.

Unfortunately, since the extended source range is defined to include comments and whitespaces just preceeding or trailing a node, they will get deleted if the node is replaced by another node. ASTRewritingModifyingReplaceTest.test0014b() shows such a case. 

Anyway, the fix takes care of preserving all other comments and will fix several UI bugs where comments are deleted upon refactoring, etc.
Comment 4 Ayushman Jain CLA 2011-02-03 01:12:21 EST 
Olivier, can you please review? Thanks.

I'll also get the JDT/UI tests run with the patch.
Comment 5 Ayushman Jain CLA 2011-02-03 05:33:57 EST 
> I'll also get the JDT/UI tests run with the patch.

All UI tests pass.
Comment 6 Olivier Thomann CLA 2011-02-07 12:36:54 EST 
Looks good to me.
Comment 7 Ayushman Jain CLA 2011-02-09 08:40:04 EST 
Released in HEAD for 3.7M6
Comment 8 Olivier Thomann CLA 2011-03-07 11:37:21 EST 
Verified for 3.7M6.