Bugzilla – Full Text Bug Listing
| Summary: | [1.7][compiler] support for diamond case | ||
|---|---|---|---|
| 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: | daniel_megert, deepakazad, markus.kell.r, Olivier_Thomann, satyam.kandula, srikanth_sankaran |
| Version: | 3.7 | Flags: | amj87.iitr:
review+
|
| Target Milestone: | 3.7.1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 190845 [details]
fix v1.0
These are the changes required to make the most basic case work, i.e. class instance creation where the class has only the default constructor.
Other complex cases still don't work. The updated substitution algorithm will have to be implemented to find out the correct constructor.
Created attachment 191332 [details]
proposed fix v1.1
This patch goes a little further by making sure that all cases where <> is illegal give an error.
+ Added tests pertaining to the usage of <>, and also to verify the difference in using <> vs. using just the raw type. Some tests are currently disabled because this patch does not implement the substitution mechanism to find the correct constructor, and hence leads to some exceptions.
+ Introduced a new error message for using <> in anonymous type declaration.
Also verified that we get an error on using <> in compliance 1.6 or below
Created attachment 191458 [details]
proposed fix v2.0 + tests
This patch
+ Implemens, in part the strategy to find out the correct constructor. So, it successfully transfers the type argument to the class instance creation expression when used for the default constructor.
So , cases such as
ArrayList<String> a = new ArrayList<>();
a.add(""); // OK
a.add(1); // ERROR
work as expected. Also, these work if its a qualified reference (java.util.ArrayList).
Main changes for these are in ParameterizedSingleTypeRef, ParameterizedQualifiedTypeRef and AllocationExpression.
+ Fixes stray IAE's and NPE's arising due to calls to TypeRef#getParameterizedTypeName() method by jdt.ui
+ Introduces a configurable option to warn the user when the diamond construct can be used but hasnt been used.
eg. ArrayList<String> a = new ArrayList<String>();
^^^^^^^^^^^^^^^^^^^
will warn "Redundant declaration of type arguments <String>. See GenericsRegressionTest_1_7#test013().
Created attachment 191707 [details]
proposed fix v2.1 + tests
Improved patch. Fixed some bugs with the previous patch. Added more tests
I have started looking into this. Ayush, to simplify matters and help retain focus, I would like to see two changes: (1) Move all changes relating to OPTION_ReportRedundantDeclarationOfTypeArguments to a separate bug. I am not even sure at this point we want to support such an option, but that discussion can come later. Let us retain the current bug for the core of the <> implementation and relegate various and sundry items to their own bug. (2) Similarly I would like to see the changes in CompilerInvocationTests.java that are not pertinent to <> to be separated out. In this case, if the other changes are only in the nature of cleanup (i.e, change based on sort order), you can release them already assuming they pass the tests and post a new patch on top of it. (3) Anything else that is not germane to the core of <>, similarly can be moved out. Please post an updated patch, so I can start the review. (In reply to comment #6) > I have started looking into this. Ayush, to simplify matters and > help retain focus, I would like to see two changes: I would also consider folding org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest_1_7 into org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest. If necessary we can always skip some tests in 1.6- modes by if (this.complianceLevel < ClassFileConstants.JDK1_7) return; Created attachment 191785 [details]
proposed fix v2.2 + tests
Apart from the changes mentioned in comments 6 and 7 above, this patch:
+ Adds support for the following case:
class X<T> {
X(T t) {}
X(T t, Integer i){}
public static void main(String[] args) {
X x1 = new X<>("");
X x2 = new X<>("",1);
}
}
(The earlier patched threw an NPE).
Note that the type for x1 and x2 remains as X, and not X<String>. This is consistent with the current javac ( version b134) behaviour, although I think according to spec it should be inferred as X<String>
+ Adds more tests, especially for scenarios in which diamond is used for field initializations.
(In reply to comment #8) > Created attachment 191785 [details] > proposed fix v2.2 + tests > > Apart from the changes mentioned in comments 6 and 7 above, this patch: Ayush, are all the changes in CompilerInvocationTests.java relevant for <> implementation ? When reviewing a large patch, including changes that are not relevant only serves to distract. Any changes that are clean up in nature can be handled separately ? Let me know if I have misunderstood something here. > Any changes that are clean up in nature can be handled separately ?
> Let me know if I have misunderstood something here.
[Ayush explained that while these changes are not relevant, they come
about because of the way test suite emits expected output, so we will
live with it.]
As a first order business, we should focus on correctly and
gracefully rejecting all ill-formed programs.
This requires us to
(a) Report errors properly in a 1.6- program if the use of <> is encountered.
(b) Report errors properly in a 1.7+ program if the use of <> is encountered
in non-ClassInstanceCreationExpression's.
The patch as it stands now has several problems in this area. I'll try and
propose a patch that tweaks the grammar in such a way that we can cleanly
reject all ill formed programs in a central place rather than checking all
over.
Here are some problems I encountered while working with the current patch:
(1) The following program fails with an internal compiler error
due to AIOOB.
import java.util.Map;
public class X implements Map<> {
static Map<> foo (Map<> x) {
return null;
}
}
(2) The following program compiles successfully, while
it should not.
import java.util.Map;
public class X {
static Map<> foo () {
return null;
}
}
(3) The following program compiles successfully while
it should not.
public class X<T> {
class Y<K> {
}
public static void main(String [] args) {
X<String>.Y<> [] y = null;
}
}
(4) The following illegal program results in an NPE in the
compiler.
public class X<T> {
class Y<K> {
}
public static void main(String [] args) {
X<String>.Y<> y = null;
}
}
(5) The following program compiles without an error while it
should not compile.
public class X<T> {
public void foo(Object x) {
if (x instanceof X<>) {
}
}
}
(6) The following program compiles without an error while it
should not compile.
public class X<T> {
public void foo(Object x) throws X.Y<>.LException {
}
static class Y<T> {
static class LException extends Throwable {}
}
}
(7) I am not sure if this is supposed to be compiled (javac also does)
public class X<T> {
public void foo () {
Object o = new X<> [10];
}
}
I am sure there are many more cases like this. If you are curious,
I started with the grammar and derived some strings which will
contain <> in productions having nothing to do with class instance
creation.
(In reply to comment #11) [...] > The patch as it stands now has several problems in this area. I'll try and > propose a patch that tweaks the grammar in such a way that we can cleanly > reject all ill formed programs in a central place rather than checking all > over. This shouldn't really require a grammar change. Our job is not really to reject <> in all places where it shouldn't feature (which 1.6 mode *already* does anyway) but rather to selectively allow <> in the class instance creation expressions only. Patch under test. (In reply to comment #11) > (7) I am not sure if this is supposed to be compiled (javac also does) > > public class X<T> { > public void foo () { > Object o = new X<> [10]; > } > } I think this is a bug in both eclipse & javac (7b131 - I didn't test with a later version). Since Object o = new X<String> [10]; (for example) is anyway wrong in attempting to create a generic array. Simply having the compiler infer the generic argument does not make it acceptable. Created attachment 191891 [details] Patch under consideration This patches fixes all the issues mentioned in https://bugs.eclipse.org/bugs/show_bug.cgi?id=339478#c11 and includes regression tests for them. This patch is under test. With this code change we should correctly reject all illegal uses of <> since we really rely on the 1.6- error reporting behavior and short circuit that ONLY when we are absolutely sure <> constitutes diamond and not merely missing type parameters. This is achieved by marking a type as diamond IFF: - it is a 1.7+ program - <> occurs in the class instance creation expression not involving anonymous types (where the use of <> is precluded by the spec). - and nowhere else. This patch also integrates the tests added to GenericsRegressionTest.java by the patch at https://bugs.eclipse.org/bugs/show_bug.cgi?id=339478#c8 (but not the changes introduced by that patch to GenericsRegressionTest_1_7.java yet) This patch obviates the need for the changes made by the prior patch to the following elements: - IProblem.java - ProblemReporter.java - messages.properties - CompilerInvocationTests.java - SourceTypeBinding.java See that while this patch is expected to reject all ill formed programs, it implements very little behavior for well formed programs that use diamond. So such programs may and will trigger all manner of problems from the compiler as of now. Created attachment 191902 [details] Revised patch for rejecting ill formed programs (In reply to comment #14) > Created attachment 191891 [details] > Patch under consideration > This patch is under test. > > With this code change we should correctly reject all illegal uses > of <> since we really rely on the 1.6- error reporting behavior and > short circuit that ONLY when we are absolutely sure <> constitutes > diamond and not merely missing type parameters. Same patch as before, with improved comments. Also includes a fix to a DOM/AST test that was failing with the earlier patch. Passes all JDT/Core tests. Ayush, please review. Basically, all <> is guilty unless proven innocent and deemed innocent only if 1.7+ and only if used class instance creation that doesn't involve an anonymous class. Ayush, please review, TIA. If this patch looks alright, I'll release it and can you prepare patch that contains the rest of the changes from your patch and post it, so we can continue with the review ? Thanks. (In reply to comment #15) > Created attachment 191902 [details] [diff] > Revised patch for rejecting ill formed programs Patch looks good. (In reply to comment #16) > (In reply to comment #15) > > Created attachment 191902 [details] [diff] [details] [diff] > > Revised patch for rejecting ill formed programs > > Patch looks good. Released this patch in BETA_JAVA7 branch Created attachment 191946 [details]
Unreviewed remaining patch fragment
The portion of Ayush's patch yet to be reviewed is
attached.
I released GenericsRegressionTest_1_7.java from the original
patch after :
- Moving negative tests that should be run in all modes to
GenericsRegressionTest.java and
- Disabling the failing tests.
Created attachment 191947 [details]
Unreviewed remaining patch fragment
Minimal patch.
Created attachment 191984 [details]
Unreviewed remaining patch fragment
This patch
- Gets rids of the changes in:
TypeBinding.java,
ReferenceBinding.java
LocalDeclaration.java
FieldDeclaration.java
- Fixes some issues in QualifiedAllocationExpresssion.java
- Various cleanups.
- Enables all the tests in GenericsRegressionTest1_7.java
that were disabled earlier - all of these pass now.
- Is still largely unreviewed though.
The following program fails to compile correctly (javac 7b135 does).
Basically, there is no need for the arity of type arguments to be the
same on LHS and RHS. See that code completions works as expected (i.e
infers correctly).
// ---------------------------8<-------------------------
import java.util.HashMap;
import java.util.Map;
class StringKeyHashMap<V> extends HashMap<String, V> {
}
class IntegerValueHashMap<K> extends HashMap<K, Integer> {
}
public class X {
Map<String, Integer> m1 = new StringKeyHashMap<>();
Map<String, Integer> m2 = new IntegerValueHashMap<>();
}
Created attachment 192058 [details] <> Implementation - Phase I I have reviewed all the changes made by Ayush and the current patch is a cleaned up version of the uncommitted changes. - Cleaned up the APIs around org.eclipse.jdt.internal.compiler.ast.SingleTypeReference.resolveTypeEnclosing(BlockScope, ReferenceBinding, TypeBinding) and org.eclipse.jdt.internal.compiler.ast.TypeReference.resolveType(BlockScope, boolean, TypeBinding) to take the expected type as a parameter instead of just the type arguments to be used for inference. This is going to necessary to handle inference in all but the most trivial cases. - Copyright message changes, improved comments, fixed new warning introduced by patch etc. - Got rid of dead code, various redundant checks etc. - Enabled all but two of the tests in GenericsRegressionTest1_7.java. - I withheld the code changes in ParameterizedTypeBinding. These bindings are to be shared across the AST and so storing any data in these bindings that is not shared is a bad idea. As a consequence, we have a couple of test failures, these need to be looked into. With these changes, we have a basic if naive implementation in place. The current code simply transfers type arguments from the expected type to the class instance creation expression. This works ok in straightforward cases where the class instance creation expression is isomorphic with respected to the expected type (i.e, same type, or a sub type with the same arity of type arguments with type arguments at each type level being used to parameterize the super type.) Next steps: - Study the spec closely to make sure the implementation conforms fully to the spec. - Generalize the implementation to handle complext cases a la https://bugs.eclipse.org/bugs/show_bug.cgi?id=339478#c21 - Fix the two remaining failures in GRT17.java Released in BETA_JAVA7 branch. I have enabled all the tests and released it into BETA_JAVA7 branch. Ayush, on the following tests we differ in behavior from javac: test001f test001g test001i test007 test008 test0016 test0016b test0017 some of these were disabled earlier, while some were not. Did you analyze the case and conclude if eclipse behavior was the reasonable thing ? I'll investigate these differences. (In reply to comment #23) > Ayush, on the following tests we differ in behavior from javac: > > test001f > test001g > test001i > test007 > test008 > test0016 > test0016b > test0017 All of these except test007 and test008 are, IMHO, cases where the type args can be easily inferred, and I think javac is currently missing the full implementation. Thats why they currenly differ with javac7 Test007 and test008 show the semantic difference between using the diamond case in the instantiation vs. using the raw type, as I inferred from the spec and javac behaviour. I had disabled them because currently we give an incorrect output for these cases. Javac7's output is correct. They should be re-disabled. (In reply to comment #24) > (In reply to comment #23) > > Ayush, on the following tests we differ in behavior from javac: > > > > test001f > > test001g > > test001i > > test007 > > test008 > > test0016 > > test0016b > > test0017 > > All of these except test007 and test008 are, IMHO, cases where the type args > can be easily inferred, and I think javac is currently missing the full > implementation. Thats why they currenly differ with javac7 In that case, I would have preferred that we had documented that list here rather than merely making these tests pass with the current eclipse behavior. OK, in anycase we have the list now and I'll see what can be done. (In reply to comment #22) > Next steps: > > - Study the spec closely to make sure the implementation conforms > fully to the spec. At least after a cursory reading, it appears that the current implementation is way off - There is no notion of "expected type" from the assignment context influencing the inference in the specification. I'll follow up on this. Comment on attachment 192058 [details]
<> Implementation - Phase I
I have reversed this patch. As noted earlier, this implementation using "expected type" from assignment context is incorrect and doesn't conform to the specifications.
(In reply to comment #24) > (In reply to comment #23) > > Ayush, on the following tests we differ in behavior from javac: > > > > test001f > > test001g > > test001i > > test007 > > test008 > > test0016 > > test0016b > > test0017 > > All of these except test007 and test008 are, IMHO, cases where the type args > can be easily inferred, and I think javac is currently missing the full > implementation. Thats why they currenly differ with javac7 javac implementation is in place and the differences are due to eclipse not implementing the spec. Working on this. Created attachment 193887 [details] Revised Implementation v0.9 While this will undergo quite a bit of cleanup, bug fixing, polish and code reorganization (most of the functionality is now in compiler.ast while it logically belongs in compiler.lookup), the current patch has the meat of the revised implementation in place and working well. org.eclipse.jdt.internal.compiler.ast.AllocationExpression.inferElidedTypes(ReferenceBinding, TypeBinding[], BlockScope) has the new implementation. All but 8 (new) tests pass. Even these tests likely need remastering (see comment# 23). QualifiedAllocationExpression and ParameterizedQualifiedTypeReference may have to undergo similar changes as done in AllocationExpression and ParameterizedSingleTypeReference. I can see the light at the end of the tunnel, folks ! Created attachment 193901 [details]
Revised Implementation v0.95
More progress. With this patch, we have only 3 tests that
fail in the JDT/Core test suite, all of them having explicit
type arguments at the generic constructor invocation site.
Interestingly javac 7b137 rejects the following program:
public class X<T> {
<E> X(){
}
X<String> x = new <String>X<>();
}
with :
C:\jtests>C:\jdk-7-ea-bin-b137-windows-i586-07_apr_2011\jdk7\jdk1.7.0\bin\javac
-Xlint:unchecked -Xlint:rawtypes -Xlint:deprecation -sourcepath c:\jtests X.java
X.java:4: error: cannot infer type arguments for X<>;
X<String> x = new <String>X<>();
^
reason: actual and formal argument lists differ in length
1 error
Looks like a bug in javac.
Created attachment 193918 [details] Revised Implementation v0.97 This patch fixes all known issues. All tests are enabled now and are passing. The behavior compares favorably with javac 7b137. GenericsRegressionTest_1_7.test0016() GenericsRegressionTest_1_7.test0016b() GenericsRegressionTest_1_7.test0017() are the only tests where eclipse behavior differs from javac. In these cases, I believe javac is the one that is buggy (see comment# 31) Created attachment 193951 [details]
Revised Implementation v0.98
Same patch as before, but is minimal so easier to review.
Ayush, no need to review yet, but you are welcome to glance
through it - I'll send a review request after some more
testing, clean up and reorganization of the code.
(In reply to comment #31) > Interestingly javac 7b137 rejects the following program: [...] > Looks like a bug in javac. We have confirmation from Oracle that this is a javac bug (bug number 7030150, supposedly fixed in 7b138) Created attachment 193962 [details]
Revised Implementation v1.0
Cleaned up implementation.
Created attachment 193963 [details]
Revised Implementation with tests v1.0
Earlier patch was just sources. Current one includes tests.
Ayush, please review. TIA. I'll be releasing this shortly into the BETA_JAVA7 branch. Apart from review, I would also request you to verify the implementation - TIA, The <> inference implementation is complete. All JDT/Core tests pass. Implementation compares favorably with javac 7b137. Released patch into BETA_JAVA7 branch. Ayush, please raise separate follow up defects for any issues uncovered during code review and verification - Thanks. (In reply to comment #32) > GenericsRegressionTest_1_7.test0016() > GenericsRegressionTest_1_7.test0016b() > GenericsRegressionTest_1_7.test0017() > > are the only tests where eclipse behavior differs from > javac. In these cases, I believe javac is the one that > is buggy (see comment# 31) Verified that eclipse compiler's behavior is identical to javac 7b138's behavior. The patch is good. A few fup defects found have been raised in bug 345968 and bug 346959. Comment 31 is rejected by both Eclipse compiler and javac. Verified. |
With the current contents of BETA_JAVA7 stream, a simple declaration using the diamond semantics fails to compile public Test<T> { public void foo() { Test<String> test = new Test<>(); } } The grammar and parsing is correct, issues arise during resolution wherever Test<String> is expected and Test<> is obtained instead. I'm looking into this.