Refactor cache to determine if an element is annotatedfor #1331

aosen-xiong · 2025-07-10T00:43:39Z

In this PR, I refactored the isElementAnnotatedForThisChecker and its cache from QualifierDefaults to AnnotatedTypeFactory to foster code reuse in both determining the default and whether error should be suppressed. I am making the method SourceChecker#isElementAnnotatedForThisChecker abstract to reduce code duplication.

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

aosen-xiong · 2025-08-26T05:02:08Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

                        return true;
                    }
-                    if (isAnnotatedForThisCheckerOrUpstreamChecker(packageElement)) {
-                        // Return false immediately. Do NOT check for AnnotatedFor in the enclosing


Those comments are no longer true because this method now returns the result from the cache with the considering the enclosing element.

This removes the comments "Return false immediately", but the behavior stays unchanged.
Should these no longer return early? What do you mean with "with the considering the enclosing element"?

Because the basetype checker implementation uses the method from ATF.

@Override protected boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(@Nullable Element elt) { return getTypeFactory().isElementAnnotatedForThisCheckerOrUpstreamChecker(elt); }

Method isElementAnnotatedForThisCheckerOrUpstreamChecker considers whether the enclosing element is annotated or not in the cache.

wmdietl

Thanks!

wmdietl · 2025-09-01T17:20:53Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

                        return true;
                    }
