From 2ce440cf9612fc32888104546e2c92ac6737992e Mon Sep 17 00:00:00 2001 From: SalvadorC Date: Wed, 27 Mar 2019 19:46:20 +0100 Subject: [PATCH] new rule: import shadowing (#114) * Adds rule superfluous-else (an extension of indent-error-flow) * Fix superfluous-else rule struct namming. * Adds superfuous-else rule to the rules table * Adds confusing-naming rule * adds multifile test * clean-up * fix config.go * clean master * new ADS rule: newerr * ADS-print working version * ads-print final version * ads-lost-err working version * fix ads-print * removes ads rules from master * new rule: import-shadowing * removes ads rules from master * new rule: import-shadowing * fix defaults * adds explanations on the rule implementation --- README.md | 1 + RULES_DESCRIPTIONS.md | 8 +++ config.go | 1 + fixtures/import-shadowing.go | 36 ++++++++++++ rule/import-shadowing.go | 102 ++++++++++++++++++++++++++++++++++ test/import-shadowing_test.go | 11 ++++ 6 files changed, 159 insertions(+) create mode 100644 fixtures/import-shadowing.go create mode 100644 rule/import-shadowing.go create mode 100644 test/import-shadowing_test.go diff --git a/README.md b/README.md index 02b8fd7..319e41b 100644 --- a/README.md +++ b/README.md @@ -293,6 +293,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`line-length-limit`](./RULES_DESCRIPTIONS.md#line-length-limit) | int | Specifies the maximum number of characters in a line | no | no | | [`call-to-gc`](./RULES_DESCRIPTIONS.md#call-to-gc) | n/a | Warns on explicit call to the garbage collector | no | no | | [`duplicated-imports`](./RULES_DESCRIPTIONS#duplicated-imports) | n/a | Looks for packages that are imported two or more times | no | no | +| [`import-shadowing`](./RULES_DESCRIPTIONS.md#import-shadowing) | n/a | Spots identifiers that shadow an import | no | no | ## Configurable rules diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 46842c7..0e9a586 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -34,6 +34,7 @@ List of all available rules. - [increment-decrement](#increment-decrement) - [indent-error-flow](#indent-error-flow) - [imports-blacklist](#imports-blacklist) + - [import-shadowing](#import-shadowing) - [line-length-limit](#line-length-limit) - [max-public-structs](#max-public-structs) - [modifies-parameter](#modifies-parameter) @@ -295,6 +296,13 @@ Example: [imports-blacklist] arguments =["crypto/md5", "crypto/sha1"] ``` +### import-shadowing + +_Description_: In GO it is possible to declare identifiers (packages, structs, +interfaces, parameters, receivers, variables, constants...) that conflict with the +name of an imported package. This rule spots identifiers that shadow an import. + +_Configuration_: N/A ## line-length-limit diff --git a/config.go b/config.go index c77a372..b1f5855 100644 --- a/config.go +++ b/config.go @@ -74,6 +74,7 @@ var allRules = append([]lint.Rule{ &rule.LineLengthLimitRule{}, &rule.CallToGCRule{}, &rule.DuplicatedImportsRule{}, + &rule.ImportShadowingRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/import-shadowing.go b/fixtures/import-shadowing.go new file mode 100644 index 0000000..06f91f6 --- /dev/null +++ b/fixtures/import-shadowing.go @@ -0,0 +1,36 @@ +package fixtures + +import ( + ast "go/ast" + "bytes" + "crypto/md5" + "fmt" + _ "net/http" + "strings" + str "strings" +) + +const str = "" // MATCH /The name 'str' shadows an import name/ + +type myAst struct { + ast *ast.GenDecl +} + +type bytes struct {} // MATCH /The name 'bytes' shadows an import name/ + +type fmt interface {} // MATCH /The name 'fmt' shadows an import name/ + +func (ast myAst) foo() {} // MATCH /The name 'ast' shadows an import name/ + +func md5() {} // MATCH /The name 'md5' shadows an import name/ + +func bar(_ string) {} + +func toto() { + strings := map[string]string{} // MATCH /The name 'strings' shadows an import name/ +} + +func titi() { + v := md5+bytes + return ast +} diff --git a/rule/import-shadowing.go b/rule/import-shadowing.go new file mode 100644 index 0000000..b78234c --- /dev/null +++ b/rule/import-shadowing.go @@ -0,0 +1,102 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/token" + "strings" + + "github.com/mgechev/revive/lint" +) + +// ImportShadowingRule lints given else constructs. +type ImportShadowingRule struct{} + +// Apply applies the rule to given file. +func (r *ImportShadowingRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + importNames := map[string]struct{}{} + for _, imp := range file.AST.Imports { + importNames[getName(imp)] = struct{}{} + } + + fileAst := file.AST + walker := importShadowing{ + importNames: importNames, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + alreadySeen: map[*ast.Object]struct{}{}, + } + + ast.Walk(walker, fileAst) + + return failures +} + +// Name returns the rule name. +func (r *ImportShadowingRule) Name() string { + return "import-shadowing" +} + +func getName(imp *ast.ImportSpec) string { + const pathSep = "/" + const strDelim = `"` + if imp.Name != nil { + return imp.Name.Name + } + + path := imp.Path.Value + i := strings.LastIndex(path, pathSep) + if i == -1 { + return strings.Trim(path, strDelim) + } + + return strings.Trim(path[i+1:], strDelim) +} + +type importShadowing struct { + importNames map[string]struct{} + onFailure func(lint.Failure) + alreadySeen map[*ast.Object]struct{} +} + +// Visit visits AST nodes and checks if id nodes (ast.Ident) shadow an import name +func (w importShadowing) Visit(n ast.Node) ast.Visitor { + switch n := n.(type) { + case *ast.AssignStmt: + if n.Tok == token.DEFINE { + return w // analyze variable declarations of the form id := expr + } + + return nil // skip assigns of the form id = expr (not an id declaration) + case *ast.CallExpr, // skip call expressions (not an id declaration) + *ast.ImportSpec, // skip import section subtree because we already have the list of imports + *ast.KeyValueExpr, // skip analysis of key-val expressions ({key:value}): ids of such expressions, even the same of an import name, do not shadow the import name + *ast.ReturnStmt, // skip skipping analysis of returns, ids in expression were already analyzed + *ast.SelectorExpr, // skip analysis of selector expressions (anId.otherId): because if anId shadows an import name, it was already detected, and otherId does not shadows the import name + *ast.StructType: // skip analysis of struct type because struct fields can not shadow an import name + return nil + case *ast.Ident: + id := n.Name + if id == "_" { + return w // skip _ id + } + + _, isImportName := w.importNames[id] + _, alreadySeen := w.alreadySeen[n.Obj] + if isImportName && !alreadySeen { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: n, + Category: "namming", + Failure: fmt.Sprintf("The name '%s' shadows an import name", id), + }) + + w.alreadySeen[n.Obj] = struct{}{} + } + } + + return w +} diff --git a/test/import-shadowing_test.go b/test/import-shadowing_test.go new file mode 100644 index 0000000..12e64f2 --- /dev/null +++ b/test/import-shadowing_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestImportShadowing(t *testing.T) { + testRule(t, "import-shadowing", &rule.ImportShadowingRule{}) +}