Skip to content
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

python rules do not match list comprehension correctly #4221

Closed
1 of 3 tasks
rgraber opened this issue Nov 5, 2021 · 5 comments · Fixed by #4412
Closed
1 of 3 tasks

python rules do not match list comprehension correctly #4221

rgraber opened this issue Nov 5, 2021 · 5 comments · Fixed by #4412
Assignees

Comments

@rgraber
Copy link

rgraber commented Nov 5, 2021

Describe the bug
Using a python rule to match a list comprehension fails, in certain cases, to correctly match the full expression, resulting in extra ]s appearing after fixes are applied. Also, possibly a separate bug, repeated metavariables don't seem to work inside a list comprehension.

To Reproduce
https://semgrep.dev/s/0ykw

Expected behavior

  1. I expect $Z = [ $X for $X in $Y] (python-replace-useless-comprehension-misses) to match thing = [ x for x in range(10) ]
  2. I expect python-replace-useless-comprehension-hits to replace thing = [ x for x in range(10)] with thing = range(10) instead of thing = range(10)]

Screenshots
Worth nothing: it looks like the second matcher is matching everything except the last ], which is odd, but explains why the replacement has an extra ] at the end

Screen Shot 2021-11-05 at 2 37 51 PM

What is the priority of the bug to you?

  • P0: blocking your adoption of Semgrep or workflow
  • P1: important to fix or quite annoying
  • P2: regular bug that should get fixed

Environment
If not using semgrep.dev: are you running off docker, an official binary, a local build?

@r2c-demo
Copy link
Collaborator

r2c-demo commented Nov 5, 2021

@rgraber
Copy link
Author

rgraber commented Nov 5, 2021

Updated version of rule to demonstrate where we first encountered this: https://semgrep.dev/s/5D50

We have a method very_important_method() that was copied and pasted across several repositories. We want to update this method with the new standard across several repositories. However, whenever the current implementation of very_important_method ends with a list comprehension, somehow the very last square bracket doesn't get replaced, resulting in unparseable code.

@rgraber
Copy link
Author

rgraber commented Nov 8, 2021

More updates: I'm unclear if this is the same issue but it seems similar even though it's not actually a list comprehension. A method that ends with a long compound boolean expression was not matched correctly: https://semgrep.dev/s/66G1

@aryx
Copy link
Collaborator

aryx commented Nov 8, 2021

hmm you see @emjin with this last example I am more and more tempted to use ParenExpr there too

aryx added a commit that referenced this issue Dec 9, 2021
This closes #4221

test plan:
We know have the correct range for the example below. the end
part used to be col 34, forgetting the trailing ']' in the
comprehension and now it ends at col 35!

```
semgrep-core -lang py -f tests/python/misc_comprehension.sgrep
tests/python/misc_comprehension.py -json | jq
...
col = 35
```
@aryx aryx closed this as completed in #4412 Dec 9, 2021
@ievans
Copy link
Member

ievans commented Dec 9, 2021

Thanks for the report @rgraber. I expect this fix will go out in 0.77 next week!

@ievans ievans added the bug Something isn't working label Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants