Move PICO to EISOP#1013
Open
aosen-xiong wants to merge 222 commits into
Open
Conversation
Co-authored-by: Werner Dietl <wdietl@gmail.com> Co-authored-by: Haifeng Shi <shihaifeng1998@gmail.com> Co-authored-by: Weitian Xing <xingweitian@gmail.com> Co-authored-by: Jeff Luo <j36luo@uwaterloo.ca> Co-authored-by: Mier Ta <m2ta@uwaterloo.ca>
aosen-xiong
force-pushed
the
pico-move
branch
from
December 14, 2024 19:35
33dfd50 to
32a297b
Compare
wmdietl
requested changes
Dec 20, 2024
wmdietl
left a comment
Member
There was a problem hiding this comment.
A quick round of initial comments.
Co-authored-by: Werner Dietl <wdietl@gmail.com>
aosen-xiong
commented
Dec 27, 2024
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces the PICO immutability checker (qualifiers, viewpoint adaptation, validation, and initialization integration) and adds a comprehensive pico-immutability test suite.
Changes:
- Added new PICO type system implementation (checker, type factory, viewpoint adapter, validator, qualifier hierarchy, and utilities).
- Added extensive negative/positive tests and a JUnit per-directory test runner for pico-immutability.
- Updated formatting tooling to recognize the new type annotations.
Reviewed changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/src/main/java/org/jmlspecs/annotation/Readonly.java | Adds a JML @Readonly annotation for aliasing into PICO. |
| framework/src/main/java/org/checkerframework/framework/type/AbstractViewpointAdapter.java | Adjusts method viewpoint adaptation behavior (varargs). |
| checker/tests/pico-immutability/ViewpointAdaptationRules.java | Adds viewpoint adaptation rules test coverage. |
| checker/tests/pico-immutability/Transitive.java | Adds transitive mutability test coverage. |
| checker/tests/pico-immutability/SupportedBuilderPattern.java | Adds builder-pattern mutability tests. |
| checker/tests/pico-immutability/SuperClassTest.java | Adds inheritance/super-call mutability tests. |
| checker/tests/pico-immutability/StaticTest.java | Adds static-context restrictions tests. |
| checker/tests/pico-immutability/ReimStudy.java | Adds study/regression tests for viewpoint adaptation constraints. |
| checker/tests/pico-immutability/RawTypeTest.java | Adds raw-type interaction tests. |
| checker/tests/pico-immutability/RGB.java | Adds example test for mutable vs immutable receivers. |
| checker/tests/pico-immutability/RDMClassUse.java | Adds receiver-dependent mutable class usage tests. |
| checker/tests/pico-immutability/RDMClassConstructor.java | Adds constructor/return qualifier tests for RDM classes. |
| checker/tests/pico-immutability/RDMClass.java | Adds constructor/method invocation tests for RDM classes. |
| checker/tests/pico-immutability/PolyMutableTest.java | Adds polymorphic mutability tests. |
| checker/tests/pico-immutability/PICOTypeUseLocation.java | Adds tests for invalid qualifier locations/uses. |
| checker/tests/pico-immutability/PICOMethodInit.java | Adds tests for initialization via helper methods. |
| checker/tests/pico-immutability/PICOInitialization.java | Adds tests for PICO-specific initialization requirements. |
| checker/tests/pico-immutability/PICOCircularInit.java | Adds circular initialization pattern tests. |
| checker/tests/pico-immutability/OverrideEquals.java | Adds override checks for equals/clone/Throwable methods. |
| checker/tests/pico-immutability/Operators.java | Adds operator write/side-effect tests for readonly receivers. |
| checker/tests/pico-immutability/ObjectMethods.java | Adds tests for Object method signatures across mutability bounds. |
| checker/tests/pico-immutability/LocalVariableRefinement.java | Adds tests for local refinement/flow interactions. |
| checker/tests/pico-immutability/ImplicitMutable.java | Adds tests for implicit shallow-immutable violations. |
| checker/tests/pico-immutability/ImmutableList.java | Adds tests around immutable collection construction patterns. |
| checker/tests/pico-immutability/ImmutableClass.java | Adds tests around immutable classes, bounds, and assignability. |
| checker/tests/pico-immutability/HashCodeSafetyExample.java | Adds object-identity/abstract-state motivation test. |
| checker/tests/pico-immutability/Generics.java | Adds generic bound/abstract-state interaction tests. |
| checker/tests/pico-immutability/FactoryPattern.java | Adds a factory/polymutable return behavior test. |
| checker/tests/pico-immutability/EnumTest.java | Adds enum implicit-immutable tests. |
| checker/tests/pico-immutability/CloneTest.java | Adds clone-related proposal/regression test content. |
| checker/tests/pico-immutability/ClassAnnotation.java | Adds tests for class-level qualifier bounds and inheritance rules. |
| checker/tests/pico-immutability/CastTest.java | Adds cast-safety tests. |
| checker/tests/pico-immutability/Builder.java | Adds additional builder-pattern examples and edge cases. |
| checker/tests/pico-immutability/AssignabilityAnnotation.java | Adds tests for @Assignable declaration validity rules. |
| checker/tests/pico-immutability/Arrays.java | Adds array qualifier behavior tests. |
| checker/src/test/java/org/checkerframework/checker/test/junit/PICOImmutabilityTest.java | Adds JUnit harness for pico-immutability directory tests. |
| checker/src/main/java/org/checkerframework/checker/pico/messages.properties | Adds user-visible diagnostics for PICO checker. |
| checker/src/main/java/org/checkerframework/checker/pico/PICOViewpointAdapter.java | Implements PICO-specific viewpoint adaptation rules. |
| checker/src/main/java/org/checkerframework/checker/pico/PICOValidator.java | Adds PICO-specific type/usage validation rules. |
| checker/src/main/java/org/checkerframework/checker/pico/PICOTypeUtil.java | Adds helper logic for PICO semantics (assignability, identity, etc.). |
| checker/src/main/java/org/checkerframework/checker/pico/PICOQualifierHierarchy.java | Customizes qualifier hierarchy behavior (e.g., non-reflexive Lost). |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitVisitor.java | Core type-checking visitor for PICO (without init qualifiers). |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitValue.java | Defines abstract value type for PICO dataflow. |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitTransfer.java | Implements transfer rules (notably null initializer refinement). |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitSubchecker.java | Registers PICO subchecker and initialization interop subchecker. |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitStore.java | Defines store for PICO dataflow analysis. |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitAnnotatedTypeFactory.java | Annotated type factory with defaults, viewpoint adapter, and annotators. |
| checker/src/main/java/org/checkerframework/checker/pico/PICONoInitAnalysis.java | Analysis factory for stores/values/transfers. |
| checker/src/main/java/org/checkerframework/checker/pico/PICOInitializationVisitor.java | Hooks PICO into initialization checker visitor. |
| checker/src/main/java/org/checkerframework/checker/pico/PICOInitializationAnnotatedTypeFactory.java | Filters uninitialized fields per PICO assignability semantics. |
| checker/src/main/java/org/checkerframework/checker/pico/PICOChecker.java | Top-level checker wiring PICO + initialization. |
| checker/src/main/java/org/checkerframework/checker/pico/ObjectIdentityMethodEnforcer.java | Enforces object-identity method restrictions in abstract-state-only mode. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/ReceiverDependentMutable.java | Adds @ReceiverDependentMutable qualifier definition. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/Readonly.java | Adds @Readonly qualifier definition and defaults. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/PolyMutable.java | Adds @PolyMutable qualifier definition and target locations. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/PICOLost.java | Adds @PICOLost qualifier definition. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/PICOBottom.java | Adds @PICOBottom qualifier definition and defaulting. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/ObjectIdentityMethod.java | Adds marker annotation for object identity methods. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/Mutable.java | Adds @Mutable qualifier definition. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/Immutable.java | Adds @Immutable qualifier definition plus DefaultFor/UpperBoundFor. |
| checker-qual/src/main/java/org/checkerframework/checker/pico/qual/Assignable.java | Adds @Assignable annotation definition. |
| build.gradle | Updates Spotless annotation formatting to recognize PICO qualifiers. |
Comments suppressed due to low confidence (5)
framework/src/main/java/org/checkerframework/framework/type/AbstractViewpointAdapter.java:1
computeVarargType()is called before the parameter types are set onmethodType. IfcomputeVarargType()relies on the current parameter list (common for determining varargs element type), this can compute the wrong varargs metadata. MovemethodType.computeVarargType()to aftersetParameterTypes(...)(or compute it fromunsubstitutedMethodTypebefore copying) so varargs info is derived from the correct parameter types.
checker/tests/pico-immutability/ViewpointAdaptationRules.java:1- These identifiers contain typos (
assingableMuatbleField,muatbleObject) that make the test harder to read and maintain. Rename them to correctly spelled names (e.g.,assignableMutableField,mutableObject) to keep the tests as clear specifications of the intended typing rules.
checker/tests/pico-immutability/ViewpointAdaptationRules.java:1 - These identifiers contain typos (
assingableMuatbleField,muatbleObject) that make the test harder to read and maintain. Rename them to correctly spelled names (e.g.,assignableMutableField,mutableObject) to keep the tests as clear specifications of the intended typing rules.
checker/tests/pico-immutability/ViewpointAdaptationRules.java:1 - These identifiers contain typos (
assingableMuatbleField,muatbleObject) that make the test harder to read and maintain. Rename them to correctly spelled names (e.g.,assignableMutableField,mutableObject) to keep the tests as clear specifications of the intended typing rules.
framework/src/main/java/org/jmlspecs/annotation/Readonly.java:1 - Consider adding an explicit
@Target(e.g.,ElementType.TYPE_USEandElementType.TYPE_PARAMETER) to document intended usage and prevent accidental application in unrelated contexts. This is especially useful since the annotation is being aliased into a type system qualifier.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+293
to
+300
| List<? extends AnnotationMirror> recieverAnnotations = | ||
| getAllRecieverAnnotation(enclosingMethod); | ||
| for (AnnotationMirror anno : recieverAnnotations) { | ||
| if (AnnotationUtils.areSame(anno, atypeFactory.UNDER_INITALIZATION)) { | ||
| // If the enclosing method receiver is under initialization, allow assignment | ||
| return; | ||
| } | ||
| } |
Comment on lines
+324
to
+328
| private List<? extends AnnotationMirror> getAllRecieverAnnotation(MethodTree tree) { | ||
| com.sun.tools.javac.code.Symbol meth = | ||
| (com.sun.tools.javac.code.Symbol) TreeUtils.elementFromDeclaration(tree); | ||
| return meth.getRawTypeAttributes(); | ||
| } |
Comment on lines
+32
to
+37
| public PICONoInitStore(PICONoInitStore s) { | ||
| super(s); | ||
| if (s.initializedFields != null) { | ||
| initializedFields = s.initializedFields; | ||
| } | ||
| } |
| object.identity.method.invocation.invalid=Cannot invoke non-object identity method %s from object identity context! | ||
| object.identity.field.access.invalid=Object identity context cannot reference non abstract state field %s! | ||
| object.identity.static.field.access.forbidden=Object identity context cannot reference static field %s! | ||
| implicit.shallow.immutable=Write an explicit mutable qualifier if wants to exclude the field from the abstract state! Otherwise change the class mutability of this object ! |
After eisop#1032, drop the local isAdaptedSubtype helper and apply the well-formedness rule (q |> bound) <: q via the shared API. Keep the PolyMutable/Lost early return; remove the outdated FIXME.
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
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.
Original code repo: https://github.com/opprop/immutability
Some of commits are presented in: https://github.com/Ao-senXiong/immutability/tree/pico-cf-only
Merge with: eisop/jdk#106
As discussed before, I will try to move PICO here gradually. Hopefully adding test cases and Javadoc with more discussion as well.