Community
Participate
Working Groups
Build ID: I20080330-1350 (3.4M6) Steps To Reproduce: Please organize imports and see that the import statement is wrongly removed, causing a compile error. In the sample below, I put both classes in the same compilation unit. But the problem also occurs if in different units. ------------ bug/Bug.java ------------------ package bug; import bug.Bug.*; class Bug{ interface Proto{}; }class Foo implements Proto{}
I can reproduce it in different compilation units: package bug; import bug.Bug.Proto; class Foo implements Proto{ } package bug; class Bug { interface Proto{}; }
I wanted to say: I can NOT reproduce it in different compilation units.
Problem in org.eclipse.jdt.core.dom.rewrite.ImportRewrite.
Created attachment 151581 [details] Test cases which show the bug is not reproducible.
I'm using build 20090920-1017. Comment 1 isn't reproducible, but the original complaint still is.
(In reply to comment #4) > Created an attachment (id=151581) [details] > Test cases which show the bug is not reproducible. I had a quick look at your tests and saw that they create two different units... Martin already verified that it's not reproducible when two separated units are used (see comment 2), hence I guess explain why your tests pass. As Brian also also reproduce it with a recent, build, it is the confirmation that this bug needs to be fixed first and bug 194358 after...
Closed as fixed as patch for bug 194358 released today (see bug 194358 comment 63) also fixes this bug...
Reopen as the fix for bug 194358 has been reverted.
A better fix would be to fix the org.eclipse.jdt.internal.core.dom.rewrite.ImportRewriteAnalyzer.isImplicitImport(String, ICompilationUnit) The method doesn't take several points into consideration, such as inner types, whether a particular import is being used or not etc. Especially for the latter, I can't think of a way to do this than passing org.eclipse.jdt.internal.compiler.ast.ASTNode.Used somehow to the ImportRewriteAnalyzer. However, this would mean that we need to resolve the compilation unit and the types, which may have an impact on performance as well. Olivier, your thoughts please.
Created attachment 152493 [details] Proposed fix When looking at the test case in comment 0: package bug; import bug.Bug.*; class Bug{ interface Proto{}; } class Foo implements Proto{} The import is needed to be able to resolve the name "Proto" for the class Foo. The problem in this case is that the ImportRewriteAnalyzer.isImplicitImport is way too simple. The import is discarded because it matches the "main" qualified type name of the cu. If you rename your unit "Z.java", you won't see the problem. Also OrganizeImport is always used with the filtering of implicit imports set to true. This is the bottom of the problem. The decision for an import to be implicit or not *cannot* be made based on its name only. At least when the name matches the "main" type name, this is clearly not enough to make a judgment. Replacing the isImplicitImport with: private static boolean isImplicitImport(String qualifier, ICompilationUnit cu) { if (JAVA_LANG.equals(qualifier)) { return true; } String packageName= cu.getParent().getElementName(); return qualifier.equals(packageName); } seems to fix this issue. If I don't have the import, it is properly added. The provided patch goes into this direction. I haven't run the tests with it. So tests need to be run with this patch to see if this would be enough. Jay, please use this patch and see if this would work with your previous fix. Thanks.
In order to fix this, we need more context inside the ImportRewriteAnalyzer. The patch attached doesn't consider this import as implicit anymore. But then it is difficult to say that the import is required or not. Organize import should not propose an import that is not required.
> ... we need more context inside the ImportRewriteAnalyzer. We already have ImportRewriteContext, I think that's good enough. I've fixed ExtractConstantRefactoring which didn't use the context and thus failed with the patch from comment 10. But we have many more cases like this, see bug 295638.
Moving to M5 since it depends on JDT/UI to use the import context.
Moving to M6 as bug 295638 won't be fixed in time for M5.
Created attachment 157791 [details] Proposed fix 2 Although the fix from comment 10 just fixes the implementation to conform to the spec of ImportRewrite#setFilterImplicitImports(boolean), it requires a lot of changes in JDT UI. Any third-party plug-in that uses ImportRewrite will see similar problems if that patch is released. Therefore, I'd categorize this as a breaking change, which we should avoid. The attached patch adds API ImportRewrite#setMainTypeTypesAreImplicitImports(boolean) that allows to fix this bug in a non-breaking way. The patch also adjusts Javadocs of existing APIs. The JDT Core parts can be released at any time -- I'll take care of the UI part once the new API is released. If someone has a better idea for the name of the new property, please speak up.
This will be released as part of the fix for bug 194358.
Created attachment 158758 [details] Proposed fix + regression tests
Created attachment 158804 [details] Proposed fix + regression tests
Released for 3.6M6. A new API is added to enable a mode where using context allows to better control the imports. UI changes are required before the fix is visible in the editor and using organize imports.
I just tried the I-build from 20100223. When I save a file that has references to Entry (java.util.Map.Entry), the import for that class just vanishes. I.e. of these two: import java.util.*; import java.util.Map.Entry; only the former is left.
I think this is a bug that is now visible because java.util.Map.Entry is in the same import group as java.util.*;. In case of reference to member types inside a type in the same package, that entry should not be removed when using import like java.util.*. I opened bug 303776 to take care of this issue. Closing as FIXED.
Verified for 3.6M6 using build I20100305-1011.
