@CreatesMustCallFor create an obligation on exceptional successors #6221
Conversation
|
@Nargeshdb please request my review once you are done with your code cleanup here, thanks! |
| * The fully-qualified names of the exception types that are ignored by this checker when | ||
| * computing dataflow stores. | ||
| */ | ||
| protected static final Set<String> ignoredExceptionTypes = |
There was a problem hiding this comment.
Not a full review but a quick question. Does this need to respect the new setting around ignored exceptions that were added recently?
| * The fully-qualified names of the exception types that are ignored by this checker when | ||
| * computing dataflow stores. | ||
| */ | ||
| protected static final Set<@CanonicalName String> ignoredExceptionTypes = |
There was a problem hiding this comment.
@Nargeshdb my question remains, shouldn't this set of exceptions be configurable using the same setting added in #6242?
There was a problem hiding this comment.
Relatedly, #6242 added a mechanism for matching all subtypes of an exception type in addition to matching by name. We should try to be as consistent as possible here and share code if possible
There was a problem hiding this comment.
@Nargeshdb any thoughts on the above?
There was a problem hiding this comment.
I've already addressed your comment here. There is a method in this class like what we have here, but it's overridden in MustCallAnnotatedTypeFactory to match all subtypes of an exception type. Is there anything else that I missed regarding consistency with 6242?
There was a problem hiding this comment.
Sorry for being unclear. #6242 added an option -AresourceLeakIgnoredExceptions to specify which exception types should be ignored by the RLC. This PR moves some of that logic to the Must Call Checker. And, as far as I can see, the method in this class, MustCallAnalysis, ignored the setting entirely. This is similar to what is done in the CalledMethodsAnalysis, I agree. What I don't fully understand is:
- Why does this PR added the method
MustCallAnalysis#isIgnoredExceptionType()? Was this an oversight from before and it should have been there all along? This method seems unrelated to the command line setting. - Why does this PR move the logic for the ignored exceptions command-line setting from the Resource Leak Checker to the Must Call Checker?
Thanks a lot!
There was a problem hiding this comment.
Some more comments below.
Generally, I am concerned that this change will lead to a lot of new reports. For Socket.bind() we have good evidence that these reports will be true positives, but there may be other cases where a @CreatesMustCallFor method only creates an obligation on its normal exit. We may need to eventually add support for a new annotation (@CreatesMustCallForOnNormal?) to express this property, with corresponding verification.
| * The fully-qualified names of the exception types that are ignored by this checker when | ||
| * computing dataflow stores. | ||
| */ | ||
| protected static final Set<@CanonicalName String> ignoredExceptionTypes = |
There was a problem hiding this comment.
Relatedly, #6242 added a mechanism for matching all subtypes of an exception type in addition to matching by name. We should try to be as consistent as possible here and share code if possible
| return MustCallAnalysis.ignoredExceptionTypes.contains( | ||
| TypesUtils.getQualifiedName((DeclaredType) exceptionType)); |
There was a problem hiding this comment.
Same comment here about relationship to #6242
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java
Outdated
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java
Outdated
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java
Outdated
Show resolved
Hide resolved
| // use the type's default must-call type instead. For String, this is @MustCall({}), | ||
| // so no error is issued. This rule is important to avoid false positives in realistic | ||
| // code (such as the first getMode() method in this class). | ||
| // :: error: required.method.not.called |
There was a problem hiding this comment.
I cannot understand why the behavior of this test changes due to this PR; there is no @CreatesMustCallFor anywhere here. Can you explain?
|
@msridhar Would you be willing to take over this pull request (either to complete it or to close it)? |
|
I was hoping @Nargeshdb could still finish this up. @Nargeshdb what do you think? |
…reatesMustCallFor
There was a problem hiding this comment.
@Nargeshdb still hoping to land this one :-) Had a couple more questions
| * The fully-qualified names of the exception types that are ignored by this checker when | ||
| * computing dataflow stores. | ||
| */ | ||
| protected static final Set<@CanonicalName String> ignoredExceptionTypes = |
There was a problem hiding this comment.
@Nargeshdb any thoughts on the above?
Fix #6050.