Skip to content

Variable capture: allow arbitrary data-flow nodes to be the source of a write #14048

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 4 commits into from
Aug 31, 2023

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Aug 24, 2023

The predicate VariableWrite.getSource() returned an Expr, which is problematic for cases where there does not exist an explicit expression for the right-hand side.

Some cases where this happens:

  • Enhanced for loops: for (String x : array) {...} writes to x but there is no expression for the right-hand side.
  • Destructuring patterns: let { x, y } = foo() contains a write to x and y, but neither variable has an Expr for its right-hand side.
  • For increment expression x++ it varies between languages exactly how this works, but at least for JS, there is no AST node corresponding to the value that gets assigned to x.

This PR replacesExpr getSource() with a class VariableWriteSourceNode to represent the source of a write. The language implementation is free to map this node to an arbitrary data-flow node, whereas before it was forced to map it to an expression node.

@github-actions github-actions bot added the Java label Aug 24, 2023
@asgerf asgerf force-pushed the shared/variable-capture-write-source-node branch from 987ce9b to f17518a Compare August 24, 2023 12:06
@asgerf asgerf marked this pull request as ready for review August 25, 2023 13:30
@asgerf asgerf requested a review from a team as a code owner August 25, 2023 13:30
@asgerf
Copy link
Contributor Author

asgerf commented Aug 25, 2023

cc @hvitved this will conflict with #11725 as it changes the API. I'm happy to update this PR if you merge first.

This is technically a breaking change to the API, but I'm thinking this counts as an internal-only qlpack so we should be able to make such changes.

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Aug 25, 2023
Expr getSource() {
result = this.(VariableAssign).getSource() or
result = this.(AssignOp)
Node getSource() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to write "I don't want to use DataFlow::Node in the input to variable capture", but then I noticed that this is an additional predicate (which apparently doesn't require the annotation since it's a member predicate). I'd like to move this predicate out of the input module to make this more clear. It can e.g. be inlined into asClosureNode.

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.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants