Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConvertToLambda hint should ignore default methods. #6658

Merged
merged 1 commit into from
Jan 13, 2024
Merged

ConvertToLambda hint should ignore default methods. #6658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToLambda.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,13 @@
*/
package org.netbeans.modules.java.hints.jdk;

import com.sun.source.tree.ClassTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;
import java.io.IOException;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
import javax.tools.Diagnostic;
import org.netbeans.api.java.source.CompilationInfo;
import org.netbeans.api.java.source.JavaSource.Phase;
import org.netbeans.api.java.source.WorkingCopy;
Expand All @@ -56,7 +54,7 @@
public class ConvertToLambda {

public static final String ID = "Javac_canUseLambda"; //NOI18N
public static final Set<String> CODES = new HashSet<String>(Arrays.asList("compiler.warn.potential.lambda.found")); //NOI18N
public static final Set<String> CODES = new HashSet<>(Arrays.asList("compiler.warn.potential.lambda.found")); //NOI18N

static final boolean DEF_PREFER_MEMBER_REFERENCES = true;

Expand All @@ -68,7 +66,6 @@ public class ConvertToLambda {
})
@NbBundle.Messages("MSG_AnonymousConvertibleToLambda=This anonymous inner class creation can be turned into a lambda expression.")
public static ErrorDescription computeAnnonymousToLambda(HintContext ctx) {
ClassTree clazz = ((NewClassTree) ctx.getPath().getLeaf()).getClassBody();
ConvertToLambdaPreconditionChecker preconditionChecker =
new ConvertToLambdaPreconditionChecker(ctx.getPath(), ctx.getInfo());
if (!preconditionChecker.passesFatalPreconditions()) {
Expand Down
3 changes: 0 additions & 3 deletions java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToLambdaConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
*/
package org.netbeans.modules.java.hints.jdk;

import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
Expand All @@ -48,9 +47,7 @@
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.ReturnTree;
import java.util.List;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.Modifier;
import org.netbeans.api.java.source.CompilationInfo;
import org.netbeans.api.java.source.TreeMaker;
import org.netbeans.api.java.source.WorkingCopy;
import org.netbeans.api.java.source.matching.Matcher;
Expand Down
33 changes: 31 additions & 2 deletions ...ava.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToLambdaPreconditionChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
import javax.lang.model.type.DeclaredType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
import javax.lang.model.util.ElementFilter;
import javax.lang.model.util.Elements;
import javax.lang.model.util.Types;
import org.netbeans.api.java.source.CompilationInfo;
import org.netbeans.api.java.source.TreeUtilities;
Expand All @@ -64,6 +66,7 @@ public class ConvertToLambdaPreconditionChecker {
private final Scope localScope;
private final CompilationInfo info;
private final Types types;
private final Elements elements;
private boolean foundRefToThisOrSuper = false;
private boolean foundShadowedVariable = false;
private boolean foundRecursiveCall = false;
Expand All @@ -85,6 +88,7 @@ public ConvertToLambdaPreconditionChecker(TreePath pathToNewClassTree, Compilati
this.newClassTree = (NewClassTree) pathToNewClassTree.getLeaf();
this.info = info;
this.types = info.getTypes();
this.elements = info.getElements();

Element el = info.getTrees().getElement(pathToNewClassTree);
if (el != null && el.getKind() == ElementKind.CONSTRUCTOR) {
Expand Down Expand Up @@ -137,7 +141,32 @@ private MethodTree getMethodFromFunctionalInterface(TreePath pathToNewClassTree)
candidate = (MethodTree)member;
}
}
return candidate;
// only abstract methods can be implemented as lambda (e.g default methods can't)
ExecutableElement candidateElement = (ExecutableElement) info.getTrees().getElement(new TreePath(pathToNewClassTree, candidate));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, two comments here:

  • should we rather check the method implements an abstract interface method, rather than checking it is not default? (Meaning, if the method based is not found, we don't try to do the conversion?)
  • should this be moved to be part of ensurePreconditionsAreChecked? (And then passesAllPreconditions and passesFatalPreconditions?)

Copy link
Member Author

@mbien mbien Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we rather check the method implements an abstract interface method, rather than checking it is not default? (Meaning, if the method based is not found, we don't try to do the conversion?)

changed it to an override-abstract-method check. More straight forward, I agree.

should this be moved to be part of ensurePreconditionsAreChecked? (And then passesAllPreconditions and passesFatalPreconditions?)

I haven't implemented this so far. since the method getMethodFromFunctionalInterface is wrong without the change - it would return a method even when the interface is not functional. The precondition checks are all working, since the existence of the method is part of the checks.

Please take another look if you agree, if not I can still make it a precondition - but we probably should also rename the method. Also please note that many methods are actually unused. E.g passesAllPreconditions isn't used anywhere which means the test coverage is also not ideal atm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlahoda forgot to ping

if (overridesAbstractMethod(candidateElement, (TypeElement) baseElement)) {
return candidate;
}
return null;
}

private boolean overridesAbstractMethod(ExecutableElement method, TypeElement superType) {
Boolean overrides = overridesAbstractMethodImpl(method, superType);
return overrides != null && overrides;
}

private Boolean overridesAbstractMethodImpl(ExecutableElement method, TypeElement superType) {
for (ExecutableElement otherMethod : ElementFilter.methodsIn(superType.getEnclosedElements())) {
if (elements.overrides(method, otherMethod, superType)) {
return otherMethod.getModifiers().contains(Modifier.ABSTRACT);
}
}
for (TypeMirror otherType : superType.getInterfaces()) {
Boolean overrides = overridesAbstractMethodImpl(method, (TypeElement) types.asElement(otherType));
if (overrides != null) {
return overrides;
}
}
return null; // no match here but check the rest of the interface tree
}

public boolean passesAllPreconditions() {
Expand Down Expand Up @@ -716,7 +745,7 @@ private int getSourceStartFromTree(Tree tree) {
}

private List<TypeMirror> getTypesFromElements(List<? extends VariableElement> elements) {
List<TypeMirror> elementTypes = new ArrayList<TypeMirror>();
List<TypeMirror> elementTypes = new ArrayList<>(elements.size());
for (Element e : elements) {
elementTypes.add(e.asType());
}
Expand Down
64 changes: 64 additions & 0 deletions java/java.hints/test/unit/src/org/netbeans/modules/java/hints/jdk/ConvertToLambdaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1101,6 +1101,70 @@ public void test234686() throws Exception {

}

// default methods don't qualify for functional interfaces
public void testThatDefaultMethodsAreIgnored1() throws Exception {
HintTest.create()
.sourceLevel("1.8")
.input("package test;\n" +
"public class Test {\n" +
" private interface NotFunctional {\n" +
" public default void a(int i) {};\n" +
" }\n" +
" NotFunctional nf = new NotFunctional() {\n" +
" @Override public void a(int i) { System.err.println(i); }\n" +
" };\n" +
"}\n")
.run(ConvertToLambda.class)
.assertWarnings();
}

public void testThatDefaultMethodsAreIgnored2() throws Exception {
HintTest.create()
.sourceLevel("1.8")
.input("package test;\n" +
"public class Test {\n" +
" private interface DefaultRunnableNotFunctional1 extends Runnable {\n" +
" @Override public default void run() {};\n" +
" }\n" +
" private interface DefaultRunnableNotFunctional2 extends DefaultRunnableNotFunctional1 {}\n" +
" DefaultRunnableNotFunctional2 nf = new DefaultRunnableNotFunctional2() {\n" +
" @Override public void run() { System.err.println(); }\n" +
" };\n" +
"}\n")
.run(ConvertToLambda.class)
.assertWarnings();
}

public void testThatDefaultMethodsAreIgnored3() throws Exception {
HintTest.create()
.sourceLevel("1.8")
.input("package test;\n" +
"public class Test {\n" +
" private interface DefaultRunnableFunctional1 extends Runnable {\n" +
" public void walk();\n" +
" @Override public default void run() {};\n" +
" }\n" +
" private interface DefaultRunnableFunctional2 extends DefaultRunnableFunctional1 {}\n" +
" DefaultRunnableFunctional2 f = new DefaultRunnableFunctional2() {\n" +
" @Override public void walk() { System.err.println(5); }\n" +
" };\n" +
"}\n")
.run(ConvertToLambda.class)
.findWarning("7:39-7:65:" + lambdaConvWarning)
.applyFix()
.assertOutput("package test;\n" +
"public class Test {\n" +
" private interface DefaultRunnableFunctional1 extends Runnable {\n" +
" public void walk();\n" +
" @Override public default void run() {};\n" +
" }\n" +
" private interface DefaultRunnableFunctional2 extends DefaultRunnableFunctional1 {}\n" +
" DefaultRunnableFunctional2 f = () -> {\n" +
" System.err.println(5);\n" +
" };\n" +
"}\n");
}

static {
TestCompilerSettings.commandLine = "-XDfind=lambda";
JavacParser.DISABLE_SOURCE_LEVEL_DOWNGRADE = true;
Expand Down