Community
Participate
Working Groups
Build Identifier: I20100129-1300 If you try to "import" a class and "import static" a method with the same name, the import static statement is "masked". This works in sun javac 1.5.0_22 Reproducible: Always Steps to Reproduce: ---- A/A.java package A; import B.B.C1; import static B.B.C1; public abstract class A { protected void A1(Object task) { C1 c = C1(task); } } ------ B/B.java package B; final public class B { private B() {} public static class C1 { } public static C1 C1(Object v0) { return new C1(); } }
(In reply to comment #0) > Build Identifier: I20100129-1300 > > If you try to "import" a class and "import static" a method with the same name, > the import static statement is "masked". > > This works in sun javac 1.5.0_22 > > Reproducible: Always > > Steps to Reproduce: > ---- A/A.java > package A; > > import B.B.C1; > import static B.B.C1; > > public abstract class A { > protected void A1(Object task) { > C1 c = C1(task); > } > } > > ------ B/B.java > package B; > > final public class B { > > private B() {} > > public static class C1 { > } > > public static C1 C1(Object v0) { > return new C1(); > } > }
Created attachment 159212 [details] Add regression test
javac 1.5, 1.6 and 1.7 are all compiling the tests case without error.
The pruning is done in: org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope.faultInImports() and there is a comment that says: // collisions between an imported static field & a type should be checked according to spec... but currently not by javac The corresponding spec is: http://java.sun.com/docs/books/jls/third_edition/html/packages.html#7.5 I am not convinced we are properly doing the shadowing check. Looking at the spec, I don't see why field or method static imports would shadow a single type import.
Moving to 3.6M7 to give more time to focus on API changes before API freeze.
Created attachment 162986 [details] rough patch > I am not convinced we are properly doing the shadowing check. Looking at the > spec, I don't see why field or method static imports would shadow a single type > import. Actually, in this code, we check whether along with the field/method static import, the client has also imported a static type or not (ie. a static type exists with the same name as the field or method). In case the client has done so, we check whether that type is already known in the current compilation unit or has been already imported, and if it is , we shadow the static import itself because the following rule comes into play: JLS 7.5.3: "If a compilation unit contains both a single-static-import (§7.5.3) declaration that imports a type whose simple name is n, and a single-type-import declaration (§7.5.1) that imports a type whose simple name is n, a compile-time error occurs. " We should ideally report an error here(perhaps a new error msg?) apart from shadowing the import. But yes the part of code that Olivier has mentioned has one problem - it allows single type imports to shadow static imports if the imported field/method has the same simple name as the imported type, ie. if in the case reported in this bug, we change C1 from being static to non-static, the single type import for C1 still hides the static import for the static method C1, even though it shouldn't Note that javac itself has a bug in implementing the above spec. (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6862569). That explains why the reported case is compiled without errors with javac as reported in comment#3. Attached patch here shows a possible way of fixing the problem. But the fact that javac itself is buggy here will need us to take a decision on how to go about this. Olivier, what do you think?
Ayush, Please work with Olivier in my absence to wrap this up -- Thanks.
Ayushman, I think there is a misunderstanding. In this case the two imports (static and non-static) don't refer to a type. In this case I think the hiding rule from the JLS is not fully supported by us.
(In reply to comment #8) > Ayushman, I think there is a misunderstanding. In this case the two imports > (static and non-static) don't refer to a type. The way I see it, import B.B.C1 -> imports static type C1 declared in B.B static import B.B.C1 -> imports both static type C1 declared in B.B and the static method C1 declared in B.B. (according to JLS 7.5, a static import imports ALL static members and not just a static method) So both of them import a common static type - C1. Isn't this correct?
I would say yes, but the method C1 should also be imported. Right now due to the hiding the method is not seen. Only the type is imported.
If the first import is removed (import B.B.C1;), then we compile the code fine. javac does the same. So I believe the bug in this case is really about why import B.B.C1; hides the static import. Looking again at the spec it is clear that importing the same type should be hidden, but when the static import also imports a field or a method it should not be shadowed. I think the actual handling of method binding imported by a static import is inconsistent with the handling of field binding imported by a static import. My understanding of the spec makes me believe that both should be consistent. Without any change in jdt.core code, the same test case compiles fine if the method reference (in static import) is replaced with a field reference.
(In reply to comment #11) > So I believe the bug in this case is really about why import B.B.C1; hides the > static import. Ok so in this case we should make sure that the static method is not hidden. Will start working on it as soon as I can. Also, do we plan to introduce some error message for this import as per the spec? Or should we align with javac's buggy behaviour and let the import go without an error?
(In reply to comment #12) > Ok so in this case we should make sure that the static method is not hidden. > Will start working on it as soon as I can. Also, do we plan to introduce some > error message for this import as per the spec? Or should we align with javac's > buggy behaviour and let the import go without an error? It looks like the compiler should not directly complain when two methods with the same name are imported using static imports (check test024 of the StaticImportTest). Also existing behavior with ImportConflictBinding has been added to handle test029 of the same test suite. Looking again at the test case, it looks like this is actually a javac bug. This code should not compile as the static import does import the type and the method. Therefore as the type is already imported with the first import, this is a compile error. You can get the right behavior with javac by adding another type B1. package p2; final public class B1 { private B1() {} public static class C1 { } public static C1 C1(Object v0) { return new C1(); } } and change A for: package p1; import p2.B.C1; import static p2.B.C1; import p2.B1.C1; public abstract class A { protected void A1(Object task) { C1 c = C1(task); } } In this case you get: p1\A.java:5: p2.B.C1 is already defined in a single-type import import p2.B1.C1; ^ 1 error So even if this code seems bogus, I prefer the error that reports a duplicate import than our current error. We should try to tune our import resolution so that we can realize that the static import collides with the first import for the type C1. In fact if you revert the order of the type and the method, you do get: 1. ERROR in D:\tests_sources\case1\p1\A.java (at line 5) import p2.B1.C1; ^^^^^^^^ The import p2.B1.C1 collides with another import statement plus other errors related to the method invocation resolution. The order of the members should not matter in this case. Let's discuss this on the phone on Monday.
Created attachment 164727 [details] proposed fix v1.0 + regression tests Here's a fix that does the following: 1) Prevents the static method to be shadowed when imported by a static import which conflicts with a single type import (in the sense that both import a static type of the same name as that of the method) 2) Complains about the conflicting static import in the above scenario, as specified in JLS 7.5.3 cited in comment 6. 3) Makes sure the compiler doesnt complain of a duplicate import on a static import when it is importing a static method of the same name as the type (non static) already imported by a single type import. This is the case mentioned in comment 6 - "But yes the part of code that Olivier has mentioned has one problem - it allows single type imports to shadow static imports if the imported field/method has the same simple name as the imported type, ie. if in the case reported in this bug, we change C1 from being static to non-static, the single type import for C1 still hides the static import for the static method C1, even though it shouldn't". This is done via the change on CompilationUnitScope.java:381. Note that this fix makes us compliant with JLS but non-compliant with javac(buggy). Also note that this fix doesnt touch the case with static fields. So in the reported case above, if the static method C1 were changed to a static field instead, eclipse compiler will not complain about the duplicate import. In this, it aligns with javac, but goes against JLS. Since the comment on line 387 already acknowledges this, I think it was a conscious decision to keep the behaviour this way, and hence I dont want to touch it.
This patch rejects two static imports importing the same type. The case that should be rejected is when a static import and a non-static import refer to the same type.
Created attachment 164880 [details] Proposed fix + regression tests Almost same patch but I iterate through the resolved import to check the static flag. Ayushman, let me know what you think.
Created attachment 164916 [details] Proposed fix + regression tests Missed a case when the static import doesn't also import a method.
Created attachment 164951 [details] proposed fix updated + regression tests Patch looks good. here's an updated patch with the same fix , but slight formatting change, adjusting the comments and changing if ((importReference.isStatic() && !resolved.isStatic()) || (!importReference.isStatic() && resolved.isStatic())) to if(importReference.isStatic() != resolved.isStatic()
Good catch. I'll release it today. Thanks.
Released for 3.6M7. Added regression tests: org.eclipse.jdt.core.tests.compiler.regression.StaticImportTest#test075 org.eclipse.jdt.core.tests.compiler.regression.StaticImportTest#test076 org.eclipse.jdt.core.tests.compiler.regression.StaticImportTest#test077 org.eclipse.jdt.core.tests.compiler.regression.StaticImportTest#test078 org.eclipse.jdt.core.tests.compiler.regression.StaticImportTest#test079 Yuk Pong, Your test case should not compile without changes. This is a bug in javac. If you want your test case to compile, you should remove the first non-static import.
Verified for 3.6M7 using build I20100424-2000
