Skip to content

Python: Fix module level flow for iterable unpacking #15755

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
Feb 29, 2024
Merged

Conversation

RasmusWL
Copy link
Member

No description provided.

(and for * patterns in match)

Since `PhaseDependentFlow` uses the following predicate, that relies on
.getScope() to be present for there to be any importTimeFlow (flow at
toplevel scope), it's important that data-flow nodes implement `.getScope`.

```
private predicate isTopLevel(Node node) { node.getScope() instanceof Module }
```

By implementing getScope, we can now rely on default implementation of
`getEnclosingCallable` in DataFlow::Node:

```
  /** Gets the enclosing callable of this node. */
  DataFlowCallable getEnclosingCallable() { result = getCallableScope(this.getScope()) }
```
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Nice! Perhaps we should consider making getEnclosingCalable a final predicate?

@yoff yoff merged commit 7beafc9 into github:main Feb 29, 2024
@RasmusWL RasmusWL deleted the it-fix branch March 1, 2024 08:35
@RasmusWL
Copy link
Member Author

RasmusWL commented Mar 1, 2024

Nice! Perhaps we should consider making getEnclosingCalable a final predicate?

I thought about it too, but it doesn't work since synthetic nodes inside synthetized summary callables needs to override it for sure 😄

RasmusWL added a commit to RasmusWL/codeql that referenced this pull request Mar 1, 2024
This would have found the problem in
github#15755.

As highlighted in the comment in the code, it's not a perfect solution
since we don't have an automatic way to ensure we don't introduce a new
PhaseDependentFlow use with a new step relation and forget to add it to
this consistency check... but I think this consistency check still adds
value!
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.

2 participants