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

Support lint for sub-package usage #2

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 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
62 changes: 42 additions & 20 deletions faillint/faillint.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,30 @@ func run(pass *analysis.Pass) (interface{}, error) {

for _, file := range pass.Files {
for _, path := range imports {
imp := usesImport(file, path)
imp, exact := usesImport(file, path)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure about the "exact" word here. What do you think about "fullMatch" or "exactMatch" ?   Or you reverse it and return the "subMatch" (or "subPath"). Then below you could say if subMatch { ... } which I think is easier to read and reason than if !exact { ... }

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the comment, I agree!

if imp == nil {
continue
}

impPath := importPath(imp)
if !exact {
importPath, err := strconv.Unquote(imp.Path.Value)
if err != nil {
continue
}
// If user has explicitly specified this path, skip this non-exact match,
// and leave to be handled in the exact path.
if _, ok := suggestions[importPath]; ok {
continue
}
}

msg := fmt.Sprintf("package %q shouldn't be imported", impPath)
if s := suggestions[impPath]; s != "" {
var msg string
if exact {
msg = fmt.Sprintf("package %q shouldn't be imported", path)
} else {
msg = fmt.Sprintf("sub-package of %q shouldn't be imported", path)
}
if s := suggestions[path]; s != "" {
msg += fmt.Sprintf(", suggested: %q", s)
}

Expand All @@ -71,27 +86,31 @@ func run(pass *analysis.Pass) (interface{}, error) {
return nil, nil
}

// usesImport reports whether a given import is used.
func usesImport(f *ast.File, path string) *ast.ImportSpec {
spec := importSpec(f, path)
// usesImport reports whether a given import package(exact) or its subpackage is used.
func usesImport(f *ast.File, path string) (*ast.ImportSpec, bool) {
spec, exact := importSpec(f, path)
if spec == nil {
return nil
return nil, false
}

name := spec.Name.String()
switch name {
case "<nil>":
// If the package name is not explicitly specified,
// make an educated guess. This is not guaranteed to be correct.
lastSlash := strings.LastIndex(path, "/")
// get the last component from import path.
impPath, err := strconv.Unquote(spec.Path.Value)
if err != nil {
return nil, false
}
lastSlash := strings.LastIndex(impPath, "/")
if lastSlash == -1 {
name = path
name = impPath
} else {
name = path[lastSlash+1:]
name = impPath[lastSlash+1:]
}
case "_", ".":
// Not sure if this import is used - err on the side of caution.
return spec
return spec, exact
}

var used bool
Expand All @@ -104,21 +123,24 @@ func usesImport(f *ast.File, path string) *ast.ImportSpec {
})

if used {
return spec
return spec, exact
}

return nil
return nil, false
}

// importSpec returns the import spec if f imports path,
// importSpec returns the import spec if f imports path (exact) or its subpackage,
// or nil otherwise.
func importSpec(f *ast.File, path string) *ast.ImportSpec {
func importSpec(f *ast.File, path string) (node *ast.ImportSpec, exact bool) {
for _, s := range f.Imports {
if importPath(s) == path {
return s
impPath := importPath(s)
if impPath != "" {
if strings.HasPrefix(impPath, path) {
return s, impPath == path
}
}
}
return nil
return nil, false
}

// importPath returns the unquoted import path of s,
Expand Down
8 changes: 8 additions & 0 deletions faillint/faillint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ func Test(t *testing.T) {
name: "e",
paths: "errors=github.com/pkg/errors,golang.org/x/net/context=context",
},
{
name: "f",
paths: "golang.org/x/net",
},
{
name: "g",
paths: "golang.org/x/net=net,golang.org/x/net/context=context",
},
}
for _, ts := range tests {
ts := ts
Expand Down
8 changes: 8 additions & 0 deletions faillint/testdata/src/f/f.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package e

import (
"golang.org/x/net/context" // want `sub-package of "golang.org/x/net" shouldn't be imported`
)

func foo(ctx context.Context) {
}
8 changes: 8 additions & 0 deletions faillint/testdata/src/g/g.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package e

import (
"golang.org/x/net/context" // want `package "golang.org/x/net/context" shouldn't be imported, suggested: "context"`
)

func foo(ctx context.Context) {
}