Debugging this state...

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java

@@ -5,7 +5,9 @@ import com.google.errorprone.refaster.ImportPolicy;
 import com.google.errorprone.refaster.Refaster;
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import com.google.errorprone.refaster.annotation.Matches;
 import com.google.errorprone.refaster.annotation.MayOptionallyUse;
+import com.google.errorprone.refaster.annotation.NotMatches;
 import com.google.errorprone.refaster.annotation.Placeholder;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
 import java.util.Iterator;
@@ -15,6 +17,8 @@ import java.util.function.Predicate;
 import java.util.function.Supplier;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
+import tech.picnic.errorprone.refaster.util.IsNonNull;
+import tech.picnic.errorprone.refaster.util.IsNullable;
 
 /** Refaster templates related to expressions dealing with {@link Optional}s. */
 final class OptionalTemplates {
@@ -109,7 +113,10 @@ final class OptionalTemplates {
     }
   }
 
-  /** Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator. */
+  /**
+   * Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator while matching
+   * non-null expressions.
+   */
   // XXX: This rule may introduce a compilation error: the `test` expression may reference a
   // non-effectively final variable, which is not allowed in the replacement lambda expression.
   // Maybe our RefasterCheck should test `compilesWithFix`?
@@ -118,34 +125,79 @@ final class OptionalTemplates {
     abstract boolean test(T value);
 
     @BeforeTemplate
-    Optional<T> before(T input) {
+    Optional<T> before(@NotMatches(IsNullable.class) T input) {
       return test(input) ? Optional.of(input) : Optional.empty();
     }
 
     @AfterTemplate
     Optional<T> after(T input) {
-      return Refaster.emitCommentBefore(
-          "Or Optional.ofNullable (can't auto-infer).", Optional.of(input).filter(v -> test(v)));
+      return Optional.of(input).filter(v -> test(v));
     }
   }
 
-  /** Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator. */
+  /**
+   * Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator and only match
+   * nullable expressions.
+   */
   // XXX: This rule may introduce a compilation error: the `test` expression may reference a
   // non-effectively final variable, which is not allowed in the replacement lambda expression.
   // Maybe our RefasterCheck should test `compilesWithFix`?
+  abstract static class OptionalOfNullableFilterPositive<T> {
+    @Placeholder
+    abstract boolean test(T value);
+
+    @BeforeTemplate
+    Optional<T> before(@Matches(IsNullable.class) T input) {
+      return test(input) ? Optional.of(input) : Optional.empty();
+    }
+
+    @AfterTemplate
+    Optional<T> after(T input) {
+      return Optional.ofNullable(input).filter(v -> test(v));
+    }
+  }
+
+  /**
+   * Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator and only matching
+   * non-null expressions.
+   */
+  //  // XXX: This rule may introduce a compilation error: the `test` expression may reference a
+  //  // non-effectively final variable, which is not allowed in the replacement lambda expression.
+  //  // Maybe our RefasterCheck should test `compilesWithFix`?
   abstract static class TernaryOperatorOptionalNegativeFiltering<T> {
     @Placeholder
     abstract boolean test(T value);
 
     @BeforeTemplate
-    Optional<T> before(T input) {
+    Optional<T> before(@Matches(IsNonNull.class) T input) {
       return test(input) ? Optional.empty() : Optional.of(input);
     }
 
     @AfterTemplate
     Optional<T> after(T input) {
-      return Refaster.emitCommentBefore(
-          "Or Optional.ofNullable (can't auto-infer).", Optional.of(input).filter(v -> !test(v)));
+      return Optional.of(input).filter(v -> !test(v));
+    }
+  }
+
+  /**
+   * Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator and only matching
+   * nullable expressions.
+   */
+  // XXX: This rule may introduce a compilation error: the `test` expression may reference a
+  // non-effectively final variable, which is not allowed in the replacement lambda expression.
+  // Maybe our RefasterCheck should test `compilesWithFix`?
+  abstract static class OptionalOfNullableFilterNegative<T> {
+    @Placeholder
+    abstract boolean test(T value);
+
+    @BeforeTemplate
+    Optional<T> before(@Matches(IsNullable.class) T input) {
+      return test(input) ? Optional.empty() : Optional.of(input);
+    }
+
+    @AfterTemplate
+    Optional<T> after(T input) {
+      return Optional.ofNullable(input).filter(v -> !test(v));
     }
   }
 

error-prone-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/OptionalTemplatesTestInput.java

@@ -5,6 +5,8 @@ import com.google.common.collect.Streams;
 import java.util.Optional;
 import java.util.function.Function;
 import java.util.stream.Stream;
+import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 final class OptionalTemplatesTest implements RefasterTemplateTestCase {
   @Override
@@ -12,48 +14,65 @@ final class OptionalTemplatesTest implements RefasterTemplateTestCase {
     return ImmutableSet.of(Streams.class);
   }
 
-  ImmutableSet<Optional<String>> testOptionalOfNullable() {
+//  ImmutableSet<Optional<String>> testOptionalOfNullable() {
+//    return ImmutableSet.of(
+//        toString() == null ? Optional.empty() : Optional.of(toString()),
+//        toString() != null ? Optional.of(toString()) : Optional.empty());
+//  }
+//
+//  ImmutableSet<Boolean> testOptionalIsEmpty() {
+//    return ImmutableSet.of(!Optional.empty().isPresent(), !Optional.of("foo").isPresent());
+//  }
+//
+//  ImmutableSet<Boolean> testOptionalIsPresent() {
+//    return ImmutableSet.of(!Optional.empty().isEmpty(), !Optional.of("foo").isEmpty());
+//  }
+//
+//  String testOptionalOrElseThrow() {
+//    return Optional.of("foo").get();
+//  }
+//
+//  Function<Optional<Integer>, Integer> testOptionalOrElseThrowMethodReference() {
+//    return Optional::get;
+//  }
+//
+//  ImmutableSet<Optional<String>> testOptionalFirstIteratorElement() {
+//    return ImmutableSet.of(
+//        ImmutableSet.of("foo").iterator().hasNext()
+//            ? Optional.of(ImmutableSet.of("foo").iterator().next())
+//            : Optional.empty(),
+//        !ImmutableSet.of("foo").iterator().hasNext()
+//            ? Optional.empty()
+//            : Optional.of(ImmutableSet.of("foo").iterator().next()));
+//  }
+//
+//  ImmutableSet<Optional<String>> testTernaryOperatorOptionalPositiveFiltering() {
+//    return ImmutableSet.of(
+//        "foo".length() > 5 ? Optional.of("foo") : Optional.empty(),
+//        !"bar".contains("baz") ? Optional.of("bar") : Optional.empty());
+//  }
+
+  @Nullable String nullableObj;
+  @NonNull Object nonnullObj;
+
+  ImmutableSet<Optional<String>> testOptionalOfNullableFilterPositive(
+      @Nullable String nullableString) {
     return ImmutableSet.of(
-        toString() == null ? Optional.empty() : Optional.of(toString()),
-        toString() != null ? Optional.of(toString()) : Optional.empty());
+        nullableObj.length() > 5 ? Optional.of(nullableObj) : Optional.empty(),
+        !nullableString.contains("baz") ? Optional.of(nullableString) : Optional.empty());
   }
 
-  ImmutableSet<Boolean> testOptionalIsEmpty() {
-    return ImmutableSet.of(!Optional.empty().isPresent(), !Optional.of("foo").isPresent());
-  }
-
-  ImmutableSet<Boolean> testOptionalIsPresent() {
-    return ImmutableSet.of(!Optional.empty().isEmpty(), !Optional.of("foo").isEmpty());
-  }
-
-  String testOptionalOrElseThrow() {
-    return Optional.of("foo").get();
-  }
-
-  Function<Optional<Integer>, Integer> testOptionalOrElseThrowMethodReference() {
-    return Optional::get;
-  }
-
-  ImmutableSet<Optional<String>> testOptionalFirstIteratorElement() {
+  ImmutableSet<Optional<String>> testTernaryOperatorOptionalNegativeFiltering(String testy) {
     return ImmutableSet.of(
-        ImmutableSet.of("foo").iterator().hasNext()
-            ? Optional.of(ImmutableSet.of("foo").iterator().next())
-            : Optional.empty(),
-        !ImmutableSet.of("foo").iterator().hasNext()
-            ? Optional.empty()
-            : Optional.of(ImmutableSet.of("foo").iterator().next()));
+            testy.length() > 5 ? Optional.empty() : Optional.of(testy),
+        !testy.contains("baz") ? Optional.empty() : Optional.of(testy));
   }
 
-  ImmutableSet<Optional<String>> testTernaryOperatorOptionalPositiveFiltering() {
+  ImmutableSet<Optional<String>> testOptionalOfNullableFilterNegative(
+      @Nullable String nullableString) {
     return ImmutableSet.of(
-        "foo".length() > 5 ? Optional.of("foo") : Optional.empty(),
-        !"bar".contains("baz") ? Optional.of("bar") : Optional.empty());
-  }
-
-  ImmutableSet<Optional<String>> testTernaryOperatorOptionalNegativeFiltering() {
-    return ImmutableSet.of(
-        "foo".length() > 5 ? Optional.empty() : Optional.of("foo"),
-        !"bar".contains("baz") ? Optional.empty() : Optional.of("bar"));
+        nullableString.length() > 5 ? Optional.empty() : Optional.of(nullableString),
+        !nullableString.contains("baz") ? Optional.empty() : Optional.of(nullableString));
   }
 
   ImmutableSet<Boolean> testMapOptionalToBoolean() {

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsNonNull.java

@@ -0,0 +1,18 @@
+package tech.picnic.errorprone.refaster.util;
+
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.NullnessMatcher;
+import com.sun.source.tree.ExpressionTree;
+
+/** A matcher of nullable expressions, for use with Refaster's {@code @Matches} annotation. */
+public final class IsNonNull implements Matcher<ExpressionTree> {
+  private static final long serialVersionUID = 1L;
+
+  @Override
+  public boolean matches(ExpressionTree tree, VisitorState state) {
+    NullnessMatcher nullnessMatcher = new NullnessMatcher(Nullness.NONNULL);
+    return nullnessMatcher.matches(tree, state);
+  }
+}

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsNullable.java

@@ -0,0 +1,18 @@
+package tech.picnic.errorprone.refaster.util;
+
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.NullnessMatcher;
+import com.sun.source.tree.ExpressionTree;
+
+/** A matcher of nullable expressions, for use with Refaster's {@code @Matches} annotation. */
+public final class IsNullable implements Matcher<ExpressionTree> {
+  private static final long serialVersionUID = 1L;
+
+  @Override
+  public boolean matches(ExpressionTree tree, VisitorState state) {
+    NullnessMatcher nullnessMatcher = new NullnessMatcher(Nullness.NULLABLE);
+    return nullnessMatcher.matches(tree, state);
+  }
+}

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsNullableTest.java

@@ -0,0 +1,83 @@
+package tech.picnic.errorprone.refaster.util;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.CompilationTestHelper;
+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.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.VariableTree;
+import org.junit.jupiter.api.Test;
+
+final class IsNullableTest {
+  private final CompilationTestHelper compilationHelper =
+      CompilationTestHelper.newInstance(TestChecker.class, getClass());
+
+  @Test
+  void identification() {
+    compilationHelper
+        .addSourceLines(
+            "IdentityTest.java",
+            "package pkg;",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "import org.checkerframework.checker.nullness.qual.NonNull;",
+            "public class IdentityTest {",
+            "  private void m(@Nullable String nullableString) {",
+            "    // BUG: Diagnostic contains:",
+            "    String s = nullableString;",
+            "    int i = nullableString.length();",
+            "  }",
+            "",
+            "  public static <T> T foo(T t) {",
+            "    return t;",
+            "  }",
+            "",
+            "  @Nullable Object nullableObj;",
+            "  @NonNull Object nonnullObj;",
+            "  <T> T id(T t) { return t; }",
+            "  void id_tests() {",
+            "    // BUG: Diagnostic contains:",
+            "    id(nullableObj);",
+            "    id(nonnullObj);",
+            "    // BUG: Diagnostic contains:",
+            "    foo(id(nullableObj));",
+            "    foo(id(nonnullObj));",
+            "  }",
+            "  void literal_tests() {",
+            "    foo(id(null));",
+            "    foo(id(this));",
+            "    foo(id(5));",
+            "    foo(id(\"hello\"));",
+            "    // BUG: Diagnostic contains:",
+            "    foo(id(nullableObj));",
+            "    foo(id(nonnullObj));",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  /** A {@link BugChecker} which simply delegates to {@link IsNullable}. */
+  @BugPattern(name = "TestChecker", summary = "Flags non-null expressions", severity = ERROR)
+  public static final class TestChecker extends BugChecker
+      implements BugChecker.VariableTreeMatcher, MethodInvocationTreeMatcher {
+    private static final long serialVersionUID = 1L;
+
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+      return new IsNullable().matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH;
+    }
+
+    @Override
+    public Description matchVariable(VariableTree tree, VisitorState state) {
+      if (tree.getInitializer() == null) {
+        return Description.NO_MATCH;
+      }
+      return new IsNullable().matches(tree.getInitializer(), state)
+          ? describeMatch(tree)
+          : Description.NO_MATCH;
+    }
+  }
+}