-
Notifications
You must be signed in to change notification settings - Fork 27
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
@PolyNull #79
Comments
I already said it somewhere, but cannot find it now. |
I don't agree that I agree that there exist properties that |
@amaembo Your earlier comments are at the top of the issue I linked to, #3. If we were to become convinced that I'm not sure whether |
Quick data dump of very quickly grepping through:
The first thing that jumps out to me from the IntelliJ annotations is that there's only a single occurrence of its The first thing that jumps out to me from the Checker Framework annotations is that there aren't as many
I do know of other cases for
I still don't have a strong opinion here, but this has nudged me slightly from " |
Also, more anecdotally, among the usages of
Both are often signs that the API could have used a different feature instead. Still, the majority of use cases (not a ton, but it comes up) look legitimate at first glance. |
Re: the previous-previous comment, I'm a little surprised, not that I expected it to be much more popular than this. I know that other utility libraries that don't have Guava's null-hostile attitude are more fond of null-passthrough, and might have much more use for this. What will be the consequence of not having this? I am not sure how good the castNotNull() outcome is. Will users complain to their checker vendor about it, and will vendors start to hardcode lists of APIs that have this property? Or, we could share those with each other as data files. In fact, those data files might as well be the same as externally-applied-annotation data files, meaning that a definition for "PolyNull" would have to exist somewhere but just not be part of the artifact you can actually depend on from code. Maybe that seems like a weird outcome, but I can imagine there being quite a few things that all fall into a bucket like this, and we really wouldn't want to have 29 real annotations. Random other point: applying |
Hmm, I think I'm seeing it the opposite way: |
For Why would it have an effect on a non-nullable |
We're probably looking at multiple things differently. Here's what I'm picturing: @DefaultNotNull
public final class Class<T [extends @NotNull Object]> {
...
public @Nullable T cast(@Nullable Object obj) { ... }
} And what public @PolyNull T cast(@PolyNull Object obj) { ... } ...which effectively provides... public [@NotNull] T cast([@NotNull] Object obj) { ... }
public @Nullable T cast(@Nullable Object obj) { ... } |
Right, I was instead thinking of the case where the type parameter is nullable, and thus intentionally admits both nullable and non-nullable instantiations. And that the cast method doesn't want to override that. It wants to not accept null and not return null when the type argument is non-nullable. And when the type argument nullable, that's the only place we need a refinement: it should accept nullable (or non-nullable), but it should return the same kind of nullness that was passed in. (This might be a weird thing to do for Class itself, but if we don't then see issue #78...) |
So I'm saying I would not expect |
I think I am still missing something on both this and #78. I'd suggest a code example, but I haven't gotten my head around the significance of the one on #78, so maybe that's not what I need. Hmm. As noted above, I suspect that Maybe we're mostly just agreeing on the weirdness of |
Instead of thinking of Let's take @PolyNull Object id(@PolyNull Object in) that is conceptually like: <<@T>> @T Object id(@T Object in) where I use the In contrast, each use of @NullnessUnspecified Object foo(@NullnessUnspecified Object in) corresponds to: <<@S, @T>> @S Object foo(@T Object in) This is similar to how each wildcard is independent of each other and captures some underlying type in a call. Thinking about class Box<S extends @Nullable Object, T extends @Nullable Object> {
S convert(T in) ...
@PolyNull S bar(@PolyNull T in) ...
} Method Maybe the correspondence between |
Thanks. Among other things, that got me thinking more about One change that might make This can come in handy in two related cases:
One way to look at this definition of public static <T extends @Nullable Object> T getFirst(
Iterable<? extends T> values, T defaultValue); But on the occasion that we want it for an instance method, we're out of luck -- except in the specific case of adding nullness, and then only if:
(Call it [edit: I keep getting myself confused here :( I do see that I claimed that |
Another thing that the This of course isn't useful for concrete types like [@DefaultNullnessUnspecified]
interface Foo<T, U extends T> {} When this class is annotated for nullness, it could have:
So maybe we treat (The idea of having 3 possibilities might(?) fall more naturally out of the |
@cpovirk thank you for the research!
For
Probably you are speaking about methods like |
We may emulate complex nullability rules with something more type-system like instead of contract language or PolyNull annotation. E.g.: @NullityParameter("T") @Nullity("? extends T") orElse(@Nullity("T") other); The Something more complex is also possible: @NullityParameter({"VAL", "DEF"}) @Nullity("? extends VAL & DEF") String coalesce(@Nullity("VAL") String value, @Nullity("DEF") String defaultValue); |
Thanks! I had seen some inferred annotations before but didn't realize it was happening even for classes that aren't being built as "part of the project." (For my reference: announcement of bytecode inference, documentation of It might someday be interesting to know which methods have an inferred contract like |
To state the obvious (which nevertheless escaped me until now): We also see "qualifier parameter"-like behavior from straight-up parametric nullness: class Foo<T extends @Nullable Object> {
T foo(T in) { ... }
} It's reassuring to think that this, The main difference may be: Does an operation need to be valid for...
And then there's a question about whether [edit: I think I later saw that this came up somewhere already. Sorry for forgetting about that.] |
I will just state that I view ideas like the |
edit: No, it doesn't need it. |
Belatedly following up on mernst's comment: Hits from Daikon: The first is the classic "null passthrough." Its callers are already null-checking the result, so they could null-check the parameter instead. (The parameter comes from a field, though, so, if they're using The other two are more interesting. In those files, I see: First, concatenation of arrays. When I saw that, I was going to say:
But it's actually not that simple. The method needs a Guava's Second, there are some more complex null-passthrough-like cases. For example, Hits from plume-util:
The first file is the classic "null passthrough." (As the name suggests, it's for interning. In Guava's The other 2 files are arrays. Arrays are weird in general. My guess is that all those files would actually work fine if we replaced |
Also: I think Finally: I forget if I've mentioned this explicitly before or not, but I suspect that a
|
It seems very likely to me now that we will ship a version of our product without |
I have a few other things I've been considering saying here, but I'll limit myself to 2 :)
|
Your answers to my last couple July 2 questions seem encouraging to me: this seems to suggest that there can be a reasonably well-behaved JSpecify without a If this is correct, it's a win, I think. |
#214 raises the idea of First: That could be used to support "unrelated" pairs of
In that example, it would be nice to be able to replace the 4 usages of Second: We should think about how this interacts with aliasing/implies. As we've noted in other discussions, we're not sure if there will be a good way to say that one annotation implies another when annotation elements (like Third: I worry that |
With two seconds' thought, it sounds like we have a pretty smooth ramp toward that, i.e. if we released a |
Here's an example where we'd put For
And default values are one of the canonical use cases, so I'd want it to generate:
Users do in fact pass |
protobuf followup: I am a little surprised to see very few errors in our codebase if I just use So I would still expect the average user to get by tolerably well with a checker that just implements special cases for APIs like |
A usage in Caffeine is, @PolyNull V get(K key, Function<? super K, ? extends @PolyNull V> mappingFunction); This corresponds to This differs from Guava's cache which instead throws an exception. This was a difficult decision on being null hostile vs real data being absent. While I prefer the null approach, that was settled by the addition of compute methods. Given the Guava's philosophy of being a natural extension to the Collections Framework, Caffeine shifted to that newer style. A quirk is
It wouldn't be awful if |
I just came across another pattern of I saw some methods along the lines of |
I believe |
For some reason, I was thinking of this again recently. I'll dump my dense notes here for possible future cleanup: I was tying this question to #69: If I have a type-variable usage
How do we "let in" (This may be important for getting the types right at all. That may make it more important than the rest of #69, which is about avoiding some "But why?" warnings or errors.) |
One thing mentioned earlier is the question of whether (That's all mostly just to tie together a few existing posts. The main new part is that there are other |
Actually, an internal discussion with kevin1e100 just touched on the idea that we might need that for casts, too: If you have an expression of type [edit: This might be especially true if you assign the result of the cast to a |
One thing that we've discussed but seemingly not written about yet here is how Kotlin reduces the need for
|
(The point here isn't that JSpecify shouldn't continue to be open to adding |
I despise PolyNull. Some probably have seen me on reddit openly rant about it so apologies if you are seeing me again. One of the reasons why we (my company at least) see way less NPE is because PolyNull APIs are nowhere near as wide spread as they used to be. I understand folks are going to complain about PolyNull and with the exception of The problem with PolyNull in the context of the JDK is that it seems very easy to assume something is PolyNull when it very well might not be in the future. After all the JDK is not annotated. For example most assume In fact I think making Map<String, SomeNonNull> m =...
// yes this is painful but it is safer
SomeNonNull n = requireNonNullElse(m.get("a"), other); I admit that Caffeine has a strong argument for PolyNull but JSpecify really needs to be like done now and it needs to be simple for adoption and most importantly easier to annotate existing APIs. Having Without the JDK I don't know what the plan is for external annotation but I feel adding more annotations is going to make this harder, longer and possibly error prone... |
BTW speaking of Caffeine
It is funny you mention that because this confused me when going to see if we were using Caffeines getOrDefault. private static final LoadingCache<CacheKey, ParsedSql> cache = Caffeine.newBuilder()
.maximumSize(100)
.<CacheKey, ParsedSql>build(new CacheLoader<CacheKey, ParsedSql>() {
@Override
public @NonNull ParsedSql load(
CacheKey key)
throws Exception {
return _parseSql(key.sql, key.config);
}
});
private static ParsedSql _parseSqlCache(
String sql,
SqlPlaceholderParserConfig config) {
return requireNonNull(cache.get(new CacheKey(config, sql)));
} If I don't have that
And it does not because Eclipse still cannot read Checkers default annotation placement: @DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.FIELD)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.PARAMETER)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.RETURN)
package com.github.benmanes.caffeine.cache; But originally I did not read that error or it indeed changed on version upgrade as Ben alluded to from The reality is I just haven't gone and externally annotated (EEA) Caffeines type parameters yet. Anyway I thought that might be just another example of how much the pain of going around annotating existing libraries is and how potentially error prone it can be. |
@agentgt fyi ben-manes/caffeine#594 in v3.0.5. It seemed reasonable to follow Guava's lead which adopted CheckerFramework's annotations as a replacement to jsr305's, where I had no strong opinion and prefered having those who want to discuss it to come to the like minded Guava/JSpecify folks than to me. 🙂 |
@ben-manes Yes I agree. I didn't mean to make it out as bad choice because I think the first time around when I did it I was annoyed probably similar to the kotlin devs about It was a safe choice and did not break anything on our end. My jaded tone was more for the libraries that either do not provide any sort of annotations or incorrectly annotated... (well that and Eclipse but Eclipse kind did ok here). |
Sorry, I'm ignoring the posts above for the moment. I haven't caught up yet, but I wanted to dump an unrelated thought. I'd forgotten another way that Kotlin can get I should look more at what happens if you write a Java API with one set of nullness annotations in a superclass (and/or interface?) and then a Java override with the other set.... I had thought that Kotlin responded by interpreting the API as having platform types, but I thought I might have seen otherwise in some cases. I'd need to look. (Might it matter whether the return type matches?) And even if it does "work," I would be hesitant to rely on what might be a bug, given that the platform-type behavior seemed to be the intended one. |
I also want to dump a link to a Guava discussion in which |
Apparently C# took the approach that you can apply their equivalent of That's a nice simplification if you can get away with it. (I do wonder if they could have let you annotate the parameter, instead, so that you wouldn't have to refer to the parameter name with a string?) It definitely does solve the common case of null passthrough, but I was surprised at first glance at how many of the (Also, more generally: C# had to do a lot of stuff to support its |
For starters I think PolyNull only makes sense if you mark at least one in-type and at least one out-type (see our wonderful glossary doc). So the questions are
|
Informally, and approximately: a method or constructor with any occurrences of
@PolyNull
in its signature is treated as if it is two distinct overloads: one having all occurrences of@PolyNull
replaced with@Nullable
, the other having all replaced with@NotNull
(under the fantasy that nullness information can actually be used for overload resolution.)All the cases I'm aware of this being useful involve applying
@PolyNull
to at least one in-type (e.g parameter type) and at least one out-type (e.g. return type).The alternative (if this annotation is not available) is that these parameters must be marked
@Nullable
; it will be like the previous situation but with the@NotNull
overload missing. Consumers passing in a not-null expression (which should generally be common) will get a non-nullable result that the compiler will not know is non-nullable.A bit of early discussion was in #3.
cpovirk edit: various cases to consider if we end up including
@PolyNull
:The text was updated successfully, but these errors were encountered: