gorules: import: table is overwritten by module name collisions

[quasilyte]

gorules: import: table is overwritten by module name collisions

[utrack]

What do you think about using symbol's FQDN in Matcher when we don't want to be ambiguous?

ex. m.Match("github.com/bradfitz/gomemcache/memcache.New")

[quasilyte]

That looks like a bug. We do m.Import() call to specify how to interpret memcache in all patterns below, so we don't need to use FQDN there. I'll take a look. Support for the FQDN can also be added.

[quasilyte]

My attempts to reproduce it are not successful so far. I'll try doing it outside of the ruleguard tests, as you described.

[utrack]

Example repo works on any machine, I've rechecked that just now.

It looks like gogrep just doesn't return any info regarding the package, and rulesRunner.handleMatch doesn't have any package-related filtering - so it's more of a gogrep's limitation.

gogrep -x 'memcache.New' ./... on an example repo returns:

[]ast.Node{
           &ast.SelectorExpr{
               X:  &ast.Ident{
                   NamePos: 393781,
                   Name:    "memcache",
                   Obj:     (*ast.Object)(nil),
               },
               Sel: &ast.Ident{
                   NamePos: 393790,
                   Name:    "New",
                   Obj:     (*ast.Object)(nil),
               },
           },
       }

gogrep.MatchData doesn't enrich that in any way.

[utrack]

I guess it's possible to use full function name in the rule, pass shortname to gogrep and then recheck it via typeutil on its way back to allow full names in rule definitions. How does that sound for you? I can hack a PR.

[quasilyte]

Please take a look at the test cases I recently added. It shows that Import() does work at least in some cases. It would be very helpful if you would submit a test case that does reproduce the problem. If it's not possible in the testdata context, then we need to figure out what's the difference and adjust the testing environment (if possible).

I haven't looked into this yet that closely, but here are my thoughts:

• We know how to resolve xyz package when Import() is used, it records the full package name, so there should be no need to use full names in rules (since they are inferred from the short name using the local import table).
• It should be possible to get the full name through types info for the given selector expression.

FQDN in rules is a valid new feature, but I would like to fix a bug first. I was aware of the potential collisions, this is why Import calls are introduced in the first place.

I don't think gogrep is a problem here, it only helps us to get the matching node. After that we get its type info and should have all FQDN on our hands at that time (because short names can be translated to the FQDN with if Import() is not bugged).

[quasilyte]

Now I see that it's related to #16.