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

Decide on the selector expressions behavior #16

Open
quasilyte opened this issue Dec 20, 2019 · 5 comments
Open

Decide on the selector expressions behavior #16

quasilyte opened this issue Dec 20, 2019 · 5 comments
Assignees
Labels
DSL May require DSL changes

Comments

@quasilyte
Copy link
Owner

quasilyte commented Dec 20, 2019

Given the path.Join(...) pattern, there are multiple interpretations that can be made:

  1. path could be a local variable (and Join is either a func-typed property or a method).
  2. path can be a package not from stdlib (e.g. github.com/blahblah/path).
  3. path is a stdlib package. (same as 2.)
  4. path is a type name, Join is a method name, path.Join is a method expression.

Our current behavior is based on what gogrep does. It's 100% syntactical, so it matches all of them.

Here is a proposal of how we can make rules that can match the pattern with a better precision.

func example() {
  // Behaves as before. Matches everything.
  m.Match(`path.Join($*_)`)

  // 1.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Var.Name("path"))

  // 2.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Package.Path("github.com/blahblah/path"))

  // 3.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Package.Path("path"))

  // 4.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Type.Name("path"))
}

CodeQL doesn't have such a problem because it requires you to specify a type of all query variables in every query. gorules have no variable definitions concept, but we can infer their "node kind" constraint through Where clause.

This proposal introduces new fields of a matcher variable: Var, Package and Type (there can be more later). If Type.Name is used, m["p"] is expected to be a type.

A slightly different approach includes adding all predicates to the fluent.Var, as it's done in reflect package:

func example() {
  // Behaves as before. Matches everything.
  m.Match(`path.Join($*_)`)

  // 1.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].VarName("path"))

  // 2.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].PkgPath("github.com/blahblah/path"))

  // 3.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].PkgPath("path"))

  // 4.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].TypeName("path"))
}

A more radical way is to introduce non-ambigous operators:

  • x.f() - f function call from a package x
  • x->f() - f method call from a variable x (rare case)
  • x::f() - f function call from a type x (very rare case)
func example() {
  // 1.
  m.Match(`path->Join($*_)`)

  // 2 and 3.
  // If custom "path" is needed, Import() can be used.
  m.Match(`path.Join($*_)`)

  // 4.
  m.Match(`path::Join($*_)`)
}

This is harder to implement and makes patterns even more incompatible with Go syntax. gogrep doesn't support such syntax => custom source preprocessing will be required.

@utrack
Copy link

utrack commented Jan 10, 2020

We could use Go-style operators:
x.bar - call of bar from a package x
(*x.Foo).bar - call of bar from a type x.Foo

however, those look more verbose than the proposed Where(var.Package.Path()) syntax.

@quasilyte
Copy link
Owner Author

We can't use that syntax, since it has different meaning in Go.

It's a method expression.
Here is an example: https://play.golang.org/p/icaUkRgqCda

Everything inside Match() argument is a plain gogrep template and it matches everything, except meta variables (things starting with $) literally, so (*x.Foo).bar would match method expression.

@nightlyone
Copy link

What about adding a match that expresses that a specific import path is imported for that file and is not renamed?

If gogrep could handle it, the import package name path could also be transparently replaced to the imported name while applying the match and the replacement.

@sarathsp06
Copy link

Is it still an active discussion?

@quasilyte
Copy link
Owner Author

@sarathsp06 it is.

But my main problem is the lack of time to work on it. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DSL May require DSL changes
Projects
None yet
Development

No branches or pull requests

4 participants