Skip to content

Dataflow: Restrict partial flow to either forward or reverse flow. #14599

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 3 commits into from
Oct 26, 2023

Conversation

aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Oct 26, 2023

When using partial flow we would calculate both partial forward flow and partial reverse flow even though only one direction was requested. This moves the choice into the parameterised module application so we know up front which one to calculate. So instead of always using module Partial = FlowExploration<limit/0> and choosing between Partial::partialFlow and Partial::partialFlowRev, you now choose between module Partial = FlowExplorationFwd<limit/0> and module Partial = FlowExplorationRev<limit/0>, and then always use Partial::partialFlow.

MathiasVP
MathiasVP previously approved these changes Oct 26, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! I guess we should also update the reference to FlowExploration in https://codeql.github.com/docs/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow/ (or make FlowExploration an alias for FlowExplorationFwd)?

@aschackmull
Copy link
Contributor Author

I guess we should also update the reference to FlowExploration

The doc needs an update regardless as the info about reverse flow needs a fix.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

The code changes LGTM! Should we have the Docs team 👀 the documentation changes?

@aschackmull
Copy link
Contributor Author

Should we have the Docs team 👀 the documentation changes?

It's very minor, so I think your review is plenty.

@MathiasVP MathiasVP merged commit b1d4ca5 into github:main Oct 26, 2023
@aschackmull aschackmull deleted the dataflow/partialflow-separate branch October 26, 2023 10:22
@alexet
Copy link
Contributor

alexet commented Oct 26, 2023

Do you need to update the test queries in java/ql/test/library-tests/dataflow/partial?

@aschackmull
Copy link
Contributor Author

Do you need to update the test queries in java/ql/test/library-tests/dataflow/partial?

Gah! Of course CI didn't run.

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.

3 participants