-
Notifications
You must be signed in to change notification settings - Fork 356
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 unsoundess in Resource Leak Checker related to owning fields and EnsuresCalledMethods #4869
Fix unsoundess in Resource Leak Checker related to owning fields and EnsuresCalledMethods #4869
Conversation
…rlc-twoclose-soundness
…rlc-twoclose-soundness
…rlc-twoclose-soundness-2
…rlc-twoclose-soundness-2
…/checker-framework into rlc-twoclose-soundness-2
This looks pretty clean! The change looks good to me. Do we have a test where the finalizer does a |
…rlc-twoclose-soundness-2
We did not, but I just added one as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM; just one style comment
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakVisitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one more question I forgot to ask before
ignoredExceptionTypes.add("java.lang.Error"); | ||
ignoredExceptionTypes.add("java.lang.RuntimeException"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding these exception types in this PR? Do the tests not pass without it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests do not pass without it.
In particular, we get these three false positives:
3 unexpected diagnostics were found
CheckFields.java:68: error: (destructor.exceptional.postcondition)
CreatesMustCallForOverride2.java:38: error: (destructor.exceptional.postcondition)
CreatesMustCallForOverride2.java:56: error: (destructor.exceptional.postcondition)
For reference, here's the relevant lines of CheckFields
:
@EnsuresCalledMethods(
value = {"this.finalOwningFoo", "this.owningFoo"},
methods = {"a"})
void b() {
this.finalOwningFoo.a();
this.finalOwningFoo.c();
this.owningFoo.a();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this lands we may want to think about another manual update like #4892
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's really necessary - that run-time errors don't invalidate CalledMethods types follows from the idea that annotations (and therefore types) specify normal behavior, and a run-time exception is definitely not normal behavior.
…EnsuresCalledMethods (typetools#4869)
Fixes #4838.
Merge after #4867.
This is a replacement for #4839, based on the comments on it from @msridhar. It relies on #4867 to resolve the false positives described in the comment chain on #4839.