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 48659799e1..3a7c1de49c 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,8 @@ private static Optional constructMethodRef( .flatMap(statements -> constructMethodRef(lambdaExpr, statements.get(0))); } + // XXX: Replace nested `Optional` usage. + @SuppressWarnings("NestedOptionals") private static Optional constructMethodRef( LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) { return matchArguments(lambdaExpr, subTree) @@ -156,6 +158,8 @@ private static Optional constructMethodRef( return constructFix(lambdaExpr, lhsType.tsym, subTree.getIdentifier()); } + // XXX: Refactor or replace inner `Optional` with a custom type. + @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..a10ae529e4 --- /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.FRAGILE_CODE; + +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 = FRAGILE_CODE) +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/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index f84351a1e8..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,6 +311,7 @@ Optional after( /** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */ abstract static class OptionalOrOtherOptional { @BeforeTemplate + @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/NestedOptionalsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java new file mode 100644 index 0000000000..d5154cf554 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NestedOptionalsTest.java @@ -0,0 +1,42 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class NestedOptionalsTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(NestedOptionals.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .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(Optional.of(1));", + "", + " Optional.ofNullable(null);", + " // BUG: Diagnostic contains:", + " Optional.ofNullable((Optional) null);", + "", + " Optional.of(\"foo\").map(String::length);", + " // BUG: Diagnostic contains:", + " Optional.of(\"foo\").map(Optional::of);", + "", + " Stream.of(\"foo\").findFirst();", + " // BUG: Diagnostic contains:", + " Stream.of(\"foo\").map(Optional::of).findFirst();", + " }", + "}") + .doTest(); + } +}