Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WPI Non-Termination Issue with @UnknownInitialization Annotations #6657

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
ebb561b
Handle WPI loop on Unknowninitialization annotations
erfan-arvan Jun 5, 2024
30d842f
Resolved merge conflicts
erfan-arvan Jun 19, 2024
05ef6fb
Rename FlowExpression to JavaExpression in tests
smillst Jun 5, 2024
ebf782c
Update dependency io.github.classgraph:classgraph to v4.8.173 (#6653)
renovate[bot] Jun 5, 2024
5167ed1
Implement `JavaExpression.SuperReference`
jyoo980 Jun 5, 2024
ee517ea
Added a Tree argument to AnnotatedTypes.adaptParameters()
smillst Jun 6, 2024
f3a08a7
Update contributors.tex
erfan-arvan Jun 6, 2024
5df02b7
Update contributors.tex to include the latest contributors
erfan-arvan Jun 6, 2024
42d7812
Update contributors.tex to include my name
erfan-arvan Jun 6, 2024
711ae4d
How to indicate that side effects can change a value's type qualifier…
mernst Jun 6, 2024
d9a6504
Define `TreeUtils.fieldsFromClassTree()` (#6659)
mernst Jun 6, 2024
342bc94
Update contributors.tex after make it again
erfan-arvan Jun 7, 2024
b5a62d8
Try to pass CI test -> Contrubutors names
erfan-arvan Jun 7, 2024
5f3f89d
Update contributors.tex to reflect latest changes
erfan-arvan Jun 7, 2024
81f1f2b
Improve Javadoc for `getAnnotationFromJavaExpression`
smillst Jun 8, 2024
8c345fd
Add `graphviz` to list of dev dependencies for macOS
jyoo980 Jun 11, 2024
118b771
Exclude my username from contributors.tex
erfan-arvan Jun 18, 2024
4818ac2
Transferred the proposed change for handling WPI loop for unknowninit…
erfan-arvan Jun 18, 2024
134e76f
Merge branch 'master' into bugfix/wpi-loop-unknowninitialization
kelloggm Jun 19, 2024
1c31423
Rename method to adjust annotations before updating annotation set, u…
erfan-arvan Jun 18, 2024
131e864
Merge remote-tracking branch 'origin/bugfix/wpi-loop-unknowninitializ…
erfan-arvan Jun 20, 2024
4691e84
Merge branch 'master' into bugfix/wpi-loop-unknowninitialization
erfan-arvan Jun 22, 2024
872d7e1
Merge branch 'master' into bugfix/wpi-loop-unknowninitialization
erfan-arvan Jun 24, 2024
33c82a6
Merge branch 'master' into bugfix/wpi-loop-unknowninitialization
kelloggm Jun 27, 2024
5b642c3
Merge branch 'typetools:master' into bugfix/wpi-loop-unknowninitializ…
erfan-arvan Jul 1, 2024
d7acb57
Changed the name of the method, enhanced the body, and transferred th…
erfan-arvan Jul 6, 2024
1b1d56f
Merge branch 'master' into bugfix/wpi-loop-unknowninitialization
erfan-arvan Jul 6, 2024
8826275
Removed unnecessary comments
erfan-arvan Jul 7, 2024
388e584
Merge branch 'typetools:master' into bugfix/wpi-loop-unknowninitializ…
erfan-arvan Jul 7, 2024
fd16cc8
Merge branch 'bugfix/wpi-loop-unknowninitialization' of https://githu…
erfan-arvan Jul 7, 2024
eb79855
Removed the imports from the test case
erfan-arvan Jul 7, 2024
6ae7fba
Changed the name of the test case
erfan-arvan Jul 7, 2024
f057894
Update checker build.gradle to fix ainferNullnessGenerateJaifs task i…
erfan-arvan Jul 10, 2024
1a32487
Rename an instance of `VarArgsTest`
mernst Jul 7, 2024
90129a3
Give `DefaultQualifier` runtime retention
mernst Jul 7, 2024
cf451ba
Minor changes related to Non-Empty Checker
mernst Jul 8, 2024
8eafe7d
Use `filePermissions` instead of deprecated `fileMode` (#6693)
mernst Jul 8, 2024
1d5fb60
Disambiguate owners of fields in output (#6699)
mernst Jul 8, 2024
217ba50
Comment changes (#6700)
mernst Jul 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -961,6 +961,30 @@ public void wpiAdjustForUpdateNonField(AnnotatedTypeMirror rhsATM) {
}
}

// For all assignments, If rhsmATM is Nullable or MonotonicNonNull and has
// the UnknownInitialization annotation, and if the annotation is not
// UnknownInitialization(java.lang.Object.class), replace it with
// UnknownInitialization(java.lang.Object.class). This is because there is
// likely a constructor where it hasn't been initialized, and we haven't
// considered its effect. Otherwise, WPI might get stuck in a loop.
@Override
public void wpiAdjustForInitializationAnnotations(AnnotatedTypeMirror rhsATM) {
if ((rhsATM.hasPrimaryAnnotation(Nullable.class)
|| rhsATM.hasPrimaryAnnotation(MonotonicNonNull.class))) {
for (AnnotationMirror anno : rhsATM.getPrimaryAnnotations()) {
if (AnnotationUtils.areSameByName(
anno, "org.checkerframework.checker.initialization.qual.UnknownInitialization")
&& !anno.getAnnotationType()
.toString()
erfan-arvan marked this conversation as resolved.
Show resolved Hide resolved
.equals(
"@org"
+ ".checkerframework.checker.initialization.qual.UnknownInitialization(java.lang.Object.class)")) {
rhsATM.replaceAnnotation(UNDER_INITALIZATION);
}
}
}
}

@Override
public boolean wpiShouldInferTypesForReceivers() {
// All receivers must be non-null, or the dereference involved in
Expand Down
1 change: 1 addition & 0 deletions docs/manual/contributors.tex
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
Di Wang,
Dilraj Singh,
Dmitriy Shepelev,
Erfan Arvan,
Eric Spishak,
Ethan Koenig,
Farzan Mirshekari,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,10 @@ protected void updateAnnotationSet(
return;
}

// Update Initialization Annotations in WPI. It is only applied to
// Nullness-related qualifiers' inference to prevent possible loops.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is inappropriate, because it may not remain true. In particular, it is mixing implementation details of a specific checker (nullness) with the generic WPI algorithm (which is what this code implements). The comment should explain the general purpose of this callback hook, not the specific use in one checker.

atypeFactory.wpiAdjustForInitializationAnnotations(rhsATM);

AnnotatedTypeMirror atmFromStorage =
storage.atmFromStorageLocation(rhsATM.getUnderlyingType(), annotationsToUpdate);
updateAtmWithLub(rhsATM, atmFromStorage);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5523,6 +5523,15 @@ public boolean wpiShouldInferTypesForReceivers() {
return true;
}

/**
* Changes the type of {@code rhsATM} when being assigned to anything, for use by whole-program
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"when being assigned to anything" isn't quite correct, I think. This method is called anytime an annotation set is updated, so it is not called just at assignments - instead, it is called any time that WPI does an update, which conceptually is at all pseudo-assignments. The other text that you have here makes that clear, as well; please make this consistent.

* inference. The default implementation does nothing.
*
* @param rhsATM the type of the rhs of the pseudo-assignment, which is side-effected by this
* method
*/
public void wpiAdjustForInitializationAnnotations(AnnotatedTypeMirror rhsATM) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name "wpiAdjustForInitializationAnnotations" doesn't convey the generic purpose of this method. Rather, it conveys the specific purpose that you've used the method for in the Nullness Checker.

The name here should be chosen to explain when this method is called and what it might be used for in the general case, not the specific case that you're trying to fix in this PR. I suggest choosing a name that will help someone reading WholeProgramInferenceImplementation#updateAnnotationSet to understand what this hook does.


/**
* Side-effects the method or constructor annotations to make any desired changes before writing
* to an annotation file.
Expand Down