From 18f2dbfc4ea6b92eaaf2ef0e304cea69a981b431 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 17 Aug 2022 20:19:40 +0200 Subject: [PATCH] Suggestions --- .../bugpatterns/MethodReferenceUsage.java | 3 +- .../bugpatterns/NestedOptionals.java | 51 +++++++++++++++ .../bugpatterns/NestingOptionals.java | 65 ------------------- .../refastertemplates/OptionalTemplates.java | 2 +- ...nalsTest.java => NestedOptionalsTest.java} | 23 +++---- 5 files changed, 66 insertions(+), 78 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestingOptionals.java rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{NestingOptionalsTest.java => NestedOptionalsTest.java} (54%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java index c0dd31fb76..82af40a1f1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java @@ -100,6 +100,7 @@ private static Optional constructMethodRef( .flatMap(statements -> constructMethodRef(lambdaExpr, statements.get(0))); } + @SuppressWarnings("NestedOptionals") private static Optional constructMethodRef( LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) { return matchArguments(lambdaExpr, subTree) @@ -156,7 +157,7 @@ private static Optional constructMethodRef( return constructFix(lambdaExpr, lhsType.tsym, subTree.getIdentifier()); } - @SuppressWarnings("NestingOptionals") + @SuppressWarnings("NestedOptionals") private static Optional> matchArguments( LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) { ImmutableList expectedArguments = getVariables(lambdaExpr); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java new file mode 100644 index 0000000000..a1da0b8442 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestedOptionals.java @@ -0,0 +1,51 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; + +import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.util.List; +import java.util.Optional; + +/** A {@link BugChecker} which flags nesting of {@link Optional Optionals}. */ +@AutoService(BugChecker.class) +@BugPattern( + summary = + "Avoid nesting `Optional`s inside `Optional`s; the resultant code is hard to reason about", + linkType = NONE, + severity = WARNING, + tags = SIMPLIFICATION) +public final class NestedOptionals extends BugChecker implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Supplier OPTIONAL = Suppliers.typeFromClass(Optional.class); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + return isOptionalOfOptional(tree, state) ? describeMatch(tree) : Description.NO_MATCH; + } + + private static boolean isOptionalOfOptional(Tree tree, VisitorState state) { + Type optionalType = OPTIONAL.get(state); + Type type = ASTHelpers.getType(tree); + if (!ASTHelpers.isSubtype(type, optionalType, state)) { + return false; + } + + List typeArguments = type.getTypeArguments(); + return !typeArguments.isEmpty() + && ASTHelpers.isSubtype(Iterables.getOnlyElement(typeArguments), optionalType, state); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestingOptionals.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestingOptionals.java deleted file mode 100644 index fb4ed1303f..0000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NestingOptionals.java +++ /dev/null @@ -1,65 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import static com.google.errorprone.BugPattern.LinkType.NONE; -import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; -import static com.google.errorprone.matchers.method.MethodMatchers.anyMethod; - -import com.google.auto.service.AutoService; -import com.google.errorprone.BugPattern; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.util.ASTHelpers; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.util.TreeScanner; -import com.sun.tools.javac.code.Type; -import java.util.Optional; -import javax.annotation.Nullable; - -/** A {@link BugChecker} which flags any (embedded) nesting of {@link Optional Optionals}. */ -@AutoService(BugChecker.class) -@BugPattern( - summary = "Avoid creating nested optionals.", - linkType = NONE, - severity = WARNING, - tags = SIMPLIFICATION) -public final class NestingOptionals extends BugChecker implements MethodInvocationTreeMatcher { - private static final long serialVersionUID = 1L; - private static final String JAVA_OPTIONAL = "java.util.Optional"; - private static final Matcher ANY_OPTIONAL_METHOD = - anyMethod().onDescendantOf(JAVA_OPTIONAL); - - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!ANY_OPTIONAL_METHOD.matches(tree, state)) { - return Description.NO_MATCH; - } - - Boolean foundNestedOptional = tree.accept(new NestedOptionalDetector(), state); - if (foundNestedOptional == null || !foundNestedOptional) { - return Description.NO_MATCH; - } - - return buildDescription(tree).build(); - } - - private static class NestedOptionalDetector extends TreeScanner { - @Nullable - @Override - public Boolean visitMethodInvocation(MethodInvocationTree node, VisitorState state) { - if (ASTHelpers.getType(node).getTypeArguments().stream() - .anyMatch(NestedOptionalDetector::typeIsJavaOptional)) { - return true; - } - return super.visitMethodInvocation(node, state); - } - - private static boolean typeIsJavaOptional(Type type) { - return type.asElement().getQualifiedName().contentEquals(JAVA_OPTIONAL); - } - } -} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index d9395b822d..f35bd720a3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java @@ -311,7 +311,7 @@ Optional after( /** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */ abstract static class OptionalOrOtherOptional { @BeforeTemplate - @SuppressWarnings("NestingOptionals") + @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */) Optional before(Optional optional1, Optional optional2) { // XXX: Note that rewriting the first and third variant will change the code's behavior if // `optional2` has side-effects. diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestingOptionalsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java similarity index 54% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestingOptionalsTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java index e7331f1877..0d9a99b1ee 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestingOptionalsTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java @@ -3,9 +3,9 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class NestingOptionalsTest { +final class NestedOptionalsTest { private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(NestingOptionals.class, getClass()); + CompilationTestHelper.newInstance(NestedOptionals.class, getClass()); @Test void identification() { @@ -13,24 +13,25 @@ void identification() { .addSourceLines( "A.java", "import java.util.Optional;", + "import java.util.stream.Stream;", "", "class A {", " void m() {", + " Optional.empty();", + " Optional.of(1);", " // BUG: Diagnostic contains:", " Optional.of(Optional.empty());", " // BUG: Diagnostic contains:", - " Optional.of(1).map(Optional::of);", + " Optional.of(Optional.of(1));", + " Optional.ofNullable(null);", " // BUG: Diagnostic contains:", - " Optional.of(2).map(Optional::of).orElseThrow();", + " Optional.ofNullable((Optional) null);", + " Optional.of(\"foo\").map(String::length);", " // BUG: Diagnostic contains:", - " Optional.of(3).map(value -> Optional.empty());", + " Optional.of(\"foo\").map(Optional::of);", + " Stream.of(\"foo\").findFirst();", " // BUG: Diagnostic contains:", - " Optional.of(4).map(value -> Optional.empty()).orElseThrow();", - "", - " Optional.of(5).map(String::valueOf);", - " Optional.of(6).map(value -> value);", - " Optional.of(7).flatMap(Optional::of);", - " Optional.of(8).flatMap(value -> Optional.empty());", + " Stream.of(\"foo\").map(Optional::of).findFirst();", " }", "}") .doTest();