Community
Participate
Working Groups
Build ID: I20070209-1006 Steps To Reproduce: In the following code: public class GenericExample { public <T> void find(T a, List<T> b) { } public void foo() { find(x, Arrays.asList("a")); } } Choose the compile error on 'x', and pick "Create local variable 'x'". The result: public class GenericExample { public <T> void find(T a, List<T> b) { } public void foo() { T x; find(x, Arrays.asList("a")); } } 'T' is a type variable with no meaning in this context. Better would be Object, and better still would be String. More information:
The problem is the method binding that is guessed by the DOM AST: The binding of 'find(x, Arrays.asList("a"))' is the generic method binding; should be a parameterized method binding (with some guess for 'T', best 'Object'). The following example is doing it right. Except that it has a strange guess for the type parameter. public class GenericExample2 { public <T> void find(T a) { } public void foo() { find(x); // hover says: <GenericExample2> void p.GenericExample2.find(GenericExample2 a) } }
The DOM/AST binding is only exposing the compiler closest match. In this case the closest match ends up being a generic method binding instead of a parameterized method binding. Kent, Would it be possible to return a parameterized method binding in this case? A method invocation binding cannot be a generic method binding.
Added disabled regression test org.eclipse.jdt.core.tests.dom.ASTConverter15Test#_test0248
Turning a generic method into parameterized is easy, but what arguments would be used ? A closest match may arise in many situations where inference cannot perform (since unable to resolve arguments, or could imagine wrong arity as well etc...). Alternatively, the generic method could be turned into a method parameterized with erasures, but this might be suboptimal. Partial inference is just not supported, and may yield strange results anyway.
As Philippe mentioned, there really is no good answer. Take this example where the argument order needs to be switched: <T extends Comparable<T>> void find(T a, String[] b, List<T> c) {} void foo(String[] s) { find(x, Arrays.asList("a"), s); } If we infer T to be Object or Comparable#RAW, we're left with bound check errors. How are we to see that the 2nd parameter matches the 3rd argument and as a result we should infer T to be a String ? I think the UI should detect the type variable and ask the user. Maxime, do you see a better way?
Not really. Current work on other bugs suggests that the choice between bounds and erasure substitution is non trivial and important for code correctness. If I read IMethodBinding and BindingResolver APIs correctly, we make no commitment at all about generic methods. We tell the caller that we may substitute 'in certain cases'. I must admit that this is less than ideal, and that there is a discrepancy between the initial code sample and the one of comment #2. We probably should consider being more consistent here, and reconsider the API contracts, but this is due to have far reaching, unforeseen implications. IMHO, the most promising direction would be to return the most precise information we have when we hit an error (that is the generic method untouched), and provide means for the caller to attempt different types of substitutions depending on his own context. As underscored by Kent's example, we may end up wanting to make several proposals, that may correspond to different substitution rules. In any case, as far as the shorter term is concerned, Martin may want to detect that the proposed type is a type parameter and attempt a guess by himself?
Don't worry about how good the guess is. The AST doesn't make any promises here, it's done best effort. But from the API users perspective it is more important that method binding is a parameterized one.
I don't buy it. The guess is going to be wrong more often that its right. The UI is better off handling this case by itself. You can see that the type of the variable is a type variable and can ask the user. What's the point in defining the variable with a type that results in compiler errors in the code? You'll just be back here in a few weeks with another case that you want changed.
I agree with Martin. (In reply to comment #6) > If I read IMethodBinding and BindingResolver APIs correctly, we make no > commitment at all about generic methods. We tell the caller that we may > substitute 'in certain cases'. I disagree about the 'make no commitment' part. The 'in certain cases' in the Javadoc of IMethodBinding is maybe not crystal clear, but isGenericMethod() and isParameterizedMethod() clearly state that the former is for declarations and the latter is for parameterized references. JDT/UI relies on this useful distinction all over the place. (In reply to comment #8) > The UI is better off handling this case by itself. You can see that the type > of the variable is a type variable and can ask the user. The UI can always get hold of the type variable from the declaration by calling getMethodDeclaration().getParameterTypes() on the parameterized method binding. The actual type arguments of the parameterized method binding should be best-effort guesses, and they should be types that make sense in the calling context (the type variable T does not). The UI can use the guesses as initial solution for quick fixes and still ask the user based on the type variable bounds. > What's the point in defining the variable with a type that results in compiler > errors in the code? Whether the parameterized method compiles with the guesses is not that important. Important is that the binding structure is reliable. Although the different kinds of method bindings are all IMethodBindings (and not split up into IParameterizedMethodBinding, etc.), client code relies on the basic assumptions about where which kind can occur.
Surfacing generic methods is indeed bad, and shouldn't occur. As discussed earlier, would a raw method or something equivalent be good enough ? Alternatively, we could nullify the binding, and resort to an alternate #recoveredBinding() API (also relevant to another bug - Jerome) to answer the generic binding ? Then we could offer an API to perform some inference from a generic method and type arguments. Reading the thread of comments, it feels that a good enough closest match is all what is needed here, and given no inference was possible, a raw method seems pretty accurate.
A raw binding would be fine. I'd rather not start the discussion about a new API like '#recoveredBinding()' here. See bug 149567 comment 5 discussing this. Another point is that we always have had guessed method bindings for places with errors and it worked out well.
(In reply to comment #9) > I agree with Martin. > > (In reply to comment #6) > > If I read IMethodBinding and BindingResolver APIs correctly, we make no > > commitment at all about generic methods. We tell the caller that we may > > substitute 'in certain cases'. > > I disagree about the 'make no commitment' part. The 'in certain cases' in the > Javadoc of IMethodBinding is maybe not crystal clear, but isGenericMethod() and > isParameterizedMethod() clearly state that the former is for declarations and > the latter is for parameterized references. JDT/UI relies on this useful > distinction all over the place. But noone tells that you can't get a declaration as the result of the resolve process on a message send, as unfortunate as it may sound. Moreover, we comment at length on the fact that the resolution process is very much context sensitive (see BindingResolver#resolveMethod comments). My point is not that we have no opportunity to make useful changes here. My point is that the spec itself should be clarified, and aligned with something acceptable to all parties. Question to Philippe: would we want to commit to the fact that BindingResolver#resolveMethod(MethodInvocation method) never returns a generic method?
Investigating a raw binding solution; I think it is the simplest approach we can take. Also, in this particular case, we can improve the situation by inferring String in place of Object (but this may not always be the case). Current story is that for missing arguments, we substituted the actual receiver type... why not rather the null type ? When doing so, inference can perform pretty good (like here), and if not, I can ensure turning a generic method in raw form. These are only error cases, for which we can provide heuristics (i.e. courtesy of the compiler). Also AllocationExpression, QualifiedAllocationExpression and ExplicitConstructorCall all have the same issue.
Created attachment 65012 [details] Proposed patch Current thinking. Unfortunately, contains a code cleanup on MessageSend.
Martin - could you give it a try (apply patch to HEAD) and let me know if it addresses your issues ? Olivier - pls produce some DOM regression tests (the compiler isn't producing visible differences with the patch, side-effects are mostly on consumers of closestMatches).
The original example works now. The example form comment 5 still returns a type binding of 'T' which is not available in the given context. A raw binding method bing would be good enough.
Testcases: import java.util.*; public class X { private <T> T find(T a, List<T> b) { return b.contains(a) ? a : null; } public void foo1() { // T x; find(x, Arrays.asList("a")); // closestMatch: #find(String,List<String>) find(x, 0); // closestMatch: #find(Object,List<Object>) } }
Testcase with alloc. import java.util.*; public class X { <T> X(T a, List<T> b) { } public void foo1() { // T x; new X(x, Arrays.asList("a")); // closestMatch: #X(String,List<String>) new X(x, 0); // closestMatch: #X(Object,List<Object>) } }
Test with qualified alloc: import java.util.*; public class X { class M { <T> M(T a, List<T> b) { } } public void foo1() { // T x; this.new M(x, Arrays.asList("a")); // closestMatch: #X(String,List<String>) this.new M(x, 0); // closestMatch: #X(Object,List<Object>) } }
Variation on qualified alloc (with anonymous type): import java.util.*; public class X { class M { <T> M(T a, List<T> b) { } } public void foo1() { // T x; this.new M(x, Arrays.asList("a")){}; // closestMatch: #X(String,List<String>) this.new M(x, 0){}; // closestMatch: #X(Object,List<Object>) } }
Test with explicit constr call: import java.util.*; class Super { <T> Super(T a, List<T> b) { } } public class X extends Super { public X() { // T x; super(x, Arrays.asList("a")){}; // closestMatch: #X(String,List<String>) } public X(boolean b) { // T x; super(x, 0){}; // closestMatch: #X(Object,List<Object>) } } }
Created attachment 65074 [details] Better patch
Olivier - pls release additional DOM tests when avail. Also include one for scenario in comment 5. Enabled ASTConverter15Test#test0248. Released for 3.3M7. Fixed
Added regression tests for each test case where I checked that the method binding is either a raw method or a parameterized method. org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0248() org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0264() org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0265() org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0266() org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0267() org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0268() org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0269()
Verified for 3.3 M7 using build I20070427-0800.
