Refactor cache to determine if an element is annotatedfor#1331
Refactor cache to determine if an element is annotatedfor#1331aosen-xiong wants to merge 15 commits intoeisop:masterfrom
Conversation
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/util/count/AnnotationStatistics.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java
Show resolved
Hide resolved
| && !isElementAnnotatedForThisChecker(annotationScope); | ||
| && !atypeFactory | ||
| .getChecker() | ||
| .isElementAnnotatedForThisCheckerOrUpstreamChecker(annotationScope); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
c10565f to
eb741f4
Compare
There was a problem hiding this comment.
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
isElementAnnotatedForThisCheckermethod and its cache fromQualifierDefaultstoAnnotatedTypeFactory - Made
SourceChecker#isElementAnnotatedForThisCheckerOrUpstreamCheckeran 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.
framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java
Outdated
Show resolved
Hide resolved
|
@aosen-xiong Please go through the copilot suggestions and see what should be addressed. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Weird, CI failed again. |
|
@wmdietl ping for review. |
|
@thisisalexandercook Hi Alex, can you also review this PR? Thanks! |
In this PR, I refactored the
isElementAnnotatedForThisCheckerand its cache fromQualifierDefaultstoAnnotatedTypeFactoryto foster code reuse in both determining the default and whether error should be suppressed. I am making the methodSourceChecker#isElementAnnotatedForThisCheckerabstract to reduce code duplication.