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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A way to declare a type parameter that wants nothing to do with nullness #78

Closed
kevinb9n opened this issue Oct 31, 2019 · 17 comments
Closed
Labels
design An issue that is resolved by making a decision, about whether and how something should work. nullness For issues specific to nullness analysis.
Milestone

Comments

@kevinb9n
Copy link
Collaborator

kevinb9n commented Oct 31, 2019

This may be an issue only with the current "explicit annotation on type variable usage wins" model, not with the "most-nullable" proposal.

Consider a class like this:

class WeakRef<T> {
  WeakRef(@NotNull T referent) { ... }
  @Nullable T get() { ... }
}

Note that every type usage of T has an explicit nullness override.

In this case, we really have no interest in seeing type usages of WeakRef<@Nullable Foo> OR WeakRef<@NotNull Foo> at all, because the distinction between these is meaningless.

Would we prefer a way to mark T itself:

class WeakRef<@HasNothingToDoWithNullness T> 

Yeah, it would be hard to name this.

Then it would be an error to leave any type usage of T within the WeakRef class unannotated. (Or let the defaults apply, but maybe let's not debate that yet.)

@cpovirk
Copy link
Collaborator

cpovirk commented Oct 31, 2019

I think I haven't grasped the whole picture here yet, so two questions:

(1) It sounds like code owners can mostly get equivalent behavior by declaring <T extends @NotNull ...>.

A couple minor differences:

  • They don't get the enforcement that they've put @Nullable where they should (except in so far as the checkers catch the resulting errors in implementation code), but that's true of all code: The main effect here would be to provide a little extra opt-in checking in this particular case.
  • Users always see @NotNull T instead of plain T, and maybe that's clearer in some cases. Again, this is presumably something that code owners can already do anyway, so we're really talking about opt-in enforcement.

Based only on those minor differences, it's possible that @HasNothingToDoWithNullness doesn't strictly need to be part of the core offerings, nor be considered when checking clients. It could be more of an extension, even checker-specific, with which people can write class WeakRef<@HasNothingToDoWithNullness T [extends @NotNull Object]>.

But, a more significant possible difference:

  • Would this permit users to write plain WeakRef<T> even if T has parametric nullness? (I should think more about whether this would come up in practice. Possibly there's overlap with @PolyNull use cases??)

(And maybe there are other differences?)

(2) You alluded to at least one "issue" that this addresses. Can you give examples? Maybe I'm missing something obvious.

@kevinb9n kevinb9n mentioned this issue Nov 1, 2019
@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Nov 1, 2019

  1. I do kind of like the idea of just recommending <extends @NotNull> in this case, because at least consumers won't be senselessly choosing between the two.

  2. I don't think I did? I said "this may be an issue".

@cpovirk
Copy link
Collaborator

cpovirk commented Nov 4, 2019

I think part of my confusion is that I'm not even sure if you're saying that @HasNothingToDoWithNullness may cause an issue or solve one.

Beyond that: Does my previous post cover the advantages you envision for @HasNothingToDoWithNullness over what users get from <extends @NotNull>?

@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Nov 4, 2019

Let's not spend more time on this yet; I mostly just wanted to capture the fact that a type parameter whose nullness is always ignored is weird, and may bear some thinking about at some point.

@stephan-herrmann
Copy link

In my p.o.v. there's nothing "wrong" with WeakRef<@Nullable Foo> or such, it's just adding information that is never used downstream. In the scope of a defaulting annotation even WeakRef<Foo> would be over-specific, as it expands to WeakRef<@Nonnull Foo>.

Nothing of this affects the soundness of the program, it's just a matter of style, and so tools my optionally inform users that they are overzealous. I think it safe to leave this unspecified / implementation specific.

IMHO, an annotation to mark this case would be overzealous itself.

I do see a problem if class WeakRef<T> {} would implicitly be interpreted a class WeakRef<T @Nonnull Object> {} thus disallowing the fully legal WeakRef<Bar> where Bar is a nullable or null-parametric type.

@cpovirk
Copy link
Collaborator

cpovirk commented Nov 5, 2019

My reasons for wanting to avoid WeakRef<@Nullable Foo>:

  • If we permit both WeakRef<@Nullable Foo> and WeakRef<@NotNull Foo>, I expect for some user to eventually have a reference of one type and need a reference of the other. We might be able to make that work in one direction with @In/@Out annotations, but it's unclear whether we'll have those, and that doesn't help in the other direction (hmm, unless we permit class WeakRef<@In @Out T>...?).
  • I also like the idea of discouraging the user from ever even stopping to think "Would it make more sense to use WeakRef<@Nullable Foo> or WeakRef<@NotNull Foo> here?" That's nice to avoid whether the two types end up being convertible in both directions or not. (Of course, the opposite spin on this is that the way to really save users from thinking it to support as many instantiations as we can.)

Now, one possibility is to allow people to write WeakRef<@Nullable Foo> but to immediately translate it internally to WeakRef<@NotNull Foo>.

I should think more about the example of using WeakRef<Bar> when Bar is a null-parametric type. Previously I'd thought about a similar example with Optional<Bar>, but that seemed fairly unlikely to come up. WeakRef<Bar> deserves more thought. Thanks.

@cpovirk
Copy link
Collaborator

cpovirk commented Nov 6, 2019

The WeakRef<Bar> example is like the earlier Equivalence.Wrapper example. I don't think I'd generalized it to the full "type parameter that wants nothing to do with nullness" case, since it first came up in the context of class Foo<@Nullable T>.

class Foo<@Nullable T>, I note, is another way of implementing a type parameter that wants nothing to do with nullness :) A Foo class defined in this way is easy to instantiate with a null-parametric type: If you have the type parameter <E extends @Nullable Object>, you can write Foo<@Nullable E>. (Note that you might also have to write Foo<@Nullable E> if E is @NotNull, rather than just writing Foo<E>.) Of course it's also easy to instantiate my proposed approach of class Foo<T extends @NotNull Object> by writing Foo<@NotNull E> -- though only if we provide projection to @NotNull. Note that projection to @NotNull may still be necessary under the class Foo<@Nullable T> approach, too, as the class may have APIs that accept or return a not-null T. So there seems to be no escaping projection to @NotNull if:

  • we feel that nullness-agnostic types are needed often enough from nullness-parametric types to justify it
  • we don't feel nullness-agnostic types deserve additional special handling
  • we don't feel that the workarounds (extra type parameters? static methods? wrapper classes? suppression?) are good enough

(As discussed above, it's also possible to declare class Foo<T extends @Nullable Object>, permitting both instantiations, as the Checker Framework does for Class and Optional. This still requires projection to @NotNull, just as class Foo<@Nullable T> would.)

I still have reasons that I dislike projecting to @NotNull, but I want to look more at this class of use cases. Previously, I've claimed:

  • In practice, users don't write APIs that accept or return "a value of null-parametric type T, but the value can't be null." Rather, if they're forbidding null, they're generally doing it universally by bounding T to @NotNull types. (Or at the very least, they could do this.)

I stand by that. But I've also claimed:

  • The way to implement a "nullness-agnostic" type is to declare it as class Foo<T extends @NotNull Object>. Bounding it to @NotNull isn't a "restriction," since any methods that can operate on null can be declared to use @Nullable T.

I still feel that that's a reasonable approach to such types. But my previous claim ignored this case: Sure, my class with <E extends @Nullable Object> probably doesn't need to accept or return a @NotNull E. But it might need to accept or return an Equivalence<@NotNull E>, WeakRef<@NotNull E>, or Class<@NotNull E> (if we require those types to be instantiated with @NotNull). I should look more at Guava to see if this comes up more there.

@cpovirk
Copy link
Collaborator

cpovirk commented Nov 6, 2019

Another thing that I've been trying to get my head around is: What other fundamental differences are there between the proposed...

  • class WeakRef<@HasNothingToDoWithNullness T>
    ...and the other possible ways of defining a nullness-agnostic type, which include...
  • class WeakRef<T extends @NotNull Object>
  • class WeakRef<@Nullable T>

I'm interested in this because: If it's convenient to be able to instantiate WeakRef with any nullness (and, ideally, to be able to convert freely between different nullnesses), then wouldn't it be nice to do the same for all types that permit only a single nullness? Why not permit a BlockingQueue<@Nullable Object> alongside a BlockingQueue<@NotNull Object>, with both requiring their actual elements to be @NotNull, despite the @Nullable instantiation? (If we were willing to go this far, we might be able to say that all type parameters have parametric nullness. That is likely to be a source of confusion on net, but it would at least sidestep #12 :)) We would write:

interface BlockingQueue<E extends @Nullable Object> extends Queue<@NotNull E> {
  ...
  @NotNull E take();
}

There are some practical reasons not to do this:

  • Users are likely to expect BlockingQueue<E extends @Nullable Object> to mean that a queue can contain null, simply because that's usually how classes are defined. (Note that the Checker Framework nevertheless defines Optional with parametric nullability.) Users might get used to this if we were crazy enough to give all type parameters parametric nullness, though, especially since the type would likely be written interface BlockingQueue<E> extends Queue<@NotNull E>.
  • This approach is likely to result in more total code, since every usage of E has to be written as @NotNull E.
  • Classes that are truly "null-hostile," not just "null-agnostic," are less likely to be used in the implementation of a class with a nullness-parametric type. For example, it's easy to imagine that such a class might want a WeakRef<E>. It's somewhat harder (though not impossible) to imagine that it might want an Optional<E>.

I'm not sure if there's more to it than that or not.

cpovirk added a commit to jspecify/jdk that referenced this issue Nov 6, 2020
All methods already work in terms of `@Nullable T`, so this doesn't
prevent any method from accepting null. That said, it _does_ prevent
some instantiations that are safe, like `WeakReference<T>` when `<T
extends @nullable Object>`. But let's try the stricter signature for now
and see how much trouble it causes.

This is related to jspecify/jspecify#78
@kevinb9n
Copy link
Collaborator Author

I feel very good about

@NullMarked
class WeakRef<T> {
  WeakRef(T referent) { ... }
  @Nullable T get() { ... }
}

I'm not sure we need to keep pulling on any of the threads above. Reopen if desired.

@kevinb9n kevinb9n changed the title A type parameter that wants nothing to do with nullness A type parameter that wants nothing to do with nullness (working decision: that's <T>) Jun 19, 2021
@cpovirk
Copy link
Collaborator

cpovirk commented Jan 6, 2022

I still feel good about using <T> for this in general, but I wanted to note here that we may end up using <T extends @Nullable Object> in one specific, unusual case, when theoretically we'd use a special-purpose "type parameter that wants nothing to do with nullness" if such a thing existed.

The case is TypeToken. There is no difference between a TypeToken<String> and a TypeToken<@Nullable String>. (OK, it's technically possible to differentiate between the two by using reflection, since TypeToken users create an anonymous subclass, which defeats erasure.) The TypeToken APIs don't expose the annotation, and they certainly don't propagate it through substitution, use it in subtyping checks, etc. In short, TypeToken wants nothing to do with nullness.

However, in order to allow users to create a TypeToken<T> for a type parameter that is declared <T extends @Nullable Object>, we'd need to declare TypeToken itself with <T extends @Nullable Object>. Or, alternatively, we'd need a @DefinitelyNonNull annotation (#4), which is, I grant, looking more likely now that Kotlin is adding such a feature.

I left a couple comments about this a while back in TypeToken and TypeParameter.

There's also a little discussion on Google-internal bug 198793040. The main thing to note is that the use case for this is Kotlin, so we can't say "just suppress."

(Now, there was also Google-internal CL 398047525, in which people were explicitly using TypeToken<@Nullable Foo>, which we'd probably rather they didn't.... But if we cared enough, we could write our own additional checks to ban that pattern.)

@kevin1e100
Copy link
Collaborator

I guess I'm wondering if TypeToken is completely nullness-oblivious: seemingly, in a nullness-annotated world, we could want it to understand and (ideally, correctly) deal with things like TypeToken<@Nullable Foo>. We'd nearly certainly want it to understand TypeToken<List<@Nullable Foo>> (which is probably "automatic"), so why not TypeToken<@Nullable Foo>? Differentiating TypeToken<Foo> from TypeToken<@Nullable Foo> seems at least "in the wheelhouse" of TypeToken's purpose. Not sure this is making a lot of sense for some of the methods defined on TypeToken, but as a token to identify a type, it seems actually important to be able to express and differentiate TypeToken<@Nullable Foo>, wdyt?

@cpovirk
Copy link
Collaborator

cpovirk commented Jan 12, 2022

It definitely would be nice for TypeToken to support nullness. I mostly doubt that we would get around to it anytime soon: The time that we have to work on tooling is likely to keep going toward compile-time checking and toward tools to add annotations to source code.

@kevinb9n kevinb9n added the design An issue that is resolved by making a decision, about whether and how something should work. label Mar 16, 2023
cpovirk added a commit to jspecify/jdk that referenced this issue Mar 20, 2023
We've declared those classes' type parameters as having a bound of
`@NonNull Object`, as discussed in
jspecify/jspecify#78. This PR changes type
arguments inside the JDK to be inside that bound.

eisop and typetools, by contrast, use a bound of `@Nullable Object`,
which they "undo" with `@NonNull T` on methods like `Class.newInstance`.
Thus, they don't need or want a change like this one. (That said, they
don't _universally_ use `@Nullable Object` as the bound in "cases like
this," so it's possible that they'd feel that a particular API, maybe
`ServiceLoader`, would be simpler with a non-nullable bound. Or, if not,
they may wish to use `@NonNull S` at the use sites.)

(The complicated API in this PR is `ServiceLoader`. I do see some docs
that suggest that `ServiceLoader` rejects any attempt to return a `null`
service instance, and that appears to be backed up by the implementation
I traced through to `ProviderImpl.get`.)
cpovirk added a commit to jspecify/jdk that referenced this issue Jun 2, 2023
We've declared those classes' type parameters as having a bound of
`@NonNull Object`, as discussed in
jspecify/jspecify#78. This PR changes type
arguments inside the JDK to be inside that bound.

eisop and typetools, by contrast, use a bound of `@Nullable Object`,
which they "undo" with `@NonNull T` on methods like `Class.newInstance`.
Thus, they don't need or want a change like this one. (That said, they
don't _universally_ use `@Nullable Object` as the bound in "cases like
this," so it's possible that they'd feel that a particular API, maybe
`ServiceLoader`, would be simpler with a non-nullable bound. Or, if not,
they may wish to use `@NonNull S` at the use sites.)

(The complicated API in this PR is `ServiceLoader`. I do see some docs
that suggest that `ServiceLoader` rejects any attempt to return a `null`
service instance, and that appears to be backed up by the implementation
I traced through to `ProviderImpl.get`.)

(#6)
@kevinb9n kevinb9n added the nullness For issues specific to nullness analysis. label Mar 13, 2024
@kevinb9n kevinb9n added this to the 0.3 milestone Mar 27, 2024
@netdpb
Copy link
Collaborator

netdpb commented Apr 9, 2024

Current decision: When a generic type is equivalent whether its type parameter's argument is nullable or not (for example, Class), then that is representable by a plain (bounded by non-null) type parameter. All usages of that type variable should be null-projected or non-null-projected.

Argument for changing: It would be more explicit to call out those type parameters, so that tools could either enforce that all type usages are projected, or treat plain usages as if they were projected.

However, no strong case was made that such a change is worth the added complexity.

Timing: This must be decided before version 1.0 of the jar.

Proposal for 1.0: Finalize the current decision. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative.

Results: Four 👍 with some requests for clarification about the need for non-null projections if the type parameter's bound is non-null (they are in fact unnecessary) and for clarification that JSpecify does not consider either a non-null bound or a nullable bound invalid in these cases. Users can choose either, although we may provide non-normative advice. The point is that there's no separate way to specify that no usages of that type parameter have parametric nullness.

@netdpb netdpb added the needs-decision-before-jar-1.0 Issues needing a finalized decision for the 1.0 jar release label Apr 9, 2024
@kevinb9n
Copy link
Collaborator Author

Current decision: When a generic type is equivalent whether its type parameter's argument is nullable or not (for example, Class), then that is representable by a plain (bounded by non-null) type parameter. All usages of that type variable should be null-projected or non-null-projected.

(To the last part, you shouldn't need to non-null-project a type variable if it's already non-null bounded, right?)

@wmdietl
Copy link
Collaborator

wmdietl commented May 7, 2024

To me it seems like it is up to API designers whether they want to use <T> (and not require projections) or use <T extends @Nullable Object> (making usage easier, but possibly requiring projections to ignore nullness).
If this issue is about whether we add a new mechanism to express that a type parameter "wants nothing to do with nullness" and the working decision for that is "no", then 👍 .
If the issue is that we prescribe how people have to use the annotations: why get mixed up in those discussions? We can separately provide "best practices" but don't need to prescribe that in the specification.

@kevinb9n
Copy link
Collaborator Author

kevinb9n commented May 8, 2024

Yes, the actual design decision is only that we won't have any special solution for this. I don't think I read David's summary as implying more than that.

Separately, I think we want to recommend a particular approach, but that doesn't strictly need to be decided here.

@netdpb
Copy link
Collaborator

netdpb commented May 21, 2024

Decision: JSpecify provides no special way to declare that none of a type parameter's usages should have parametric nullness. Users can declare the type parameter with a non-null bound and add nullable projection to some usages, or they can declare it with a nullable bound and add nullable or non-null projection to all usages.

@netdpb netdpb removed the needs-decision-before-jar-1.0 Issues needing a finalized decision for the 1.0 jar release label May 21, 2024
@netdpb netdpb changed the title A type parameter that wants nothing to do with nullness (working decision: that's <T>) A way to declare a type parameter that wants nothing to do with nullness May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design An issue that is resolved by making a decision, about whether and how something should work. nullness For issues specific to nullness analysis.
Projects
None yet
Development

No branches or pull requests

6 participants