Added RedundantNullCheck bug pattern #5121

bdragan · 2025-06-25T20:44:20Z

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 · 2025-06-25T20:44:25Z

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.

cpovirk

Thank you for doing this, and I apologize again for not getting to it.

My suggested edits assume a few static imports:

import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
import static javax.lang.model.type.TypeKind.TYPEVAR;

Whatever special cases you're up for adding tests for are great, but I can't complain when you already have a bunch of tests and I've left you hanging so long.

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

cpovirk · 2025-09-22T14:45:57Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+    return buildDescription(tree).build();
+  }
+
+  private static boolean isEffectivelyNullable(MethodSymbol methodSymbol, VisitorState state) {


A totally optional thing that you could look into is handling of constructors: I think those end up in the MethodSymbol code path, and of course any call to a constructor produces a non-null value, regardless of annotations. Given that, you might well be able to return false if methodSymbol.isConstructor().

Makes sense, done. That reminded me of literal initializers, which I now also covered.

I hadn't considered how much more we could do here if we wanted. (Even without getting into true "dataflow," we could cover many of the basics that we have in NullnessPropagationTransfer.) The cases you've already covered strike me as the most important ones: It's easier to imagine that someone might wrongly conclude that foo() can return null than that i + 1 can produce null :) The next steps that come to mind are to cover expressions that "pass through" values, like the ternary operator or a cast. I would actively recommend not trying to squeeze that into this PR, since I fear that I will disappear again if I force enough rounds of review. But that's something else you could consider as a follow-up change or in your internal version if it feels valuable enough.

…ck`](google/error-prone#5121). Also, use that check as motivation to finally make `AbstractBiMap` more understandable to nullness checkers. While in the area, I also made `inverse` be `private`. That required changing a reference in a subclass in the file to use `super.inverse`. As I understand it from my experience in 856123e, a `private` field like `inverse` is [accessible](https://docs.oracle.com/javase/specs/jls/se24/html/jls-6.html#jls-6.6.1) but not inherited ([1](https://docs.oracle.com/javase/specs/jls/se24/html/jls-8.html#d5e13771), [2](https://docs.oracle.com/javase/specs/jls/se24/html/jls-8.html#jls-8.2)). (And marginally simplify a couple `AbstractBiMap` subclasses by removing unnecessary type arguments.) RELNOTES=n/a PiperOrigin-RevId: 809750887

…ck`](google/error-prone#5121). Also, use that check as motivation to finally make `AbstractBiMap` more understandable to nullness checkers. While in the area, I also made `inverse` be `private`. That required changing a reference in a subclass in the file to use `super.inverse`. As I understand it from my experience in 856123e, a `private` field like `inverse` is [accessible](https://docs.oracle.com/javase/specs/jls/se24/html/jls-6.html#jls-6.6.1) but not inherited ([1](https://docs.oracle.com/javase/specs/jls/se24/html/jls-8.html#d5e13771), [2](https://docs.oracle.com/javase/specs/jls/se24/html/jls-8.html#jls-8.2)). (And marginally simplify a couple `AbstractBiMap` subclasses by removing unnecessary type arguments.) RELNOTES=n/a PiperOrigin-RevId: 810029934

bdragan · 2025-09-23T08:22:27Z

@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 · 2025-09-23T09:07:33Z

@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 · 2025-09-23T20:27:53Z

@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

Thanks again. I could approve this as it is now to move the import process along, but I'll hold off in case you want to try to add requireNonNull support or make other changes. In the meantime, I'll retest what you have on our depot to see if anything new (or anything that I Just missed) jumps out at me.

cpovirk · 2025-09-23T20:33:52Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+import javax.lang.model.element.ElementKind;
+
+@BugPattern(
+    summary = "Explicit null check on a variable or method call that is not @Nullable within a @NullMarked scope.",


I got to wondering if someone might read the current text as saying that the null check has to be within a @NullMarked scope, as opposed to saying that the field or method needs to be so. Maybe we can avoid that by focusing on what we know instead of how we know it, as in the following...?

Suggested change

summary = "Explicit null check on a variable or method call that is not @Nullable within a @NullMarked scope.",

summary = "Explicit null check on an expression that is known not to be null.",

That said, when I tested this out on our codebase, I saw a number of cases in which the value could have been null but the annotations were wrong :) So I'm tempted to say something like:

Suggested change

summary = "Explicit null check on a variable or method call that is not @Nullable within a @NullMarked scope.",

summary = "Explicit null check on an expression that is non-null according to nullness annotations.",

But then that's not true for cases like literals and constructors... :)

Maybe this is ultimately best covered in the Markdown docs. But if you have more thoughts on how to phrase things here, maybe we can figure something out.

Indeed, we should improve the wording here. Maybe just explicitly mention both (language semantics and annotations), something like:

Null check on an expression that is statically determined to be non-null according to language semantics or nullness annotations.

Or maybe simply should instead of statically determined (which is redundant phrase in the context of static analysis itself):

Null check on an expression that should be non-null according to language semantics or nullness annotations.

I think I like your "statically determined" phrasing over "should": Even when we qualify it with "according to language semantics or nullness annotations," I worry that "should" could be misread as somehow related to what the code does if it encounters null. I'm not sure I'm phrasing that well :\ Like, if I write...

if (next == null) { throw new IllegalStateException(); }

...then clearly next "should" be non-null. And that's "should" in a different sense than the one we'd be talking about here.

I agree that "statically determined" is technically redundant, but I think it's worth it for the emphasis. I say that especially because I'm seeing a lot of cases in which the actual problem is that a type is missing a @Nullable annotation :)

