diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 0ff9ffc..0e843c1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - java: [ '17', '21' ] + java: [ '21' ] name: Unit tests Java ${{ matrix.Java }} steps: - uses: actions/checkout@v4 diff --git a/pom.xml b/pom.xml index 357cc1c..af1e45d 100644 --- a/pom.xml +++ b/pom.xml @@ -41,7 +41,7 @@ 8.2.0.36672 5.6.0.2578 - 2.39.0 + 2.44.0 0.12.7 0.1.29 0.23.0 @@ -56,6 +56,7 @@ 5.10.1 4.5.1 3.21.0 + 0.8.14 UTF-8 @@ -95,7 +96,7 @@ org.jacoco jacoco-maven-plugin - 0.8.11 + ${jacoco.version} prepare-agent diff --git a/sonar-erroraway-lib/src/main/java/com/github/erroraway/rules/ErrorAwayRulesMapping.java b/sonar-erroraway-lib/src/main/java/com/github/erroraway/rules/ErrorAwayRulesMapping.java index ffac2e1..2a1623a 100644 --- a/sonar-erroraway-lib/src/main/java/com/github/erroraway/rules/ErrorAwayRulesMapping.java +++ b/sonar-erroraway-lib/src/main/java/com/github/erroraway/rules/ErrorAwayRulesMapping.java @@ -34,7 +34,7 @@ public final class ErrorAwayRulesMapping { public static final String ERRORPRONE_SLF4J_REPOSITORY = "errorprone-slf4j"; public static final String PICNIC_REPOSITORY = "picnic-errorprone"; - public static final int ERRORPRONE_REPOSITORY_RULES_COUNT = 471; + public static final int ERRORPRONE_REPOSITORY_RULES_COUNT = 476; public static final int NULLAWAY_REPOSITORY_RULES_COUNT = 1; public static final int ERRORPRONE_SLF4J_REPOSITORY_RULES_COUNT = 8; public static final int PICNIC_REPOSITORY_RULES_COUNT = 45; diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/AndroidJdkLibsChecker.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/AndroidJdkLibsChecker.md deleted file mode 100644 index d6c1f13..0000000 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/AndroidJdkLibsChecker.md +++ /dev/null @@ -1,17 +0,0 @@ -Code that needs to be compatible with Android cannot use types or members that -only the latest or unreleased devices can handle - -## Suppression - -WARNING: We *strongly* recommend checking your code with Android Lint if -suppressing or disabling this check. - -The check can be suppressed in code that deliberately only targets newer Android -SDK versions. - -To suppress for a particular statement, method, or class, use -`@SuppressWarnings`: - -``` -@SuppressWarnings("AndroidJdkLibsChecker") // TODO(user): document suppression -``` diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/EqualsIncompatibleType.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/EqualsIncompatibleType.md index 6ff71b9..bc38438 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/EqualsIncompatibleType.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/EqualsIncompatibleType.md @@ -146,6 +146,6 @@ if (set.contains(hi)) { } ``` -[equalstester]: https://static.javadoc.io/com.google.guava/guava-testlib/19.0/com/google/common/testing/EqualsTester.html -[objeq]: https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals(java.lang.Object) -[av]: https://github.com/google/auto/blob/master/value/userguide/index.md +[equalstester]: https://www.javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/EqualsTester.html +[objeq]: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/lang/Object.html#equals(java.lang.Object) +[av]: https://github.com/google/auto/blob/main/value/userguide/index.md diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/FutureReturnValueIgnored.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/FutureReturnValueIgnored.md index 791d44f..5357519 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/FutureReturnValueIgnored.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/FutureReturnValueIgnored.md @@ -1,7 +1,7 @@ Methods that return `java.util.concurrent.Future` and its subclasses generally indicate errors by returning a future that eventually fails. -If you don’t check the return value of these methods, you will never find out if +If you don't check the return value of these methods, you will never find out if they threw an exception. Nested futures can also result in missed cancellation signals or suppressed diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/IncorrectMainMethod.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/IncorrectMainMethod.md index 2e3e228..534daa2 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/IncorrectMainMethod.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/IncorrectMainMethod.md @@ -1,5 +1,7 @@ -A `main` method must be `public`, `static`, and return `void` (see -[JLS §12.1.4]). +## Pre Java 25 + +Prior to Java 25, a `main` method must be `public`, `static`, and return `void` +(see [JLS §12.1.4]). For example, the following method is confusing, because it is an overload of a valid `main` method (it has the same name and signature), but is not a valid @@ -18,7 +20,25 @@ $ java T.java error: 'main' method is not declared 'public static' ``` -[JLS §12.1.4]: https://docs.oracle.com/javase/specs/jls/se11/html/jls-12.html#jls-12.1.4 +## Java 25 and later + +For Java 25 and later, a `main` method must return `void` (see [JLS §12.1.4]). +The `public` and `static` requirements have been dropped. + +For example, the following method is confusing, because it is an overload of a +valid `main` method (it has the same name and arguments), but does not return +`void`: + +```java +class Test { + public static int main(String[] args) { + System.err.println("hello world"); + return 0; + } +} +``` + +[JLS §12.1.4]: https://docs.oracle.com/javase/specs/jls/se25/html/jls-12.html#jls-12.1.4 TIP: If you're declaring a method that isn't intended to be used as the main method of your program, prefer to use a name other than `main`. It's confusing diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/InjectedConstructorAnnotations.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/InjectedConstructorAnnotations.md index 14e63b6..09012a6 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/InjectedConstructorAnnotations.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/InjectedConstructorAnnotations.md @@ -1,4 +1,4 @@ The constructor is annotated with @Inject(optional=true), or it is annotated with @Inject and a binding annotation. This will cause a Guice runtime error. -See [https://code.google.com/p/google-guice/wiki/InjectionPoints] for details. +For more information, see https://github.com/google/guice/wiki/InjectionPoints. diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/LoopOverCharArray.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/LoopOverCharArray.md index e0e8d64..02200d9 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/LoopOverCharArray.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/LoopOverCharArray.md @@ -6,8 +6,8 @@ That is, prefer this: ```java boolean isDigits(String string) { - for (int i = 0; i < s.length(); i++) { - char c = s.charAt(i); + for (int i = 0; i < string.length(); i++) { + char c = string.charAt(i); if (!Character.isDigit(c)) { return false; } diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SelfAssertion.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SelfAssertion.md index 2e805c1..c5a2e9a 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SelfAssertion.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SelfAssertion.md @@ -1,8 +1,8 @@ -If a test subject and the argument to `isEqualTo` are the same instance (e.g. -`assertThat(x).isEqualTo(x)`), then the assertion will always pass. Truth -implements `isEqualTo` using [`Objects#equal`] , which tests its arguments for -reference equality and returns true without calling `equals()` if both arguments -are the same instance. +When using Truth, if a test subject and the argument to `isEqualTo` are the same +instance (for example `assertThat(x).isEqualTo(x)`), then the assertion will +always pass. Truth implements `isEqualTo` using [`Objects#equal`] , which tests +its arguments for reference equality and returns true without calling `equals()` +if both arguments are the same instance. JUnit's `assertEquals` (and similar) methods are implemented in terms of `Object#equals`. However, this is not explicitly documented, so isn't a @@ -14,4 +14,8 @@ To test the implementation of an `equals` method, use [Guava's EqualsTester][javadoc], or explicitly call `equals` as part of the test. +In our experience, `assertThat(x).isEqualTo(x)` and similar are *more likely to +be typos* than assertions about an `equals` method. This alone is sufficient +motivation to choose a dedicated approach for testing `equals` implementations. + [javadoc]: https://static.javadoc.io/com.google.guava/guava-testlib/21.0/com/google/common/testing/EqualsTester.html diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SuppressWarningsWithoutExplanation.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SuppressWarningsWithoutExplanation.md index c7a2651..7505895 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SuppressWarningsWithoutExplanation.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SuppressWarningsWithoutExplanation.md @@ -1,5 +1,5 @@ -Suppressions for `unchecked` or `rawtypes` warnings should have an accompanying -comment to explain why the javac warning is safe to ignore. +`@SuppressWarnings` should have an accompanying comment to explain why the javac +warning is safe to ignore. Rather than just suppressing the warning: diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SystemConsoleNull.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SystemConsoleNull.md index 079e649..7e939e4 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SystemConsoleNull.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/SystemConsoleNull.md @@ -1,7 +1,7 @@ Null-checking `System.console()` is not a reliable way to detect if the console is connected to a terminal. -See +See JDK 22 [Release Note: JLine As The Default Console Provider](https://bugs.openjdk.org/browse/JDK-8309155): > `System.console()` now returns a `Console` object when the standard streams @@ -15,6 +15,18 @@ See > A new method `Console.isTerminal()` has been added to test if console is > connected to a terminal. +and JDK 25 release note +[Release Note: Default Console Implementation No Longer Based On JLine](https://bugs.openjdk.org/browse/JDK-8351576): + +> The default Console obtained via `System.console()` is no longer based on +> JLine. Since JDK 20, the JDK has included a JLine-based Console +> implementation, offering a richer user experience and better support for +> virtual terminal environments, such as IDEs. This implementation was initially +> opt-in via a system property in JDK 20 and JDK 21 and became the default in +> JDK 22. However, maintaining the JLine-based Console proved challenging. As a +> result, in JDK 25, it has reverted to being opt-in, as it was in JDK 20 and +> JDK 21. + To prepare for this change while remaining compatible with JDK versions prior to JDK 22, consider using reflection to call `Console#isTerminal` on JDK versions that support it: @@ -23,7 +35,7 @@ that support it: @SuppressWarnings("SystemConsoleNull") // https://errorprone.info/bugpattern/SystemConsoleNull private static boolean systemConsoleIsTerminal() { Console systemConsole = System.console(); - if (Runtime.version().feature() < 22) { + if (Runtime.version().feature() < 22 || systemConsole == null) { return systemConsole != null; } try { diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnsafeLocaleUsage.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnsafeLocaleUsage.md index 38be98d..db3fd1b 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnsafeLocaleUsage.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnsafeLocaleUsage.md @@ -4,26 +4,29 @@ examples of error prone issues below: #### Constructors The constructors don't validate the parameters at all, they just "trust" it -100%. +100%. \ +This is also true for the static method `Locale.of`, introduced in JDK 19. For example: -``` -Locale locale = new Locale("en_AU"); -toString() : "en_au" -getLanguage() : "en_au" -locale.getCountry : "" - -locale = new Locale("somethingBad#!34, too long, and clearly not a locale ID"); -toString() : "somethingbad#!34, too long, and clearly not a locale id" -getLanguage() : "somethingbad#!34, too long, and clearly not a locale id" -getCountry() : "" +```java +Locale locale = new Locale("en_AU"); // or Locale.of("en_AU") +locale.toString(); // "en_au" +locale.getLanguage(); // "en_au" +locale.getCountry(); // "" + +locale = new Locale("somethingBad#!34, long, and clearly not a locale ID"); +// or Locale.of("somethingBad#!34, long, and clearly not a locale ID") +locale.toString(); // "somethingbad#!34, long, and clearly not a locale id" +locale.getLanguage(); // "somethingbad#!34, long, and clearly not a locale id" +locale.getCountry(); // "" ``` As you can see, the full string is interpreted as language, and the country is empty. -For `new Locale("zh", "tw", "#Hant")` you get: +For `new Locale("zh", "tw", "#Hant")` and `Locale.of("zh", "tw", "#Hant")` you +get: ``` toString() : zh_TW_#Hant @@ -51,20 +54,70 @@ There's no reliable way of getting a correct result through a `Locale` constructor, so we should prefer using `Locale.forLanguageTag()` (and the IETF BCP 47 format) for correctness. -**Note:** You might see a `.replace("_", "-")` appended to a suggested fix for +**Note:** You might see a `.replace('_', '-')` appended to a suggested fix for the error prone checker for this bug pattern. This is sanitization measure to handle the fact that `Locale.forLanguageTag()` accepts the "minus form" of a tag (`en-US`) but not the "underscore form" (`en_US`). It will silently default to `Locale.ROOT` if the latter form is passed in. +**Note:** This error-prone rule cannot reliably fix constructors and static +method `Locale.of` with two or three parameters, because a proper fix requires +more context. + +If the initial code started with a `String` that was split at `'_'` or `'-'`, +just to be used for locale, the right fix is to use `toLanguageTag()`. + +```java +// Initial code +void someMethod(String localeId) { + String[] parts = localeId.split("_"); + Locale locale = switch (parts.size) { + case 1 -> new Locale(part[0]), // or Locale.of + case 2 -> new Locale(part[0], part[1]), // or Locale.of + case 3 -> new Locale(part[0], part[1], part[2]), // or Locale.of + } + // use the locale +} + +// Fixed code +void someMethod(String localeId) { + Locale locale = Locale.forLanguageTag.replace('_', '-'); + // use the locale +} +``` + +If the initial code started separate "pieces" (language, region, variant) the +right fix is to use a `Locale.Builder()`. + +```java +// Initial code +void someMethod(@NotNull String langId, String regionId) { + Locale locale (regionId == null) + ? new Locale(langId) // or Locale.of + : new Locale(langId, regionId); // or Locale.of + // use the locale +} + +// Fixed code +void someMethod(@NotNull String langId, String regionId) { + Locale.Builder builder = new Locale.Builder(); + builder.setLanguage(langId); + if (regionId == null) { + builder.setCountry(regionId); + } + Locale locale = builder.build(); + // use the locale +} +``` + #### toString() -This poses the inverse of the constructor problem +This poses the inverse of the constructor problem. -``` +```java Locale myLocale = Locale.forLanguageTag("zh-hant-tw") String myLocaleStr = myLocale.toString() // zh_TW_#Hant -Locale derivedLocale = ??? // Not clean way to get a correct locale from this string +Locale derivedLocale = ??? // Not clean way to get a correct locale from myLocaleStr ``` The `toString()` implementation for `Locale` isn't necessarily incorrect in diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/javadoc/NotJavadoc.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/javadoc/NotJavadoc.md index 13a7c6d..56f9e86 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/javadoc/NotJavadoc.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/javadoc/NotJavadoc.md @@ -2,6 +2,8 @@ This comment starts with `/**`, but isn't actually Javadoc. Javadoc comments [must precede a class, field, constructor, or method declaration](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#format). +Javadoc cannot be applied to classes or methods nested within a method, as those +elements never form part of an API. Using `/**` comments in locations where Javadoc is not supported is confusing and unnecessary. diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/nullness/NullNeedsCastForVarargs.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/nullness/NullNeedsCastForVarargs.md new file mode 100644 index 0000000..4d6f949 --- /dev/null +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/nullness/NullNeedsCastForVarargs.md @@ -0,0 +1,2 @@ +Note: The Truth methods covered by this check still accept a literal null in +Truth 1.4.5, but we expect to change that in a forthcoming version. diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/nullness/RedundantNullCheck.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/nullness/RedundantNullCheck.md new file mode 100644 index 0000000..b2d3c26 --- /dev/null +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/nullness/RedundantNullCheck.md @@ -0,0 +1,69 @@ +A null check (e.g., `x == null` or `x != null`) is redundant if it is performed +on an expression that is statically determined to be non-null according to +language semantics or nullness annotations. This check can optionally be +configured to flag redundant calls to `Objects.requireNonNull(x)` as well. + +Within a `@NullMarked` scope, types are non-null by default unless explicitly +annotated with `@Nullable`. Therefore, checking a variable or method return +value (that isn't `@Nullable`) for nullness is unnecessary, as it should never +be null. + +Example: + +```java +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class MyClass { + void process(String definitelyNonNull) { + // BUG: Diagnostic contains: RedundantNullCheck + if (definitelyNonNull == null) { + System.out.println("This will never happen"); + } + // ... + } + + String getString() { + return "hello"; + } + + @Nullable String getNullableString() { + return null; + } + + void anotherMethod() { + String s = getString(); + // BUG: Diagnostic contains: RedundantNullCheck + if (s == null) { + // s is known to be non-null because getString() is not @Nullable + // and we are in a @NullMarked scope. + System.out.println("Redundant check"); + } + + String nullableStr = getNullableString(); + if (nullableStr == null) { // This check is NOT redundant + System.out.println("Nullable string might be null"); + } + } +} +``` + +This check helps to clean up code and reduce clutter by removing unnecessary +null checks, making the code easier to read and maintain. It also reinforces the +contract provided by `@NullMarked` and `@Nullable` annotations. + +## Configuration + +By default, this check only flags redundant null checks using `== null` and `!= +null`. To also flag redundant calls to `Objects.requireNonNull(x)`, enable it +with the following flag: + +``` +-XepOpt:RedundantNullCheck:CheckRequireNonNull=true +``` + +## Suppression + +Suppress false positives by adding the suppression annotation +`@SuppressWarnings("RedundantNullCheck")` to the enclosing element.