Skip to content

Ruby: Restrict use-use flow #7208

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 2 commits into from
Nov 23, 2021
Merged

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 22, 2021

Same as e.g. ec5d8ab.

@hvitved hvitved requested a review from a team as a code owner November 22, 2021 12:08
@github-actions github-actions bot added the Ruby label Nov 22, 2021
or
read1 = nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()
)
localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
Copy link
Contributor

Choose a reason for hiding this comment

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

The context here is the public predicate localSsaFlowStep(Ssa::Definition def, Node nodeFrom, Node nodeTo), which now no longer includes the regular use-use steps. Currently this is fine, as all its references are in this file and have suitable additions of LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo), but this makes the public exposure of localSsaFlowStep (through an internal qll, but still) somewhat shaky. Also, the def column of the predicate is unused. I suggest making localSsaFlowStep private and removing its def column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved localFlowStepCommon into the LocalFlow module (where it actually belongs), and could then make the predicate private.

@aschackmull
Copy link
Contributor

You just pushed the bubble. Granted, the useless column is now gone, but the semantic issue has just moved to localFlowStepCommon, which is now being publicly exposed (and doesn't include the use-use step). Could you make the containing LocalFlow module private?

@hvitved
Copy link
Contributor Author

hvitved commented Nov 22, 2021

Could you make the containing LocalFlow module private?

That module is referenced in TypeTrackerSpecific.qll, so doesn't work. Also, I don't think it is really that problematic, since DataFlowPrivate is very much an internal library.

@aschackmull aschackmull merged commit a68b55b into github:main Nov 23, 2021
@hvitved hvitved deleted the ruby/restrict-use-use branch November 23, 2021 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants