From fbefad85581a9d5d54133914cc4e59a3f51d4b49 Mon Sep 17 00:00:00 2001 From: SalvadorC Date: Wed, 20 Mar 2019 19:54:03 +0100 Subject: [PATCH 1/2] New rule: duplicated-imports (#111) * 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 * adds duplicated-imports rule * adds duplicated-imports rule --- README.md | 1 + RULES_DESCRIPTIONS.md | 7 ++++++ config.go | 1 + fixtures/duplicated-imports.go | 8 +++++++ rule/duplicated-imports.go | 39 ++++++++++++++++++++++++++++++++++ test/duplicated-import_test.go | 11 ++++++++++ 6 files changed, 67 insertions(+) create mode 100644 fixtures/duplicated-imports.go create mode 100644 rule/duplicated-imports.go create mode 100644 test/duplicated-import_test.go diff --git a/README.md b/README.md index 65d5e28..2e4b65e 100644 --- a/README.md +++ b/README.md @@ -292,6 +292,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`empty-lines`](./RULES_DESCRIPTIONS.md#empty-lines) | n/a | Warns when there are heading or trailing newlines in a block | no | no | | [`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 | ## Configurable rules diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index db093ca..46842c7 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -18,6 +18,7 @@ List of all available rules. - [cyclomatic](#cyclomatic) - [deep-exit](#deep-exit) - [dot-imports](#dot-imports) + - [duplicated-imports](#duplicated-imports) - [empty-block](#empty-block) - [empty-lines](#empty-lines) - [error-naming](#error-naming) @@ -168,6 +169,12 @@ More information [here](https://github.com/golang/go/wiki/CodeReviewComments#imp _Configuration_: N/A +## duplicated-imports + +_Description_: It is possible to unintentionally import the same package twice. This rule looks for packages that are imported two or more times. + +_Configuration_: N/A + ## empty-block _Description_: Empty blocks make code less readable and could be a symptom of a bug or unfinished refactoring. diff --git a/config.go b/config.go index 6f4d4d2..c77a372 100644 --- a/config.go +++ b/config.go @@ -73,6 +73,7 @@ var allRules = append([]lint.Rule{ &rule.EmptyLinesRule{}, &rule.LineLengthLimitRule{}, &rule.CallToGCRule{}, + &rule.DuplicatedImportsRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/duplicated-imports.go b/fixtures/duplicated-imports.go new file mode 100644 index 0000000..07211c7 --- /dev/null +++ b/fixtures/duplicated-imports.go @@ -0,0 +1,8 @@ +package fixtures + +import( + "crypto/md5" + "strings" + _ "crypto/md5" // MATCH /Package "crypto/md5" already imported/ + str "strings" // MATCH /Package "strings" already imported/ +) diff --git a/rule/duplicated-imports.go b/rule/duplicated-imports.go new file mode 100644 index 0000000..485b6a2 --- /dev/null +++ b/rule/duplicated-imports.go @@ -0,0 +1,39 @@ +package rule + +import ( + "fmt" + + "github.com/mgechev/revive/lint" +) + +// DuplicatedImportsRule lints given else constructs. +type DuplicatedImportsRule struct{} + +// Apply applies the rule to given file. +func (r *DuplicatedImportsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + impPaths := map[string]struct{}{} + for _, imp := range file.AST.Imports { + path := imp.Path.Value + _, ok := impPaths[path] + if ok { + failures = append(failures, lint.Failure{ + Confidence: 1, + Failure: fmt.Sprintf("Package %s already imported", path), + Node: imp, + Category: "imports", + }) + continue + } + + impPaths[path] = struct{}{} + } + + return failures +} + +// Name returns the rule name. +func (r *DuplicatedImportsRule) Name() string { + return "duplicated-imports" +} diff --git a/test/duplicated-import_test.go b/test/duplicated-import_test.go new file mode 100644 index 0000000..4cdb897 --- /dev/null +++ b/test/duplicated-import_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestDuplicatedImports(t *testing.T) { + testRule(t, "duplicated-imports", &rule.DuplicatedImportsRule{}) +} From 44eed7edb77b52d80d3cc3a828caea8a424b5caf Mon Sep 17 00:00:00 2001 From: Dian Date: Thu, 21 Mar 2019 02:54:27 +0800 Subject: [PATCH 2/2] fix error return rule to allow multiple error return values (#110) * fix error return rule to allow multiple error return values please check golint related updates here: golang/lint@8379672 When returning multiple values with an error, lint checks if the error is the last return value. But the implementation actually is checking for all return values except for the last one, and throw the alert if it found an error. There is a (edge) case where some function returning more than one error is getting a false positive even when the last return value is an error. This patch adds an early check, to see if the last return value is an error and if so, it will pass silently. * Fix return error * add test changes --- fixtures/golint/error-return.go | 10 ++++++++++ rule/error-return.go | 3 +++ 2 files changed, 13 insertions(+) diff --git a/fixtures/golint/error-return.go b/fixtures/golint/error-return.go index b6a0eda..783ab74 100644 --- a/fixtures/golint/error-return.go +++ b/fixtures/golint/error-return.go @@ -41,3 +41,13 @@ func l() (int, error, int) { // MATCH /error should be the last type when return func m() (x int, err error, y int) { // MATCH /error should be the last type when returning multiple items/ return 0, nil, 0 } + +// Check for multiple error returns but with errors at the end. +func n() (int, error, error) { // ok + return 0, nil, nil +} + +// Check for multiple error returns mixed in order, but keeping one error at last position. +func o() (int, error, int, error) { // ok + return 0, nil, 0, nil +} diff --git a/rule/error-return.go b/rule/error-return.go index df847b3..737d8c6 100644 --- a/rule/error-return.go +++ b/rule/error-return.go @@ -47,6 +47,9 @@ func (w lintErrorReturn) Visit(n ast.Node) ast.Visitor { if len(ret) <= 1 { return w } + if isIdent(ret[len(ret)-1].Type, "error") { + return nil + } // An error return parameter should be the last parameter. // Flag any error parameters found before the last. for _, r := range ret[:len(ret)-1] {