Resolve a name that names a type variable, and use the bound when deciding relevance - #8246
Merged
Merged
Conversation
Resolving one type name looks up many candidate names -- one per enclosing type declaration, one per import, one per on-demand import, one in the same package, one in `java.lang`, and one for the name as a fully-qualified name. Most of those names name no type, and a client that resolves many names looks up the same names over and over. Add an overload of `resolveTypeName` that takes a cache, and route every lookup through it, so that a client that resolves many names pays for each distinct name only once. The cache records a lookup that finds no type, which is the common case. The existing one-argument overload is unchanged from a caller's point of view; it allocates a cache that lives for the one call. This is a performance change; it does not change any result. A cache must not be reused across annotation processing rounds or across `Elements` instances, because a name that names no type in one round might name a generated type in a later round; the Javadoc says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
To avoid cluttering an ajava file, whole-program inference does not print the invisible qualifiers. It suppressed them by overriding the pretty-printer's three `visit(...AnnotationExpr)` methods to return without printing. By the time the pretty-printer visits an annotation, it has already printed the whitespace that separates the annotation from what follows it, so each suppressed annotation leaves a stray space or blank line: `java.util. Date`, `static double`, `String [] []`. That partly defeats the purpose of not printing the annotation, which is to reduce clutter. Instead, remove the annotations that should not be printed from a clone of the compilation unit, and print that. The pretty-printer then outputs no separator for them. The clone is necessary because the removal is a side effect, and the AST is printed once per checker that was run. No test output changes: the test checkers declare no invisible qualifier, so nothing is removed in the test suite. This refactoring is worthwhile on its own for the stray whitespace it fixes for a checker that does declare one, and it is a prerequisite for omitting irrelevant annotations, which would otherwise leave the same stray whitespace on every annotation it omits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When a checker declares `@RelevantJavaTypes`, whole-program inference could write, into an `.ajava` file, an annotation on a Java type that the checker treats as irrelevant. Such an annotation is clutter: omitting it does not change the result of type-checking. Omit such an annotation when writing the `.ajava` file. The test is conservative: it discards an annotation only when the annotation is definitely irrelevant where it appears. Relevance constrains the Java types on which a qualifier may be *written*, so it says nothing about a declaration annotation, even one that is also a type qualifier. JavaParser attaches an annotation that precedes a declaration's type to the declaration rather than to the type, so the two cases cannot be told apart from the AST alone. Mark each annotation that inference adds as a declaration annotation with a JavaParser `DataKey`, and always retain it. Update the `ainfer-relevance` goal files and the test inputs' comments, which previously recorded the old behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`JavaParserUtil.resolveTypeName` did not model the scope of a local class or of an anonymous class, so a name that such a class declares or inherits resolved to a same-named type that is declared outside it. If that type is irrelevant, inference silently discarded a correct annotation. `TypeDeclaration.getFullyQualifiedName()` is part of the problem: for a local class `Foo` in `Outer`, it returns "Outer.Foo", which `Elements` resolves to a member type of `Outer`. Model these scopes: * A local class shadows every type of the same name, and `Elements` cannot look up a local class, so return null (that is, be conservative). * Likewise for a member type of a class that `Elements` cannot look up: a local class, an anonymous class (including the body of an enum constant), or a class nested within one. * Such a class also inherits its supertype's member types. Resolve the supertype and search it, which is precise rather than conservative. The supertype names are excluded from the class's own body scope, both because that is the Java rule and because it bounds the recursion. Add `nameableFullyQualifiedName`, which returns a fully-qualified name only when `Elements` can look it up, in place of `TypeDeclaration.getFullyQualifiedName()`. Resolving a supertype's name multiplies the number of name lookups, so pass a single cache from the ajava writer to every call, rather than letting each call allocate one that it discards. Add tests for a local class, an anonymous class, an enum constant's body, and a member type inherited into a local or anonymous class. Each test loses an annotation if its part of this fix is reverted. `InheritedTypeShadows` also shows that the supertype search is precise: an annotation on an inherited member type that is irrelevant is still omitted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… in scope A class's member types, declared and inherited, are in scope only in its body -- not in its annotations, its type parameter section, or its supertype names, and not in the arguments of the object creation expression that declares an anonymous class. Test the child of the enclosing declaration that contains the name, rather than testing only whether the name is the supertype name. Also include the implicit superclass `java.lang.Enum` in an enum's supertypes, because it declares the member type `Enum.EnumDesc`. Add tests for a name in an anonymous class's arguments and for a member type that an unnameable enum inherits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`resolveMemberType` returned any non-private member type that it found in a supertype. A package-private member type is inherited only within its own package, so a package-private member type of a supertype in a different package was wrongly returned. Also, a declaration hides what its declaring type would otherwise inherit even when the declaration itself is not inherited, so the search must not continue past it into that type's supertypes. Either error made `annotationIsRelevant` ask about the wrong type, which could discard a relevant annotation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every other `ainfer*Generate*` task deletes the WPI output directory with `wpiOutputDirectory()` and `DirectoryDeleter`. This task still inlined an equivalent loop, which the merge of the commit that introduced those helpers left behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-variable-upper-bound
A use of a type variable is relevant exactly when the type variable's upper bound is relevant, so resolve such a use to its bound rather than conservatively retaining every annotation that is written on it. `JavaParserUtil.resolveTypeVariableName` answers which type variable a name refers to. It shares its scope walk with `resolveTypeName`, so the two agree about which declaration a name refers to. That walk now searches a type declaration's declared member types before its type parameters, because a declared member type shadows a type parameter of the same name -- whereas a member type that the declaration merely inherits does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tests cover which member type a simple name refers to, which depends on accessibility and on hiding, and when a name refers to a type variable rather than to a type declaration that shadows it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When the checker supports no invisible qualifier, `removeUnprintedAnnotations` has no effect, so skip both it and the clone that it requires. Also, walk the AST rather than building a list of every node in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ork-fork-mernst-branch-ajava-remove-annotations-from-ast into ajava-omit-irrelevant # Conflicts: # framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java
…nst-branch-ajava-omit-irrelevant into resolve-type-name-local-anonymous
…ork-fork-mernst-branch-resolve-type-name-local-anonymous into resolve-type-name-member-scope
Their computation is reflective, and they do not change over the lifetime of a `WholeProgramInferenceJavaParserStorage`. The computation is lazy rather than in the constructor, because `getSupportedTypeQualifiers()` might not yet yield its final result when the storage is constructed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-fork-mernst-branch-resolve-type-name-member-scope into resolve-member-type-inheritance
…k-fork-mernst-branch-resolve-member-type-inheritance into relevance-type-variable-upper-bound
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
…ork-fork-mernst-branch-ajava-remove-annotations-from-ast into ajava-omit-irrelevant
…nst-branch-ajava-omit-irrelevant into resolve-type-name-local-anonymous
…ork-fork-mernst-branch-resolve-type-name-local-anonymous into resolve-type-name-member-scope
…-fork-mernst-branch-resolve-type-name-member-scope into resolve-member-type-inheritance
…lve-type-name-memoize
…k-fork-mernst-branch-resolve-member-type-inheritance into relevance-type-variable-upper-bound
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
…-mernst-branch-resolve-type-name-memoize into resolve-type-name-local-anonymous
…a-remove-annotations-from-ast
…ork-fork-mernst-branch-ajava-remove-annotations-from-ast into ajava-omit-irrelevant
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
…lve-member-type-inheritance Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…k-fork-mernst-branch-resolve-member-type-inheritance into relevance-type-variable-upper-bound
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
…vance-type-variable-upper-bound # Conflicts: # framework/src/main/java/org/checkerframework/framework/util/JavaParserUtil.java
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
Fix the `anno.on.irrelevant` warning in `IrrelevantTypeVariable.java` with `@SuppressWarnings`, the idiom that the other tests in this directory use, rather than by deleting the file from the validation pass in `build.gradle`. The test now runs in both passes. `resolveName` no longer returns a TypeParameter for a name with more than one component, such as `T.Inner`: a type variable has no member types, so such a name names nothing. This moves the check from the caller into the resolver. Make `ResolvedName` and `resolveName` public, so that a client that needs to know both whether a name names a type and whether it names a type variable can resolve it once. `WholeProgramInferenceJavaParserStorage.typeToTypeMirror` walked the enclosing scopes twice for every such name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
…vance-type-variable-upper-bound
…ework-fork-mernst-branch-relevance-type-variable-upper-bound into javaparserutil-unit-tests
Commit 72813fd changed `resolveName` so that a name such as `T.Inner`, whose first component names a type variable, names nothing at all. Update the unit test to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…parserutil-unit-tests
Track the enclosing type to compare in a single nullable variable, so that the nullness checker can relate the null check to the use.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: typetools/checker-framework/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughJavaParserUtil now distinguishes resolved type elements from resolved type parameters and updates name-resolution behavior for shadowing and inherited member types. Whole-program inference uses a type variable’s effective upper bound when checking annotation relevance and preserves array levels during type conversion. New tests cover resolver behavior and relevant or irrelevant type-variable uses, including array element cases. Merge Risk: ⚪ Minimal · up to The resolver and inference changes have no identified issue requiring a fix before merge; proceed with normal checks. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 8 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 22, 2026
# Conflicts: # framework/src/main/java/org/checkerframework/framework/util/JavaParserUtil.java # framework/src/test/java/org/checkerframework/framework/util/JavaParserUtilTest.java
Rename `ResolvedName` to `ResolvedTypeName`, `resolveName` to `resolveTypeName`, `resolveTypeName` to `resolveTypeNameAsTypeElement`, and `resolveTypeVariableName` to `resolveTypeNameAsTypeParameter`. Use `TypesUtils.getObjectTypeMirror` for the implicit upper bound of a type variable, and clarify why a use of a type variable has no TypeMirror in `WholeProgramInferenceJavaParserStorage`. Test resolving a multi-component name whose first component is a member type that shadows a type parameter, a member type of a local class that shadows the local class's type parameter, and inference for a type variable whose bound is another type variable or that is an array's element type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mernstcheckerframework
enabled auto-merge (squash)
September 24, 2026 14:34
mernstcheckerframework
approved these changes
Sep 24, 2026
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
2 participants
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.
Supersedes #8190, which was a test-only change; this pull request adds the production changes that the tests exercise.
Resolve a name that names a type variable
JavaParserUtil.resolveTypeNamereturns null both when a name names a type variable and when the name cannot be resolved at all, so a client cannot tell the two apart.Add
JavaParserUtil.resolveName, which returns aResolvedNamerecord that says whether the name names a type, a type variable, or neither, andJavaParserUtil.resolveTypeVariableName, which returns just the type variable. A client that needs both answers callsresolveNameonce rather than walking the enclosing scopes twice.resolveTypeNamedelegates toresolveName.Two name resolution fixes come with the new entry point:
T.Inner, never names a type variable, because a type variable has no member types.Use a type variable's upper bound when deciding relevance
Whole-program inference wrote an annotation into an
.ajavafile for a use of a type variable whose upper bound is not relevant to the checker. A use of a type variable has noTypeMirrorinWholeProgramInferenceJavaParserStorage, soJavaParserUtil.typeToTypeMirrorreturned null and the code conservatively treated the type as relevant.WholeProgramInferenceJavaParserStorage.typeToTypeMirrornow yields the type variable's effective upper bound. That matchesGenericAnnotatedTypeFactory.isRelevant, which erases the type and treats a type variable as relevant exactly when its upper bound is. When the upper bound is an intersection type, whichTypescannot create, the leftmost bound -- that is, the erasure -- is used.checker/tests/ainfer-relevance/IrrelevantTypeVariable.javatests both the simple and the intersection upper bound.Unit tests for name resolution
JavaParserUtil.resolveTypeNamewas exercised only indirectly, through theainfer-relevancegoal files, which cannot reach the cases that need types in more than one package.JavaParserUtilTestruns the resolver directly against fixture sources injavaparserutil/superpkgandjavaparserutil/subpkg, covering inherited member types, package-private members that are not inherited across packages, shadowing, and type variable resolution.Unrelated cleanup
AnnotationFileParser.sameTypeArgumentssetjavaParserScopeto null instead of tracking a separatecompareScopeboolean, which avoids a nullness warning.🤖 Generated with Claude Code