Community
Participate
Working Groups
"Convert anonymous to lambda" needs to consider ambiguous target types, see example in bug 403749 comment 22. This is essentially the same problem as bug 408966 and should be solved with common infrastructure.
See TODOs in LambdaExpressionsFix#isInTargetTypeContext(..).
Created attachment 238124 [details] WIP Patch Attached a WIP patch (based on BETA_JAVA8) for the common code in ASTNodes. Markus, could you please have a quick look and let me know if this is the correct approach?
Yes, that looks good. Please also have a look at the implementation of ASTResolving#getPossibleReferenceBinding(ASTNode). This addresses a similar problem (handle all possible contexts for an expression), but it is broader (not only for target types), and the implementation takes some dangerous shortcuts (should use locationInParent like your code does). But it could help find contexts that we missed. I think we're missing this one, but there could be more: // add to ASTNodes#getTargetType(Expression): } else if (locationInParent == ParenthesizedExpression.EXPRESSION_PROPERTY) { return getTargetType((ParenthesizedExpression) parent); // > // TODO: Also use the new API from JDT Core to check for overload semantics I'm not sure any more if we really need that. In bug 403749 comment 22 and in bug 408966 comment 6, the question is not "did the target type influence the typing of the lambda expression?", but it's really "is the target context ambiguous for the lambda we want to put there?". Unlike MethodInvocation#isResolvedTypeInferredFromExpectedType(), this is not a question about the current state of the AST, but it's a hypothetical question about what will happen after a modification. The overloading problem can only happen if the lambda expression is a method/constructor argument, and this not too hard to detect. We can just check IMethodBinding > getDeclaringClass() > getDeclaredMethods() and then do a simple check for other methods with the same name, parameter count, and a FunctionalInterface parameter at the given position. If it looks like we'll run into trouble, we can just add the cast to be sure. For a full-fledged solution, we probably need much more infrastructure (bug 150657), or we'd have to create a working copy with the modified code, compile it, and then see whether there's a semantic shift. Let's avoid that for now.
Created attachment 238485 [details] Patch - Common infrastructure code (In reply to Markus Keller from comment #0) > This is essentially the same problem as bug 408966 and should be solved with > common infrastructure. Attached a patch for the common infrastructure that contains the following : - ASTNodes#isTargetAmbiguous(Expression expression) that checks whether overloaded methods can result in ambiguous method call. - ASTNodes#getTargetType(Expression expression) that returns the target type defined at the location of the given expression. - ASTResolving#findEnclosingLambdaExpression(ASTNode node) to return the lambda expression node which encloses the given node. Markus, please review. I will attach the bug fix (that uses the common infrastructure) and tests in a separate patch.
Created attachment 238511 [details] Fix + Tests Attached a patch with fix and tests that uses the common infrastructure code. The earlier code with TODOs is commented in the patch and can be removed. The cast expression creation does not check if parentheses are required around the lambda expression as it would not be required in case of method argument. More tests can be added for each type of invocation expression (super method, constructor, super constructor, class instance creation, enum constant declaration), negative scenarios etc. Please let me know if I should include all these tests.
Created attachment 238540 [details] Patch - Common infrastructure code - Ignoring static interface methods (In reply to Noopur Gupta from comment #4) > Created attachment 238485 [details] [diff] > Patch - Common infrastructure code Updated AmbiguousTargetMethodAnalyzer to ignore static methods from interfaces in the hierarchy as they are not inherited and cannot lead to ambiguity. This patch is based on top of the patch in comment #4.
Updated the new helper methods in ASTNodes to use methodDeclaration binding instead of the method binding from invocations. Also, updated AmbiguousTargetMethodAnalyzer to use declaringType binding instead of the enclosing type. Released in mmathew/BETA_Java8 branch with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=mmathew/BETA_JAVA8&id=f60966bd4d2efc97ed3b1a2178bcbd9c92de0c04
*** Bug 428452 has been marked as a duplicate of this bug. ***
Any chance this will hit the mainstream update site?
(In reply to Steve Northover from comment #9) > Any chance this will hit the mainstream update site? It's expected to be available for Java 8 GA.
Markus, please review.
Bindings.visitInterfaces(ITypeBinding, TypeBindingVisitor) was buggy. Fixed with bug 24941 comment 20. The patch looked pretty good! Released with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=83df1933c5f04c6057040de182a6e2d78e2190e0 and added a few tweaks with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3cf054348babb5073375a3acdc6add9ea8f0f712 : * found some additional quirks with parenthesized lambdas and conditional expressions, and with varargs methods * In ASTNodes#getTargetType(Expression), I've removed this: } else if (locationInParent == FieldAccess.NAME_PROPERTY || locationInParent == SuperFieldAccess.NAME_PROPERTY || locationInParent == QualifiedName.NAME_PROPERTY) { return getTargetType((Expression) parent); The expression is a ClassInstanceCreation (or some other non-Name expression), so I don't see how this code would ever be used.
We have a test failure in the official build (N20140425-2000): CleanUpTest18.testConvertToLambdaNestedWithImports
(In reply to Dani Megert from comment #13) > We have a test failure in the official build (N20140425-2000): > > CleanUpTest18.testConvertToLambdaNestedWithImports Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=e8832d2ef575efd3e8fea8679591e106160a0ee8 The compiler distinguishes void-compatible from value-compatible lambda expressions, so we can be sure it doesn't report an ambiguous method invocation error or binds to the wrong type. Also fixed a missing import when converting the innermost lambda alone here: package test; import java.util.concurrent.Callable; import java.util.concurrent.Executors; public class E { void foo() { new Thread(new Runnable() { @Override public void run() { Executors.newSingleThreadExecutor().submit(new Callable<String>() { @Override public String call() throws Exception { return "hi"; } }); } }); } }
*** Bug 433533 has been marked as a duplicate of this bug. ***
When applying the "Use lambda" cleanup to o.e.jdt.core, there's one unnecessary cast created in org.eclipse.jdt.internal.core.DeltaProcessor#isAffectedBy(IResourceDelta) at rootDelta.accept((IResourceDeltaVisitor) delta -> { /*...*/ }, IContainer.INCLUDE_HIDDEN); I first though it would be OK to avoid the cast if the two overloaded accept(..) methods have the same parameter type for the given argument index, but the counterexample below shows that it's not always so simple. Therefore, we'll live with that cast for now. import java.util.*; import java.util.function.*; import java.util.stream.Stream; public class Ambiguous { void foo(MyStream<String> stream) { stream.reduce("hi", (a, b) -> new Random().nextBoolean() ? a : b); stream.reduce("hi", new BinaryOperator<String>() { @Override public String apply(String a, String b) { return new Random().nextBoolean() ? a : b; } }); // ambiguous, needs cast to BinaryOperator<Integer>: stream.reduce(null, (a, b) -> new Random().nextBoolean() ? a : b); stream.reduce(null, new BinaryOperator<Integer>() { @Override public Integer apply(Integer a, Integer b) { return new Random().nextBoolean() ? a : b; } }); } } interface MyStream<E> extends Stream<E> { public E reduce(Integer nasty, BinaryOperator<Integer> accumulator); }
Verified as fixed in Eclipse Version: Luna (4.4) Build id: I20140428-0800
