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.

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

Introduce NestedOptionals check #202

Merged
merged 5 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ private static Optional<SuggestedFix.Builder> constructMethodRef(
.flatMap(statements -> constructMethodRef(lambdaExpr, statements.get(0)));
}

@SuppressWarnings("NestedOptionals")
private static Optional<SuggestedFix.Builder> constructMethodRef(
LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) {
return matchArguments(lambdaExpr, subTree)
Expand Down Expand Up @@ -156,6 +157,7 @@ private static Optional<SuggestedFix.Builder> constructMethodRef(
return constructFix(lambdaExpr, lhsType.tsym, subTree.getIdentifier());
}

@SuppressWarnings("NestedOptionals")
private static Optional<Optional<Name>> matchArguments(
LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) {
ImmutableList<Name> expectedArguments = getVariables(lambdaExpr);
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

We are not actually doing the simplification, we are flagging fragile code 😄.

Suggested change
tags = SIMPLIFICATION)
tags = FRAGILE_CODE)

Right?

public final class NestedOptionals extends BugChecker implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Supplier<Type> 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<Type> typeArguments = type.getTypeArguments();
return !typeArguments.isEmpty()
&& ASTHelpers.isSubtype(Iterables.getOnlyElement(typeArguments), optionalType, state);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ Optional<R> after(
/** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */
abstract static class OptionalOrOtherOptional<T> {
@BeforeTemplate
@SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)
Optional<T> before(Optional<T> optional1, Optional<T> optional2) {
// XXX: Note that rewriting the first and third variant will change the code's behavior if
// `optional2` has side-effects.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
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));",
Copy link
Member

Choose a reason for hiding this comment

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

For readability we could add an empty line here, below the two ofNullable examples and before the Stream.of examples. Not a big win though.

We do this in some other tests as well.

" 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();
}
}