-                    if (isAnnotatedForThisCheckerOrUpstreamChecker(packageElement)) {
-                        // Return false immediately. Do NOT check for AnnotatedFor in the enclosing


This removes the comments "Return false immediately", but the behavior stays unchanged.
Should these no longer return early? What do you mean with "with the considering the enclosing element"?

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

wmdietl · 2025-09-01T17:59:31Z

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

+     * @param elt the source code element to check, or null
+     * @return true if the element is annotated for this checker or an upstream checker
+     */
+    public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(@Nullable Element elt) {


Why add this method here? Why isn't this the implementation in SourceChecker? Or, if you want to keep that implementation abstract, move this to BaseTypeChecker, the only place that externally calls this method.
Why have two methods with the same name in both the Checker and the ATF?

Why isn't this the implementation in SourceChecker?

This method need a ATF because getDeclAnnotation is defined in ATF.

Or, if you want to keep that implementation abstract, move this to BaseTypeChecker, the only place that externally calls this method.

Because QualifierDefaults also called this method.

Because QualifierDefaults also called this method.

If QualifierDefaults only has an ATF and no checker, simply call atf.getChecker() somewhere?

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

framework/src/main/java/org/checkerframework/common/util/count/AnnotationStatistics.java

framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java

aosen-xiong · 2025-09-03T04:33:15Z

framework/src/main/java/org/checkerframework/framework/util/defaults/QualifierDefaults.java

-                    && !isElementAnnotatedForThisChecker(annotationScope);
+                    && !atypeFactory
+                            .getChecker()
+                            .isElementAnnotatedForThisCheckerOrUpstreamChecker(annotationScope);


This call is causing the problem :-1: warning: junit-assertions.astub: Parse problem: org.checkerframework.javacutil.TypeSystemError: Called getTypeFactory() before initialization was complet.

It is interesting I can not get the type factory from the checker itself because the visitor is not initialized, even though I already had atypeFactory.

According to stacktrace, the method call went wrong because the chain initialization is init_checker -> init_visitor -> init_atf -> postinit in atf's constructor -> parsestubfiles -> determine default -> atf.getchecker().getTypeFactory(). We can get the checker from the atf successfully because it is assigned in the AnnotatedTypeFactory's constructor. But we don't have the visitor yet from the checker because it is the one underinitialization. Looks like this error too conservative because a partially initialized atf is able to determine whether an element is annotated or not.

at org.checkerframework.framework.stub.AnnotationFileElementTypes.parseStubFiles(AnnotationFileElementTypes.java:210) at org.checkerframework.framework.type.AnnotatedTypeFactory.parseAnnotationFiles(AnnotatedTypeFactory.java:4097) at org.checkerframework.framework.type.GenericAnnotatedTypeFactory.postInit(GenericAnnotatedTypeFactory.java:438) at org.checkerframework.checker.initialization.InitializationAnnotatedTypeFactory.<init>(InitializationAnnotatedTypeFactory.java:60) at org.checkerframework.checker.initialization.InitializationVisitor.createTypeFactory(InitializationVisitor.java:84) at org.checkerframework.checker.initialization.InitializationVisitor.createTypeFactory(InitializationVisitor.java:52) at org.checkerframework.common.basetype.BaseTypeVisitor.<init>(BaseTypeVisitor.java:343) at org.checkerframework.common.basetype.BaseTypeVisitor.<init>(BaseTypeVisitor.java:329) at org.checkerframework.checker.initialization.InitializationVisitor.<init>(InitializationVisitor.java:77) at org.checkerframework.checker.initialization.InitializationChecker.createSourceVisitor(InitializationChecker.java:149) at org.checkerframework.checker.initialization.InitializationChecker.createSourceVisitor(InitializationChecker.java:73) at org.checkerframework.framework.source.SourceChecker.initChecker(SourceChecker.java:1175)

Previous version works with code duplication because works flow is init_checker -> init_visitor -> init_atf -> postinit in atf's constructor -> parsestubfiles -> determine default -> atf. I am going to revert to previous version with some comments.

Copilot

Pull request overview

This PR refactors the cache and method for determining if an element is annotated for a checker, moving it from QualifierDefaults to AnnotatedTypeFactory to enable code reuse. The method isElementAnnotatedForThisChecker is renamed to isElementAnnotatedForThisCheckerOrUpstreamChecker and made abstract in SourceChecker.

Moved the isElementAnnotatedForThisChecker method and its cache from QualifierDefaults to AnnotatedTypeFactory
Made SourceChecker#isElementAnnotatedForThisCheckerOrUpstreamChecker an abstract method
Implemented the abstract method in all subclasses (BaseTypeChecker, AggregateChecker, and utility checkers)

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
QualifierDefaults.java	Removed private `isElementAnnotatedForThisChecker` method and `elementAnnotatedFors` cache; updated calls to use the factory method
AnnotatedTypeFactory.java	Added public `isElementAnnotatedForThisCheckerOrUpstreamChecker` method with the cache implementation
SourceChecker.java	Changed method from private to protected abstract; removed implementation and imports; updated method calls
BaseTypeChecker.java	Implemented abstract method by delegating to the type factory
AggregateChecker.java	Implemented abstract method to return false (delegates to subcheckers)
SignaturePrinter.java	Implemented abstract method to throw `BugInCF` (not expected to be called)
JavaCodeStatistics.java	Implemented abstract method to throw `BugInCF` (not expected to be called)
AnnotationStatistics.java	Implemented abstract method to throw `BugInCF` (not expected to be called)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-19T06:38:56Z

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

+
+        if (elt == null) {
+            throw new BugInCF(
+                    "Call of QualifierDefaults.isElementAnnotatedForThisChecker with null");


The error message incorrectly refers to "QualifierDefaults.isElementAnnotatedForThisChecker" but this method is now in AnnotatedTypeFactory. The error message should be updated to reflect the new location.

Suggested change

"Call of QualifierDefaults.isElementAnnotatedForThisChecker with null");

"Call of AnnotatedTypeFactory.isElementAnnotatedForThisCheckerOrUpstreamChecker with null");

Copilot · 2025-12-19T06:38:56Z

framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java

+     * Return false, determine whether an aggregatechecker is annotated for depends on its
+     * subcheckers. For checkers that have upstream checker and want to handle annotatedfor in both
+     * this and upstream checker, see InitializationChecker#getUpstreamCheckerNames().


Spelling and grammar issues: "aggregatechecker" should be "aggregate checker" (two words), and "annotatedfor" should be "@AnnotatedFor". The sentence also needs grammatical improvements for clarity.

Suggested change

* Return false, determine whether an aggregatechecker is annotated for depends on its

* subcheckers. For checkers that have upstream checker and want to handle annotatedfor in both

* this and upstream checker, see InitializationChecker#getUpstreamCheckerNames().

* Always returns false. Whether an aggregate checker is annotated with {@code @AnnotatedFor}

* depends on its subcheckers. For checkers that have an upstream checker and want to handle

* {@code @AnnotatedFor} in both this and the upstream checker, see

* {@link org.checkerframework.checker.initialization.InitializationChecker#getUpstreamCheckerNames()}.

Copilot · 2025-12-19T06:38:56Z

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java

+        // The implementation of this method is at AnnotatedTypeFactory because postinit() is called
+        // in the constructor of ATFs and use the same method before the ATF is fully initialized.
+        // Implement it here and call this method by atf.getChecker() would fail as it will call
+        // getTypeFactory() in the body and would result type system error as the visitor is not
+        // initialized yet.


The comment has several issues: "postinit()" should likely be "postInit()", "ATFs" is unclear (should be "AnnotatedTypeFactory instances"), "use the same method" is confusing, "Implement it here and call this method by atf.getChecker() would fail as it will call getTypeFactory()" has grammatical problems. The explanation needs to be clearer about why the implementation delegates to the factory.

Suggested change

// The implementation of this method is at AnnotatedTypeFactory because postinit() is called

// in the constructor of ATFs and use the same method before the ATF is fully initialized.

// Implement it here and call this method by atf.getChecker() would fail as it will call

// getTypeFactory() in the body and would result type system error as the visitor is not

// initialized yet.

// The main implementation of this method resides in AnnotatedTypeFactory. The factory's

// postInit() method is invoked from the constructors of AnnotatedTypeFactory instances,

// and during postInit() the factory may need to call

// isElementAnnotatedForThisCheckerOrUpstreamChecker. If this method were implemented

// here and invoked via atf.getChecker(), its body would call getTypeFactory(), which

// could access or create the factory before it is fully initialized and before the

// visitor is set up, leading to a type-system error. To avoid this initialization

// cycle, this checker method simply delegates to the AnnotatedTypeFactory implementation.

Copilot · 2025-12-19T06:38:57Z

framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java

+     * Return false, determine whether an aggregatechecker is annotated for depends on its
+     * subcheckers. For checkers that have upstream checker and want to handle annotatedfor in both
+     * this and upstream checker, see InitializationChecker#getUpstreamCheckerNames().


Grammar issue: "have upstream checker" should be "have an upstream checker" (missing article), and "annotatedfor" should be "@AnnotatedFor".

Suggested change

* Return false, determine whether an aggregatechecker is annotated for depends on its

* subcheckers. For checkers that have upstream checker and want to handle annotatedfor in both

* this and upstream checker, see InitializationChecker#getUpstreamCheckerNames().

* Returns false. Whether an aggregate checker is {@code @AnnotatedFor} depends on its

* subcheckers. For checkers that have an upstream checker and want to handle {@code @AnnotatedFor}

* in both this and the upstream checker, see InitializationChecker#getUpstreamCheckerNames().

Copilot · 2025-12-19T06:38:57Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

-
-        return false;
-    }
+    protected abstract boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt);


