Diagnose intersection-bound annotation homogenization; add disabled regression test#1875
Draft
wmdietl wants to merge 2 commits into
Draft
Diagnose intersection-bound annotation homogenization; add disabled regression test#1875wmdietl wants to merge 2 commits into
wmdietl wants to merge 2 commits into
Conversation
…nization AnnotatedIntersectionType.copyIntersectionBoundAnnotations summarizes the bounds' annotations into the primary location via addAnnotations, whose intersection override calls fixupBoundAnnotations and copies that primary back onto every bound. An unannotated bound thus loses its default qualifier and a differently-qualified bound is overwritten, order-dependently, homogenizing the per-bound annotations that downstream checkers (e.g. the JSpecify reference checker) need. The narrowest fix (add to the primary location only, bypassing the writeback) is a single line, but it changes intersection-bound defaulting and subtyping semantics that the standard Nullness Checker relies on: checker/tests/nullness Issue868.java and Issue3349.java deliberately encode the homogenized behavior, including the explicit.annotation.ignored advisory from BaseTypeVisitor.checkExplicitAnnotationsOnIntersectionBounds. Reconciling that with per-bound preservation is a semantics decision beyond a contained bug fix, so the fix is not landed here. Commit the regression test as an @ignore'd JUnit class plus its EvenOdd-checker input, documenting the desired per-bound behavior. See cf-tasks/task-2-findings.md for the full diagnosis, instrumentation evidence, and proposed patch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
1 participant
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.
Diagnosis of the intersection-bound annotation homogenization bug, with a disabled regression test. No production change is included — the one-line fix exists but flips deliberate Nullness Checker test expectations, so it needs a maintainer semantics decision first. Draft until that decision is made.
Diagnosis
AnnotatedIntersectionType.copyIntersectionBoundAnnotations()(AnnotatedTypeMirror.java:2982) is documented as bounds → primary, but its finaladdAnnotations(annos)hits the intersection'saddAnnotationoverride, which callsfixupBoundAnnotations()— and that writes the primary back onto every bound viareplaceAnnotations. So the first bound's qualifier (per hierarchy) is round-tripped onto all bounds: a differently-qualified bound is overwritten and an unannotated bound loses its default. First-bound-wins is exactly the order-dependence observed through the JSpecify reference checker, and it is visible today inframework/tests/lubglb/IntersectionTypes.java(call1/call2: same bounds, opposite order, only one flagged).Both construction paths funnel through it:
TypeParamElementAnnotationApplier.applyUpperBounds(line 178) andTypeFromTypeTreeVisitor.visitTypeParameter(line 232). No cache or defaulting path is involved.The candidate fix, and why it is not landed
Adding the collected annotations to the primary only (bypassing the writeback, e.g. via
super.addAnnotation) fixes the regression test and:framework:test(after updating 3 test expectations), but:checker:NullnessTestfails with 4 flipped diagnostics:Issue3349.java:8andIssue868.java:42/43/48.Issue868.javais a deliberate spec of the homogenized behavior, including theexplicit.annotation.ignoredadvisory fromBaseTypeVisitor.checkExplicitAnnotationsOnIntersectionBounds, which encodes the homogenization contract. The JSpecify case (unannotated bound should default) is structurally identical toIssue868test2/test3 (which accept homogenization), so no mechanical fix satisfies both — it is a defaulting/subtyping policy decision for intersection bounds.Landing the fix would require updating
Issue868.java,Issue3349.java,MultiBoundTypeVar.java,lubglb/IntersectionTypes.java, and reconsideringcheckExplicitAnnotationsOnIntersectionBounds.Full write-up (including the candidate patch) in
cf-tasks/task-2-findings.mdin the local workspace.This PR contains
framework/tests/disabled-intersection-bounds/IntersectionBoundAnnos.java— EvenOdd-checker input asserting per-bound behavior, with an order-dependence guard (@Oddbound first vs. second).DisabledIntersectionBoundAnnosTest.java—@Ignored runner pointing at the diagnosis.Tests:
:framework:testand:checker:NullnessTestpass unmodified (the disabled test compiles but is skipped).🤖 Generated with Claude Code
https://claude.ai/code/session_01EaHhVqLoJukm4kKsE7UfY4