Bug 140565 - [clean up] 'change all acces through instances' + 'remove unused imports' leads to code with fatal errors
Summary: [clean up] 'change all acces through instances' + 'remove unused imports' lea...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows 2000
: P3 normal (vote)
Target Milestone: 3.2.1   Edit
Assignee: Benno Baumgartner CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-05-08 10:12 EDT by Thomas Boeck CLA
Modified: 2006-08-31 08:41 EDT (History)
2 users (show)

See Also:


Attachments
Sources leading to the error (1.67 KB, application/octet-stream)
2006-05-08 10:18 EDT, Thomas Boeck CLA
no flags Details
test case (2.77 KB, patch)
2006-05-10 07:45 EDT, Benno Baumgartner CLA
no flags Details | Diff
fix in clean up refac (811 bytes, patch)
2006-05-10 07:57 EDT, Benno Baumgartner CLA
no flags Details | Diff
fix (3.00 KB, patch)
2006-05-12 06:56 EDT, Benno Baumgartner CLA
no flags Details | Diff
fix and test (8.35 KB, patch)
2006-07-03 06:16 EDT, Benno Baumgartner CLA
no flags Details | Diff
fix and test for 3.2.1 (6.70 KB, patch)
2006-08-14 05:26 EDT, Benno Baumgartner CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Boeck CLA 2006-05-08 10:12:22 EDT
Version: 3.2.0 RC2
Build id: I20060428-1315

Given are the following example classes:

ClassA.java:
public class ClassA { static ClassB instanceOfClassB; }

ClassB.java:
public class ClassB { static ClassC instanceOfClassC; }

ClassC.java:
public class ClassC { static ClassD instanceOfClassD; }

ClassD.java:
public class ClassD { }

Example.java:
import java.io.*;
public class Example {
  private void testFunction() {
    ClassA.instanceOfClassB.instanceOfClassC.instanceOfClassD.toString();
  }
}

A 'clean up' of Example.java with 'change all acces through instances' and 'remove unused imports' checked lead's to
  ClassBClassA.instanceOfClassC.instanceOfClassD.toString();
which renders the code unusable. It should at least lead to
  ClassB.instanceOfClassC.instanceOfClassD.toString();
or to
  ClassD.toString();

Best regards
  Thomas Boeck
Comment 1 Thomas Boeck CLA 2006-05-08 10:18:46 EDT
Created attachment 40585 [details]
Sources leading to the error

Sources leading to the error
Comment 2 Olivier Thomann CLA 2006-05-08 10:24:42 EDT
Move to JDT/UI
Comment 3 Martin Aeschlimann CLA 2006-05-08 12:32:26 EDT
Benno, can you investigate?
Comment 4 Benno Baumgartner CLA 2006-05-10 07:45:22 EDT
Created attachment 40921 [details]
test case

A yet failing test case for CleanUpTest
Comment 5 Benno Baumgartner CLA 2006-05-10 07:56:36 EDT
In short, it looks like that I get a DeleteEdit from the ASTRewrite deleting "ClassA.B" the problem is that this edit has childrens: An insert Edit inserting "ClassA". When mergin two edit trees (as in this case) I'm flattening the trees and the insert edit "ClassA" is moved to the top and has an effect.
Spec:
'A delete edit is equivalent to ReplaceEdit(offset, length, "").'
So, it should IMHO not contain the insert edit it does. The fix on my side is easy and low risk as long as having childrens in DeleteEdits never makes sense, I will attach a patch. But this seams to be an ASTRewrite bug. I can't tell how often this occures (not very often I guess or it was introduced shortly). Martin, can you have a look at it?
Comment 6 Benno Baumgartner CLA 2006-05-10 07:57:21 EDT
Created attachment 40922 [details]
fix in clean up refac
Comment 7 Benno Baumgartner CLA 2006-05-12 06:31:49 EDT
More investigation:
Due to Bug 141504 first 'ClassA' is replaced by 'ClassA' with ASTRewrite#replace(SimpleName, SimpleType, TextEditGroup)
then on the same ASTRewrite 'ClassA.B' is replaced by 'ClassB' with
ASTRewrite#replace(QualifiedName, SimpleType, TextEditGroup)

Resulting in 
replaced: 'ClassA.B' -> 'ClassB' and
replaced: 'ClassA' -> 'ClassA'

The second replace is a child of the generated DeleteEdit for 'ClassA.B'. This delete edit is generated in org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer.doTextRemoveAndVisit(int, int, ASTNode, TextEditGroup)
where also the call to visit the child node is located. I don't know why the child is visited, maybe we have some optimization possibilities here. But I woulden't wanna touch this for 3.2, there is a better fix...
Comment 8 Benno Baumgartner CLA 2006-05-12 06:33:44 EDT
Another bug is described here: The result should be ClassD.D.toString(); after running the clean up and all previous qualifiers should be removed. Fixing this bug also fixes the ClassBClassA problem.
Comment 9 Benno Baumgartner CLA 2006-05-12 06:53:41 EDT
Ok, let me summarize:
Given:
import java.io.*;
public class E1 {
	static class ClassA {static ClassB B;}
	static class ClassB {static ClassC C;}
	static class ClassC {static ClassD D;}
	static class ClassD {}
	
	public void foo() {
		ClassA.B.C.D.toString();
	}
}
1. Source->Clean Up
2. Change all accesses to static through instances
3. Remove unused imports
4. Finish
Is:
  ClassBClassA.C.D.toString();
Should:
  ClassC.D.toString();
Comment 10 Benno Baumgartner CLA 2006-05-12 06:56:13 EDT
Created attachment 41297 [details]
fix

After applying this patch the generated result is:
ClassB.C.D.toString();
this due to bug 141504
Comment 11 Martin Aeschlimann CLA 2006-05-12 09:21:40 EDT
As the scenario is a corner case, I suggest to fix this in 3.2.1.
Comment 12 Benno Baumgartner CLA 2006-07-03 06:16:06 EDT
Created attachment 45660 [details]
fix and test
Comment 13 Benno Baumgartner CLA 2006-07-03 06:19:46 EDT
fixed with patch id=45660 > I20060628-1135 for 3.3M1
Comment 14 Benno Baumgartner CLA 2006-08-09 04:39:59 EDT
fixed > I20060628-1135
Comment 15 Martin Aeschlimann CLA 2006-08-10 09:30:48 EDT
verified in 20060910-0010
Comment 16 Benno Baumgartner CLA 2006-08-14 05:09:18 EDT
Reopen to fix in 3.2.1
Comment 17 Benno Baumgartner CLA 2006-08-14 05:26:42 EDT
Created attachment 47834 [details]
fix and test for 3.2.1
Comment 18 Benno Baumgartner CLA 2006-08-14 05:28:18 EDT
fixed > M20060810-0800 for 3.2.1
Comment 19 Martin Aeschlimann CLA 2006-08-31 08:41:28 EDT
verified in 20060830-0800