Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove explicit nullable annotations if they depend on the generic type argument #337

Closed
DavyLandman opened this issue Aug 12, 2019 · 23 comments
Closed

Comments

@DavyLandman
Copy link

@DavyLandman DavyLandman commented Aug 12, 2019

Hi Ben,

I was pleasantly surprised by CheckerFramework annotations on the caffeine code, since we also switched a while back, it makes life easier. 馃憤

A pattern I saw and I wanted to check your opinion on is for example Cache::get:

  @Nullable
  V get(@NonNull K key, @NonNull Function<? super K, ? extends V> mappingFunction);

Now reading the documentation, if the mapping function never returns null, neither should the get function, right?

If that is so, than I think the @Nullable should be removed from the return type. Since CF will just propagate the nullability of the V type parameter. If V is @Nullable, then the result will be, but if you provide a mapping function that has a @NonNull return type, it's a bit strange to still have to handle the null case.

I hope this question makes sense?

@cruftex
Copy link

@cruftex cruftex commented Aug 12, 2019

Mind the JavaDoc on the method:

@return the current (existing or computed) value associated with the specified key, or null if
the computed value is null

The function is nonnull, not the result of it.

@DavyLandman
Copy link
Author

@DavyLandman DavyLandman commented Aug 12, 2019

I did not mean the mapping function, I'm talking specifically about the V generic type parameter that can be either @NonNull or @Nullable depending on the client code.

Example:

public @NonNull String foo(@NonNull String bar) { // note that in CF @NonNull is assumed, just here for clarity sake
     return cache.get(bar, String::toUpperCase);
}

Since String.toUpperCase returntype is @NonNull, this (sh/c)ould be valid. Since the V type parameter would be bound to @NonNull String.

I hope this makes my question a bit more clear, sorry if I use the wrong terminology, I'm always open to learn.

(currently, because of the @Nullable annotation, the example becomes something like:)

public @NonNull String foo(@NonNull String bar) { // note that in CF @NonNull is assumed, just here for clarity sake
     return Objects.requireNonNull(cache.get(bar, String::toUpperCase), "Unexpected null result from cache");
}
@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Aug 12, 2019

Yes, if V is never null then the results is non-null. How would we express this, though? It鈥檚 the same as computeIfAbsent.

I originally meant it for documentation as a hint to the reader, but static analysis tools have gotten better. I think dropping the annotation would imply non-null by default.

@cruftex
Copy link

@cruftex cruftex commented Aug 12, 2019

.... I'm always open to learn.

@DavyLandman: Actually I learned something now. Thanks for the more detailed explanation!

....If that is so, than I think the @nullable should be removed from the return type. Since CF will just propagate the nullability of the V type parameter.

The nullability might be part of the return type of the mapping function in CF, but there is no way to propagate this. Implementations of the same method signature can legally return always null.

There is no contract on the method that CF can use to determine how the returned value relates to the mapper function.

@DavyLandman
Copy link
Author

@DavyLandman DavyLandman commented Aug 13, 2019

Yes, if V is never null then the results is non-null. How would we express this, though? It鈥檚 the same as computeIfAbsent.

Indeed, similar to that, maybe we can do the same as the CF jdk annotations for map?

