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] Cannot write a pattern to find useless list comprehensions #4260

Closed
1 of 3 tasks
IagoAbal opened this issue Nov 11, 2021 · 3 comments · Fixed by #4399
Closed
1 of 3 tasks

[Python] Cannot write a pattern to find useless list comprehensions #4260

IagoAbal opened this issue Nov 11, 2021 · 3 comments · Fixed by #4399
Assignees
Labels
bug Something isn't working feature:matching lang:python user:external requested by someone outside of r2c

Comments

@IagoAbal
Copy link
Contributor

Reporting on behalf of @mhoffm-aiven.

Describe the bug
Pattern [$X for $X in $ITERATOR] does not match [v for v in iterator]. The cause seems to be that in Generic, the first v is an expression id, whereas the second v is a pattern id. We should probably consider both equal when they bind to the same metavariable.

To Reproduce
https://semgrep.dev/s/Q91Z

Expected behavior
Pattern works, or we provide some workaround.

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
@IagoAbal IagoAbal added bug Something isn't working user:external requested by someone outside of r2c lang:python feature:matching labels Nov 11, 2021
@IagoAbal IagoAbal changed the title [Python] Cannot write a pattern to find useless iterators [Python] Cannot write a pattern to find useless list comprehensions Nov 11, 2021
@aryx
Copy link
Collaborator

aryx commented Nov 12, 2021

I had some refactoring I wanted to do to remove Metavariable.Id and just keep Metavariable.Name. This might solve this issue.

@aryx aryx self-assigned this Nov 12, 2021
@aryx
Copy link
Collaborator

aryx commented Nov 30, 2021

Hmm it's also because Naming_AST does not handle correctly those variables. The PatId has a resolved of Global 1
while the Id is unresolved.

@aryx
Copy link
Collaborator

aryx commented Nov 30, 2021

(found via -dump_named_ast)

aryx added a commit that referenced this issue Dec 8, 2021
This closes #4260

test plan:
test files included and also see how the same variables "v" now
resolve to the same Local each time
```
$ semgrep-core -lang py -dump_named_ast python/metavar_iterator.py
Pr(
  [DefStmt(
     ({
       name=EN(
              Id(("test", ()),
                {id_hidden=false; id_resolved=Ref(None); id_type=Ref(
                 None); id_constness=Ref(None); }));
       attrs=[]; tparams=[]; },
      FuncDef(
        {fkind=(Function, ()); fparams=[]; frettype=None;
         fbody=FBStmt(
                 Block(
                   [ExprStmt(
                      Comprehension(List,
                        (N(
                           Id(("v", ()),
                             {id_hidden=false;
                              id_resolved=Ref(Some((Local, 1)));
                              id_type=Ref(None); id_constness=Ref(None); })),
                         [CompFor((),
                            PatId(("v", ()),
                              {id_hidden=false;
                               id_resolved=Ref(Some((Local, 1)));
                               id_type=Ref(None); id_constness=Ref(None); }),
                            (),
                            N(
                              Id(("iterator", ()),
                                {id_hidden=false; id_resolved=Ref(None);
                                 id_type=Ref(None); id_constness=Ref(
                                 None); })))])), ());
                    ExprStmt(
                      Comprehension(List,
                        (Call(
                           N(
                             Id(("f", ()),
                               {id_hidden=false; id_resolved=Ref(None);
                                id_type=Ref(None); id_constness=Ref(None); })),
                           [Arg(
                              N(
                                Id(("v", ()),
                                  {id_hidden=false;
                                   id_resolved=Ref(Some((Local, 2)));
                                   id_type=Ref(None); id_constness=Ref(
                                   None); })))]),
                         [CompFor((),
                            PatId(("v", ()),
                              {id_hidden=false;
                               id_resolved=Ref(Some((Local, 2)));
                               id_type=Ref(None); id_constness=Ref(None); }),
                            (),
                            N(
                              Id(("iterator", ()),
                                {id_hidden=false; id_resolved=Ref(None);
                                 id_type=Ref(None); id_constness=Ref(
                                 None); })))])), ())]));
         })))])
```
@aryx aryx closed this as completed in #4399 Dec 8, 2021
aryx added a commit that referenced this issue Dec 8, 2021
This closes #4260

test plan:
test files included and also see how the same variables "v" now
resolve to the same Local each time
```
$ semgrep-core -lang py -dump_named_ast python/metavar_iterator.py
Pr(
  [DefStmt(
     ({
       name=EN(
              Id(("test", ()),
                {id_hidden=false; id_resolved=Ref(None); id_type=Ref(
                 None); id_constness=Ref(None); }));
       attrs=[]; tparams=[]; },
      FuncDef(
        {fkind=(Function, ()); fparams=[]; frettype=None;
         fbody=FBStmt(
                 Block(
                   [ExprStmt(
                      Comprehension(List,
                        (N(
                           Id(("v", ()),
                             {id_hidden=false;
                              id_resolved=Ref(Some((Local, 1)));
                              id_type=Ref(None); id_constness=Ref(None); })),
                         [CompFor((),
                            PatId(("v", ()),
                              {id_hidden=false;
                               id_resolved=Ref(Some((Local, 1)));
                               id_type=Ref(None); id_constness=Ref(None); }),
                            (),
                            N(
                              Id(("iterator", ()),
                                {id_hidden=false; id_resolved=Ref(None);
                                 id_type=Ref(None); id_constness=Ref(
                                 None); })))])), ());
                    ExprStmt(
                      Comprehension(List,
                        (Call(
                           N(
                             Id(("f", ()),
                               {id_hidden=false; id_resolved=Ref(None);
                                id_type=Ref(None); id_constness=Ref(None); })),
                           [Arg(
                              N(
                                Id(("v", ()),
                                  {id_hidden=false;
                                   id_resolved=Ref(Some((Local, 2)));
                                   id_type=Ref(None); id_constness=Ref(
                                   None); })))]),
                         [CompFor((),
                            PatId(("v", ()),
                              {id_hidden=false;
                               id_resolved=Ref(Some((Local, 2)));
                               id_type=Ref(None); id_constness=Ref(None); }),
                            (),
                            N(
                              Id(("iterator", ()),
                                {id_hidden=false; id_resolved=Ref(None);
                                 id_type=Ref(None); id_constness=Ref(
                                 None); })))])), ())]));
         })))])
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature:matching lang:python user:external requested by someone outside of r2c
Development

Successfully merging a pull request may close this issue.

2 participants