External ".Type.Implements" not working?

[ghostiam]

I can't write a rule that only finds methods from the logger and ignores fmt.Errorf()/etc. When using only Where(m["log"].Type.Implements(`logrus.FieldLogger`)) I get panic.

go version go1.15.3 darwin/amd64

rules:

func _(m fluent.Matcher) {
    m.Import("github.com/sirupsen/logrus")

    // Detected "fmt" package
    // m.Match(`$log.Error($*_, $x, $*_)`,`$log.Errorf($*_, $x, $*_)`).
    //  Where(m["err"].Type.Is(`error`)).
    //  Report(`$log.WithError(err).Error(...)`)

    // Empty output
    m.Match(`$log.Error($*_, $x, $*_)`,`$log.Errorf($*_, $x, $*_)`).
        Where(m["err"].Type.Is(`error`) && m["log"].Type.Implements(`logrus.FieldLogger`)).
        Report(`$log.WithError(err).Error(...)`)

    // Panic
    // m.Match(`$log`).
    //  Where(m["log"].Type.Implements(`logrus.FieldLogger`)).
    //  Report(`$log`)

    // Worked only for *logrus.Logger
    // m.Match(`$log.Error($*_, $x, $*_)`,`$log.Errorf($*_, $x, $*_)`).
    //  Where(m["err"].Type.Is(`error`) && m["log"].Type.Is(`*logrus.Logger`)).
    //  Report(`$log.WithError(err).Error(...)`)
}

example:

package main

import (
    "errors"
    "fmt"

    "github.com/sirupsen/logrus"
)

func main() {
    logger := logrus.New()
    err := errors.New("test")

    // Ignore me
    err = fmt.Errorf("fmt wrap: %w", err)

    // Bad (type *logrus.Logger)
    logger.Error(err)
    logger.Error(42, err)
    logger.Error(fmt.Errorf("wrap: %w", err))
    logger.Errorf("logf wrap: %v", err)
    logger.Errorf("logf wrap: %d: %v", 42, err)

    // Bad (type *logrus.Entry)
    loggerEntry := logrus.NewEntry(logger)
    loggerEntry.Error(err)
    loggerEntry.Error(42, err)
    loggerEntry.Error(fmt.Errorf("wrap: %w", err))
    loggerEntry.Errorf("logf wrap: %v", err)
    loggerEntry.Errorf("logf wrap: %d: %v", 42, err)

    // Bad (interface logrus.FieldLogger)
    var loggerIface logrus.FieldLogger = logger
    loggerIface.Error(err)
    loggerIface.Error(42, err)
    loggerIface.Error(fmt.Errorf("wrap: %w", err))
    loggerIface.Errorf("logf wrap: %v", err)
    loggerIface.Errorf("logf wrap: %d: %v", 42, err)

    // Good
    logger.WithError(err).Error()
    logger.WithError(err).Error(42)
    logger.WithError(err).Error("log")
    logger.WithError(err).Errorf("logf")
    logger.WithError(err).Errorf("logf: %d", 42)

    loggerEntry.WithError(err).Error()
    loggerEntry.WithError(err).Error(42)
    loggerEntry.WithError(err).Error("log")
    loggerEntry.WithError(err).Errorf("logf")
    loggerEntry.WithError(err).Errorf("logf: %d", 42)

    loggerIface.WithError(err).Error()
    loggerIface.WithError(err).Error(42)
    loggerIface.WithError(err).Error("log")
    loggerIface.WithError(err).Errorf("logf")
    loggerIface.WithError(err).Errorf("logf: %d", 42)
}

panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x12305b7]

goroutine 273 [running]:
go/types.(*Checker).missingMethod(0x0, 0x0, 0x0, 0xc000955a90, 0x1, 0x0, 0x0)
        /usr/local/opt/go/libexec/src/go/types/lookup.go:286 +0x77
go/types.MissingMethod(...)
        /usr/local/opt/go/libexec/src/go/types/lookup.go:266
go/types.Implements(0x0, 0x0, 0xc000955a90, 0x0)
        /usr/local/opt/go/libexec/src/go/types/api.go:384 +0x4d
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesParser).walkFilter.func11(0x0, 0x0, 0xc0009872f0, 0x0)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/ruleguard/parser.go:618 +0x46
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).handleMatch(0xc00096edc0, 0x7ffeefbfe879, 0x11, 0x0, 0x0, 0xc000925dc0, 0xc00019a217, 0x4, 0x0, 0x0, ...)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:128 +0x91f
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).run.func2.1(0x146a6e0, 0xc00028aee0, 0xc000987350)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:80 +0xcc
github.com/quasilyte/go-ruleguard/internal/mvdan.cc/gogrep.(*Pattern).MatchNode(0xc000925dc0, 0x146a6e0, 0xc00028aee0, 0xc0005b7608)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/internal/mvdan.cc/gogrep/kludge.go:41 +0xb4
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).run.func2(0x146a6e0, 0xc00028aee0, 0x203000)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:79 +0x15f
go/ast.inspector.Visit(0xc00029e090, 0x146a6e0, 0xc00028aee0, 0x203000, 0x203000)
        /usr/local/opt/go/libexec/src/go/ast/walk.go:373 +0x3a
go/ast.Walk(0x1468a60, 0xc00029e090, 0x146a6e0, 0xc00028aee0)
        /usr/local/opt/go/libexec/src/go/ast/walk.go:52 +0x63
go/ast.Walk(0x1468a60, 0xc00029e090, 0x146a520, 0xc000248800)
        /usr/local/opt/go/libexec/src/go/ast/walk.go:352 +0x2305
go/ast.Inspect(...)
        /usr/local/opt/go/libexec/src/go/ast/walk.go:385
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).run(0xc00096edc0, 0xc000248800, 0x0, 0xc000044900)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:75 +0x27f
github.com/quasilyte/go-ruleguard/ruleguard.RunRules(...)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/ruleguard/ruleguard.go:30
github.com/quasilyte/go-ruleguard/analyzer.runAnalyzer(0xc00018b040, 0xc00018b040, 0x0, 0x0, 0x0)
        ${GOPATH}/src/github.com/quasilyte/go-ruleguard/analyzer/analyzer.go:81 +0x23a
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc0001021e0)
        ${GOPATH}/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:691 +0x8b9
sync.(*Once).doSlow(0xc0001021e0, 0xc0000ec790)
        /usr/local/opt/go/libexec/src/sync/once.go:66 +0xec
sync.(*Once).Do(...)
        /usr/local/opt/go/libexec/src/sync/once.go:57
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0xc0001021e0)
        ${GOPATH}/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:579 +0x65
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0xc0001021e0)
        ${GOPATH}/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:567 +0x34
created by golang.org/x/tools/go/analysis/internal/checker.execAll
        ${GOPATH}/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:573 +0x125

[quasilyte]

I'll take a look at it this weekend. :open_hands: Thank you for the detailed report!

[ghostiam]

Updated examples and rules, and also added an example with interface implementation: https://gist.github.com/ghostiam/3c53ee547893e801662a8fb6087c9651

[quasilyte]

I solved this issue on my local branch, but my problem is: I can't reproduce this issue in tests. I would feel uncomfortable by sending a fix that would not have a good tests that would protect against regressions.

[quasilyte]

I also noted that in your examples there is an err filter, but the pattern uses $x instead.

[quasilyte]

But I can reproduce this issue in Docker tests. Locally, it doesn't want to be reproduced.

I have a solution, but it's ugly. I don't think I have a choice here though.