Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for unwanted function; added more tests. #8

Merged
merged 3 commits into from
Mar 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 19 additions & 13 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
# faillint [![](https://github.com/fatih/faillint/workflows/build/badge.svg)](https://github.com/fatih/faillint/actions)

Faillint is a simple Go linter that fails when a specific set of import paths
are used. It's meant to be used in CI/CD environments to catch rules you want
to enforce in your projects.
or exported path's functions, constant, vars or types are used. It's meant to be
used in CI/CD environments to catch rules you want to enforce in your projects.

As an example, you could enforce the usage of `github.com/pkg/errors` instead
of the `errors` package. To prevent the usage of the `errors` package, you can
configure `faillint` to fail whenever someone imports the `errors` package in
this case.
this case. To make sure `fmt.Errorf` is not used for creating errors as well,
you can configure to fail on such single function of `fmt` as well.

![faillint](https://user-images.githubusercontent.com/438920/74105802-f7158300-4b15-11ea-8e23-16be5cd3b971.gif)

Expand All @@ -22,37 +23,42 @@ go get github.com/fatih/faillint
`faillint` works on a file, directory or a Go package:

```sh
$ faillint -paths "errors" foo.go # pass a file
$ faillint -paths "errors" ./... # recursively analyze all files
$ faillint -paths "errors" github.com/fatih/gomodifytags # or pass a package
$ faillint -paths "errors,fmt.{Errorf}" foo.go # pass a file
$ faillint -paths "errors,fmt.{Errorf}" ./... # recursively analyze all files
$ faillint -paths "errors,fmt.{Errorf}" github.com/fatih/gomodifytags # or pass a package
```

By default, `faillint` will not check any import paths. You need to explicitly
define it with the `-paths` flag, which is comma-separated list. Some examples are:

```
# fail if the errors package is used
# Fail if the errors package is used.
-paths "errors"

# fail if the old context package is imported
# Fail if the old context package is imported.
-paths "golang.org/x/net/context"

# fail both on stdlib log and errors package to enforce other internal libraries
# Fail both on stdlib log and errors package to enforce other internal libraries.
-paths "log,errors"
```

# Fail if any of Print, Printf of Println function were used from fmt library.
-paths "fmt.{Print,Printf,Println}"
```

If you have a preferred import path to suggest, append the suggestion after a `=` character:

```
# fail if the errors package is used and suggest to use github.com/pkg/errors
# Fail if the errors package is used and suggest to use github.com/pkg/errors.
-paths "errors=github.com/pkg/errors"

# fail for the old context import path and suggest to use the stdlib context
# Fail for the old context import path and suggest to use the stdlib context.
-paths "golang.org/x/net/context=context"

# fail both on stdlib log and errors package to enforce other libraries
# Fail both on stdlib log and errors package to enforce other libraries.
-paths "log=go.uber.org/zap,errors=github.com/pkg/errors"

# Fail on fmt.Errorf and suggest the Errorf function from github.compkg/errors instead.
-paths "fmt.{Errorf}=github.com/pkg/errors.{Errorf}"
```

## Example
Expand Down
177 changes: 114 additions & 63 deletions faillint/faillint.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,134 +5,185 @@ package faillint
import (
"fmt"
"go/ast"
"go/token"
"regexp"
"strconv"
"strings"
"unicode"

"golang.org/x/tools/go/analysis"
)

// pathsRegexp represents a regexp that is used to parse -paths flag.
// It parses flag content in set of 3 subgroups:
//
// * import: Mandatory part. Go import path in URL format to be unwanted or have unwanted declarations.
// * declarations: Optional declarations in `{ }`. If set, using the import is allowed expect give declarations.
// * suggestion: Optional suggestion to print when unwanted import or declaration is found.
//
var pathsRegexp = regexp.MustCompile(`(?P<import>[\w/.-]+[\w])(\.?{(?P<declarations>[\w-,]+)}|)(=(?P<suggestion>[\w/.-]+[\w](\.?{[\w-,]+}|))|)`)

type faillint struct {
paths string // -paths flag
ignoretests bool // -ignore-tests flag
}

// Analyzer global instance of the linter (if possible use NewAnalyzer)
// Analyzer is a global instance of the linter.
// DEPRECATED: Use faillint.New instead.
var Analyzer = NewAnalyzer()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was here to provide backwards compatibility. The initial release of faillint was providing this global variable hence I wanted to make sure we keep it. However it's not something I like and I wonder if we should just remove it and break the compatibility. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally see your point. I think there is no harm in getting this back as deprecated var.. (: and remove in v2


// NewAnalyzer create a faillint analyzer
// NewAnalyzer create a faillint analyzer.
func NewAnalyzer() *analysis.Analyzer {
f := faillint{
paths: "",
ignoretests: false,
}
a := &analysis.Analyzer{
Name: "faillint",
Doc: "report unwanted import path usages",
Doc: "Report unwanted import path or exported declaration usages",
Run: f.run,
RunDespiteErrors: true,
}

a.Flags.StringVar(&f.paths, "paths", "", "import paths to fail")
a.Flags.StringVar(&f.paths, "paths", "", `import paths or exported declarations (i.e: functions, constant, types or variables) to fail.
E.g. errors=github.com/pkg/errors,fmt.{Errorf}=github.com/pkg/errors.{Errorf},fmt.{Println,Print,Printf},github.com/prometheus/client_golang/prometheus.{DefaultGatherer,MustRegister}`)
a.Flags.BoolVar(&f.ignoretests, "ignore-tests", false, "ignore all _test.go files")
return a
}

// Run is the runner for an analysis pass
func (f *faillint) run(pass *analysis.Pass) (interface{}, error) {
if f.paths == "" {
return nil, nil
func trimAllWhitespaces(str string) string {
var b strings.Builder
b.Grow(len(str))
for _, ch := range str {
if !unicode.IsSpace(ch) {
b.WriteRune(ch)
}
}
return b.String()
}

p := strings.Split(f.paths, ",")

suggestions := make(map[string]string, len(p))
imports := make([]string, 0, len(p))

for _, s := range p {
imps := strings.Split(s, "=")
type path struct {
imp string
decls []string
sugg string
}

imp := imps[0]
suggest := ""
if len(imps) == 2 {
suggest = imps[1]
func parsePaths(paths string) []path {
pathGroups := pathsRegexp.FindAllStringSubmatch(trimAllWhitespaces(paths), -1)

parsed := make([]path, 0, len(pathGroups))
for _, group := range pathGroups {
p := path{}
for i, name := range pathsRegexp.SubexpNames() {
switch name {
case "import":
p.imp = group[i]
case "suggestion":
p.sugg = group[i]
case "declarations":
if group[i] == "" {
break
}
p.decls = strings.Split(group[i], ",")
}
}

imports = append(imports, imp)
suggestions[imp] = suggest
parsed = append(parsed, p)
}
return parsed
}

// run is the runner for an analysis pass.
func (f *faillint) run(pass *analysis.Pass) (interface{}, error) {
if f.paths == "" {
return nil, nil
}
for _, file := range pass.Files {
if f.ignoretests && strings.Contains(pass.Fset.File(file.Package).Name(), "_test.go") {
continue
}
for _, path := range imports {
imp := usesImport(file, path)
if imp == nil {
for _, path := range parsePaths(f.paths) {
specs := importSpec(file, path.imp)
if len(specs) == 0 {
continue
}

impPath := importPath(imp)

msg := fmt.Sprintf("package %q shouldn't be imported", impPath)
if s := suggestions[impPath]; s != "" {
msg += fmt.Sprintf(", suggested: %q", s)
for _, spec := range specs {
usages := importUsages(file, spec)
if len(usages) == 0 {
continue
}

if _, ok := usages[unspecifiedUsage]; ok || len(path.decls) == 0 {
// File using unwanted import. Report.
msg := fmt.Sprintf("package %q shouldn't be imported", importPath(spec))
if path.sugg != "" {
msg += fmt.Sprintf(", suggested: %q", path.sugg)
}
pass.Reportf(spec.Path.Pos(), msg)
continue
}

// Not all usages are forbidden. Report only unwanted declarations.
for _, declaration := range path.decls {
positions, ok := usages[declaration]
if !ok {
continue
}
msg := fmt.Sprintf("declaration %q from package %q shouldn't be used", declaration, importPath(spec))
if path.sugg != "" {
msg += fmt.Sprintf(", suggested: %q", path.sugg)
}
for _, pos := range positions {
pass.Reportf(pos, msg)
}
}
}

pass.Reportf(imp.Path.Pos(), msg)
}
}

return nil, nil
}

// usesImport reports whether a given import is used.
func usesImport(f *ast.File, path string) *ast.ImportSpec {
spec := importSpec(f, path)
if spec == nil {
return nil
}
const unspecifiedUsage = "unspecified"

name := spec.Name.String()
switch name {
// importUsages reports all exported declaration used for a given import.
func importUsages(f *ast.File, spec *ast.ImportSpec) map[string][]token.Pos {
importRef := spec.Name.String()
switch importRef {
case "<nil>":
// If the package name is not explicitly specified,
importRef, _ = strconv.Unquote(spec.Path.Value)
// If the package importRef is not explicitly specified,
// make an educated guess. This is not guaranteed to be correct.
lastSlash := strings.LastIndex(path, "/")
if lastSlash == -1 {
name = path
} else {
name = path[lastSlash+1:]
lastSlash := strings.LastIndex(importRef, "/")
if lastSlash != -1 {
importRef = importRef[lastSlash+1:]
}
case "_", ".":
// Not sure if this import is used - err on the side of caution.
return spec
// Not sure if this import is used - on the side of caution, report special "unspecified" usage.
return map[string][]token.Pos{unspecifiedUsage: nil}
}
usages := map[string][]token.Pos{}

var used bool
ast.Inspect(f, func(n ast.Node) bool {
sel, ok := n.(*ast.SelectorExpr)
if ok && isTopName(sel.X, name) {
used = true
if !ok {
return true
}
if isTopName(sel.X, importRef) {
usages[sel.Sel.Name] = append(usages[sel.Sel.Name], sel.Sel.NamePos)
}
return true
})

if used {
return spec
}

return nil
return usages
}

// importSpec returns the import spec if f imports path,
// or nil otherwise.
func importSpec(f *ast.File, path string) *ast.ImportSpec {
// importSpecs returns all import specs for f import statements importing path.
func importSpec(f *ast.File, path string) (imports []*ast.ImportSpec) {
for _, s := range f.Imports {
if importPath(s) == path {
return s
imports = append(imports, s)
}
}
return nil
return imports
}

// importPath returns the unquoted import path of s,
Expand Down