Community
Participate
Working Groups
When converting an anonymous to a lambda expression, we have to consider that names declared in the functional interface are not in scope any more. JLS7 6.3: "The scope of a declaration of a member m declared in or inherited by a class type C (§8.1.6) is the entire body of C, including any nested type declarations." package jsr335.funint; @FunctionalInterface interface Func { enum En { A } void foo(); String NAME = ""; static String staticName() { return NAME; } default String getName() { return NAME; } } class Test { void bar() { Func f = new Func() { @Override public void foo() { System.out.println(En.A); System.out.println(NAME); // already not in scope, since static interface methods are not inherited: System.out.println(staticName()); // OK: System.out.println(Test.this); // accessing this or super: System.out.println(getName()); foo(); System.out.println(this); System.out.println(super.toString()); Class<? extends Func> tc= this.getClass(); Class<? extends Object> sc= super.getClass(); System.out.println(Func.super.getName()); } }; } }
*** Bug 430573 has been marked as a duplicate of this bug. ***
Manju, please handle this with priority. If the fix is simple enough, then we should fix this for RC1. Conversion of a full-source Eclipse SDK workspace revealed that this is a common problem, e.g.: - org.eclipse.ui.ide CopyFilesAndFoldersOperation.java - org.eclipse.pde.ui FeatureImportOperation.java PluginImportHelper.java Note: The problem "Cannot reference a field before it is defined" (happens e.g. in AbstractTextEditor) is not part of this bug. That's a bug 421926 (a bug in the JLS, for which we won't implement a workaround for now).
Created attachment 243021 [details] WIP Patch+Tests There are two issues to be handled in this bug: a) qualify references to static fields and types that are no longer in scope in the lambda b) add missing cases to SuperThisReferenceFinder, so that all implicit references to 'this' block the quick assist (e.g. the recursive call to foo()) This patch handles the first case and a partial fix for the second case. Markus, let me know if i am on the right direction. I am yet to test all the different cases.
Created attachment 243077 [details] Patch+Tests With this patch i have covered all the cases that were encountered and the tests depicts the cases. Markus, let me know if this is correct.
(In reply to Manju Mathew from comment #4) LambdaExpressionsFix.CreateLambdaOperation#qualifyNameNodes(..) looks like it does too much String-handling. It should use ImportRewrite to convert the ITypeBinding into ASTNodes. Pass the CURewrite to qualifyNameNodes, so that the ImportRewrite only gets created when it's really used. Since we later use the ImportRemover, we also have to call importRemover.registerAddedImports(..) on the result of addImports(..). Example: interface Func<T> { void go(); String NAME = Func.class.getName(); } public class Test { void foo() { Func<String> f= new Func<String>() { @Override public void go() { System.out.println(NAME); } }; } } java.lang.IllegalArgumentException: Invalid identifier : >Func<String>< at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:199) at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2202) at org.eclipse.jdt.core.dom.AST.newName(AST.java:1929) at org.eclipse.jdt.internal.corext.fix.LambdaExpressionsFix$CreateLambdaOperation.qualifyNameNodes(LambdaExpressionsFix.java:343) at org.eclipse.jdt.internal.corext.fix.LambdaExpressionsFix$CreateLambdaOperation.rewriteAST(LambdaExpressionsFix.java:315) ... The InterfaceAccessQualifier#visit(QualifiedName) method also shouldn't use getFullyQualifiedName() but compare bindings. String#startsWith(..) always misses some cases when you try to compare packages.
package jsr335.funint; @FunctionalInterface interface Func { class C { static class CIn { static int i= 0; } static String NAME = ""; } void foo(); } class Test { static class C { static class CIn { static int i= 1; } static String NAME = ""; } void bar() { Func f = new Func() { @Override public void foo() { System.out.println(C.CIn.i); System.out.println(C.NAME); } }; } } Manju, with the latest patch, apply quick assist in the above example. - Additional import is added: import jsr335.funint.Func.C; - C.CIn.i is not qualified with Func. As a result, it gives value 0 before conversion and value 1 after the conversion.
Created attachment 243339 [details] Updated patch + Tests Thanks Noopur for the usecase. I have taken care of all the cases mentioned and corresponding tests are also added. Markus and Noopur, kindly review.
Apply the conversion in the following example: interface FI { int e= 0; void run(int x); class Test { public static void test(int e) { FI fi = new FI() { @Override public void run(int x) { System.out.println(e); } }; fi.run(e); } } } 'e' in System.out.println(e) refers to the field "int e= 0" in FI. After conversion, it refers to the method parameter 'e' in #test.
Tested the patch and other cases look fine. It can be released after fixing the issue mentioned in comment #8.
Please ignore the example in comment #8, it works correctly. However, for the case in comment #6, we get the result as: Func f = () -> { System.out.println(jsr335.funint.Func.C.CIn.i); System.out.println(Func.C.NAME); }; The 1st statement has package name included in the qualifier and the 2nd one doesn't. Is it OK or should it be consistent?
(In reply to Noopur Gupta from comment #10) > Please ignore the example in comment #8, it works correctly. The example which does not work is: interface FI extends FIS { void run(int x); public static void test(int e) { FI fi = new FI() { @Override public void run(int x) { System.out.println(e); } }; fi.run(e); } } interface FIS { int e= 0; }
The LambdaExpressionsFix.CreateLambdaOperation#isTypeImported(..) is not good. E.g. AssistQuickFixTest18.testConvertToLambda21() already fails if you change the import to "import test1.Func2.En.*;". In InterfaceAccessQualifier#visit(QualifiedName), I don't see why we should use resolveTypeBinding(). Why is the type of a variable relevant? I guess this just works by accident in some special cases with enums, where the type of the enum constant is the same as the declaring type. But in general, we only care about the declaring type of a variable, or about the type itself if the name refers to a type. The same applies to qualifyNameNodes(..): resolveTypeBinding() should not be used there. We're only interested in resolveBinding(). I would also expect that the InterfaceAccessQualifier produces a list of SimpleNames. Everything after the first '.' doesn't need to be considered for qualification. I'm also not sure why we need the list at all. Can't we just add the qualifications while we visit the AST? The "importRemover.registerAddedImports(addImport);" call should go right after the addImport(..) call (not in an else branch). We don't know whether an import was added, so we have to register the type all the time.
Created attachment 243415 [details] Updated patch + Tests 1. #isTypeImported(..)is updated to consider onDemand import statements. 2. Updated to use #resolveBinding instead of #resolveTypeBinding 3. InterfaceAccessQualifier now qualifies the nodes during the AST visit. 3. importRemover.registerAddedImports(addImport); is added soon after the #addImport(..) 4. Added appropriate tests. Markus and Noopur, kindly have a look.
Still working on a soln for the case reported in comment 11.
(In reply to Markus Keller from comment #12) > The LambdaExpressionsFix.CreateLambdaOperation#isTypeImported(..) is not > good. This was not a criticism of the implementation, but of the whole approach. It's the ImportRewrite's job to deal with imports, and we should not try to rebuild that. Please explain which problem can't be solved by the ImportRewrite.
Created attachment 243417 [details] Updated patch + Tests (In reply to Markus Keller from comment #15) > Please explain which problem can't be solved by the ImportRewrite. When i tried to explain to myself why the method was needed in the first place i realized the mistake. It was introduced in the initial phase of the development of this issue and never validated whether it was actually required. This patch also contains the fix for the usecase reported in comment 11.
*** Bug 475260 has been marked as a duplicate of this bug. ***
I noticed that converting to lambda expressions still fails in Eclipse 4.12 when the functional interface contains generics. For instance: @FunctionalInterface public interface Calculator<M> { double calculate(int a); default double sqrt(int a) { return Math.sqrt(a); } } Quick assist/clean up will transform a bit of code similar to the following: Calculator<?> calculator = new Calculator<>() { @Override public double calculate(int a) { return sqrt(a) + 1; } }; to: Calculator<?> calculator = a -> sqrt(a) + 1; This results in invalid code. When I remove the <M> generics, the invalid fix is not suggested.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
