[Xamarin.Android.Tools.Bytecode] Support JSpecify nullness annotations#1483
Open
jonathanpeppers wants to merge 5 commits into
Open
[Xamarin.Android.Tools.Bytecode] Support JSpecify nullness annotations#1483jonathanpeppers wants to merge 5 commits into
jonathanpeppers wants to merge 5 commits 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>
Member
Author
|
/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. |
![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
- Mirror non-null annotation provider list in IsNullableAnnotation so an unlisted-provider TYPE_USE @nullable cannot fall through to the @NullMarked scope default and become not-null="true". - Merge ParameterAnnotation entries across visible+invisible tables when resolving parameter nullness (was FirstOrDefault picking one). - Skip the null-marked scope default for unannotated type-variable usages (parametric nullness per JSpecify); detected via the generic Signature. - Cache GetPackageInfo() per package in ClassPath.ToXElement; was being rescanned per class. - Test: replace null-forgiving `!` with throw-on-null helpers; add Unmarked_ExplicitNonNullAnnotation_IsHonored and PackageMarked_TypeVariableUsage_HasParametricNullness coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
/review |
|
✅ Java.Interop PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Re-review of the JSpecify nullness support (second /review pass).
Verdict: ⚠️ Needs Changes
Prior review follow-up — all 6 issues resolved ✅
Verified that commit 284c157 correctly addresses every item from the earlier review:
- Parameter-annotation merge now iterates all
ParameterAnnotationentries instead ofFirstOrDefault. packageInfosis built once inToXElement(the O(n2) per-class lookup is gone).IsNullableAnnotationnow mirrors the provider list ofIsNotNullAnnotation— the two omitted entries (javax.validation.constraints.NotNull,lombok.NonNull) genuinely have no@Nullablecounterpart, so that asymmetry is correct.- Tests replaced the null-forgiving
!with throw-on-null helpers. - Type-variable parametric nullness is handled via
IsTopLevelTypeVariableSignature(+PackageMarked_TypeVariableUsage_HasParametricNullness). - Added
Unmarked_ExplicitNonNullAnnotation_IsHonoredto exercise the TYPE_USE@NonNullpath.
The JVMS 4.7.20 target_info parsing in TypeAnnotation.cs is correct for every target kind and keeps the stream aligned, and the new test coverage is strong. Nice work.
New findings
⚠️ Correctness gap (1) — declaration-level@Nullableis not honored inside a@NullMarkedscope, so members explicitly annotated with a declaration-style@Nullable(e.g.androidx.annotation.Nullable) are wrongly emitted asnot-null="true"while declaration@NonNullis honored. Inline comment onHasDeclarationNotNullAnnotation. This is the one I'd like resolved (fix, or document/test as a known limitation) before merge.- 💡 Dead
'T'branch inIsReferenceTypeDescriptor(inline). - 💡 Missing test for the non-top-level
type_pathbranch ofAppliesToTopLevelType(inline).
Other notes (non-blocking)
- Behavioral change worth noting in the PR description:
package-infoclasses are now filtered out of class output (.Where (c => !c.IsPackageInfo)), which drops their entries from the generated API XML. Looks intentional and correct — just flagging it. - An unrecognized
target_typeinTypeAnnotationthrowsNotSupportedException; sinceClassPath.Loadcatches per class, one unexpected (e.g. future-JVMS) byte would drop the whole class rather than skipping the attribute as the oldUnknownAttributepath did. All current spec targets are handled, so this is a minor forward-compat robustness note, not a blocker. - CI (
dotnet.java-interop) is currently pending — it must go green before merge.
The 💡 items are optional; the @Nullable asymmetry is the only substantive change request.
Generated by Java.Interop PR Reviewer for issue #1483 · 1.7K AIC · ⌖ 47.4 AIC · ⊞ 34.6K
Comment /review to run again
- Honor declaration-level @nullable inside a @NullMarked scope: added HasDeclarationNullableAnnotation and consult it (and the per-parameter IsNullableAnnotation set) before applying the scope default in all three Get*Nullness paths. Previously a member annotated only with a declaration-style @nullable (e.g. androidx.annotation.Nullable) was wrongly emitted as not-null="true". - Drop the unreachable `'T'` branch in IsReferenceTypeDescriptor; all callers pass erased JVM descriptors, which never start with `T`. - Add tests: PackageMarked_DeclarationNullable_OverridesScope (uses an androidx.annotation.Nullable stub) and PackageMarked_NestedNullable_DoesNotLeakToContainer (locks in that List<@nullable String> field still emits not-null="true"). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
/review |
|
✅ Java.Interop PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Review: JSpecify nullness annotation support
Verdict: dotnet.java-interop Azure pipeline is still pending/queued on the head commit (the two green checks are the Copilot reviewer + CLA, not the build/test run) — final merge should wait for that to go green.
Findings
- 1
⚠️ Performance + latent correctness (inline,XmlClassDeclarationBuilder.cs):method.GetSignature ()is uncached and re-parsed once per parameter in the@NullMarkedpath; the already-alignedparameters[i].Type.TypeSignatureshould be used instead. This also avoids a real (if narrow) index-misalignment when the descriptor and generic-signature parameter lists differ in length. Same re-parse pattern affects the return and field paths — flagged once inline to avoid piling on.
Concurrence (already raised, still open)
- I agree with the existing unresolved note on
JSpecifyAnnotationTests.cs(~line 95):PackageMarked_NullUnmarkedMethod_RevertsToUnknownis misnamed — the assertion verifies the value stays not-null (method-level@NullUnmarkedis intentionally not honored), the opposite of "reverts to unknown". Worth a rename + a clarifying comment so the intentional behavior is obvious. Not re-posting inline to avoid a duplicate thread.
Positive callouts 👍
- Correct JSR 308
type_annotationstream handling inTypeAnnotation.cs, with all JVMS target types enumerated and a clear fail-fast on unknown ones. - Sensible precedence model (declaration
@NonNull/@Nullable→ TYPE_USE →@NullMarkedscope default), with primitives correctly excluded and type-variable parameters handled via the generic signature. - Strong test coverage: package-vs-class scope, declaration vs type-use, nested/parametric nullability, and the deferred-cases documented in comments.
- Good responsiveness to prior review rounds — the earlier substantive issues (provider-list symmetry,
GetPackageInfocomplexity, declaration-@Nullablehandling) are all addressed.
Nice work overall — the one inline item is a straightforward simplification that improves both speed and robustness.
Generated by Java.Interop PR Reviewer for issue #1483 · 1.3K AIC · ⌖ 46.9 AIC · ⊞ 34.6K
Comment /review to run again
- Use the already-parsed, descriptor↔signature-reconciled ParameterInfo.Type.TypeSignature and method.ReturnType.TypeSignature in the type-variable check instead of method.GetSignature(). GetSignature() allocates a fresh MethodTypeSignature each call, so the prior code re-parsed once per parameter. It also indexed the raw signature parameter list directly, which misaligns when descriptor and signature parameter counts differ (e.g. synthetic params). - Rename PackageMarked_NullUnmarkedMethod_RevertsToUnknown to PackageMarked_MethodLevelNullUnmarked_IsNotYetHonored to match the assertion (the slot stays not-null because method-level @NullUnmarked is intentionally not honored yet). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
/review |
|
✅ Java.Interop PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Java.Interop PR Review
✅ Looks good — 2 minor 💡 suggestions, no blocking issues. (Verdict assumes the queued Azure DevOps build goes green — see CI note below.)
I formed my own read of the bytecode-parsing and nullness-resolution logic before reviewing the PR history, and the code holds up well.
Strengths
- JSR 308
target_infoparsing matches JVMS §4.7.20 across every variant (each is read past so the stream stays aligned), andtype_pathis consumed correctly withAppliesToTopLevelTypegating top-level-only nullness. - Nullness precedence is consistent across the return / parameter / field paths: declaration
@NonNull→ declaration@Nullable→ TYPE_USE →@NullMarkedscope default. Scanning both Visible and Invisible annotation tables also fixes a latent gap where RUNTIME-retention declaration annotations were previously ignored. - Type-variable usages correctly get parametric nullness via the generic
Signaturerather than the erased descriptor — a subtle JSpecify detail that's easy to get wrong. - Deferred limitations (method-level
@NullUnmarked, module scope, inner-class inheritance, nestedtype_pathnullness) are documented and pinned with tests so they're easy to find when extended. - Strong, behavior-focused test coverage: package/class scope, primitive exclusion,
@Nullableopt-out at each slot kind, declaration-vs-type-use, nested@Nullable, type-variable parametric nullness, and direct attribute-parse verification.
Suggestions (both non-blocking, see inline):
ClassPath.ToXElementcan buildpackageInfosin O(n) by reusingpackagesDictionary[p]instead of rescanning all class files once per package.TypeAnnotationnow throws on an unknowntarget_typewhere these attributes were previously skipped — worth a conscious fail-fast-vs-tolerate decision sinceclass-parsereads arbitrary jars.
CI: license/cla ✅. The dotnet.java-interop Azure DevOps build is queued/pending on b6f9e6cb — not failing, but not yet green, so treat this as conditional rather than a final build sign-off.
Nice iteration through the earlier rounds — all 11 prior review threads look correctly resolved.
Generated by Java.Interop PR Reviewer for issue #1483 · 1.3K AIC · ⌖ 46.9 AIC · ⊞ 34.6K
Comment /review to run again
GetPackageInfo() rescans the full classFiles list per call, so the prior packageInfos ToDictionary was O(packages * classes). Reusing packagesDictionary[p] makes it O(n) — every class in package p is already grouped there, including package-info.class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.