Merge branch 'master' into sschroevers/inline-variable-return-statements

.github/ISSUE_TEMPLATE/bug_report.md

@@ -42,9 +42,9 @@ Please replace this sentence with log output, if applicable.
 <!-- Please complete the following information: -->
 
 - Operating system (e.g. MacOS Monterey).
-- Java version (i.e. `java --version`, e.g. `17.0.3`).
-- Error Prone version (e.g. `2.15.0`).
-- Error Prone Support version (e.g. `0.3.0`).
+- Java version (i.e. `java --version`, e.g. `17.0.6`).
+- Error Prone version (e.g. `2.18.0`).
+- Error Prone Support version (e.g. `0.8.0`).
 
 ### Additional context
 

.github/workflows/build.yaml

@@ -10,16 +10,16 @@ jobs:
     strategy:
       matrix:
         os: [ ubuntu-22.04 ]
-        jdk: [ 11.0.16, 17.0.4, 19 ]
+        jdk: [ 11.0.18, 17.0.6, 19.0.2 ]
         distribution: [ temurin ]
         experimental: [ false ]
         include:
           - os: macos-12
-            jdk: 17.0.4
+            jdk: 17.0.6
             distribution: temurin
             experimental: false
           - os: windows-2022
-            jdk: 17.0.4
+            jdk: 17.0.6
             distribution: temurin
             experimental: false
           - os: ubuntu-22.04

.github/workflows/pitest-analyze-pr.yml

@@ -18,7 +18,7 @@ jobs:
       - name: Set up JDK
         uses: actions/[email protected]
         with:
-          java-version: 17.0.4
+          java-version: 17.0.6
           distribution: temurin
           cache: maven
       - name: Run Pitest

.github/workflows/pitest-update-pr.yml

@@ -22,7 +22,7 @@ jobs:
       - name: Set up JDK
         uses: actions/[email protected]
         with:
-          java-version: 17.0.4
+          java-version: 17.0.6
           distribution: temurin
           cache: maven
       - name: Download Pitest analysis artifact

documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternExtractorTest.java

@@ -14,7 +14,6 @@
 import com.google.errorprone.matchers.Description;
 import com.sun.source.tree.ClassTree;
 import java.io.IOException;
-import java.nio.file.Files;
 import java.nio.file.Path;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -120,7 +119,8 @@ void bugPatternAnnotationIsAbsent() {
 
   private static void verifyFileMatchesResource(
       Path outputDirectory, String fileName, String resourceName) throws IOException {
-    assertThat(Files.readString(outputDirectory.resolve(fileName)))
+    assertThat(outputDirectory.resolve(fileName))
+        .content(UTF_8)
         .isEqualToIgnoringWhitespace(getResource(resourceName));
   }
 

error-prone-contrib/pom.xml

@@ -37,7 +37,7 @@
         <dependency>
             <groupId>${groupId.error-prone}</groupId>
             <artifactId>error_prone_test_helpers</artifactId>
-            <scope>test</scope>
+            <scope>provided</scope>
         </dependency>
         <dependency>
             <groupId>${project.groupId}</groupId>

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java

@@ -74,9 +74,13 @@ public final class RequestMappingAnnotation extends BugChecker implements Method
                   isSameType("java.time.ZoneId"),
                   isSameType("java.util.Locale"),
                   isSameType("java.util.TimeZone"),
+                  isSameType("jakarta.servlet.http.HttpServletRequest"),
+                  isSameType("jakarta.servlet.http.HttpServletResponse"),
                   isSameType("javax.servlet.http.HttpServletRequest"),
                   isSameType("javax.servlet.http.HttpServletResponse"),
                   isSameType("org.springframework.http.HttpMethod"),
+                  isSameType("org.springframework.ui.Model"),
+                  isSameType("org.springframework.validation.BindingResult"),
                   isSameType("org.springframework.web.context.request.NativeWebRequest"),
                   isSameType("org.springframework.web.context.request.WebRequest"),
                   isSameType("org.springframework.web.server.ServerWebExchange"),

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJStringRules.java

@@ -1,11 +1,16 @@
 package tech.picnic.errorprone.refasterrules;
 
 import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import org.assertj.core.api.AbstractAssert;
 import org.assertj.core.api.AbstractStringAssert;
 import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
@@ -89,4 +94,30 @@ static final class AssertThatDoesNotMatch {
       return assertThat(string).doesNotMatch(regex);
     }
   }
