bblfsh / go-driver

GNU General Public License v3.0
10 stars 9 forks source link

"Incomplete" role for very common structure #42

Closed smacker closed 5 years ago

smacker commented 5 years ago

go:SelectorExpr has role Incomplete which is weird.

ncordon commented 5 years ago

This is coming from here: https://github.com/bblfsh/go-driver/blob/57cc13f9a962dc47829c381a61d4372d37b38358/driver/normalizer/annotation.go#L398 which contains a TODO of assigning a new role to these nodes @dennwc :see_no_evil:

This is an example of code which generates a go:SelectorExpr node (the c.b part):

package main

type a struct {
    b int
}

func main(){
    var c = a{1}
    var _ = c.b
}
ncordon commented 5 years ago

Also we have the same situation with the previous line: https://github.com/bblfsh/go-driver/blob/57cc13f9a962dc47829c381a61d4372d37b38358/driver/normalizer/annotation.go#L397

An example of code generating a go:Stmt node is fixtures/concurrency.go

ncordon commented 5 years ago

Let's take the following code, named selector.java:

class A {
  int b = 0;
}

public class field_access {
  public static void main (String[] args) {
    A c = new A();
    int d = c.b;
  }

}

Since the java-driver is labeling the c.d part as QualifiedIdentifier (see UAST chunk below extracted from running bblfsh-cli selector.java):

    { '@type': "uast:QualifiedIdentifier",
       '@pos': { '@type': "uast:Positions",
                       start: { '@type': "uast:Position",
                                  offset: 129,
                                  line: 8,
                                  col: 13,
                       },
                       end: { '@type': "uast:Position",
                                 offset: 132,
                                 line: 8,
                                 col: 16,
                       },
    }

@dennwc and me think we should make this consistent across languages. go:SelectorExpr is being labeled with role.Incomplete and we should attach role.Qualified instead

dennwc commented 5 years ago

We agree with @ncordon that we should reuse the Qualified role for this node.

Regarding the go:GoStmt, this is a different question. Although the go keyword is used more or less regularly in Go language, most other languages don't have a direct equivalent. Thus, I think it shouldn't be added as a role right now. We may open another issue to track it though. Maybe a generic "async statement" role may be a good fit for it.