Added RedundantNullCheck bug pattern

[bdragan]

Closes #5107.

A null check is redundant if it is performed on a variable or method call that is known to be non-null within a @NullMarked scope.

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:

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.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[bdragan]

@cpovirk Great, thank you, I incorporated and pushed your suggestions (with two additions - implicit vars which aren't parameters and literal initializers). Also added tests for the new use cases.

[bdragan]

@cpovirk Also we think that extending this with support for redundant Objects.requireNonNull calls would be beneficial, wdyt? If you agree I will also push it here - I already have a working solution for direct calls which are most common in practice, i.e. Objects.requireNonNull(x) (so method references are not covered i.e. Object::requireNonNull but that should be fine I guess).

[cpovirk]

@cpovirk Also we think that extending this with support for redundant Objects.requireNonNull calls would be beneficial, wdyt? If you agree I will also push it here - I already have a working solution for direct calls which are most common in practice, i.e. Objects.requireNonNull(x) (so method references are not covered i.e. Object::requireNonNull but that should be fine I guess).

That is a good question. I tend to have different views for different code:

In places like Guava, we actively want to make sure that we produce NPE immediately if users pass null where they shouldn't. This prepares us for the potential that the JVM performs similar checks automatically and unavoidably someday, but more immediately, it ensures that we aren't setting up a trap that will spring later in the program (like we would be doing if we were to let you put null into a collection that shouldn't support it). More often, the NPE does happen immediately "naturally," so there's no need to check. But we at least occasionally still check explicitly so that we can provide a nicer exception message (or so that we can support a weird environment that doesn't guarantee NPE, as we sometimes have to do).

For more typical code, I don't usually bother with defensive checks at all, since the damage from getting things "wrong" is more contained. There, I'd expect to see requireNonNull only for the "non-redundant" cases.

I would say that it's fine for you to do whatever suits you, but I will eventually end up running this by someone for internal review, so we may end up changing course then. We also always have the option of making the behavior configurable with a flag. That still leaves the question of which behavior to make the default, but it at least offers flexibility to users who have strong enough opinions.

[cpovirk]

I also wanted to write down somewhere how this differs from https://errorprone.info/bugpattern/ImpossibleNullComparison, at least as I understand it:

• ImpossibleNullComparison does not trust annotations.
• ImpossibleNullComparison does not handle literals.
• ImpossibleNullComparison does not track fields after assignment.
• ImpossibleNullComparison does look for operations like requireNonNull that this PR at least currently doesn't cover.
• ImpossibleNullComparison has some suggested fixes, at least a couple of which (the ones for List/Collection and Optional) we could probably someday steal for here.
• ImpossibleNullComparison handles case null in switch.
• ImpossibleNullComparison handles specific, well-known APIs.

The long-term plan should likely be to merge these, probably offering configuration options and enhanced docs. But I'm hoping to be able to make the case that we shouldn't block on that.

[bdragan]

@cpovirk Also we think that extending this with support for redundant Objects.requireNonNull calls would be beneficial, wdyt? If you agree I will also push it here - I already have a working solution for direct calls which are most common in practice, i.e. Objects.requireNonNull(x) (so method references are not covered i.e. Object::requireNonNull but that should be fine I guess).

That is a good question. I tend to have different views for different code:

In places like Guava, we actively want to make sure that we produce NPE immediately if users pass null where they shouldn't. This prepares us for the potential that the JVM performs similar checks automatically and unavoidably someday, but more immediately, it ensures that we aren't setting up a trap that will spring later in the program (like we would be doing if we were to let you put null into a collection that shouldn't support it). More often, the NPE does happen immediately "naturally," so there's no need to check. But we at least occasionally still check explicitly so that we can provide a nicer exception message (or so that we can support a weird environment that doesn't guarantee NPE, as we sometimes have to do).

For more typical code, I don't usually bother with defensive checks at all, since the damage from getting things "wrong" is more contained. There, I'd expect to see requireNonNull only for the "non-redundant" cases.

I would say that it's fine for you to do whatever suits you, but I will eventually end up running this by someone for internal review, so we may end up changing course then. We also always have the option of making the behavior configurable with a flag. That still leaves the question of which behavior to make the default, but it at least offers flexibility to users who have strong enough opinions.

Makes sense. I pushed it now too, it is optional and disabled by default (can be enabled with RedundantNullCheck:CheckRequireNonNull flag).

[bdragan]

The results from running this over our depot look excellent—so many people who think that methods might return null when they actually throw an exception, for example, plus all the cases of missing @Nullable annotations that I'd mentioned before. I'm still not sure if we'll be able to justify turning it on for everyone, since some people have chosen to perform manual defensive checks on parameters, but I hope we can get there when we're finally able to focus more on nullness again!

I have run out of behavioral suggestions, so all that's left is some style-type things.

Great, I have pushed the style-type changes you suggested, all valid points, thanks!