-
Notifications
You must be signed in to change notification settings - Fork 300
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
Kotlinc explicit non-nullability of varargs arguments #720
Comments
lazaroclapp
added a commit
that referenced
this issue
Jan 26, 2023
See #720 for a detailed explanation. Long story short: 1. `kotlinc` will add `@org.jetbrains.annotations.NotNull` as a declaration annotation for any argument that is non-null, which we pick up from jars when running with `AcknowledgeRestrictiveAnnotations=true`. 2. Unfortunately, it will mark code such as `open fun w(vararg args: Any?)` as having `@NotNull args` (meaning the array itself). 3. This is indistinguishable at the bytecode level from Java code such as `w(@NotNull Object... args)`, which we believe should be interpreted as marking the elements of `args` as being `@NotNull`. 4. This PR hacks `RestrictiveAnnotationHandler` to skip `@org.jetbrains.annotations.NotNull` on var args only. Additionally, our basic handling of var args is pretty broken. I went over our test cases and commented where I think our current behavior is different from the desired behavior as documented in #720 and #674. Foregoing fixing it for now, as I am worried about the breaking change and I think we want to avoid changing that before 0.10.9.
We have applied fix number 3 here. Wonder if we should just close this issue and move on or leave it open and report the potentially confusing annotation to Kotlin maintainers. |
IMO we can close this as long as #674 tracks the remaining known issues with our handling of varargs. We should track reporting the Kotlin issue separatley. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Context Part 1: Var args nullability on NullAway and JSpecify
Handling of nullability of var args in NullAway is admittedly a bit hacky (see #674). There is a clear answer on what the correct way to interpret
@Nullable
annotations is for type-use annotations, which we currently don't follow, namely:Means: the elements of the
varargs
array can be null, but the array reference itself is non-null (99.8% of the time, this is what we want, though it is possible in Java to pass a nullvarargs
array by e.g. passing(Object[]) null
)Whereas:
Can be used to indicate that the array
varargs
itself is@Nullable
, if absolutely needed.For declaration annotations (i.e. not type-use), the only valid position is:
And it seems reasonable to treat it equivalently to the same syntax for type-use, both for consistency and because it's more likely than not what the developer intends.
Note: We currently interpret the annotation above to mean both the array and its elements can be nullable, i.e. we read it as what, using type-use annotations, would be written as
foo(@Nullable Object @Nullable... varargs)
, but I think this is a bug/artifact of previous limitations and should be (carefully) fixed.Context Part 2: Kotlinc generated jars and restrictive
@NonNull
annotationsThe Kotlin compiler adds
@Nullable
and@NonNull
annotations to the bytecode it produces, but only declaration annotations at the full parameter level (i.e. no annotations on generics, elements of arrays, etc). So, code like:Becomes the equivalent of:
And:
Becomes the equivalent of:
(The annotations in question are
org.jetbrains.annotations.Nullable
andorg.jetbrains.annotations.NotNull
)This is generally a big help to NullAway, when configured with
-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=...
, as it can usekotlinc
added nullness information to reason about methods returning nullable or taking non-null arguments.The issue:
Unfortunately, the following Kotlin code:
Actually becomes:
Which is the equivalent of:
because
kotlinc
interprets the annotation as applying to the first parameterargs
as a whole and the var args array itself is non-null.The solution(s):
kotlinc
to change the default meaning of declaration annotations in var args to match NullAway's (desired) semantics. However, even if they agree with our interpretation, this would be a big breaking change to their jar ABI.foo(@Nullable Object... varargs)
with a declaration annotation asfoo(Object @Nullable... varargs)
with a type-use annotation. This is very ugly and confusion prone from the point of view of developers using either type of annotation, or worse, a mixture of both in different projects/targets.org.jetbrains.annotations.NotNull
when looking at a varargs argument.Neither option is perfect, but this is a rather long brain-dump to convince myself that the hack from Option 3 is the best we can do here (and to document why).
The text was updated successfully, but these errors were encountered: