ignoring generic Nullable on Supplier

[xenoterracide]

maybe I don't understand how the generic is supposed to work... Git here is from jgit but I don't think that's relevant. get can return null, and so this should produce an error. In fact 2 errors, firstly the field doesn't match the constructor signature, but even if you add the nullable generic to the field it still doesn't error.

note: I have tested this in my build, but not independently yet. So if it fails to do so outside... I can try to find something better.

public class SemverExtension {

  private final Supplier<Git> git;

  public SemverExtension(@NonNull Supplier<@Nullable Git> git) {
    this.git = Objects.requireNonNull(git);
  }
  Object anon() {
    return Optional.empty().orElseGet(() -> this.git.get().add()); // should error as add can npe
  }
}

[msridhar]

@xenoterracide this is due to the fact that NullAway's support for generic types is still a WIP. We're slowly approaching something usable but I'd say it's not quite there yet. If you want to test it out, you can pass the flag -XepOpt:NullAway:JSpecifyMode=true and see what happens. For this particular case, I think things won't work, as we need fuller support for importing the annotations from jspecify/jdk, in particular the @Nullable upper bound here. @akulk022 are you actively working on adding support for this?

[akulk022]

@msridhar Yes I am working on adding support for pulling in @Nullable upper bounds for generic type parameters from jspecify/jdk.

[xenoterracide]

Try is from vavr, maybe I don't understand how this is supposed to work? but I feel like this should compile.

  private final Supplier<Optional<Git>> git;


  Try<@Nullable String> describe() {
    return this.git.get()
      .map(g -> Try.of(() -> g.describe().setMatch(VERSION_GLOB).setTags(true)))
      .orElseGet(NoGitDirException::failure)
      .mapTry(DescribeCommand::call)
      .recover(NoGitDirException.class, e -> null);
  }

/home/xeno/IdeaProjects/gradle-semver/src/main/java/com/xenoterracide/gradle/semver/GitMetadataExtension.java:44: error: [NullAway] Generic type parameter cannot be @Nullable, as type variable T of type io.vavr.control.Try does not have a @Nullable upper bound
  Try<@Nullable String> describe() {
      ^
    (see http://t.uber.com/nullaway )
/home/xeno/IdeaProjects/gradle-semver/src/main/java/com/xenoterracide/gradle/semver/GitMetadataExtension.java:49: error: [NullAway] Cannot return expression of type Try<String> from method with return type Try<@Nullable String> due to mismatched nullability of type parameters
      .recover(NoGitDirException.class, e -> null);
              ^
    (see http://t.uber.com/nullaway )

[msridhar]

I think you're just running into shortcomings / missing features of our implementation. Can you point me at the source for the Try class?

[msridhar]

But bottom line our support for JSpecify is not ready for real-world use yet. The only open implementation I know of that would probably work is https://github.com/jspecify/jspecify-reference-checker. But these test cases are really valuable for prioritizing our work.

[xenoterracide]

It's vavr ... https://github.com/vavr-io/vavr

[xenoterracide]

probably another, again vavr, d is after a potential nullable return in the chain. Interestingly jetbrains and checker see this, although they aren't better when I put a filter before it. Splitter is guava, and is annotated.

      .map(d -> Iterables.get(Splitter.on('-').split(d), 1))

[msridhar]

It's vavr ... https://github.com/vavr-io/vavr

Ok, so there are multiple things going on here. First, let's assume you've made io.vavr an annotated package in your NullAway config. Here is the declaration of Try:

https://github.com/vavr-io/vavr/blob/master/src/main/java/io/vavr/control/Try.java#L64

Notice the declaration is Try<T>. In JSpecify, this means that T can never be @Nullable. If you want to allow for @Nullable type arguments, it would have to be Try<T extends @Nullable Object>. See here for discussion: https://jspecify.dev/docs/user-guide#defining-generics

I'm guessing, however, that io.vavr was not being treated as an annotated package. In that case, we should not be reporting errors for your code snippet, since we should allow for instantiating Try however you want (since essentially Try is @NullUnmarked). But we have a bug around that, #872, which we will get to hopefully soon.

[xenoterracide]

Ok, so there are multiple things going on here. First, let's assume you've made io.vavr an annotated package in your NullAway config. Here is the declaration of Try

I did not, because it is not. I Don't do that for libraries that are either, am I supposed to? I've only been doing it for my own. I don't do it on Spring for example. I'm not sure how Nullaway interacts with NonNullApi and NonNullFields... tbh. note: I'm not aware of any libraries that actually use jspecify's annotations. I know that spring has seemed to decide that jspecify will not be meeting its needs anytime soon.

As far as Try goes, it's @Nullable, not sure how inferance works though on something with a functional chain (like Try or Stream), where explicitly it' Nullable, but it might be never null depending on your code. Not worried about it, decided to mention it because of generics.

[msridhar]

So for libraries outside of annotated packages, like vavr, to me the most sensible JSpecify behavior is to treat them as @NullUnmarked absent some explicit @NullMarked annotation somewhere. With that behavior, you shouldn't get any errors for this code, I think. Does that not match your expectations @xenoterracide?

[xenoterracide]

That matches my expectations

I'm just not sure how detection of annotated libraries works.

[msridhar]

Our plan for detection of annotated libraries is as follows:

• We will continue to support the annotated packages option. If the library code is in an annotated package, it will be treated as @NullMarked, absent any @NullUnmarked annotations that override things.
• If we see an explicit @NullMarked annotation on a library class or package, we'll treat it as @NullMarked (absent overriding @NullUnmarked annotations). This should also work for @NullMarked on a module, but I'm not 100% sure that we'll always be able to read that depending on a project's JPMS configuration, as per Figure out and document whether module-level @NullMarked helps users who don't declare a module jspecify/jspecify#496
• Otherwise, we treat the code as @NullUnmarked

If you have any feedback on this please let us know

[xenoterracide]

If there aren't module/package annotations we can set a library to marked ourselves? presumably like I do my own. Is that still necessary if I @NullMarked my module-info.java (with/without JspecifyMode=true)? I honestly haven't thought/looked at this bit in a long time. So it just occurred to me to check.

[msridhar]

If there aren't module/package annotations we can set a library to marked ourselves? presumably like I do my own.

Yes, by just using the annotated packages option.

Is that still necessary if I @NullMarked my module-info.java (with/without JspecifyMode=true)? I honestly haven't thought/looked at this bit in a long time. So it just occurred to me to check.

I'm not sure honestly if we added code yet to look up the module-info.java. Even if/when we do (we plan to), jspecify/jspecify#496 means it may not always be picked up by clients of the library.