From 372015deb48c7445a077b889e4136daa51545a39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Du=C5=A1an=20Kasan?= Date: Fri, 20 Jul 2018 17:05:08 +0200 Subject: [PATCH] add linter to check for missing finishers (#85) --- cmd/lint/lint.go | 175 +++++++++++++++++++++++++++++++++++++++++++++ cmd/lint/readme.md | 37 ++++++++++ 2 files changed, 212 insertions(+) create mode 100644 cmd/lint/lint.go create mode 100644 cmd/lint/readme.md diff --git a/cmd/lint/lint.go b/cmd/lint/lint.go new file mode 100644 index 0000000..700371f --- /dev/null +++ b/cmd/lint/lint.go @@ -0,0 +1,175 @@ +package main + +import ( + "flag" + "fmt" + "go/ast" + "go/token" + "go/types" + "os" + "path/filepath" + "strings" + + "golang.org/x/tools/go/loader" +) + +var ( + recursivelyIgnoredPkgs arrayFlag + ignoredPkgs arrayFlag + ignoredFiles arrayFlag + allowedFinishers arrayFlag = []string{"Msg", "Msgf"} + rootPkg string +) + +// parse input flags and args +func init() { + flag.Var(&recursivelyIgnoredPkgs, "ignorePkgRecursively", "ignore the specified package and all subpackages recursively") + flag.Var(&ignoredPkgs, "ignorePkg", "ignore the specified package") + flag.Var(&ignoredFiles, "ignoreFile", "ignore the specified file by its path and/or go path (package/file.go)") + flag.Var(&allowedFinishers, "finisher", "allowed finisher for the event chain") + flag.Parse() + + // add zerolog to recursively ignored packages + recursivelyIgnoredPkgs = append(recursivelyIgnoredPkgs, "github.com/rs/zerolog") + args := flag.Args() + if len(args) != 1 { + fmt.Fprintln(os.Stderr, "you must provide exactly one package path") + os.Exit(1) + } + rootPkg = args[0] +} + +func main() { + // load the package and all its dependencies + conf := loader.Config{} + conf.Import(rootPkg) + p, err := conf.Load() + if err != nil { + fmt.Fprintf(os.Stderr, "Error: unable to load the root package. %s\n", err.Error()) + os.Exit(1) + } + + // get the github.com/rs/zerolog.Event type + event := getEvent(p) + if event == nil { + fmt.Fprintln(os.Stderr, "Error: github.com/rs/zerolog.Event declaration not found, maybe zerolog is not imported in the scanned package?") + os.Exit(1) + } + + // get all selections (function calls) with the github.com/rs/zerolog.Event (or pointer) receiver + selections := getSelectionsWithReceiverType(p, event) + + // print the violations (if any) + hasViolations := false + for _, s := range selections { + if hasBadFinisher(p, s) { + hasViolations = true + fmt.Printf("Error: missing or bad finisher for log chain, last call: %q at: %s:%v\n", s.fn.Name(), p.Fset.File(s.Pos()).Name(), p.Fset.Position(s.Pos()).Line) + } + } + + // if no violations detected, return normally + if !hasViolations { + fmt.Println("No violations found") + return + } + + // if violations were detected, return error code + os.Exit(1) +} + +func getEvent(p *loader.Program) types.Type { + for _, pkg := range p.AllPackages { + if strings.HasSuffix(pkg.Pkg.Path(), "github.com/rs/zerolog") { + for _, d := range pkg.Defs { + if d != nil && d.Name() == "Event" { + return d.Type() + } + } + } + } + + return nil +} + +func getSelectionsWithReceiverType(p *loader.Program, targetType types.Type) map[token.Pos]selection { + selections := map[token.Pos]selection{} + + for _, z := range p.AllPackages { + for i, t := range z.Selections { + switch o := t.Obj().(type) { + case *types.Func: + // this is not a bug, o.Type() is always *types.Signature, see docs + if vt := o.Type().(*types.Signature).Recv(); vt != nil { + typ := vt.Type() + if pointer, ok := typ.(*types.Pointer); ok { + typ = pointer.Elem() + } + + if typ == targetType { + if s, ok := selections[i.Pos()]; !ok || i.End() > s.End() { + selections[i.Pos()] = selection{i, o, z.Pkg} + } + } + } + default: + // skip + } + } + } + + return selections +} + +func hasBadFinisher(p *loader.Program, s selection) bool { + pkgPath := strings.TrimPrefix(s.pkg.Path(), rootPkg+"/vendor/") + absoluteFilePath := strings.TrimPrefix(p.Fset.File(s.Pos()).Name(), rootPkg+"/vendor/") + goFilePath := pkgPath + "/" + filepath.Base(p.Fset.Position(s.Pos()).Filename) + + for _, f := range allowedFinishers { + if f == s.fn.Name() { + return false + } + } + + for _, ignoredPkg := range recursivelyIgnoredPkgs { + if strings.HasPrefix(pkgPath, ignoredPkg) { + return false + } + } + + for _, ignoredPkg := range ignoredPkgs { + if pkgPath == ignoredPkg { + return false + } + } + + for _, ignoredFile := range ignoredFiles { + if absoluteFilePath == ignoredFile { + return false + } + + if goFilePath == ignoredFile { + return false + } + } + + return true +} + +type arrayFlag []string + +func (i *arrayFlag) String() string { + return fmt.Sprintf("%v", []string(*i)) +} + +func (i *arrayFlag) Set(value string) error { + *i = append(*i, value) + return nil +} + +type selection struct { + *ast.SelectorExpr + fn *types.Func + pkg *types.Package +} diff --git a/cmd/lint/readme.md b/cmd/lint/readme.md new file mode 100644 index 0000000..a15cba5 --- /dev/null +++ b/cmd/lint/readme.md @@ -0,0 +1,37 @@ +# Zerolog Lint + +This is a basic linter that checks for missing log event finishers. Finds errors like: `log.Error().Int64("userID": 5)` - missing the `Msg`/`Msgf` finishers. + +## Problem + +When using zerolog it's easy to forget to finish the log event chain by calling a finisher - the `Msg` or `Msgf` function that will schedule the event for writing. The problem with this is that it doesn't warn/panic during compilation and it's not easily found by grep or other general tools. It's even prominently mentioned in the project's readme, that: + +> It is very important to note that when using the **zerolog** chaining API, as shown above (`log.Info().Msg("hello world"`), the chain must have either the `Msg` or `Msgf` method call. If you forget to add either of these, the log will not occur and there is no compile time error to alert you of this. + +## Solution + +A basic linter like this one here that looks for method invocations on `zerolog.Event` can examine the last call in a method call chain and check if it is a finisher, thus pointing out these errors. + +## Usage + +Just compile this and then run it. Or just run it via `go run` command via something like `go run cmd/lint/lint.go`. + +The command accepts only one argument - the package to be inspected - and 4 optional flags, all of which can occur multiple times. The standard synopsis of the command is: + +`lint [-finisher value] [-ignoreFile value] [-ignorePkg value] [-ignorePkgRecursively value] package` + +#### Flags + +- finisher + - specify which finishers to accept, defaults to `Msg` and `Msgf` +- ignoreFile + - which files to ignore, either by full path or by go path (package/file.go) +- ignorePkg + - do not inspect the specified package if found in the dependecy tree +- ignorePkgRecursively + - do not inspect the specified package or its subpackages if found in the dependency tree + +## Drawbacks + +As it is, linter can generate a false positives in a specific case. These false positives come from the fact that if you have a method that returns a `zerolog.Event` the linter will flag it because you are obviously not finishing the event. This will be solved in later release. +