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

Ignore control flow due to NullPointerExceptions in Called Methods Checker #4867

Merged
merged 5 commits into from
Aug 12, 2021

Conversation

msridhar
Copy link
Contributor

@msridhar msridhar commented Aug 5, 2021

Fixes #4843

The Called Methods Checker assumes that the checked code is free of NullPointerExceptions (this property should be verified by the Nullness Checker). This PR enhances the Called Methods Checker's type inference to ignore control flow due to NullPointerExceptions, enabling it to verify more code. See #4843 (comment) for more context.

The change is implemented by enhancing the dataflow framework to allow for ignoring exceptional control flow for particular exception types.

We may want to ignore more exception types in the future; we can add that in a follow-up PR.

Copy link
Contributor

@kelloggm kelloggm left a comment

Choose a reason for hiding this comment

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

Thanks for this work!

I'm not sure whether we should override the new method in the Called Methods Checker or in the Resource Leak Checker (which is a Called Methods Checker subclass). I would be inclined to override it in the RLC rather than in the CMC as you've done here, since I don't see a pressing need for the CMC to make this assumption (but I think making the assumption in the CMC is sound, so it shouldn't make much of a difference).

I pulled this branch into the branch I've been using to try to resolve the RLC soundness bug related to exceptional exits from destructors, and this dramatically improves the code there. There are a few remaining new FPs that I can't currently explain, but given the error messages look non-sensical I suspect there is a bug in my new code. I can confirm that this change gets rid of most of the new FPs of the kind we discussed before.

Someone else with more knowledge of the dataflow framework should review this PR before it is merged. Maybe @mernst or @smillst?

@msridhar
Copy link
Contributor Author

msridhar commented Aug 6, 2021

I'm not sure whether we should override the new method in the Called Methods Checker or in the Resource Leak Checker (which is a Called Methods Checker subclass). I would be inclined to override it in the RLC rather than in the CMC as you've done here, since I don't see a pressing need for the CMC to make this assumption (but I think making the assumption in the CMC is sound, so it shouldn't make much of a difference).

Fair enough. The main motivation for this change is the RLC, so we can make the change there if desired, to narrow the scope. I'm ok either way, so let's leave it for now and get an opinion from another reviewer.

@mernst mernst merged commit 2d64c76 into typetools:master Aug 12, 2021
@msridhar msridhar deleted the dataflow-ignore-exception-types branch November 9, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive in resource leak checker when there is no catch block before finally block
3 participants