The abstract method lacks documentation. Since this is now an abstract method that subclasses must implement, it should have Javadoc explaining its purpose, parameters, and return value. The documentation from the previous implementation or from AnnotatedTypeFactory.isElementAnnotatedForThisCheckerOrUpstreamChecker should be added here.

Copilot · 2025-12-19T06:38:57Z

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java

+     * @return true if the element is annotated for this checker or an upstream checker
+     */
+    @Override
+    public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) {


The method is declared as public here, but it's declared as protected abstract in the parent class SourceChecker. This is widening the visibility from protected to public, which may be intentional but is inconsistent. The method should use protected to match the abstract declaration in SourceChecker, unless there's a specific reason to make it public.

Suggested change

public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) {

protected boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) {

aosen-xiong mentioned this pull request Aug 26, 2025

Introduce @NullMarked aliasing #1304

Open

aosen-xiong marked this pull request as ready for review August 26, 2025 04:00

aosen-xiong requested a review from wmdietl August 26, 2025 04:35

aosen-xiong assigned wmdietl Aug 26, 2025

aosen-xiong commented Aug 26, 2025

View reviewed changes

wmdietl requested changes Sep 1, 2025

View reviewed changes

wmdietl assigned aosen-xiong and unassigned wmdietl Sep 1, 2025

aosen-xiong commented Sep 3, 2025

View reviewed changes

aosen-xiong requested a review from wmdietl September 4, 2025 02:25

aosen-xiong assigned wmdietl and unassigned aosen-xiong Sep 4, 2025

Reapply code changes with profile

eb741f4

aosen-xiong force-pushed the code-refactor-annotatedfor branch from c10565f to eb741f4 Compare October 7, 2025 23:47

aosen-xiong added 2 commits October 7, 2025 21:53

Merge branch 'master' into code-refactor-annotatedfor

fcc3a22

Merge branch 'master' into code-refactor-annotatedfor

a456dd4

Copilot AI review requested due to automatic review settings December 19, 2025 06:28

Copilot started reviewing on behalf of aosen-xiong December 19, 2025 06:28 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

	"Call of QualifierDefaults.isElementAnnotatedForThisChecker with null");
	"Call of AnnotatedTypeFactory.isElementAnnotatedForThisCheckerOrUpstreamChecker with null");

-     * Return false, determine whether an aggregatechecker is annotated for depends on its
-     * subcheckers. For checkers that have upstream checker and want to handle annotatedfor in both
-     * this and upstream checker, see InitializationChecker#getUpstreamCheckerNames().
+     * Always returns false. Whether an aggregate checker is annotated with {@code @AnnotatedFor}
+     * depends on its subcheckers. For checkers that have an upstream checker and want to handle
+     * {@code @AnnotatedFor} in both this and the upstream checker, see
+     * {@link org.checkerframework.checker.initialization.InitializationChecker#getUpstreamCheckerNames()}.

-        // The implementation of this method is at AnnotatedTypeFactory because postinit() is called
-        // in the constructor of ATFs and use the same method before the ATF is fully initialized.
-        // Implement it here and call this method by atf.getChecker() would fail as it will call
-        // getTypeFactory() in the body and would result type system error as the visitor is not
-        // initialized yet.
+        // The main implementation of this method resides in AnnotatedTypeFactory. The factory's
+        // postInit() method is invoked from the constructors of AnnotatedTypeFactory instances,
+        // and during postInit() the factory may need to call
+        // isElementAnnotatedForThisCheckerOrUpstreamChecker. If this method were implemented
+        // here and invoked via atf.getChecker(), its body would call getTypeFactory(), which
+        // could access or create the factory before it is fully initialized and before the
+        // visitor is set up, leading to a type-system error. To avoid this initialization
+        // cycle, this checker method simply delegates to the AnnotatedTypeFactory implementation.

	public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) {
	protected boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) {

Refactor cache to determine if an element is annotatedfor #1331

Are you sure you want to change the base?

Refactor cache to determine if an element is annotatedfor #1331

Uh oh!

Conversation

aosen-xiong commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aosen-xiong Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmdietl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aosen-xiong commented Jul 10, 2025 •

edited

Loading

aosen-xiong Aug 26, 2025 •

edited

Loading