+
+  static final class AssertThatPathContent {
+    @BeforeTemplate
+    AbstractStringAssert<?> before(Path path, Charset charset) throws IOException {
+      return assertThat(Files.readString(path, charset));
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    AbstractStringAssert<?> after(Path path, Charset charset) {
+      return assertThat(path).content(charset);
+    }
+  }
+
+  static final class AssertThatPathContentUtf8 {
+    @BeforeTemplate
+    AbstractStringAssert<?> before(Path path) throws IOException {
+      return assertThat(Files.readString(path));
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    AbstractStringAssert<?> after(Path path) {
+      return assertThat(path).content(UTF_8);
+    }
+  }
 }

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

@@ -0,0 +1,53 @@
+package tech.picnic.errorprone.refasterrules;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChooser;
+import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers;
+import com.google.errorprone.refaster.Refaster;
+import com.google.errorprone.refaster.annotation.AfterTemplate;
+import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+
+/** Refaster rules related to {@link com.google.errorprone.bugpatterns.BugChecker} classes. */
+@OnlineDocumentation
+final class BugCheckerRules {
+  private BugCheckerRules() {}
+
+  /**
+   * Avoid calling {@link BugCheckerRefactoringTestHelper#setFixChooser(FixChooser)} or {@link
+   * BugCheckerRefactoringTestHelper#setImportOrder(String)} with their respective default values.
+   */
+  static final class BugCheckerRefactoringTestHelperIdentity {
+    @BeforeTemplate
+    BugCheckerRefactoringTestHelper before(BugCheckerRefactoringTestHelper helper) {
+      return Refaster.anyOf(
+          helper.setFixChooser(FixChoosers.FIRST), helper.setImportOrder("static-first"));
+    }
+
+    @AfterTemplate
+    BugCheckerRefactoringTestHelper after(BugCheckerRefactoringTestHelper helper) {
+      return helper;
+    }
+  }
+
+  /**
+   * Prefer {@link BugCheckerRefactoringTestHelper.ExpectOutput#expectUnchanged()} over repeating
+   * the input.
+   */
+  // XXX: This rule assumes that the full source code is specified as a single string, e.g. using a
+  // text block. Support for multi-line source code input would require a `BugChecker`
+  // implementation instead.
+  static final class BugCheckerRefactoringTestHelperAddInputLinesExpectUnchanged {
+    @BeforeTemplate
+    BugCheckerRefactoringTestHelper before(
+        BugCheckerRefactoringTestHelper helper, String path, String source) {
+      return helper.addInputLines(path, source).addOutputLines(path, source);
+    }
+
+    @AfterTemplate
+    BugCheckerRefactoringTestHelper after(
+        BugCheckerRefactoringTestHelper helper, String path, String source) {
+      return helper.addInputLines(path, source).expectUnchanged();
+    }
+  }
+}

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java

@@ -21,11 +21,14 @@
 final class NullRules {
   private NullRules() {}
 
-  /** Prefer the {@code ==} operator over {@link Objects#isNull(Object)}. */
+  /**
+   * Prefer the {@code ==} operator (with {@code null} as the second operand) over {@link
+   * Objects#isNull(Object)}.
+   */
   static final class IsNull {
     @BeforeTemplate
     boolean before(@Nullable Object object) {
-      return Objects.isNull(object);
+      return Refaster.anyOf(null == object, Objects.isNull(object));
     }
 
     @AfterTemplate
@@ -34,11 +37,14 @@ boolean after(@Nullable Object object) {
     }
   }
 
-  /** Prefer the {@code !=} operator over {@link Objects#nonNull(Object)}. */
+  /**
+   * Prefer the {@code !=} operator (with {@code null} as the second operand) over {@link
+   * Objects#nonNull(Object)}.
+   */
   static final class IsNotNull {
     @BeforeTemplate
     boolean before(@Nullable Object object) {
-      return Objects.nonNull(object);
+      return Refaster.anyOf(null != object, Objects.nonNull(object));
     }
 
     @AfterTemplate

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PreconditionsRules.java

@@ -6,11 +6,13 @@
 import static com.google.common.base.Preconditions.checkPositionIndex;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
+import static java.util.Objects.requireNonNull;
 
 import com.google.common.base.Preconditions;
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
+import java.util.Objects;
 import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
 
 /** Refaster templates related to statements dealing with {@link Preconditions}. */
@@ -72,8 +74,22 @@ void after(int index, int size, String message) {
     }
   }
 
-  /** Prefer {@link Preconditions#checkNotNull(Object)} over more verbose alternatives. */
-  static final class CheckNotNull<T> {
+  /** Prefer {@link Objects#requireNonNull(Object)} over non-JDK alternatives. */
+  static final class RequireNonNull<T> {
+    @BeforeTemplate
+    T before(T object) {
+      return checkNotNull(object);
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    T after(T object) {
+      return requireNonNull(object);
+    }
+  }
+
+  /** Prefer {@link Objects#requireNonNull(Object)} over more verbose alternatives. */
+  static final class RequireNonNullStatement<T> {
     @BeforeTemplate
     void before(T object) {
       if (object == null) {
@@ -84,12 +100,26 @@ void before(T object) {
     @AfterTemplate
     @UseImportPolicy(STATIC_IMPORT_ALWAYS)
     void after(T object) {
-      checkNotNull(object);
+      requireNonNull(object);
+    }
+  }
+
+  /** Prefer {@link Objects#requireNonNull(Object, String)} over non-JDK alternatives. */
+  static final class RequireNonNullWithMessage<T> {
+    @BeforeTemplate
+    T before(T object, String message) {
+      return checkNotNull(object, message);
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    T after(T object, String message) {
+      return requireNonNull(object, message);
     }
   }
 
-  /** Prefer {@link Preconditions#checkNotNull(Object, Object)} over more verbose alternatives. */
-  static final class CheckNotNullWithMessage<T> {
+  /** Prefer {@link Objects#requireNonNull(Object, String)} over more verbose alternatives. */
+  static final class RequireNonNullWithMessageStatement<T> {
     @BeforeTemplate
     void before(T object, String message) {
       if (object == null) {
@@ -100,7 +130,7 @@ void before(T object, String message) {
     @AfterTemplate
     @UseImportPolicy(STATIC_IMPORT_ALWAYS)
     void after(T object, String message) {
-      checkNotNull(object, message);
+      requireNonNull(object, message);
     }
   }
 

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

@@ -1204,12 +1204,14 @@ StepVerifier.FirstStep<? extends T> after(Flux<T> flux) {
     }
   }
 
-  /** Don't unnecessarily call {@link StepVerifier.Step#expectNext(Object[])}. */
-  static final class StepVerifierStepExpectNextEmpty<T> {
+  /** Don't unnecessarily have {@link StepVerifier.Step} expect no elements. */
+  // XXX: Given an `IsEmpty` matcher that identifies a wide range of guaranteed-empty `Iterable`
+  // expressions, consider also simplifying `step.expectNextSequence(someEmptyIterable)`.
+  static final class StepVerifierStepIdentity<T> {
     @BeforeTemplate
     @SuppressWarnings("unchecked")
     StepVerifier.Step<T> before(StepVerifier.Step<T> step) {
-      return step.expectNext();
+      return Refaster.anyOf(step.expectNext(), step.expectNextCount(0));
     }
 
     @AfterTemplate

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

@@ -67,7 +67,6 @@ void identification() {
   @Test
   void replacementFirstSuggestedFix() {
     BugCheckerRefactoringTestHelper.newInstance(FluxFlatMapUsage.class, getClass())
-        .setFixChooser(FixChoosers.FIRST)
         .addInputLines(
             "A.java",
             "import reactor.core.publisher.Flux;",

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

@@ -180,7 +180,6 @@ void identification() {
   @Test
   void replacementFirstSuggestedFix() {
     BugCheckerRefactoringTestHelper.newInstance(IdentityConversion.class, getClass())
-        .setFixChooser(FixChoosers.FIRST)
         .addInputLines(
             "A.java",
             "import static com.google.errorprone.matchers.Matchers.staticMethod;",

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

@@ -16,6 +16,8 @@ void identification() {
             "import javax.servlet.http.HttpServletRequest;",
             "import javax.servlet.http.HttpServletResponse;",
             "import org.springframework.http.HttpMethod;",
+            "import org.springframework.ui.Model;",
+            "import org.springframework.validation.BindingResult;",
             "import org.springframework.web.bind.annotation.DeleteMapping;",
             "import org.springframework.web.bind.annotation.GetMapping;",
             "import org.springframework.web.bind.annotation.PatchMapping;",
@@ -82,6 +84,12 @@ void identification() {
             "  A properHttpMethod(HttpMethod method);",
             "",
             "  @RequestMapping",
+            "  A properModel(Model model);",
+            "",
+            "  @RequestMapping",
+            "  A properBindingResult(BindingResult result);",
+            "",
+            "  @RequestMapping",
             "  A properNativeWebRequest(NativeWebRequest request);",
             "",
             "  @RequestMapping",

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

@@ -48,7 +48,6 @@ void identification() {
   @Test
   void replacementFirstSuggestedFix() {
     BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass())
-        .setFixChooser(FixChoosers.FIRST)
         .addInputLines(
             "A.java",
             "class A {",