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
| && !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.
aosen-xiong
force-pushed
the
code-refactor-annotatedfor
branch
from
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.
|
@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! |
There was a problem hiding this comment.
EDIT
This refactor makes sense in light of the postInit(); chain discussed here.
I've PRed a potential tightening here: aosen-xiong#23
Alternatively we could also consider publishing the type factory for use in its postInit() phase, i.e. set something like this.checker.setPostInitTypeFactory(this); in the body of GenericAnnotatedTypeFactory.postInit();. The downside is that it exposes a partially initialized type factory through the checker, which sounds like something we would want to avoid. But we are technically querying the factory during postInit(); already anyway, this would just give the checker a route to it, so maybe worth considering?
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.