Community
Participate
Working Groups
Convert lambda to anonymous must qualify references to 'this'/'super': package p; import java.util.function.IntSupplier; public class A { public static void main(String[] args) { new A(); } A() { IntSupplier i = () -> this.m(); System.out.println(i.getAsInt()); } public int m() { return 7; } } => In the anonymous, the method must return "A.this.m()", not just "this.m()".
*** This bug has been marked as a duplicate of bug 421479 ***
Not a dup, this is about the other direction.
FYI, I am working on this one. Hope to have a patch soon.
I have a patch now. Hope to be able to publish it soon.
Created attachment 243182 [details] My patch to review
(In reply to Jerome Cambon from comment #5) > Created attachment 243182 [details] [diff] > My patch to review - Add an explicit CoO sign-off comment: http://wiki.eclipse.org/Development_Resources/Contributing_via_Git#via_Bugzilla - Update copyright header and follow JDT UI coding conventions: https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions - Add test cases in AssistQuickFixTest18.java - Note that lambda being converted can also be present in an anonymous class. Modified example from comment #0: A() { Runnable r = new Runnable() { int x= 10; @Override public void run() { IntSupplier i = () -> this.x; } }; } In this case, we get: java.lang.IllegalArgumentException: Invalid identifier : >< at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:195) at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2202) at org.eclipse.jdt.internal.corext.fix.LambdaExpressionsFix$CreateAnonymousClassCreationOperation.rewriteAST(LambdaExpressionsFix.java:453) - Instead of creating separate Finder classes, we should have a single class and the qualification can also be added in that class while visiting the AST. - We should qualify only the *unqualified* 'super' and 'this' references *directly* present in lambda body. (Refer LambdaExpressionsFix.SuperThisReferenceFinder and bug 421479 for the other direction i.e. conversion of anonymous to lambda.)
This contribution complies with http://www.eclipse.org/legal/CoO.php
(In reply to Noopur Gupta from comment #6) > - Add an explicit CoO sign-off comment: > http://wiki.eclipse.org/Development_Resources/ > Contributing_via_Git#via_Bugzilla Done > > - Update copyright header and follow JDT UI coding conventions: > https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions Done > > - Add test cases in AssistQuickFixTest18.java Done. (testConvertToAnonymousClassCreation11 is failing, see below) > > - Note that lambda being converted can also be present in an anonymous > class. Modified example from comment #0: > A() { > Runnable r = new Runnable() { > int x= 10; > @Override > public void run() { > IntSupplier i = () -> this.x; > } > }; > } > In this case, we get: > java.lang.IllegalArgumentException: Invalid identifier : >< > at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:195) > at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2202) > at > org.eclipse.jdt.internal.corext.fix. > LambdaExpressionsFix$CreateAnonymousClassCreationOperation. > rewriteAST(LambdaExpressionsFix.java:453) Indeed, good catch. I am now checking if the declaring class is valid, and removing the 'this' reference to avoid compilation error. I still have an issue (see testConvertToAnonymousClassCreation11, which is failing). Note this code works well if I replace: "IntSupplier i = () -> this.x;" by "IntSupplier i = () -> {return this.x;};" Any hint on this ? > > - Instead of creating separate Finder classes, we should have a single class > and the qualification can also be added in that class while visiting the AST. Done. Indeed, this simplify a bit the code. > > - We should qualify only the *unqualified* 'super' and 'this' references > *directly* present in lambda body. Done. (Not sure I understand what you mean by "directly" though) > > (Refer LambdaExpressionsFix.SuperThisReferenceFinder and bug 421479 for the > other direction i.e. conversion of anonymous to lambda.)
Created attachment 244333 [details] New patch to review
- Copyright is still not updated in the test file. - In LambdaExpressionsFix.CreateAnonymousClassCreationOperation#rewriteAST, lambdaExpression.resolveMethodBinding() can return null (=> null check is missing). However, instead of using bindings to get the parent type, use: ASTResolving.findParentType(ASTNode node). - The new class 'SuperThisQualifier' should extend HierarchicalASTVisitor instead of ASTVisitor (Refer LambdaExpressionsFix.SuperThisReferenceFinder). - SuperThisQualifier#perform should accept LambdaExpression instead of ASTNode. Also pass TextEditGroup to it. - > > We should qualify only the *unqualified* 'super' and 'this' references > > *directly* present in lambda body. > Not sure I understand what you mean by "directly" though class TX { IntSupplier i = () -> { class CX { int n=10; { this.n= 0; } } return -1; }; } Here, this.n is not directly present in lambda and hence should not be affected. We should not visit AnonymousClassDeclaration and BodyDeclaration within lambda (Refer LambdaExpressionsFix.SuperThisReferenceFinder). - > > Note that lambda being converted can also be present in an anonymous class > I am now checking if the declaring class is valid, and removing the 'this' > reference to avoid compilation error. It can happen with 'super' (field access / method invocation) also (not just 'this'). And, removing 'this' or 'super' is not the correct solution. In the following example, if you remove 'this' while conversion, it will refer to the local 'x' with value 2 which is wrong. new Runnable() { int x= 1; public void run() { IntSupplier i = () -> { int x= 2; return this.x; }; } }; Markus, how should it be handled? - For the tests, you could group similar examples within a single lambda in test methods instead of creating different test methods for individual cases.
(In reply to Noopur Gupta from comment #10) > It can happen with 'super' (field access / method invocation) also (not just > 'this'). And, removing 'this' or 'super' is not the correct solution. > In the following example, if you remove 'this' while conversion, it will > refer to the local 'x' with value 2 which is wrong. > new Runnable() { > int x= 1; > public void run() { > IntSupplier i = () -> { > int x= 2; > return this.x; > }; > } > }; > > Markus, how should it be handled? It can't be solved without major rewrites (e.g. renaming one of the 'x' variables or adding a getter inside 'new Runnable() {...}' that returns 'this.x'. Let's assume this is sufficiently uncommon that it doesn't require an actual solution. It would be highly unexpected to make such major changes in this Quick Assist / Clean Up, but it would also be strange if we just disabled the conversion in this uncommon case. I think the best solution is to generate an invalid qualification like this to make the user aware of the problem: public int getAsInt() { int x= 2; return Runnable.this.x; } (You should get this for free if you use ImportRewrite, see below.) Implementation notes: - Don't pass an unqualified String className to ThisOrSuperQualifier#perform(..). Pass the ITypeBinding itself and use ImportRewrite#addImport(..) to turn the binding into a name when you need it. Make sure it also works fine with generic outer types (maybe need to call ITypeBinding#getTypeDeclaration() somewhere to strip type arguments). - Don't use ASTNode.copySubtree unless absolutely necessary. This loses all formatting and comments. Use e.g. this to rewrite an ASTNode property: fRewrite.set(node, SuperMethodInvocation.QUALIFIER_PROPERTY, newQualifier, null); For List-valued properties, you would use ASTRewrite#getListRewrite(..).
(In reply to Markus Keller from comment #11) > Implementation notes: > > - Don't pass an unqualified String className to > ThisOrSuperQualifier#perform(..). Pass the ITypeBinding itself and use > ImportRewrite#addImport(..) to turn the binding into a name when you need > it. Make sure it also works fine with generic outer types (maybe need to > call ITypeBinding#getTypeDeclaration() somewhere to strip type arguments). Ok Markus, but Noopur suggested to use ASTResolving.findParentType() instead of using bindings in comment 10. What is the best?
(In reply to Jerome Cambon from comment #12) > (In reply to Markus Keller from comment #11) > > Implementation notes: > > > > - Don't pass an unqualified String className to > > ThisOrSuperQualifier#perform(..). Pass the ITypeBinding itself and use > > ImportRewrite#addImport(..) to turn the binding into a name when you need > > it. Make sure it also works fine with generic outer types (maybe need to > > call ITypeBinding#getTypeDeclaration() somewhere to strip type arguments). > > Ok Markus, but Noopur suggested to use ASTResolving.findParentType() instead > of using bindings in comment 10. What is the best? Use ASTResolving.findParentType() to get the parent type node and call #resolveBinding on it to get the corresponding ITypeBinding. Handle anonymous classes and generic types as suggested by Markus.
(In reply to Markus Keller from comment #11) > Implementation notes: > > - Don't pass an unqualified String className to > ThisOrSuperQualifier#perform(..). Pass the ITypeBinding itself and use > ImportRewrite#addImport(..) to turn the binding into a name when you need > it. Make sure it also works fine with generic outer types (maybe need to > call ITypeBinding#getTypeDeclaration() somewhere to strip type arguments). My last issue is related to this. I got indeed an exception with the generic types. I have a generic type (e.g. 'F<Object>', 'F' being a functional interface), and I need a SimpleType ('F'). I tried ITypeBinding#getTypeDeclaration() as suggested, but it doesn't help: it returns the same ITypeBinding. I don't find a method from ITypeBinding that allow to strip this. Any advice?
(In reply to Jerome Cambon from comment #14) > (In reply to Markus Keller from comment #11) > > Implementation notes: > > > > - Don't pass an unqualified String className to > > ThisOrSuperQualifier#perform(..). Pass the ITypeBinding itself and use > > ImportRewrite#addImport(..) to turn the binding into a name when you need > > it. Make sure it also works fine with generic outer types (maybe need to > > call ITypeBinding#getTypeDeclaration() somewhere to strip type arguments). > > My last issue is related to this. > I got indeed an exception with the generic types. > I have a generic type (e.g. 'F<Object>', 'F' being a functional interface), > and I need a SimpleType ('F'). > I tried ITypeBinding#getTypeDeclaration() as suggested, but it doesn't help: > it returns the same ITypeBinding. > I don't find a method from ITypeBinding that allow to strip this. Any advice? How are you using ITypeBinding with ImportRewrite#addImport(..) to get the name? And where are you getting the exception? - Please attach the example and patch. See if ITypeBinding.getErasure() is required. Try ImportRewrite.addImport(ITypeBinding binding) which returns a String that can be used to create the new Name.
(In reply to Jerome Cambon from comment #14) > (In reply to Markus Keller from comment #11) > > Implementation notes: > > > > - Don't pass an unqualified String className to > > ThisOrSuperQualifier#perform(..). Pass the ITypeBinding itself and use > > ImportRewrite#addImport(..) to turn the binding into a name when you need > > it. Make sure it also works fine with generic outer types (maybe need to > > call ITypeBinding#getTypeDeclaration() somewhere to strip type arguments). > > My last issue is related to this. > I got indeed an exception with the generic types. > I have a generic type (e.g. 'F<Object>', 'F' being a functional interface), > and I need a SimpleType ('F'). > I tried ITypeBinding#getTypeDeclaration() as suggested, but it doesn't help: > it returns the same ITypeBinding. > I don't find a method from ITypeBinding that allow to strip this. Any advice? For anonymous class binding, Bindings.normalizeTypeBinding(ITypeBinding binding) can also be helpful before calling ITypeBinding#getTypeDeclaration().
Created attachment 244673 [details] New patch to review This patch includes all the comments from Noopur and Markus.
(In reply to Jerome Cambon from comment #17) > Created attachment 244673 [details] [diff] > New patch to review > > This patch includes all the comments from Noopur and Markus. Patch looks good. I have made the following changes: - Renamed ThisOrSuperQualifier to SuperThisQualifier and reordered the methods. - Passed "TextEditGroup" to #perform. - Extracted "cuRewrite.getImportRewrite()" and "cuRewrite.getASTRewrite()" to fields. - Renamed #getNewDeclaringClass to #getQualifierTypeName and added part of its functionality to CreateAnonymousClassCreationOperation#rewriteAST with null check. - Used #newName instead of #newSimpleName in #getQualifierTypeName as #addImport may return a qualified name also. Released the patch in master branch: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=235b7bd4c74d4f3e28f322f6c94c6e7c20df2c43 Keeping the bug open for any comments from Markus.
.
