From a83118ba2aec34f9c52ec13b5523794a00d65f76 Mon Sep 17 00:00:00 2001 From: ydah <13041216+ydah@users.noreply.github.com> Date: Thu, 21 Apr 2022 16:27:18 +0900 Subject: [PATCH 1/3] Fixes issue #619 imports-blacklist support regex --- RULES_DESCRIPTIONS.md | 4 ++-- rule/imports-blacklist.go | 26 +++++++++++++++++++------- test/import-blacklist_test.go | 4 ++-- testdata/imports-blacklist.go | 16 ++++++++++++++-- 4 files changed, 37 insertions(+), 13 deletions(-) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index a788e4a69..da765fd91 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -426,13 +426,13 @@ _Configuration_: N/A _Description_: Warns when importing black-listed packages. -_Configuration_: black-list of package names +_Configuration_: black-list of package names (or regular expression package names). Example: ```toml [imports-blacklist] - arguments =["crypto/md5", "crypto/sha1"] + arguments =["crypto/md5", "crypto/sha1", "crypto/**/pkix"] ``` ## import-shadowing diff --git a/rule/imports-blacklist.go b/rule/imports-blacklist.go index 7c98abea9..c3e2b3562 100644 --- a/rule/imports-blacklist.go +++ b/rule/imports-blacklist.go @@ -2,6 +2,7 @@ package rule import ( "fmt" + "regexp" "sync" "github.com/mgechev/revive/lint" @@ -9,30 +10,41 @@ import ( // ImportsBlacklistRule lints given else constructs. type ImportsBlacklistRule struct { - blacklist map[string]bool + blacklist []*regexp.Regexp sync.Mutex } +var replaceRegexp = regexp.MustCompile(`/?\*\*/?`) + func (r *ImportsBlacklistRule) configure(arguments lint.Arguments) { r.Lock() if r.blacklist == nil { - r.blacklist = make(map[string]bool, len(arguments)) + r.blacklist = make([]*regexp.Regexp, 0) for _, arg := range arguments { argStr, ok := arg.(string) if !ok { panic(fmt.Sprintf("Invalid argument to the imports-blacklist rule. Expecting a string, got %T", arg)) } - // we add quotes if not present, because when parsed, the value of the AST node, will be quoted - if len(argStr) > 2 && argStr[0] != '"' && argStr[len(argStr)-1] != '"' { - argStr = fmt.Sprintf(`%q`, argStr) + regStr, err := regexp.Compile(fmt.Sprintf(`(?m)"%s"$`, replaceRegexp.ReplaceAllString(argStr, `(\W|\w)*`))) + if err != nil { + panic(fmt.Sprintf("Invalid argument to the imports-blacklist rule. Expecting a regular expression is parsable %T", argStr)) } - r.blacklist[argStr] = true + r.blacklist = append(r.blacklist, regStr) } } r.Unlock() } +func (r *ImportsBlacklistRule) matchBlacklist(path string) bool { + for _, regex := range r.blacklist { + if regex.MatchString(path) { + return true + } + } + return false +} + // Apply applies the rule to given file. func (r *ImportsBlacklistRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { r.configure(arguments) @@ -45,7 +57,7 @@ func (r *ImportsBlacklistRule) Apply(file *lint.File, arguments lint.Arguments) for _, is := range file.AST.Imports { path := is.Path - if path != nil && r.blacklist[path.Value] { + if path != nil && r.matchBlacklist(path.Value) { failures = append(failures, lint.Failure{ Confidence: 1, Failure: "should not use the following blacklisted import: " + path.Value, diff --git a/test/import-blacklist_test.go b/test/import-blacklist_test.go index d427f0b7b..9a7798790 100644 --- a/test/import-blacklist_test.go +++ b/test/import-blacklist_test.go @@ -8,7 +8,7 @@ import ( ) func TestImportsBlacklist(t *testing.T) { - args := []interface{}{"crypto/md5", "crypto/sha1"} + args := []interface{}{"github.com/full/match", "wildcard/**/between", "wildcard/backward/**", "**/wildcard/forward", "full"} testRule(t, "imports-blacklist", &rule.ImportsBlacklistRule{}, &lint.RuleConfig{ Arguments: args, @@ -16,7 +16,7 @@ func TestImportsBlacklist(t *testing.T) { } func BenchmarkImportsBlacklist(b *testing.B) { - args := []interface{}{"crypto/md5", "crypto/sha1"} + args := []interface{}{"github.com/full/match", "wildcard/**/between", "wildcard/backward/**", "**/wildcard/forward", "full"} var t *testing.T for i := 0; i <= b.N; i++ { testRule(t, "imports-blacklist", &rule.ImportsBlacklistRule{}, &lint.RuleConfig{ diff --git a/testdata/imports-blacklist.go b/testdata/imports-blacklist.go index cdd5ef4f5..aca95fc71 100644 --- a/testdata/imports-blacklist.go +++ b/testdata/imports-blacklist.go @@ -1,7 +1,19 @@ package fixtures import ( - "crypto/md5" // MATCH /should not use the following blacklisted import: "crypto/md5"/ - "crypto/sha1" // MATCH /should not use the following blacklisted import: "crypto/sha1"/ + "github.com/full/match" // MATCH /should not use the following blacklisted import: "github.com/full/match"/ + "bithub.com/full/match" + "github.com/full/matche" + "wildcard/between" // MATCH /should not use the following blacklisted import: "wildcard/between"/ + "wildcard/pkg1/between" // MATCH /should not use the following blacklisted import: "wildcard/pkg1/between"/ + "wildcard/pkg1/pkg2/between" // MATCH /should not use the following blacklisted import: "wildcard/pkg1/pkg2/between"/ + "wildcard/backward" // MATCH /should not use the following blacklisted import: "wildcard/backward"/ + "wildcard/backward/pkg" // MATCH /should not use the following blacklisted import: "wildcard/backward/pkg"/ + "wildcard/backward/pkg/pkg1" // MATCH /should not use the following blacklisted import: "wildcard/backward/pkg/pkg1"/ + "wildcard/forward" // MATCH /should not use the following blacklisted import: "wildcard/forward"/ + "pkg/wildcard/forward" // MATCH /should not use the following blacklisted import: "pkg/wildcard/forward"/ + "pkg/pkg1/wildcard/forward" // MATCH /should not use the following blacklisted import: "pkg/pkg1/wildcard/forward"/ + "full" // MATCH /should not use the following blacklisted import: "full"/ + "github.com/partical/match/fully" "strings" ) From db25072044dd190f1e32a9284eb74538696004e0 Mon Sep 17 00:00:00 2001 From: chavacava Date: Thu, 21 Apr 2022 13:33:41 +0000 Subject: [PATCH 2/3] refactors method name and error message --- rule/imports-blacklist.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/rule/imports-blacklist.go b/rule/imports-blacklist.go index c3e2b3562..710662815 100644 --- a/rule/imports-blacklist.go +++ b/rule/imports-blacklist.go @@ -18,6 +18,8 @@ var replaceRegexp = regexp.MustCompile(`/?\*\*/?`) func (r *ImportsBlacklistRule) configure(arguments lint.Arguments) { r.Lock() + defer r.Unlock() + if r.blacklist == nil { r.blacklist = make([]*regexp.Regexp, 0) @@ -28,15 +30,14 @@ func (r *ImportsBlacklistRule) configure(arguments lint.Arguments) { } regStr, err := regexp.Compile(fmt.Sprintf(`(?m)"%s"$`, replaceRegexp.ReplaceAllString(argStr, `(\W|\w)*`))) if err != nil { - panic(fmt.Sprintf("Invalid argument to the imports-blacklist rule. Expecting a regular expression is parsable %T", argStr)) + panic(fmt.Sprintf("Invalid argument to the imports-blacklist rule. Expecting %q to be a valid regular expression, got: %v", argStr, err)) } r.blacklist = append(r.blacklist, regStr) } } - r.Unlock() } -func (r *ImportsBlacklistRule) matchBlacklist(path string) bool { +func (r *ImportsBlacklistRule) isBlacklisted(path string) bool { for _, regex := range r.blacklist { if regex.MatchString(path) { return true @@ -57,7 +58,7 @@ func (r *ImportsBlacklistRule) Apply(file *lint.File, arguments lint.Arguments) for _, is := range file.AST.Imports { path := is.Path - if path != nil && r.matchBlacklist(path.Value) { + if path != nil && r.isBlacklisted(path.Value) { failures = append(failures, lint.Failure{ Confidence: 1, Failure: "should not use the following blacklisted import: " + path.Value, From ef004b3b023b765c96f1e3d7538f30cebec519b2 Mon Sep 17 00:00:00 2001 From: chavacava Date: Thu, 21 Apr 2022 13:43:20 +0000 Subject: [PATCH 3/3] restores original test cases --- test/import-blacklist_test.go | 8 ++++++++ testdata/imports-blacklist-original.go | 8 ++++++++ 2 files changed, 16 insertions(+) create mode 100644 testdata/imports-blacklist-original.go diff --git a/test/import-blacklist_test.go b/test/import-blacklist_test.go index 9a7798790..21aeddd86 100644 --- a/test/import-blacklist_test.go +++ b/test/import-blacklist_test.go @@ -7,6 +7,14 @@ import ( "github.com/mgechev/revive/rule" ) +func TestImportsBlacklistOriginal(t *testing.T) { + args := []interface{}{"crypto/md5", "crypto/sha1"} + + testRule(t, "imports-blacklist-original", &rule.ImportsBlacklistRule{}, &lint.RuleConfig{ + Arguments: args, + }) +} + func TestImportsBlacklist(t *testing.T) { args := []interface{}{"github.com/full/match", "wildcard/**/between", "wildcard/backward/**", "**/wildcard/forward", "full"} diff --git a/testdata/imports-blacklist-original.go b/testdata/imports-blacklist-original.go new file mode 100644 index 000000000..e8d6f7d9d --- /dev/null +++ b/testdata/imports-blacklist-original.go @@ -0,0 +1,8 @@ +package fixtures + +import ( + "crypto/md5" // MATCH /should not use the following blacklisted import: "crypto/md5"/ + "crypto/sha1" // MATCH /should not use the following blacklisted import: "crypto/sha1"/ + "strings" +) +