gogrep icon indicating copy to clipboard operation
gogrep copied to clipboard

Add a gogrep talk link into a readme?

Open quasilyte opened this issue 5 years ago • 4 comments

I think the documentation could be slightly improved with gogrep-related materials, like https://github.com/mvdan/talks/blob/master/2018/gogrep.slide

It gives a pretty good introduction for people who are new to a concept of ast-based code matching.

That .slide can be compiled into PDF, uploaded to a speakerdeck and that speakerdeck link can be added to a README.

Does it make sense? :)

quasilyte avatar Dec 18 '19 13:12 quasilyte

Hm. I worry that the material would go stale, and confuse people more than anything.

But maybe that won't be so bad if we add a date right next to the link.

We also don't need to mess with PDFs; https://talks.godoc.org/github.com/mvdan/talks/2018/gogrep.slide should suffice. I get that it stinks on mobile, but we can also link the original plaintext version.

mvdan avatar Dec 21 '19 00:12 mvdan

I am trying to understand how gogrep matches AST nodes when invoked as gogrep -x '$x{$*_}'. It's not immediately obvious how it works.

That command seems to match any statement initializing a composite literal (maps, struct, array, slice), which makes sense. For example, that command matches the following statements:

MyStruct{}    // ast.CompositeLit, ast.Ident
mypkg.MyStruct{}   // ast.CompositeLit, ast.SelectorExpr, ast.Ident, ast.Ident
v := MyStruct{}  // ast.AssignStmt, ast.Ident, ast.CompositeLit, ast.Ident
v := mypkg.MyStruct{}  // ast.AssignStmt, ast.Ident, ast.CompositeLit, ast.SelectorExpr, ast.Ident, ast.Ident
v := mypkg.MyStruct{f1: "abc"}
v := &mypkg.MyStruct{f1: "abc"}
myFunc(mypkg.MyStruct{f1: "abc"})
primes := [6]int{2, 3, 5, 7, 11, 13}
s := []struct {
    i int
    b bool
}{
    {2, true},
    {3, false},
}

What is less obvious is that gogrep -x '$x{$*_}' does not seem to match other code patterns, where an ast node is followed by a {. In particular, that does not match the following statements:

switch { }  // ast.SwitchStmt, ast.BlockStmt
if ok := true ; ok {  }
if true { }
for { }
func main() {} 

In the above code, it would be reasonable to assume gogrep -x '$x{$*_}' will match switch {}, since switch is represented in AST as a ast.SwitchStmt and { is a ast.BlockStmt, but it does not. Just to be clear, I'm not trying to argue whether that should or should not match; I am merely suggesting to improve the documentation.

Another behavior that would be worth documenting is how exactly gogrep matches the code:

mypkg.MyStruct{}   // which is represented in AST as ast.CompositeLit, ast.SelectorExpr, ast.Ident, ast.Ident

In the following scenarios:

  1. gogrep -x '$x{$*_}'
  2. gogrep -x '$x.$y{$*_}'

The question is which AST nodes are $x and $y mapped to. For case (1), my observation is that it seems to consume the entire composite literal mypkg.MyStruct, but that wasn't obvious. I wasn't sure if this would map to mypkg.MyStruct or just MyStruct. In case (2), it's less ambiguous.

sebastien-rosset avatar Nov 30 '20 18:11 sebastien-rosset

The matching could be more aggressive; it's always been a fine balance between "matches too much" and "matches too little". This is also why I added an "aggressive" option to the pattern syntax, but I honestly don't like it all that much either.

mvdan avatar Dec 11 '20 12:12 mvdan

I'm just starting to read the code. Is aggressive supposed to be an option like like regex greedy vs reluctant vs possessive?

BTW, I found it is very helpful to add this line after https://github.com/mvdan/gogrep/blob/master/main.go#L162:

ast.Fprint(m.out, m.fset, n, nil)

It helps to understand the details of the matching results. You can quickly look at how any matching piece of code is modeled in AST. Maybe add a --debug flag or similar?

sebastien-rosset avatar Dec 11 '20 17:12 sebastien-rosset