Bugzilla – Full Text Bug Listing
| Summary: | [1.7][compiler] NPE while using diamond for inner class allocation | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Ayushman Jain <amj87.iitr> | ||||||||
| Component: | Core | Assignee: | Srikanth Sankaran <srikanth_sankaran> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | Olivier_Thomann, satyam.kandula, srikanth_sankaran | ||||||||
| Version: | 3.7 | Flags: | satyam.kandula:
review+
amj87.iitr: review+ |
||||||||
| Target Milestone: | 3.7.1 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
tests org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest_1_7._test0034() to 36 can be enabled after this is fixed. I will have a look at this. This test gives a compiler error with javac too. As far as I see, type is not being inferred in this case - Inner class constructor uses the outer class's type parameter. I couldn't find any references to inner classes in the spec. Srikanth, do you think this is illegal according to the spec or is it a bug with javac? (In reply to comment #3) > This test gives a compiler error with javac too. As far as I see, type is not > being inferred in this case - Inner class constructor uses the outer class's > type parameter. I couldn't find any references to inner classes in the spec. > Srikanth, do you think this is illegal according to the spec or is it a bug > with javac? I think this code should be rejected and javac is doing the right thing. Basically the outer <>'s type parameter is deduced to be Object and there can be no inference on the subsequent <> that would produce an X2<String> that has an enclosing type X<String>. Obviously, eclipse should not result in an NPE and should produce a clean error message. That said, it does looks suspicious that the following would
compile with javac:
// ------------
public class X {
X() {
}
class Y<T, Z> {
Y (Z a) {
System.out.println("const1");
}
Y(T a, Z b) {
System.out.println("const2");
}
}
public static void main(String[] args) {
X.Y<String, String> x = new X().new Y<>("","");
}
}
WHILE the following would not:
public class X<T> {
X() {
}
class Y<Z> {
Y (Z a) {
System.out.println("const1");
}
Y(T a, Z b) {
System.out.println("const2");
}
}
public static void main(String[] args) {
X<String>.Y<String> x = new X<String>().new Y<>("","");
}
}
So while I believe the program from comment#0 is ill-formed, I doubt
that javac is rejecting it on the right grounds.
Basically we need to also rename the enclosing type's type parameters, making provisions for type parameter's being redefined. (In reply to comment #6) > Basically we need to also rename the enclosing type's type > parameters, making provisions for type parameter's being > redefined. We also don't handle the case of a generic method redefining its enclosing's class's type variables. That case would need to be handled too. Created attachment 196194 [details]
Patch to fix the NPE
This is a partial fix. We still need to (a) understand why the
inference still fails (b) handle the redefinition cases (c) handle
the case where the type variable of outer class is referenced.
(In reply to comment #4) > (In reply to comment #3) > > Srikanth, do you think this is illegal according to the spec or is it a bug > > with javac? After thinking about some more I am not sure: I don't see the spec covering this case at all. (In reply to comment #9) > (In reply to comment #4) > > (In reply to comment #3) > > > Srikanth, do you think this is illegal according to the spec or is it a bug > > > with javac? > > After thinking about some more I am not sure: I don't see the spec covering > this case at all. I'm downloading the new javac7b142. I'll report back with how it behaves vis-a-vis the above cases. We should also ask for a spec clarification. Was the inner class case intentionally left off or was it an oversight? It seems counter-intuitive to not have the diamond case work for an inner class from a user POV. (In reply to comment #10) > Was the inner class case > intentionally left off or was it an oversight? It seems counter-intuitive to > not have the diamond case work for an inner class from a user POV. <> does work with inner classes, as shown by the following example. class X<T> { X() { } class Y<Z> { Y (Z a) { System.out.println("const1"); } Y(String a, Z b) { System.out.println("const2"); } } public static void main(String[] args) { X<String>.Y<String> x = new X<String>().new Y<>("",""); } } When I said I don't see the spec covering this case at all, I was referring to the fact that substitution function being defined speaks only of the type parameters of C the class and c, the possibly generic constructor. We have confirmation from Sun that they have a bug in javac that
results in the following program being rejected:
public class X<T> {
X() {
}
class Y<Z> {
Y (Z a) {
System.out.println("const1");
}
Y(T a, Z b) {
System.out.println("const2");
}
}
public static void main(String[] args) {
X<String>.Y<String> x1 = new X<String>().new Y<String>("","");
X<String>.Y<String> x2 = new X<String>().new Y<>("",""); // javac wrong error
}
}
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7046778 )(not visible
externally as of now) has been raised.
Comment# 0 code should be rejected for reasons outlined in comment# 4.
(In reply to comment #6) > Basically we need to also rename the enclosing type's type > parameters, making provisions for type parameter's being > redefined. [...] (In reply to comment #7) [...] > We also don't handle the case of a generic method redefining > its enclosing's class's type variables. That case would need > to be handled too. (In reply to comment #8) > Created attachment 196194 [details] > Patch to fix the NPE > > This is a partial fix. We still need to (a) understand why the > inference still fails (b) handle the redefinition cases (c) handle > the case where the type variable of outer class is referenced. Upon further analysis, these have all proved to be non-considerations There is no action called for to handle hiding/redefinition of type variables, as shown by the following case: class X<T> { <T> X(T t) { } X<String> x = new X<>(""); } We do synthesize the factory method to be <T', T''> X<T'> <factory>(T'') {} which is correct. No action is needed because the map lookup key is not a string, but is a type variable binding. So when a type variable is being redefined the bindings are different even though the variable name is the same. The one remaining task is to understand the behavior against: public class X<T> { class Y<Z> { Y(T a, Z b) { } } public static void main(String[] args) { X<String>.Y<String> x1 = new X<String>().new Y<String>("",""); // OK X<String>.Y<String> x2 = new X<String>().new Y<>("",""); // ERROR } } See that both javac and eclipse fail to compile this code at this time - perhaps there is something there. Created attachment 196266 [details]
Proposed patch + tests
This patch fixes all the known issues. Under test.
All tests pass, Satyam please review - TIA. Ayush, please verify the behavior on the disabled tests and enable them if alright. Raise a follow up defect as needed - TIA. My apologies for hijacking this defect - Curiosity as what could be going on there got the better of me. I'll sleep better tonight :) +1 for the patch. I understood what mistake I was doing, I wasn't using genericType() and was struggling. Thanks, I understand this better now :). Released in BETA_JAVA7 branch. Ayush, do follow up to enable the tests written for bug 345559 - Thanks. There's a problem with the fix. The loop introduced uses the enclosingType parameter passed to the getStaticFactory(..) method and ends up modifying it. So while this works fine in the first for loop i.e. for the first found constructor, this will break for subsequent constrcutors, since enclosingType will always be null from 2nd iteration of foor loop. (In reply to comment #18) > There's a problem with the fix. The loop introduced uses the enclosingType > parameter passed to the getStaticFactory(..) method and ends up modifying it. > So while this works fine in the first for loop i.e. for the first found > constructor, this will break for subsequent constrcutors, since enclosingType > will always be null from 2nd iteration of foor loop. Excellent catch ! I'll post a patch shortly. Created attachment 196406 [details]
Additional patch + test
Ayush, for further review, testing and verification, include this patch also.
Let me know if you see any other problems - Thanks,
+1 with the new patch. Released additional fix & test into BETA_JAVA7 branch. . (In reply to comment #1) > tests > org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest_1_7._test0034() > to 36 can be enabled after this is fixed. Released. Verified using v_B65. |
BETA_JAVA7 The following code snippet throws an NPE at org.eclipse.jdt.internal.compiler.lookup.ParameterizedMethodBinding.<init>(ParameterizedMethodBinding.java:122) class X<T> { X() { } class Y<Z> { Y (Z a) { System.out.println("const1"); } Y(T a, Z b) { System.out.println("const2"); } } public static void main(String[] args) { X<String>.Y<String> x = new X<>().new Y<>("",""); } } This happens because (1) the inner class uses the type argument of the enclosing class and (2) the static factory method generated corresponding to the const. Y(T a, Z b) has the arguments [null,Z'].