default V computeIfAbsent(K key, Function<? super K, ? extends @Nullable V> mappingFunction) {

I have to admit, I would have to dive deep into the CF docs to understand how the @Nullable works together with the wildcard and it's position of the type, but at least that's how the authors of CF defined it.

I originally meant it for documentation as a hint to the reader, but static analysis tools have gotten better. I think dropping the annotation would imply non-null by default.

I understand this tradeoff, I think the example from the jdk annotations might be a good middle ground.

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Aug 13, 2019

Thanks. I think that's a reasonable solution.

I use ErrorProne + NullAway instead, so there will likely be other minor mistakes in the future.

@DavyLandman
Copy link
Author

@DavyLandman DavyLandman commented Aug 13, 2019

Great, I imagine might be more cases like this throughout the big caffeine api?

I hadn't heard about nullaway, looks like a good alternative to checker framework. CF takes some investment and getting used too, but after the initial pain, keeping it updated is not that hard.

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Aug 13, 2019

I wouldn't be surprised. This is a little special because its a computation, so we just need to fix each of the get(key, func) style methods. Most other usages are obvious or we can defer to the asMap view.

I tried CF once but it was too invasive for me, but I'm sure it's matured a lot since then. ErrorProne + NullAway are a bit easier to add progressively, though I suspect they are not as powerful.

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Apr 27, 2020

I forgot to link here earlier, but I am waiting to see how the effort in google/guava#2960 (comment) pans out.

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 16, 2021

Reviewing this anew, and I think maybe this case should use @PolyNull.

As an example of the use of @PolyNull, method Class.cast returns null if and only if its argument is null:

@PolyNull T cast(@PolyNull Object obj) { ... }

This is like writing:

@NonNull T cast( @NonNull Object obj) { ... }
@Nullable T cast(@Nullable Object obj) { ... }

If so, then I would change this one method signature to,

@PolyNull
V get(@NonNull K key, @NonNull Function<? super K, ? extends @PolyNull V> mappingFunction);

What do you think @DavyLandman?

@vlsi
Copy link
Contributor

@vlsi vlsi commented Jan 16, 2021

The following should be enough:

// `K extends Object` means K is non-null (provided default checkerframework configuration is used)
public interface Cache<K extends Object, V> {

  V get(/* @NonNull <== not needed */K key, @NonNull Function<? super K, ? extends V> mappingFunction);
@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 16, 2021

I haven't used checker for a long time, but it used to default to null (not nonnull). Is that no longer the case? That was one reason that I decided not to use it as it became very verbose with that coding style.

@vlsi
Copy link
Contributor

@vlsi vlsi commented Jan 16, 2021

PS. I'm surprised you have lots of @NonNull annotations. Is it intentional? I guess the typical recommendation (e.g. from checkerframework) is non-nullable by default (e.g. @DefaultQualifier(NonNull...))

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 16, 2021

For my usages, the goal wasn't actually about static analysis but for clearer documentation. The jsr305 were used that way to be more explicit for those scanning the JavaDoc that the return value is nullable. When migrating in #242, it was requested that these are more explicit to better conform to the checker framework's analysis. That made sense, but became verbose.

Would using @DefaultQualifier help reduce that verbosity by again assuming non-null unless specified? It would then go back to being more reader-friendly?

@vlsi
Copy link
Contributor

@vlsi vlsi commented Jan 16, 2021

it used to default to null (not nonnull)

The defaults are described here: https://checkerframework.org/manual/#climb-to-top

They suggest non-null by default, however, things get a bit complicated with generics.

Would using @DefaultQualifier help reduce that verbosity by again assuming non-null unless specified?

Exactly.

the goal wasn't actually about static analysis but for clearer documentation

I see. However, tools like IntelliJ IDEA recognize the annotations and they might produce false warnings in case the annotations are not at their best :)
AFAIK Kotlin does not understand checkerframework annotations, however, it looks like they are going to converge on https://github.com/jspecify/jspecify

Would using @DefaultQualifier help reduce that verbosity by again assuming non-null unless specified? It would then go back to being more reader-friendly?

I guess so.

For instance, even if you write public interface Cache<K extends @NonNull Object, V> {, then it would be more-or-less clear that K is a non-nullable type, and it would avoid adding @NonNull K even without DefaultQualifier.

PS. I've added a gist on checkerframework behavior, however, it is more for machine verification rather than for "documenting nullability in code"
https://github.com/apache/calcite/blob/6908d21ddecab8a10d7754800eed8f15b9c32f78/site/develop/index.md#null-safety

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 16, 2021

Right, modern Java generally assumes non-null by default. The IDE nullness checkers often did too, or at least Eclipse's as one of the early ones. I am positive checker went against that in early versions, as I recall that in their docs in 2014 when first trying it out here. If that's changed or by using default qualifier then that is great.

I'll clean up the annotations in the v3 branch and ping you for a quick review.

@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 16, 2021

@vlsi Why not include the generic bounds or use TypeUseLocation.ALL in your package?

@vlsi
Copy link
Contributor

@vlsi vlsi commented Jan 16, 2021

TypeUseLocation.ALL

Generic bounds are tricky, and trying to make every bound nullable or non-nullable does not really work for cases like extends, super, etc.

I recently annotated Apache Calcite codebase (see apache/calcite#2268), and it does pass the verification. I learned that the Checker Framework's recommended approach to generics works OK, it is readable, and it is more-or-less consistent.

ben-manes added a commit that referenced this issue Jan 17, 2021
@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 17, 2021

@vlsi Can you please review f19f547? I removed all of the @NonNull annotations, used @PolyNull in the case above, and added the package defaults.

ben-manes added a commit that referenced this issue Jan 17, 2021
ben-manes added a commit that referenced this issue Jan 17, 2021
ben-manes added a commit that referenced this issue Jan 18, 2021
@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Jan 21, 2021

@vlsi I spent a few hours trying to resolve checker framework warnings, but I don't think it's worthwhile. It complains about types being nullable despite non-null if conditions asserting that to no longer be true. By checking only the types and not logic flow, hundreds of pointless warnings are produced. These have to be suppressed, which makes the code harder to read and negates any value in catching errors.

This may be a bad fit because as a data structure null is very common. In most code one can and should be very null adverse, and use a coding style that makes them very rare. Then handling and suppressing the warnings is likely a minor deal.

I'll try to port over any public api interface changes to try and make that better conform. That's your and @DavyLandman intent anyway, I just got too ambitious by trying to pass its rules too.

@DavyLandman
Copy link
Author

@DavyLandman DavyLandman commented Jan 21, 2021

@ben-manes I agree, especially for map like structures, checker-framework is really hard to get right. One of the problems is caused by arrays. I think at some points maps where hard-coded in their internals.

So I think providing the right annotations on the interface is a good enough trade-off. If you want, you could take a look at the CF annotations for Map and look at how they set it up?

ben-manes added a commit that referenced this issue Feb 1, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 1, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 1, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 1, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 15, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 15, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 15, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 15, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 16, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 16, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
ben-manes added a commit that referenced this issue Feb 16, 2021
Additional wildcards are used throughput the APIs to more flexibly
accept parameters. For example this allows a wider range of method
references to be used as load functions.

The generic types now match the Checker Framework's rules [1]. This
should improve usage of the cache apis in projects that run the checker.
This project does not and its implementation classes are not compliant
for the nullness checker.

[1] https://checkerframework.org/manual/#generics-instantiation
@ben-manes ben-manes closed this in 79e3f5c Feb 21, 2021
@ben-manes
Copy link
Owner

@ben-manes ben-manes commented Feb 22, 2021

Released in 3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants