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

golang package import not binding metavariable #2031

Closed
ievans opened this issue Nov 12, 2020 · 6 comments · Fixed by #2076
Closed

golang package import not binding metavariable #2031

ievans opened this issue Nov 12, 2020 · 6 comments · Fixed by #2076
Assignees
Labels
bug Something isn't working lang:golang user:external requested by someone outside of r2c

Comments

@ievans
Copy link
Member

ievans commented Nov 12, 2020

The snippet below works if you use a generic metavariable (eg $X) for the line inside $F, but not if you try to re-reference the $P metavar from the import.

import $P "github.com/sirupsen/logrus"

func $F() {
  ...
  $P.WithError(...).Info(...)
  ...
}

https://semgrep.dev/s/3e7K

@ievans ievans added bug Something isn't working lang:golang labels Nov 12, 2020
@nbrahms nbrahms added the user:internal requested only by someone within Semgrep Inc. label Nov 12, 2020
@nbrahms
Copy link
Contributor

nbrahms commented Nov 12, 2020

Possibly related to #1771

@nbrahms
Copy link
Contributor

nbrahms commented Nov 12, 2020

Also see #2029

@ievans ievans added the user:external requested by someone outside of r2c label Nov 12, 2020
@nbrahms nbrahms removed the user:internal requested only by someone within Semgrep Inc. label Nov 12, 2020
@mschwager
Copy link
Contributor

Looks like we have the same thing in Python: https://semgrep.dev/s/5W70/

However, our Python equivalences mitigate this concern, so we can't really compare the two: https://semgrep.dev/s/GRqq/

I'll have to defer to @aryx on this one. Is it possible to do a similar equivalence as Python and have users specify the fully qualified name, e.g. logrus.WithError(...).Info(...), which automatically works with import aliasing?

@glb
Copy link

glb commented Nov 13, 2020

$P.WithError(...).Info(...), interestingly, matches both of the following:

  log.WithFields(nil).WithError(nil).Info("something")
  logctx.Foo().WithError(nil).Info("something")

which is ... good? ... but not especially expected.

@aryx
Copy link
Collaborator

aryx commented Nov 18, 2020

log.WithFields(nil).WithError(nil).Info("something") is parsed as
( ( ( log.WithFields(nil) ) .WithError(nil) ).Info("something"))
so yes $P can match the log.WithFields(nil) expression

@aryx
Copy link
Collaborator

aryx commented Nov 18, 2020

Note that fully qualifying with logrus.WithError(...).Info(...) currently works for Go too.
However, it feels like again, just like for attributes, forcing users to fully qualify is confusing.
I will allow to not fully qualify if the aliased name match the actual code.
We do that already for attributes/annotations.

@aryx aryx self-assigned this Nov 18, 2020
aryx added a commit that referenced this issue Nov 18, 2020
…ame code

We used to force to fully qualify entities but this is confusing,
especially when you use metavar for imported module/entity which prevent
you to reuse the same metavar later.

Related to #2029
but this time for expression identifiers (Id), not attributes.

Fixes #2031

Note that semgrep-rules will fail because of some code
that now correctly match but was not tagged as. I'll fix that
later (mutual recursivity issue between CI of semgrep and semgrep-rules).

test plan:
many test files included
make test
x
aryx added a commit that referenced this issue Nov 18, 2020
…ame code (#2076)

We used to force to fully qualify entities but this is confusing,
especially when you use metavar for imported module/entity which prevent
you to reuse the same metavar later.

Related to #2029
but this time for expression identifiers (Id), not attributes.

Fixes #2031

Note that semgrep-rules will fail because of some code
that now correctly match but was not tagged as. I'll fix that
later (mutual recursivity issue between CI of semgrep and semgrep-rules).

test plan:
many test files included
make test
x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lang:golang user:external requested by someone outside of r2c
Development

Successfully merging a pull request may close this issue.

5 participants