Agreed; then for now I changed it (both in pattern summary and in md) to:

Null check on an expression that is statically determined to be non-null according to language semantics or nullness annotations.

Let's think and improve more if needed.

cpovirk · 2025-09-23T20:45:54Z

docs/bugpattern/nullness/RedundantNullCheck.md

@@ -0,0 +1,52 @@
+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.


Returning to what I was saying about above how the nullness information we have may or may not come from annotations and how those annotations may or may not be correct....

I'm not sure if we have a consistent style for how we phrase these, but here are some things I might say:

Some code checks for null when null is "impossible." In some cases, null is truly impossible according to the rules of the Java language, such as when testing the result of a constructor call. In other cases, null is impossible according to nullness annotations on a parameter or method that is being used: Either the type is annotated with @NonNull, or it has no annotation but appears in the scope of a @NullMarked annotation, which makes most unannotated types non-null.

An "impossible" null check may indicate that the null check is a bug. For example, maybe it should instead be checking whether the value is an empty collection or Optional. Alternatively, the check might merely have become unnecessary. Or, in some cases, the value can be null at runtime, and the real problem is that a method or parameter is missing a @Nullable annotation.

cpovirk · 2025-09-23T20:47:57Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+        return NO_MATCH;
+      }
+
+      if ((varSymbol.getKind() == ElementKind.LOCAL_VARIABLE


This is another case in which I'm going to ask something more a question for the future: Now that we restrict checking to final (and effectively final) VarSymbol declaration, would it make sense to also cover fields?

(This also reminds me that exception parameters are always non-null, at least if no one assigns back to them! But I say that mostly just to get it out of my system.)

Other thoughts on this proposal (in addition to the ones above on line 61), again with the motivation of capturing them for the future:

If we cover fields, we could identify additional opportunities (while also keeping all the current ones) by looking for @NonNull annotations on them and @NullMarked annotations on the enclosing elements. That would let us treat even more fields as non-null, including but not limited to non-final fields.

That said, we do sometimes see users who claim that a field is non-null when it's merely non-null after some initialization occurs. So it's possible that those users would be annoyed if we were to complain that their lazy-initialization code checks for the nullness of a "non-null" field. That, at least, is an issue only for non-final fields.

Err, sorry, you do cover fields; I'm just focused on the branch this is not responsible for them.

I will be out for a couple hours, but I'll try to reevaluate when I get back. Several of my comments need revisions.

OK, let's see if I can evaluate the full logic instead of over-focusing on specific details and losing the bigger picture.

There are a few VarSymbol cases in which we could report that a value is never null:

for a field that matches either of the following conditions:

@NonNull (I'm not sure we check at the moment. That's fine)

a non-type-variable-typed, non-@Nullable field in the scope of @NullMarked

for a parameter that matches either of the "field" conditions and is not an implicitly typed lambda parameter

for a variable that matches both of the following conditions:

"considered final"

initialized to a !isEffectivelyNullable method or to a literal (other than null)

(probably also for a field that matches all the "variable" conditions, though that's another thing we don't check at the moment, and that's fine, too)

We should figure out whether that is even right. Whatever we settle on, and whatever part of that we implement now, it might be easiest to express with an isEffectivelyNullable(VarSymbol, ...) method. That would let us have a separate section for each case, from which we can return early, since we can't really do that with the logic inline in matchBinary. (If we have a separate method, that could also help with other possible future generalizations that we've discussed.)

Yes, all points are correct.

for a field that matches either of the following conditions:

@NonNull (I'm not sure we check at the moment. That's fine)

Good catch. I was focused mostly on @NullMarked scope (where @NonNull is redundant, at least when it comes to tools implementing null-marked spec), but since I already added support for it for methods out of @NullMarked scope, I changed it now for var symbols too.

probably also for a field that matches all the "variable" conditions

We have to think more when it comes to fields in some situations, including alignment with other tools, e.g. a field that is not marked as nullable in @NullMarked scope will not be considered nullable even if it is not considered final or is initialized with an isEffectivelyNullable method, so we'd need to take care of it during implementation. Hence I left the logic for fields to be annotation-driven only for now.

isEffectivelyNullable(VarSymbol, ...)

Good idea, I already did it for methods and the flow is easier to follow for them with that. Now I refactored the logic for var symbols that way too. 👍

Another aspect about fields is that they are more affected by build-time or runtime code manipulation tools (i.e. like Lombok, dependency injection, bytecode weaving in runtime, etc) than local variables I guess, so the static analysis assumptions we make might not be suitable to all scenarios in practice.

I think that Error Prone conveniently blows up on Lombok code, at least :) But, yes, good point: Fields do get weird treatment from tools in a variety of situations. I also expect for humans to be less reliable at annotating their fields than their APIs simply because I expect them to focus on public members, while fields are usually private (aside from the occasional constants).

(I have some other distractions going on at the moment but should be able to get back to this tomorrow if I don't manage to today.)

cpovirk

Sorry, the results looked great, but I ended up with more thoughts anyway. I hope that it's just simplifications at this point.

cpovirk · 2025-09-24T14:12:07Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+      if (NullnessUtils.isAlreadyAnnotatedNullable(varSymbol)) {
+        return NO_MATCH;
+      }
+      if (varSymbol.asType().getKind() == TYPEVAR) {


Sorry, I was just looking at this suggestion of mine again, and I think I was mistaken: We shouldn't need to check TYPEVAR for variables: I'm thinking that the only case in which we could ever produce a finding for a type-variable-typed variable is a call to a method that returns @NonNull T for some type-variable type T. In that case, we're already relying on isEffectivelyNullable(MethodSymbol, ...) to do the right thing for TYPEVAR.

So I think we can remove the TYPEVAR check here, leaving only the one below (the one for methods).

Oops, sorry again: I am mixed up, as I'll cover below. [edit: here]

cpovirk · 2025-09-24T14:16:52Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+        return NO_MATCH;
+      }
+
+      if ((varSymbol.getKind() == ElementKind.LOCAL_VARIABLE


Other thoughts on this proposal (in addition to the ones above on line 61), again with the motivation of capturing them for the future:

If we cover fields, we could identify additional opportunities (while also keeping all the current ones) by looking for @NonNull annotations on them and @NullMarked annotations on the enclosing elements. That would let us treat even more fields as non-null, including but not limited to non-final fields.

That said, we do sometimes see users who claim that a field is non-null when it's merely non-null after some initialization occurs. So it's possible that those users would be annoyed if we were to complain that their lazy-initialization code checks for the nullness of a "non-null" field. That, at least, is an issue only for non-final fields.

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

cpovirk · 2025-09-24T14:32:11Z

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 · 2025-09-24T17:00:25Z

@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).

@nonnull

- VarSymbol that is @nonnull outside @NullMarked scope is considered as non-null - Improved wording in the documentation

…bles.

cpovirk

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.

cpovirk · 2025-09-25T15:34:03Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+    VarSymbol varSymbol = nullCheck.varSymbolButUsuallyPreferBareIdentifier();
+    if (varSymbol != null) {
+      isRedundant = !isEffectivelyNullable(varSymbol, state);
+    } else {
+      MethodSymbol methodSymbol = nullCheck.methodSymbol();
+      if (methodSymbol != null) {
+        isRedundant = !isEffectivelyNullable(methodSymbol, state);
+      }
+    }


Suggested change

VarSymbol varSymbol = nullCheck.varSymbolButUsuallyPreferBareIdentifier();

if (varSymbol != null) {

isRedundant = !isEffectivelyNullable(varSymbol, state);

} else {

MethodSymbol methodSymbol = nullCheck.methodSymbol();

if (methodSymbol != null) {

isRedundant = !isEffectivelyNullable(methodSymbol, state);

}

}

VarSymbol varSymbol = nullCheck.varSymbolButUsuallyPreferBareIdentifier();

MethodSymbol methodSymbol = nullCheck.methodSymbol();

boolean isRedundant = false;

if (varSymbol != null) {

isRedundant = !isEffectivelyNullable(varSymbol, state);

} else if (methodSymbol != null) {

isRedundant = !isEffectivelyNullable(methodSymbol, state);

}

It might be even better to go a little further and, instead of setting isRedundant, just return immediately, especially after the slightly simplification I'm suggesting to the return statement. You could merge the checks into lines like:

if (varSymbol != null && !isEffectivelyNullable(varSymbol, state) { return describeMatch(tree); }

cpovirk · 2025-09-25T15:37:48Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+    if (!OBJECTS_REQUIRE_NON_NULL.matches(tree, state)) {
+      return NO_MATCH;
+    }
+    if (tree.getArguments().isEmpty()) {


I suspect we could safely ignore this possibility, since the code shouldn't compile and thus shouldn't ever get to Error Prone.

cpovirk · 2025-09-25T16:09:51Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+      return true;
+    }
+
+    if (isLocalOrResourceVariable && varDecl != null) {


Suggested change

if (isLocalOrResourceVariable && varDecl != null) {

if (isLocalOrResourceVariable) {

If varDecl is null, then we would have already thrown NPE during the call to hasImplicitType. We could move the check up, but my hope would be that a local/resource variable would always have a declaration, so I'm proposing to just remove it.

(If we did want to handle the null case, then we'd probably want to do something different so that we don't just fall through to the isInNullMarkedScope check at the end of the method.)

cpovirk · 2025-09-25T18:01:47Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+    }
+
+    if (isRedundant) {
+      return buildDescription(tree).build();


Suggested change

return buildDescription(tree).build();

return describeMatch(tree);

cpovirk · 2025-09-25T18:03:46Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+      if (initializer.getKind() == Tree.Kind.METHOD_INVOCATION) {
+        MethodInvocationTree methodInvocation = (MethodInvocationTree) initializer;


Suggested change

if (initializer.getKind() == Tree.Kind.METHOD_INVOCATION) {

MethodInvocationTree methodInvocation = (MethodInvocationTree) initializer;

if (initializer instanceof MethodInvocationTree methodInvocation) {

cpovirk · 2025-09-25T18:04:39Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+    if (symbol instanceof VarSymbol) {
+      isRedundant = !isEffectivelyNullable((VarSymbol) symbol, state);
+    } else if (symbol instanceof MethodSymbol) {
+      isRedundant = !isEffectivelyNullable((MethodSymbol) symbol, state);
+    }


Suggested change

if (symbol instanceof VarSymbol) {

isRedundant = !isEffectivelyNullable((VarSymbol) symbol, state);

} else if (symbol instanceof MethodSymbol) {

isRedundant = !isEffectivelyNullable((MethodSymbol) symbol, state);

}

if (symbol instanceof VarSymbol varSymbol) {

isRedundant = !isEffectivelyNullable(varSymbol, state);

} else if (symbol instanceof MethodSymbol methodSymbol) {

isRedundant = !isEffectivelyNullable(methodSymbol, state);

}

cpovirk · 2025-09-25T18:06:57Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+  private final boolean checkRequireNonNull;
+
+  @Inject
+  public RedundantNullCheck(ErrorProneFlags flags) {


Suggested change

public RedundantNullCheck(ErrorProneFlags flags) {

RedundantNullCheck(ErrorProneFlags flags) {

Just to follow our internal style of reducing the visibility of constructors that are going to be called only through reflection.

cpovirk · 2025-09-25T18:10:59Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheck.java

+    }
+
+    if (isRedundant) {
+      return buildDescription(tree).build();


Suggested change

return buildDescription(tree).build();

return describeMatch(tree);

bdragan · 2025-09-25T22:12:55Z

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!

cpovirk

OK, let's see if my approval accomplishes anything in this repo. I can also keep an eye out for the internal review and see what I can do there.

Most of these came to my attention during the testing of [RedundantNullCheck](google/error-prone#5121). Notes: - For `BufferedInputStream` and `FilterInputStream`, see #128. - For `URL`, the docs for `getAuthority()` don't mention `null`, but it's easy enough to demonstrate that `null` is a possible return value: ```java var u = new URL("mailto:foo@bar.com"); System.out.println("authority " + u.getAuthority()); ``` - `AlgorithmParameters.toString()` is gross but documented as such, as eamonnmcmanus points out. - `Matcher.group()` is unfortunate: At one point, it could not return `null`, but that changed, as documented since [JDK-8274663](https://bugs.openjdk.org/browse/JDK-8274663). The change is a consequence of [`usePattern`](https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/regex/Matcher.html#usePattern(java.util.regex.Pattern)), which was added all the way back in Java 5. I am a little tempted to lie here and leave the type as non-null. - In `Component`, I also removed an annotation on the package-private helper method used by `getName()`. - In `ResultSet`, `getBlob` and `getObject` have no documentatino about `null` returns. Gemini [wrote me some code that demonstrates the possible null returns](https://g.co/gemini/share/776a7669ae47). - Also in `ResultSet`, I simplified some type-parameter declarations. This ensures that the `Class<T>` parameter has its type argument in bounds, and it makes no difference to the result type, which is always nullable, regardless of the type argument. Fixes #128

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: ```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. Fixes #5121 FUTURE_COPYBARA_INTEGRATE_REVIEW=#5121 from bdragan:5107-redundant_null_check f935970 PiperOrigin-RevId: 811786926

Add some missing `@Nullable` annotations. Most of these came to my attention during the testing of [RedundantNullCheck](google/error-prone#5121). Notes: - For `BufferedInputStream` and `FilterInputStream`, see #128. - For `URL`, the docs for `getAuthority()` don't mention `null`, but it's easy enough to demonstrate that `null` is a possible return value: ```java var u = new URL("mailto:foo@bar.com"); System.out.println("authority " + u.getAuthority()); ``` - `AlgorithmParameters.toString()` is gross but documented as such, as eamonnmcmanus points out. - `Matcher.group()` is unfortunate: At one point, it could not return `null`, but that changed, as documented since [JDK-8274663](https://bugs.openjdk.org/browse/JDK-8274663). The change is a consequence of [`usePattern`](https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/regex/Matcher.html#usePattern(java.util.regex.Pattern)), which was added all the way back in Java 5. I am a little tempted to lie here and leave the type as non-null. - In `Component`, I also removed an annotation on the package-private helper method used by `getName()`. - In `ResultSet`, `getBlob` and `getObject` have no documentation about `null` returns. Gemini [wrote me some code that demonstrates the possible null returns](https://g.co/gemini/share/776a7669ae47). - Also in `ResultSet`, I simplified some type-parameter declarations. This ensures that the `Class<T>` parameter has its type argument in bounds, and it makes no difference to the result type, which is always nullable, regardless of the type argument. Fixes #128

…llCheck`](https://errorprone.info/bugpattern/RedundantNullCheck). `RedundantNullCheck` was from google/error-prone#5121. I had already taken some of its suggestions in 125fd02, but I'd been on the fence about this one. What pushed me over the edge was that my 34f3253 causes failures in nullness checking. I'm sure I could find another way to resolve those, but this approach probably makes more sense, anyway: If we imagine that nullness is enforced as in Kotlin (or possibly someday Java), then the call to `checkEntryNotNull` would immediately throw a `NullPointerException`, possibly with no message, instead of allowing the method to execute far enough to produce our custom `NullPointerException`, which includes both the key and the value. (I think there were a handful of other parameters like this one on which I also had declined to add `@Nullable`. I'm not revisiting that right now, but we could someday.) RELNOTES=n/a PiperOrigin-RevId: 830554169

…dundantNullCheck`](https://errorprone.info/bugpattern/RedundantNullCheck). `RedundantNullCheck` was from google/error-prone#5121. I had already taken some of its suggestions in 125fd02, but I'd been on the fence about these ones. What pushed me over the edge was that my 34f3253 causes failures in nullness checking. I'm sure I could find another way to resolve those, but this approach probably makes more sense, anyway: If we imagine that nullness is enforced as in Kotlin (or possibly someday Java), then the call to `checkEntryNotNull` would immediately throw a `NullPointerException`, possibly with no message, instead of allowing the method to execute far enough to produce our custom `NullPointerException`, which includes both the key and the value. (I think there were a handful of other parameters like this one on which I also had declined to add `@Nullable`. I'm not revisiting that right now, but we could someday.) RELNOTES=n/a PiperOrigin-RevId: 830554169

…dundantNullCheck`](https://errorprone.info/bugpattern/RedundantNullCheck). `RedundantNullCheck` was from google/error-prone#5121. I had already taken some of its suggestions in 125fd02, but I'd been on the fence about these ones. What pushed me over the edge was that my 34f3253 causes failures in nullness checking. I'm sure I could find another way to resolve those, but this approach probably makes more sense, anyway: If we imagine that nullness is enforced as in Kotlin (or possibly someday Java), then the call to `checkEntryNotNull` would immediately throw a `NullPointerException`, possibly with no message, instead of allowing the method to execute far enough to produce our custom `NullPointerException`, which includes both the key and the value. (I think there were a handful of other parameters like this one on which I also had declined to add `@Nullable`. I'm not revisiting that right now, but we could someday.) RELNOTES=n/a PiperOrigin-RevId: 830586416

5107: Added RedundantNullCheck bug pattern

a9ca764

bdragan mentioned this pull request Jun 25, 2025

UnnecessaryCheckNotNull doesn't work with @NullMarked #5107

Closed

bdragan changed the title ~~5107: Added RedundantNullCheck bug pattern~~ Added RedundantNullCheck bug pattern Jun 27, 2025

bdragan mentioned this pull request Jun 27, 2025

5107: Added RedundantNullCheck bug pattern BrokkAi/error-prone#1

Merged

More relaxed checks for local and resource variables

7683373

cpovirk reviewed Sep 22, 2025

View reviewed changes

cpovirk self-assigned this Sep 22, 2025

copybara-service bot mentioned this pull request Sep 22, 2025

Add a missing @Nullable annotation identified by [RedundantNullCheck](https://github.com/google/error-prone/pull/5121). google/guava#8010

Merged

Improve RedundantNullCheck to handle more cases

1aba736

bdragan requested a review from cpovirk September 23, 2025 08:23

cpovirk reviewed Sep 23, 2025

View reviewed changes

cpovirk reviewed Sep 24, 2025

View reviewed changes

bdragan added 2 commits September 24, 2025 17:49

Add support for Objects.requireNonNull

5ede3fb

Optional check for Objects.requireNonNull

0f7b322

cpovirk mentioned this pull request Sep 24, 2025

Warn on unnecessary null checks uber/NullAway#1223

Closed

bdragan added 5 commits September 24, 2025 20:47

- Refactored by introducing isEffectivelyNullable(varSymbol,...) method;

3e1b542

- VarSymbol that is @nonnull outside @NullMarked scope is considered as non-null - Improved wording in the documentation

Excluded local variables from interpretation of nullness annotations

2d89f8f

Mistakenly omitted change for annotations on local and resource varai…

ef8f24d

…bles.

Fixed handling of method and literal initializers

70e06c5

Fixed handling of method initializers

c3ce40e

cpovirk reviewed Sep 25, 2025

View reviewed changes

Refactoring and code improvements

f935970

cpovirk approved these changes Sep 25, 2025

View reviewed changes

cpovirk mentioned this pull request Sep 26, 2025

Add some missing @Nullable annotations. jspecify/jdk#129

Merged

copybara-service bot mentioned this pull request Sep 26, 2025

Add RedundantNullCheck bug pattern. #5253

Merged

copybara-service bot closed this in a12903a Sep 26, 2025

copybara-service bot mentioned this pull request Nov 10, 2025

Add a couple more @Nullable annotations that were suggested by [RedundantNullCheck](https://errorprone.info/bugpattern/RedundantNullCheck). google/guava#8086

Closed

	summary = "Explicit null check on a variable or method call that is not @Nullable within a @NullMarked scope.",
	summary = "Explicit null check on an expression that is known not to be null.",

		@@ -0,0 +1,52 @@
		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.

	if (isLocalOrResourceVariable && varDecl != null) {
	if (isLocalOrResourceVariable) {

	return buildDescription(tree).build();
	return describeMatch(tree);

		if (initializer.getKind() == Tree.Kind.METHOD_INVOCATION) {
		MethodInvocationTree methodInvocation = (MethodInvocationTree) initializer;

	if (initializer.getKind() == Tree.Kind.METHOD_INVOCATION) {
	MethodInvocationTree methodInvocation = (MethodInvocationTree) initializer;
	if (initializer instanceof MethodInvocationTree methodInvocation) {

	public RedundantNullCheck(ErrorProneFlags flags) {
	RedundantNullCheck(ErrorProneFlags flags) {

Added RedundantNullCheck bug pattern #5121

Added RedundantNullCheck bug pattern #5121

Uh oh!

Conversation

bdragan commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla bot commented Jun 25, 2025

Uh oh!

cpovirk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdragan commented Sep 23, 2025

Uh oh!

bdragan commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpovirk commented Sep 23, 2025

Uh oh!

cpovirk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpovirk Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdragan Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpovirk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpovirk Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpovirk Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cpovirk commented Sep 24, 2025

Uh oh!

bdragan commented Sep 24, 2025

bdragan commented Jun 25, 2025 •

edited

Loading

bdragan commented Sep 23, 2025 •

edited

Loading

cpovirk Sep 24, 2025 •

edited

Loading

bdragan Sep 24, 2025 •

edited

Loading

cpovirk Sep 24, 2025 •

edited

Loading

cpovirk Sep 24, 2025 •

edited

Loading