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 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 108 additions & 66 deletions faillint/faillint.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,134 +5,176 @@ package faillint
import (
"fmt"
"go/ast"
"go/token"
"regexp"
"strconv"
"strings"
"unicode"

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

var (
pathRegexp = regexp.MustCompile(`(?P<import>[\w/.-]+[\w])(\.?{(?P<functions>[\w-,]+)}|)(=(?P<suggestion>[\w/.-]+[\w](\.?{[\w-,]+}|))|)`)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a doc comment describing the regexp. I think anyone reading this will scratch their head trying to understand it. At least documenting what the import, functions ,etc... identifiers are would be very useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will do

)

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

// Analyzer global instance of the linter (if possible use NewAnalyzer)
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
func NewAnalyzer() *analysis.Analyzer {
// New create a faillint analyzer.
func New() *analysis.Analyzer {
Copy link
Owner

Choose a reason for hiding this comment

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

I like NewAnalyzer better, would be happy if you can revert the naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? To me it's like including the type in variable name... (: mInt someVarString .. we used to do that 20 years ago when there were no modern IDEs.. now I think it's just a noise. You can check return type to learn about that it's analyser ((: Wdyt? What would be the argument to have Analyser in name?

Copy link
Owner

Choose a reason for hiding this comment

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

Just using New() doesn't explain immediately what it is returning. However NewAnalyzer() means exactly what it is. You're creating a new analzyer. You can also see some examples in the stdlib. For example from the bufio package:

NewReader()
NewBuffer()
NewWriter

Or from the flag package:

NewFlagSet()

There are many examples like this. You could argue that errors has a New() function, but that's a little bit different than these.

There it would be repetitive to say errors.NewError() and it's used in every single return expression as well, such as return nil, errors.New(). So keeping it short is important.

But for faillint, or any other package that provide a certain functionality, I would like to see the name of what it is creating in the New... name.

Copy link
Contributor Author

@bwplotka bwplotka Mar 7, 2020

Choose a reason for hiding this comment

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

Fair, but currently we have only one constructor, so if we would follow YAGNI rule, New is just enough. (:

Let me know if that convinces you. If not, I will disagree but will revert back 😄 also not a big deal (:

Copy link
Owner

Choose a reason for hiding this comment

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

You're hard to convince I have to admit it :)

So yeah I still thing NewAnalyzer() is better than New() for this package. This is used only in one place, in our cmd/main.go file, but I want it to be descriptive. I don't think New() is descriptive enough. Thank you for understanding, I appreciate your co-operation .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, Reverting 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

f := faillint{
paths: "",
ignoretests: false,
}
a := &analysis.Analyzer{
Name: "faillint",
Doc: "report unwanted import path usages",
Doc: "Report unwanted import path, and function usages",
Run: f.run,
RunDespiteErrors: true,
}

a.Flags.StringVar(&f.paths, "paths", "", "import paths to fail")
a.Flags.StringVar(&f.paths, "paths", "", `import paths, functions or methods to fail.
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should not use the name functions here. I didn't tried it yet locally, but the code also encapsulates exported Constants, Types or Variables. These are called declarations . Hence maybe it should be import paths or exported declarations (i.e: functions, constant, types or variables) to fail. 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.

Happy with that. Now is the question if declarations will be actually blocked as well .. will add to the tests 🤗

E.g: foo,github.com/foo/bar,github.com/foo/bar/foo.{A}=github.com/foo/bar/bar.{C},github.com/foo/bar/foo.{D,C}`)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should provide a usable example, such as errors=github.com/pkg/errors,fmt.{Errorf}=github.com/pkg/errors.{Errorf}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

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
fn []string
sugg string
}

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

Choose a reason for hiding this comment

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

Any reason we don't use https://golang.org/pkg/strings/#TrimSpace ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not trim spaces in between, only the outrim spaces.


parsed := make([]path, 0, len(pathGroups))
for _, group := range pathGroups {
p := path{}
for i, name := range pathRegexp.SubexpNames() {
switch name {
case "import":
p.imp = group[i]
case "suggestion":
p.sugg = group[i]
case "functions":
if group[i] == "" {
break
}
p.fn = 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.fn) == 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 functions.
for _, fn := range path.fn {
positions, ok := usages[fn]
if !ok {
continue
}
msg := fmt.Sprintf("function %q from package %q shouldn't be used", fn, 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 usages of 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
Loading