parameters seem to assume non null by default

[xenoterracide]

related to #26 my preference would just be a setting to say that parameters are nullable unless marked @NonNull

dependencies {
  errorprone("com.google.errorprone:error_prone_core:2.4.+")
  errorprone("com.uber.nullaway:nullaway:0.8.+")
}

nullaway {
  annotatedPackages.add("com.xenoterracide")
}

tasks.withType<JavaCompile>().configureEach {
  options.errorprone {
    nullaway {
      severity.set(CheckSeverity.ERROR)
      acknowledgeRestrictiveAnnotations.set(true)
      handleTestAssertionLibraries.set(true)
    }
    disableWarningsInGeneratedCode.set(true)
    error("NullOptional",
          "NullableConstructor",
          "NullablePrimitive",
          "NullableVoid",
          "Overrides",
          "MissingOverride",
          "Var",
          "WildcardImport")
  }
}

package com.xenoterracide.ppm.authn;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.springframework.security.core.Authentication;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController
class OIDCTestController {

  private final Logger log = LogManager.getLogger(this.getClass());

  @GetMapping
  String index( Authentication details) {
    log.info("{}", details);
    return "Hello, " + details.getName();
  }
}

this passed, imho a false negative, as details can be null.

[lazaroclapp]

NullAway, by design, assumes that "no annotation" implies non-null for code being directly checked by it. Any method argument, return value, or field that can have a null value must be explicitly typed as @Nullable (we perform dataflow analysis for local variables, so those don't need any annotation). @NonNull is much more rarely used, and mostly for third-party (unchecked) jars.

If the call to index(...) is being checked by NullAway, then the caller won't be allowed to pass a null details. If this is being called from third party code (which I assume is what @GetMapping causes), and this is a common case, then you might need to add a handler for that annotation, requiring that methods annotated with @GetMapping have @Nullable arguments.

A similar issue arrises with some third-party library callbacks.

For example, assume the following interface in third-party (not-NullAway-typed) code:

interface Foo {
    void onEvent(Event e);
}

Where e can possibly be null. Then, the following code will pass NullAway, despite the potential NPE, since the code that introduces null and calls the callback with that value is not visible to NullAway:

setFoo( e -> log(e.toString()) );

For callbacks, the solution is easy, just make a library model indicating that Foo.onEvent takes a @Nullable value. With that, methods overriding onEvent, like the lambda above, will be required to handle the case.

With methods being hooked up by reflection using annotations, like the spring case above, we might need a custom handler/extension to interpret @GetMapping as introducing a call from third-party code.

[xenoterracide]

right, I mean, wouldn't it be possible to add a setting to invert the default assumption/make parameters assumed to be @Nullable by default? and have to mark them @NonNull in order to not have to check? I've been looking through error prone to see if there is a setting that makes marking these required, as an alternative.

[lazaroclapp]

Unfortunately, as far as I know, there isn't currently an option to reverse that default in NullAway, nor is a single place of the codebase that would need to be changed.

We might eventually support other defaults that "default: non-null" through our eventual implementation of the proposed jspecify nullability annotation standard, but, short-term the short-answer is no.

I think the that the easier thing to do for this use case in particular is to require all @GetMapping annotated methods to have @Nullable non-primitive parameters, either through a separate Error Prone check or a NullAway handler.

[xenoterracide]

lol, and somehow I'm back here. I was just reading https://github.com/jakartaee/common-annotations-api/blob/master/api/src/main/java/jakarta/annotation/Nullable.java and it says

• This annotation is useful mostly for overriding a {@link Nonnull} annotation.
• Static analysis tools should generally treat the annotated items as though they
• had no annotation, unless they are configured to minimize false negatives.

I don't know, guess wanting #907 makes a super amount of sense to avoid excessive work, because if people are following this advice (which I assume comes from jsr305), then the defaults of nullaway are wrong, and to fix it you have to annotate everything. This all really comes up if writing a library for 3rd parties.

[msridhar]

Hi @xenoterracide we are unlikely to have any resources in the near future to devote to adding a mode where NullAway assumes vars are @Nullable by default. I think it would be a lot of work, and beyond that, standardization efforts like JSpecify are focused on the @NonNull by default scenario. Sorry about that. If you're looking for a checker that has more flexibility with defaulting you could consider the Checker Framework Nullness Checker.