diff --git a/pom.xml b/pom.xml index 0f2f352..7eecf3e 100644 --- a/pom.xml +++ b/pom.xml @@ -41,7 +41,7 @@ 8.2.0.36672 5.6.0.2578 - 2.38.0 + 2.39.0 0.12.6 0.1.28 0.23.0 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 16caa45..ffac2e1 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 = 468; + public static final int ERRORPRONE_REPOSITORY_RULES_COUNT = 471; 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 index 2cd14f1..d6c1f13 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/AndroidJdkLibsChecker.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/AndroidJdkLibsChecker.md @@ -3,7 +3,7 @@ only the latest or unreleased devices can handle ## Suppression -WARNING: We _strongly_ recommend checking your code with Android Lint if +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 diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/ExpensiveLenientFormatString.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/ExpensiveLenientFormatString.md new file mode 100644 index 0000000..d5ce532 --- /dev/null +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/ExpensiveLenientFormatString.md @@ -0,0 +1,16 @@ +Lenient format strings, such as those accepted by `Preconditions`, are often +constructed lazily. The message is rarely needed, so it should either be cheap +to construct or constructed only when needed. This check ensures that these +messages are not constructed using expensive methods that are evaluated eagerly. + +Prefer this: + +```java +checkNotNull(foo, "hello %s", name); +``` + +instead of this: + +```java +checkNotNull(foo, String.format("hello %s", name)); +``` diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MathAbsoluteNegative.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MathAbsoluteNegative.md index 46ea31f..47dce67 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MathAbsoluteNegative.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MathAbsoluteNegative.md @@ -28,5 +28,7 @@ or map negative numbers onto the non-negative range: ```java long lng = r.nextLong(); -lng = (lng == Long.MIN_VALUE) ? 0 : Math.abs(lng); + +long bestForHashCodes = lng & Long.MAX_VALUE; +long bestForMath = LongMath.saturatedAbs(lng); ``` diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MissingCasesInEnumSwitch.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MissingCasesInEnumSwitch.md index a2667e4..f4d97e0 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MissingCasesInEnumSwitch.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/MissingCasesInEnumSwitch.md @@ -29,3 +29,41 @@ switch statement on an enum type to either handle all values of the enum, or have a default statement group. [style]: https://google.github.io/styleguide/javaguide.html#s4.8.4.3-switch-default + +## Library skew + +If libraries are compiled against different versions of the same enum it's +possible for the switch statement to encounter an enum value despite it +otherwise being thought to be exhaustive. If there is no default branch code +execution will simply fall out of the switch statement. + +Since developers may have assumed this to be impossible, it may be helpful to +add a default branch when library skew is a concern, however, you may not want +to give up checking to ensure that all cases are handled. Therefore if a default +branch exists with a comment containing "skew", the default will not be +considered for exhaustiveness. For example: + +```java +enum TrafficLightColour { RED, GREEN, YELLOW } + +void approachIntersection(TrafficLightColour state) { + switch (state) { + case GREEN: + proceed(); + break; + case YELLOW: + case RED: + stop(); + break; + default: // In case of skew we may get an unknown value, always stop. + stop(); + break; + } +} +``` + +In this case the default branch is providing runtime safety for unknown enum +values while also still enforcing that all known enum values are handled. + +Note: The [UnnecessaryDefaultInEnumSwitch](UnnecessaryDefaultInEnumSwitch.md) +check will not classify the default as unnecessary if it has the "skew" comment. diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/OperatorPrecedence.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/OperatorPrecedence.md index 0a92d0c..d34d912 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/OperatorPrecedence.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/OperatorPrecedence.md @@ -14,15 +14,15 @@ misinterpreted. For example, consider this: ```java -boolean d = (a && b) || c;", -boolean e = (a || b) ? c : d;", -int z = (x + y) << 2;", +boolean d = (a && b) || c; +boolean e = (a || b) ? c : d; +int z = (x + y) << 2; ``` Instead of this: ```java -boolean r = a && b || c;", -boolean e = a || b ? c : d;", -int z = x + y << 2;", +boolean r = a && b || c; +boolean e = a || b ? c : d; +int z = x + y << 2; ``` diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/PreconditionsExpensiveString.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/PreconditionsExpensiveString.md deleted file mode 100644 index 3e5d115..0000000 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/PreconditionsExpensiveString.md +++ /dev/null @@ -1,16 +0,0 @@ -Preconditions checks take an error message to display if the check fails. The -error message is rarely needed, so it should either be cheap to construct or -constructed only when needed. This check ensures that these error messages are -not constructed using expensive methods that are evaluated eagerly. - -Prefer this: - -```java -checkNotNull(foo, "hello %s", name); -``` - -instead of this: - -```java -checkNotNull(foo, String.format("hello %s", name)); -``` diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/StatementSwitchToExpressionSwitch.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/StatementSwitchToExpressionSwitch.md index ded86f3..210fac6 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/StatementSwitchToExpressionSwitch.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/StatementSwitchToExpressionSwitch.md @@ -1,28 +1,34 @@ -We're trying to make `switch` statements simpler to understand at a glance. -Misunderstanding the control flow of a `switch` block is a common source of -bugs. +We're trying to make `switch`es simpler to understand at a glance. +Misunderstanding the control flow of a `switch` is a common source of bugs. -### Statement `switch` statements: +As part of this simplification, new-style arrow (`->`) switches are encouraged +instead of old-style colon (`:`) switches. And where possible, neighboring cases +are grouped together (e.g. `case A, B, C`). -* Have a colon between the `case` and the case's code. For example, `case +### Old-style colon (`:`) `switch`es: + +* Have a colon between the `case` and the `case`'s code. For example, `case HEARTS:` * Because of the potential for fall-through, it takes time and cognitive load - to understand the control flow for each `case` -* When a `switch` block is large, just skimming each `case` can be toilsome -* Fall-though can also be conditional (see example below). In this scenario, - one would need to reason about all possible flows for each `case`. When - conditionally falling-through multiple `case`s in a row is possible, the - number of potential control flows can grow rapidly - -### Expression `switch` statements - -* Have an arrow between the `case` and the case's code. For example, `case + to understand the control flow. When a `switch` block is large, just + skimming each `case` can be toilsome. Fall-through can also be conditional + (see example 5. below). In this scenario, one would potentially need to + reason about all possible flows for each `case`. When conditionally + falling-through multiple `case`s, the number of potential control flows can + grow rapidly +* Lexical scopes overlap, which can lead to surprising behaviors: definitions + of local variables from earlier `case`s are propagated down to later + `case`s, however the *values* that initialize those local variables do not + propagate in the same way + +### New-style arrow (`->`) `switch`es: + +* Have an arrow between the `case` and the `case`'s code. For example, `case HEARTS ->` -* With an expression `switch` statement, you know at a glance that no cases - fall through. No control flow analysis needed +* No `case`s fall through; no control flow analysis needed * Safely and easily reorder `case`s (within a `switch`) -* It's also possible to group identical cases together (`case A, B, C`) for - improved readability +* Lexical scopes are isolated between different `case`s; if you define a local + variable within a `case`, it can only be used within that specific `case`. ### Examples @@ -48,7 +54,7 @@ private void foo(Suit suit) { } ``` -Which can be simplified into the following expression `switch`: +Which can be simplified by grouping and using a new-style switch: ``` {.good} enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; @@ -65,16 +71,14 @@ private void foo(Suit suit) { } ``` -#### 2. Return switch +#### 2. `return switch ...` -Sometimes `switch` is used with `return`. Below, even though a `case` is -specified for each possible value of the `enum`, note that we nevertheless need -a "should never happen" clause: +Sometimes `switch` is used with a `return` for each `case`, like this: ``` {.bad} enum SideOfCoin {OBVERSE, REVERSE}; -private String foo(SideOfCoin sideOfCoin) { +private String renderName(SideOfCoin sideOfCoin) { switch(sideOfCoin) { case OBVERSE: return "Heads"; @@ -86,13 +90,14 @@ private String foo(SideOfCoin sideOfCoin) { } ``` -Using an expression switch simplifies the code and removes the need for an -explicit "should never happen" clause. +Note that even though a `case` is present for each possible value of the `enum`, +a boilerplate "should never happen" clause is still needed. The transformed code +is simpler and doesn't need a "should never happen" clause. ``` enum SideOfCoin {OBVERSE, REVERSE}; -private String foo(SideOfCoin sideOfCoin) { +private String renderName(SideOfCoin sideOfCoin) { return switch(sideOfCoin) { case OBVERSE -> "Heads"; case REVERSE -> "Tails"; @@ -100,29 +105,30 @@ private String foo(SideOfCoin sideOfCoin) { } ``` -If you nevertheless wish to have an explicit "should never happen" clause, this -can be accomplished by placing the logic under a `default` case. For example: +If you nevertheless wish to define an explicit "should never happen" clause, +this can be accomplished by placing the logic inside a `default` case. For +example: ``` - enum SideOfCoin {OBVERSE, REVERSE}; private String foo(SideOfCoin sideOfCoin) { return switch(sideOfCoin) { case OBVERSE -> "Heads"; case REVERSE -> "Tails"; - default -> { - // This should never happen - throw new RuntimeException("Unknown side of coin"); - } + default -> throw new RuntimeException("Unknown side of coin"); // should never happen }; } ``` -#### 3. Assignment switch +When the checker detects an existing `default` that appears to be redundant, it +may suggest a secondary auto-fix which removes the redundant `default` and its +code (if any). + +#### 3. Assignment `switch` -If every branch of a `switch` is making an assignment to the same variable, it -can be re-written as an assignment switch: +If every branch of a `switch` is making an assignment to the same variable, the +code can be simplified into a combined assignment and `switch`: ``` {.bad} enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; @@ -157,14 +163,107 @@ private void updateScore(Suit suit) { case HEARTS, DIAMONDS -> -1; case SPADES -> 2; case CLUBS -> 3; - }; + }; +} +``` + +Taking this one step further: if a local variable is defined, and then +immediately followed by a `switch` in which every `case` assigns to that same +variable, then all three (the `switch`, the variable declaration, and the +assignment) can be merged: + +``` {.bad} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void updateStatus(Suit suit) { + int score; + + switch(suit) { + case HEARTS: + // Fall thru + case DIAMONDS: + score = 1; + break; + case SPADES: + score = 2; + break; + case CLUBS: + score = 3; + } + ... + +} +``` + +Becomes: + +``` +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void updateStatus(Suit suit) { + int score = switch(suit) { + case HEARTS, DIAMONDS -> 1; + case SPADES -> 2; + case CLUBS -> 3; + }; + ... } ``` -#### 4. Complex control flows +#### 4. Just converting to new arrow `switch` + +Even when the simplifications discussed above are not applicable, conversion to +new arrow `switch`es can be automated by this checker: + +``` {.bad} +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void processEvent(Suit suit) { + switch (suit) { + case CLUBS: + String message = "hello"; + var anotherMessage = "salut"; + processMessages(message, anotherMessage); + break; + case DIAMONDS: + anotherMessage = "bonjour"; + processMessage(anotherMessage); + } +} +``` + +Note that the local variables referenced in multiple cases are hoisted up out of +the `switch` statement, and `var` declarations are converted to explicit types, +resulting in: + +``` +enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; + +private void processEvent(Suit suit) { + String anotherMessage; + switch (suit) { + case CLUBS -> { + String message = "hello"; + anotherMessage = "salut"; + processMessages(message, anotherMessage); + } + case DIAMONDS -> { + anotherMessage = "bonjour"; + processMessage(anotherMessage); + } + } +} +``` + +#### 5. Complex control flows Here's an example of a complex statement `switch` with conditional fall-through -and complex control flows. How many potential execution paths can you spot? +and various control flows. Unfortunately, the checker does not currently have +the ability to automatically convert such code to new-style arrow `switch`es. +Manually converting the code could be a good opportunity to improve its +readability. + +How many potential execution paths can you spot? ``` {.bad} enum Suit {HEARTS, CLUBS, SPADES, DIAMONDS}; diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UndefinedEquals.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UndefinedEquals.md index d464d21..a3aae3a 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UndefinedEquals.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UndefinedEquals.md @@ -62,7 +62,14 @@ TIP: `java.util.Date` is a legacy, bug-prone API. Prefer `java.time.Instant` or Prefer subtypes such as `ImmutableSet` or `ImmutableList`, which have well-defined `equals`. +## For [`Future`] + +Prefer calling `#get` and checking the result, or using +`assertThat(...).isSameInstanceAs()` or `assertThat(...).isNotSameInstanceAs()` +for cases where the same/different instance is expected. + [`Collection`]: https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html +[`Future`]: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Future.html [`Iterable`]: https://docs.oracle.com/javase/8/docs/api/java/lang/Iterable.html [`Iterables.elementsEqual`]: https://guava.dev/releases/snapshot/api/docs/com/google/common/collect/Iterables.html#elementsEqual-java.lang.Iterable-java.lang.Iterable- [`LinkedList`]: https://docs.oracle.com/javase/8/docs/api/java/util/LinkedList.html diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryDefaultInEnumSwitch.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryDefaultInEnumSwitch.md index 8b53a5f..3f9d481 100644 --- a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryDefaultInEnumSwitch.md +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryDefaultInEnumSwitch.md @@ -153,3 +153,8 @@ If it can complete normally, the default should be merged with an added UNRECOGNIZED case. [complete normally]: https://docs.oracle.com/javase/specs/jls/se10/html/jls-14.html#jls-14.1 + +## Library skew + +A default branch that has a comment containing "skew" will not be classified as +unnecessary. See: [MissingCasesInEnumSwitch](MissingCasesInEnumSwitch.md). diff --git a/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryQualifier.md b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryQualifier.md new file mode 100644 index 0000000..1053c3a --- /dev/null +++ b/sonar-erroraway-maven-plugin/src/main/resources/errorprone/bugpattern/UnnecessaryQualifier.md @@ -0,0 +1,39 @@ +A `@Qualifier` or a `@BindingAnnotation` has no effect here, and can be removed. +Its presence may be misleading. + +For example: + +```java +final class MyInjectableClass { + @Username private final String username; + + @Inject + MyInjectableClass(@Username String username) { + this.username = username; + } +} +``` + +The annotation on the constructor parameter is important, but the field +annotation is redundant. + +```java +final class MyInjectableClass { + private final String username; + + @Inject + MyInjectableClass(@Username String username) { + this.username = username; + } +} +``` + +There are a couple of ways this check can lead to false positives: + +* You're using a custom framework we don't know about which makes the location + of the finding an injection point. File a bug, and we'll happily incorporate + it. + +* Your annotation is annotated with `@Qualifier` or `@BindingAnnotation` but + isn't actually used as a qualifier (perhaps you have a framework that just + uses it reflectively). Try removing those annotations from the *annotation*.