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 23 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,28 @@ 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 wpiAdjustAnnotationsBeforeUpdate(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")
&& !AnnotationUtils.areSameByName(
Copy link
Contributor

Choose a reason for hiding this comment

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

This call to areSameByName cannot possibly return true, because the string input to areSameByName must be a qualified name of an annotation - that is, it cannot have arguments. You could replace this with a call to areSame that takes UNDER_INITIALIZATION as the other argument, I think, to get the semantics that you're looking for.

I'm not sure that this check is really necessary, though. When it resolves to true, it doesn't really matter whether the call to replaceAnnotation below occurs or not, since "replacing" an annotation with another copy of the same annotation has no effect. So, I'd just remove this condition rather than try to get the semantics correct.

anno,
"org.checkerframework.checker.initialization.qual.UnderInitialization(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,9 @@ protected void updateAnnotationSet(
return;
}

// Update the right-hand side (rhs) annotations if necessary before updating the annotation set
atypeFactory.wpiAdjustAnnotationsBeforeUpdate(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 wpiAdjustAnnotationsBeforeUpdate(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.

I suggest the name wpiAdjustForUpdate, to match the existing methods in AnnotatedTypeFactory.

Moreover, I'm not sure that you actually need this new method at all. Conceptually, I don't see any difference between this and calling the code you've written in the implementation in the Nullness Checker from both wpiAdjustForUpdateField and wpiAdjustForUpdateNonField (in the Nullness Checker, of course). In fact, it may be sufficient to only override the latter, which also takes the RHS of a pseudo-assignment as its only argument.


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