[Xamarin.Android.Tools.Bytecode] Support JSpecify nullness annotations#1483
[Xamarin.Android.Tools.Bytecode] Support JSpecify nullness annotations#1483jonathanpeppers wants to merge 1 commit into
Conversation
Context: #1335 AndroidX `core` 1.16 switched from `androidx.annotation.NonNull` to JSpecify nullness annotations (`@NullMarked`, `@NullUnmarked`, `@Nullable`, `@NonNull`). Two structural mismatches kept the existing class-parse nullness pipeline from picking these up: 1. JSpecify is scope-based: `@NullMarked` on a class or `package-info` means "every reference type in this scope is non-null unless annotated `@Nullable`". class-parse previously only honored per-member declaration annotations. 2. JSpecify `@Nullable` and `@NonNull` are `@Target(TYPE_USE)`, so they live in `RuntimeInvisibleTypeAnnotations` (JSR 308) which class-parse did not parse. This commit adds the minimum-useful subset that correctly annotates the bulk of `@NullMarked` APIs: * Parse `RuntimeVisible/InvisibleTypeAnnotations`. The new parser understands every JSR 308 `target_info` variant well enough to read past it, but only `Field`, `MethodReturn`, and `MethodFormalParameter` feed into nullness. Annotations with a non-empty `type_path` are ignored (they describe inner types like `Map<@nullable K, V>` which the API XML schema cannot represent). * Also parse `RuntimeVisibleParameterAnnotations` for completeness. * `ClassFile.IsPackageInfo` plus `ClassPath.GetPackageInfo()` keep `package-info.class` available without emitting it as a regular class in the API XML. * `XmlClassDeclarationBuilder` computes "is this scope null-marked?" from class + package-info `@NullMarked`/`@NullUnmarked` (in both the Visible and Invisible annotation tables, because `@NullMarked` is `@Retention(RUNTIME)`). Inside a null-marked scope, reference-typed returns / parameters / fields receive `not-null="true"` unless a top-level TYPE_USE `@Nullable` overrides. Out of scope for this minimum slice (issues to track separately): * Module-level `@NullMarked`. * Inner-class scope inheritance from outer classes. * Method-level `@NullUnmarked` opt-outs (only class- and package-info level scopes are consulted today — a test pins down this limitation). * `type_path`-aware nullness for inner generic type arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Java.Interop PR Reviewer completed successfully! |
There was a problem hiding this comment.
Pull request overview
This PR extends Xamarin.Android.Tools.Bytecode to understand JSpecify nullness by parsing JSR-308 type annotations and applying @NullMarked scope defaults (class/package) when generating API XML, improving downstream C# nullable reference type output.
Changes:
- Added parsing for
RuntimeVisible/InvisibleTypeAnnotations(JSR-308) via newTypeAnnotationmodel and attribute readers. - Implemented JSpecify scope-default detection using
package-info.classand class-level annotations, and resolved nullness for returns/parameters/fields accordingly. - Added a focused test suite and Java fixtures to validate parsing and nullness emission behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Xamarin.Android.Tools.Bytecode-Tests/Xamarin.Android.Tools.Bytecode-Tests.csproj | Embeds new compiled class fixtures for JSpecify scenarios. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/JSpecifyAnnotationTests.cs | Adds coverage for package/class scope defaults, opt-outs, and type-annotation parsing. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/org/jspecify/annotations/NullUnmarked.java | Test stub for scope-default opt-out annotation. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/org/jspecify/annotations/NullMarked.java | Test stub for scope-default opt-in annotation. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/org/jspecify/annotations/Nullable.java | Test stub for TYPE_USE nullable annotation. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/org/jspecify/annotations/NonNull.java | Test stub for TYPE_USE non-null annotation. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/com/jspecifyunmarked/JSpecifyUnmarked.java | Test fixture for “no scope default” behavior. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/com/jspecifytest/package-info.java | Test fixture for package-level @NullMarked. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/com/jspecifytest/JSpecifyPackageMarked.java | Test fixture for package-marked nullness defaults and overrides. |
| tests/Xamarin.Android.Tools.Bytecode-Tests/java/com/jspecifytest/JSpecifyClassMarked.java | Test fixture for class-level @NullMarked path. |
| src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs | Computes JSpecify scope defaults and resolves slot nullness using declaration + TYPE_USE annotations. |
| src/Xamarin.Android.Tools.Bytecode/TypeAnnotation.cs | Introduces a parser/model for JVMS type annotations (target_info + type_path + annotation). |
| src/Xamarin.Android.Tools.Bytecode/ClassPath.cs | Retains/uses package-info.class during XML generation to apply package-level defaults. |
| src/Xamarin.Android.Tools.Bytecode/ClassFile.cs | Adds IsPackageInfo helper to identify package-info.class. |
| src/Xamarin.Android.Tools.Bytecode/AttributeInfo.cs | Adds support for visible parameter/type annotations and corresponding attribute readers. |
| bool? GetParameterNullness (MethodInfo method, IList<ParameterAnnotation>? annotations, int parameterIndex) | ||
| { | ||
| var ann = annotations?.FirstOrDefault (a => a.ParameterIndex == parameterIndex)?.Annotations; | ||
| if (ann?.Any (a => IsNotNullAnnotation (a)) == true) | ||
| return new XAttribute ("not-null", "true"); | ||
| return true; |
| .Select (p => new XElement ("package", | ||
| new XAttribute ("name", p), | ||
| new XAttribute ("jni-name", p.Replace ('.', '/')), | ||
| packagesDictionary [p].OrderBy (c => c.ThisClass.Name.Value, StringComparer.OrdinalIgnoreCase) | ||
| .Select (c => new XmlClassDeclarationBuilder (c).ToXElement ())))); | ||
| packagesDictionary [p].Where (c => !c.IsPackageInfo) | ||
| .OrderBy (c => c.ThisClass.Name.Value, StringComparer.OrdinalIgnoreCase) | ||
| .Select (c => new XmlClassDeclarationBuilder (c, GetPackageInfo (p)).ToXElement ())))); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Nicely structured addition. The JSR 308 target_info parser carefully reads past every variant (even ones not acted on) to keep the stream aligned, the package-info scope plumbing through ClassPath.GetPackageInfo is clean, and the new tests cover package/class scope, primitive exclusion, and @Nullable opt-out at each slot kind. Documenting the deferred limitations and pinning one (PackageMarked_NullUnmarkedMethod_RevertsToUnknown) with a test is exactly the right discipline. A few refinements before merge:
- Test uses the banned null-forgiving
!. The new test file enables#nullable enablethen sprinklesp!/typeAnn!to silence warnings — repo-banned, and it turns a missing element into anNREinstead of a clean assert failure. Assert non-null first. IsNullableAnnotationis asymmetric withIsNotNullAnnotation(4 providers vs 16). A top-level TYPE_USE@Nullablefrom an unlisted provider (e.g.RecentlyNullable, whoseRecentlyNonNullcounterpart is listed) is silently treated as non-null inside a@NullMarkedscope, emitting an incorrectnot-null="true".- Erased type-variable slots are over-marked.
<T> T get()(descriptor()Ljava/lang/Object;) andTparams/fields are markednot-nullin a null-marked scope; JSpecify gives type-variable usages parametric nullness. This generics gap isn't in the documented deferred list — handle via the genericSignatureor document + pin it.
💡 Suggestions (1)
NonNull.javais added but unused — the explicit TYPE_USE@NonNullresolution path has no test. Exercise it in an unmarked class.
CI: the dotnet.java-interop Azure Pipelines build was still queued at review time, so a green build isn't yet confirmed — please make sure it passes before merging.
Overall a solid, well-scoped first slice; the items above are mostly about hardening the nullable-resolution edges and closing a test gap.
Generated by Java.Interop PR Reviewer for issue #1483 · 1.1K AIC · ⌖ 50.1 AIC · ⊞ 34.6K
Comment /review to run again
| yield return a; | ||
| } | ||
|
|
||
| static bool IsNullableAnnotation (Annotation annotation) |
There was a problem hiding this comment.
🤖 IsNullableAnnotation recognizes only 4 providers, while IsNotNullAnnotation just above recognizes 16. The lists are asymmetric in a way that affects emitted nullness: e.g. androidx.annotation.RecentlyNonNull is in the not-null list but RecentlyNullable is absent here, and the same goes for the jetbrains/checkerframework/javax @Nullable variants. Inside a @NullMarked scope, a top-level TYPE_USE @Nullable from any unlisted provider isn't matched, so GetTypeUseNullness returns null, the slot falls through to the scope default, and it's wrongly emitted as not-null="true" — i.e. a nullable slot becomes a non-null C# reference. Consider mirroring the established not-null provider list so the opt-out is symmetric.
Rule: Disambiguate/return-type consistency; avoid incorrect not-null metadata
| "unannotated reference return in @NullMarked package must be not-null"); | ||
|
|
||
| var p = m.Element ("parameter"); | ||
| Assert.AreEqual ("true", Attr (p!, "not-null"), |
There was a problem hiding this comment.
🤖 #nullable enable and then leans on the postfix null-forgiving ! to silence the resulting warnings: Attr (p!, ...) here and at lines 82, 110, 128, 146, plus typeAnn! at 164/168. The repo bans the postfix ! operator, and it's also a worse test: if m.Element ("parameter") ever returns null, this throws a NullReferenceException instead of failing with a useful message. Assert first, then use the local — e.g. var p = m.Element ("parameter"); Assert.NotNull (p); and reference p afterward (or change Attr to take XElement?).
Rule: Never use the null-forgiving ! operator
| ta => ta.TargetType == TypeAnnotationTargetType.MethodReturn); | ||
| if (typeNullness.HasValue) | ||
| return typeNullness; | ||
| if (isNullMarked && IsReferenceTypeDescriptor (GetReturnDescriptor (method.Descriptor))) |
There was a problem hiding this comment.
🤖 not-null="true": <T> T get() has descriptor ()Ljava/lang/Object;, and a T parameter/field (same pattern at lines 631 and 645) erases to Ljava/lang/Object;. Per JSpecify an unannotated type-variable usage has parametric nullness — it follows the type argument and is not automatically non-null — so e.g. List<@nullable String>.get() would be emitted as non-null incorrectly. This generics gap isn't in the PR's documented "deliberately deferred" list. Consider detecting type-variable usages via the generic Signature, or at minimum document it and pin the current behavior with a test (as you did for method-level @NullUnmarked).
Rule: Confidently-wrong domain facts — verify JSpecify semantics
|
|
||
| @Target(ElementType.TYPE_USE) | ||
| @Retention(RetentionPolicy.CLASS) | ||
| public @interface NonNull { |
There was a problem hiding this comment.
🤖 💡 Testing — This @NonNull annotation is defined but never referenced by any test class, so the explicit TYPE_USE @NonNull resolution path (GetTypeUseNullness → IsNotNullAnnotation → return true) is never exercised: every not-null="true" assertion in the suite comes from the scope default, and every opt-out from @Nullable. Add a member annotated with @NonNull to an unmarked class (e.g. JSpecifyUnmarked) and assert it gains not-null="true", so the type-use NonNull branch is covered.
Rule: Bug fixes / new code paths need regression tests
Fixes #1335.
Why
JSpecify is becoming the standard way Java libraries (AndroidX, Guava, etc.) declare nullness. Unlike the older
@NonNull/@Nullabledeclaration annotations we already recognize, JSpecify usesTYPE_USEannotations plus a scope-default model (@NullMarkedon a package or class means "every reference here is non-null unless marked@Nullable"). The bytecode parser was missing two pieces required to consume any of this: it never read JSR 308 type annotations, and it had no notion of a scope default. As a result, generated bindings for JSpecify-annotated Java libraries were emitting reference types withoutnot-null="true", which downstream becomes nullable C# references.Approach
This change adds the minimum useful slice so that most APIs in a JSpecify-annotated library get correct C# nullable reference type annotations:
TypeAnnotationtype andRuntimeVisible/InvisibleTypeAnnotationsAttributeclasses that fully parse thetarget_infodiscriminated union (every variant is read past, even ones we don't act on yet, so the stream stays aligned) and skip pasttype_pathwhile remembering its length.ClassPathnow retainspackage-info.classfiles and exposesGetPackageInfo(packageName).XmlClassDeclarationBuildercomputesisNullMarkedfrom the class's own@NullMarked/@NullUnmarkedplus the package-info fallback, reading from bothRuntimeVisibleAnnotationsandRuntimeInvisibleAnnotations(JSpecify scope annotations are@Retention(RUNTIME)).GetMethodReturnNullness/GetParameterNullness/GetFieldNullnesshelpers combine declaration-level@NonNull, top-levelTYPE_USE@Nullable/@NonNullannotations, and the scope default. Reference-typed slots in a null-marked scope default tonot-null="true"; explicit@Nullableopts back out.Trade-offs and deliberately deferred
To keep the change small enough to land, the following are out of scope and tracked as known limitations (one test,
PackageMarked_NullUnmarkedMethod_RevertsToUnknown, pins down the current behavior so it's easy to find when extending):@NullUnmarkedis not honored (only class and package scope).@NullMarkedis not honored.Map<@Nullable K, V>for inner generic type arguments) is intentionally only respected at the top of the type. The parser readstype_pathcorrectly so@Nullable String[]vsString @Nullable[]are distinguished byAppliesToTopLevelType(type_path_length == 0), which is the case that matters for surface API nullness.Tests
10 new tests in
JSpecifyAnnotationTests.cscovering package-level@NullMarked, class-level@NullMarked, primitive exclusion,@Nullableopt-out at each slot kind, control unmarked classes, and direct verification that the new type-annotation attribute parses. All 92Xamarin.Android.Tools.Bytecode-Testspass.