Community
Participate
Working Groups
(1) From bug 317858 comment 5: ------------------------------ This bug Eclipse also thinks it is importing a class when it shouldn't be (due to obscuring). I have 2 packages with the following classes: ================================ package p1; import static p2.OuterClass.Inner; public class myRunner { public static void main(String [] args) { System.out.println("Value1 = " + Inner().toString()); System.out.println("Value2 = " + new Inner().toString()); } } ------------------------ package p1; public class Inner { public String toString() { return "The Class -- p1.Inner"; } } ------------------------- package p2; public class OuterClass { public class Inner{ @Override public String toString() { return "The Class -- p2.OuterClass.In"; } } public static String Inner() { return "The Method -- p2.OuterClass.In()"; } } ---------------------------------- ==================================== Since my import statement is STATIC, then it shouldn't be importing the non-static class with the same name as the STATIC method. "Value1" should be "The Method -- p2.OuterClass.In()" and "Value2" should be "The Class -- p1.Inner" The problem is that on the line: System.out.println("Value2 = " + new Inner().toString()); It should be invoking the class p1.Inner, but Eclipse thinks it is referring to p1.OuterClass.Inner (which isnt static and shouldn't be imported) And "new Inner()" is not a valid way to call an non-static inner Class; (thus the error) ----------------------------------------- (2) From bug 317858 comment 9: ------------------------------ I got the same bug, abused differently, to actually compile in Eclipse and give a different answer, I thought you might like to see it: ------------------------------------ package p2; import static p1.Bar.B; import p3.Foo.*; public class OuterTest { public static void main(String [] args) { new OuterTest().beginTest(); } public void beginTest(){ System.out.print("1 + 1 = "); if(alwaysTrue()){ System.out.println("2"); } else{ System.out.println("3"); } } public boolean alwaysTrue(){ // Returns FALSE in Eclipse String myB = B.class.getCanonicalName(); String realB = p1.Bar.B.class.getCanonicalName(); return myB.equals(realB); } } ------------------------------------ package p1; public class Bar { public static class B{} final public static String B = new String("random"); } ------------------------------------ package p3; public class Foo { public class B {} } ------------------------------------ Eclipse will print out "1 + 1 = 3" ----------------------------------------------------------- (3) From bug 317858 comment 10: ------------------------------ In the above bug, it appears that in "Bar" the field "B" is obscuring the class with the same name, and then it looks like the import-on-demand from "Foo.*" is shadowing the single-import "Bar.B". Perhaps the JSP is a bit to vague on these corner cases.
(1) I cannot reproduce this case any longer. I get the correct output. Value1 = The Method -- p2.OuterClass.In() Value2 = The Class -- p1.Inner (2) Here also, eclipse behaviour is correct. There are 2 things mentioned in the JLS - shadowing (6.3.1) and obscuring(6.3.2). In this case, because the single static import "import static p1.Bar.B" imports two things with the same name "B", obscuring rules come into play, which say "a variable will be chosen in preference to a type" when a simple name can be interpreted as the name of a variable, type, or a package. So the static import brings in the field Bar#B and not the type Bar.B. Now the on demand import "import p3.Foo.*" brings in the type Foo.B. So in our CU, there are two things named B - the field Bar#B and the type Foo.B. So, in alwaysTrue(), myB refers to Foo.B.class and realB refers to Bar.B.class (This is because .class can only be done on a type name and hence in the above 2 statements, the unqualified B is the type Foo.B, and the qualified B is ofcourse the type Bar.B) Hence, output is correct. In order to prove this hypothesis, consider what would happen if there was no "import p3.Foo.*". As soon as I remove that statement, I get an error on the line "String myB = B.class.getCanonicalName();" because the unqualified B now only stands for the field Bar#B and we can't do a .class on it. Hence proved that obscuring rules come into play to import the field Bar#b and not the type Bar.b. The rest of the analysis follows logically. Closing as INVALID.
(1) agreed, it seems now to work with the latest Eclipse. (2)You said: "In order to prove this hypothesis, consider what would happen if there was no "import p3.Foo.*". As soon as I remove that statement, I get an error on the line "String myB = B.class.getCanonicalName();" " Yes you get an error, but thats the bug. You get an error removing "import p3.Foo.*" in Eclipse, but not any other IDE, and it also compiles fine with javac. You also said: " In this case, because the single static import "import static p1.Bar.B" imports two things with the same name "B", obscuring rules come into play, which say "a variable will be chosen in preference to a type" when a simple name can be interpreted as the name of a variable, type, or a package. So the static import brings in the field Bar#B and not the type Bar.B." This is wrong. The JLS specifically states: "A single-static-import declaration imports all accessible static members with a given simple name from a type." AND it states: "Note that it is permissable for one single-static-import declaration to import several fields or types with the same name, or several methods with the same name and signature." See: http://java.sun.com/docs/books/jls/third_edition/html/packages.html#7.5.3 So the import statement SHOULD be importing BOTH the field and the class.
(In reply to comment #2) > You also said: > " In this case, because the single static import "import static p1.Bar.B" > imports two things with the same name "B", obscuring rules come into play, > which say "a variable will be chosen in preference to a type" when a simple > name can be interpreted as the name of a variable, type, or a package. So the > static import brings in the field Bar#B and not the type Bar.B." .... > So the import statement SHOULD be importing BOTH the field and the class. I think I wrote something other than what I wanted to imply. Yes I agree that the static import does import both the field and the class. But if we just stop here, our analysis won't be complete. We need to look at "obscuring" (JLS 6.3.2) also to determine what will be used when we refer to the name "B", and those rules clearly state that a variable is preferred over a type. So, in the usage "String myB = B.class.getCanonicalName();", B is actually resolved to the field Bar#B. I don't know why javac does not respect these rules here. Anyway, looking at the way org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope.findSingleStaticImport(char[][], int) is written, it seems like we only add either the field, or method, or type from a static import in the cache and not all three as should be expected. So maybe a bug there.
>(In reply to comment #3) > I think I wrote something other than what I wanted to imply. Yes I agree that > the static import does import both the field and the class. But if we just stop > here, our analysis won't be complete. We need to look at "obscuring" (JLS > 6.3.2) also to determine what will be used when we refer to the name "B", and > those rules clearly state that a variable is preferred over a type. So, in the > usage "String myB = B.class.getCanonicalName();", B is actually resolved to > the field Bar#B. I don't know why javac does not respect these rules here. > > Anyway, looking at the way > org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope.findSingleStaticImport(char[][], > int) is written, it seems like we only add either the field, or method, or type > from a static import in the cache and not all three as should be expected. So > maybe a bug there. The key here is that in the expression "B.class", the ".class" implies that "B" is a type, not a field. So, since it cannot be a field, the obscuring rule doesn't come into play. In the same way, I should also be able to say: B myB = null; // B can only be a class here as well, and not obscured by the field. or likewise: class foo extends B{} // again B is not obscured, since it can only a type here. That is how javac appears to be following these rules. If i add a new method to use in "OuterTest" public void test2() { System.out.println(B.toString());// Field obscures class B System.out.println(p1.Bar.B.toString()); // Field obscures the class B System.out.println(B.class.getCanonicalName().toString()); // the class B System.out.println(p1.Bar.B.class.getCanonicalName().toString()); // class B } running this method, Java gives me: random random p1.Bar.B p1.Bar.B And Eclipse gives me: random random p3.Foo.B p1.Bar.B
Created attachment 204564 [details] proposed fix v1.0 + regression tests Ok so there was a problem in type resolution. We were not traversing the static imports to look for a type (we need to do this because the resolved import binding may correspond to a field or a method, if a field or a method with the same name as the type exists in the unit being imported). Added that in Scope#getTypeOrPackage(..). The position of the code is interesting to note. It is added right after the single type imports are traversed, and just before other types with the same name are looked for in the same package. This is done to respect the hiding rules in http://java.sun.com/docs/books/jls/third_edition/html/packages.html#7.5.3 , i.e.: "A single-static-import declaration d in a compilation unit c of package p that imports a type named n shadows the declarations of: -any static type named n imported by a static-import-on-demand declaration in c. -any top level type (§7.6) named n declared in another compilation unit (§7.3) of p. -any type named n imported by a type-import-on-demand declaration (§7.5.2) in c. throughout c." Added tests and updated an old test, which now shows the right behaviour (checked against javac)
Srikanth, I'd prefer a review. Not so much for the implementation details as for the approach. Thanks!
Created attachment 205405 [details] Simpler patch (?) Ayush, is this patch simpler while doing the same thing ? It passes all StaticImportTests (nothing else has been run) - It merges the blocks for method binding and field binding "mis-hits" - Removes what appears to be redundant calls to Charperation.equals - Fixes comments. - Fixes a potential problem with earlier patch regarding membertype lookups. The current patch looks up for member types in the leading import type component, while the earlier patch looks it up in the static method/field declaring class.
(In reply to comment #7) > Created attachment 205405 [details] [diff] > Simpler patch (?) Thanks Stephan. I had an exact same patch on test when you submitted this one here! Srikanth's review had pointed out those problems already. > - Fixes a potential problem with earlier patch regarding membertype > lookups. The current patch looks up for member types in the leading > import type component, while the earlier patch looks it up in the > static method/field declaring class. Also wrote a test for this. So will submit a cumulative patch.
Found another issue - we currently don't report an error in this scenario although 2 imports import a type with same name. This happens if i take the comment 0 example (2), and change Foo.B to be a static class, and in OuterTest, I change "import p3.Foo.*" to "import static p3.Foo.B". Javac complains on this new import line saying that the type it imports has already been imported, but eclipse doesn't.
(In reply to comment #9) > Found another issue ------------------------------------ package p2; import static p1.Bar.B; import static p3.Foo.B; // javac complains, eclipse doesn't public class OuterTest { public static void main(String [] args) { new OuterTest().beginTest(); } public void beginTest(){ System.out.print("1 + 1 = "); if(alwaysTrue()){ System.out.println("2"); } else{ System.out.println("3"); } } public boolean alwaysTrue(){ // Returns FALSE in Eclipse String myB = B.class.getCanonicalName(); String realB = p1.Bar.B.class.getCanonicalName(); return myB.equals(realB); } } ------------------------------------ package p1; public class Bar { public static class B{} final public static String B = new String("random"); } ------------------------------------ package p3; public class Foo { public static class B {} } ------------------------------------
(In reply to comment #9) > Found another issue - we currently don't report an error in this scenario > although 2 imports import a type with same name. While it is tempting to pack related fixes into the same bug, if this new issue isn't introduced by the patch we are considering, but rather predates the candidate fix, I recommend that we maintain the concerns separate and address it via a new bug.
(In reply to comment #11) > I recommend that we maintain the concerns separate > and address it via a new bug. Opened bug 361327
Created attachment 205471 [details] proposed fix v1.1 + regression tests This patch adds a regression test for the inheritance case, and removes some redundant parts of the above patch.
Patch looks good.
Released in HEAD for 3.8M3 via commit 817ed71692e2e331f7e0ac3a90af87bc08553ba4
Verified for 3.8 M3 using build id: N20111022-2000
