Skip to content

Data flow: Guard against viableImplInCallContext not being a subset of viableCallable #10505

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

Merged
merged 5 commits into from
Sep 23, 2022

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Sep 21, 2022

This PR adds a guard against, and consistency query for, cases where callable = viableImplInCallContext(call, _) does not imply callable = viableCallable(call).

As it turned out, this revealed a bug in the C# implementation, which is also fixed on this PR.

@hvitved hvitved force-pushed the dataflow/viable-impl-in-ctx-consistency branch 4 times, most recently from b2b8aa6 to 27c9986 Compare September 21, 2022 17:36
@hvitved hvitved force-pushed the dataflow/viable-impl-in-ctx-consistency branch from 27c9986 to 167ad27 Compare September 21, 2022 17:55
@hvitved hvitved force-pushed the dataflow/viable-impl-in-ctx-consistency branch from 167ad27 to 2334099 Compare September 22, 2022 11:38
@hvitved hvitved force-pushed the dataflow/viable-impl-in-ctx-consistency branch from 2334099 to 914c711 Compare September 22, 2022 13:01
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Sep 22, 2022
@hvitved hvitved marked this pull request as ready for review September 22, 2022 17:56
@hvitved hvitved requested review from a team as code owners September 22, 2022 17:56
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Python/shared changes look reasonable to me. 👍

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Right, this was already an implicit consistency requirement, so making it more explicit LGTM.
Also, adding the filter to enforce it in DataFlowImplCommon looks fairly free, so that seems like a reasonable belt-and-suspenders fix.

@hvitved hvitved merged commit 8b424d1 into github:main Sep 23, 2022
@hvitved hvitved deleted the dataflow/viable-impl-in-ctx-consistency branch September 23, 2022 08:38
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants