Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 331138 - ASTRewrite#replace(..) does not consider the TargetSourceRangeComputer
Summary: ASTRewrite#replace(..) does not consider the TargetSourceRangeComputer
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 3.7 M6   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 331116
  Show dependency tree
 
Reported: 2010-11-25 11:30 EST by Markus Keller CLA
Modified: 2011-03-07 11:37 EST (History)
2 users (show)

See Also:
Olivier_Thomann: review+

Attachments
proposed fix v1.0 + regression tests (4.55 KB, patch)
2011-02-02 09:15 EST, Ayushman Jain CLA 		no flags Details | Diff
proposed fix v1.1 + regression tests (7.23 KB, patch)
2011-02-03 01:11 EST, Ayushman Jain CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.