Repository: mgechev/revive
Pull request #946 (2023-11-28 23:30:00)

refactor: rename blacklist to blocklist and whitelist to allowlist
related to #690 

I thougt that rename Whitelist to Allowlist and Blacklist to Blocklist (all variable of course respect lower case and upper case)

I dont have any idea how to implement deprecation for old `&rule.ImportsBlacklistRule{}`, one idea is that to leave empty ImportsBlacklistRule and throw info about deprecation what you think @chavacava 

ps. i checked other repos in github with `&rule.ImportsBlacklistRule{}` and there are 75 repos matching, so we dont have any simple solution :(



chavacava: Hi @mfederowicz thanks for the PR.
I propose to do these changes in two steps:
1.  Remove white and black list names from the internal code
2. Remove white and black list names from the external interfaces (config)

Use this PR for the step 1 (the easy one) and discuss how to go on with step 2
mfederowicz: ok @chavacava i cleaned up code from my changes related with deprecatedRules list, now there are only changes related with removing white and black lists (of course ImportsBlacklistRule was reverted to old state from master)

now iam listening sugestions/ideas how to  remove black list from the external interfaces (config), white list luckly was used only in lists internaly (not in config or dedicated rules) 
chavacava: Side note: changing the failure messages could be also considered as a breaking change because users might rely on failure messages thus changing them will force users to update their code.
Do not worry, I'm okay with the new messages you pushed, my note is just to highlight that managing "breaking changes" is a big deal :) 
denisvmedia: > my note is just to highlight that managing "breaking changes" is a big deal :)

We'll have to put some release notes on that, I think.
mfederowicz: @chavacava what is the status, any new sugestions?
chavacava: > @chavacava what is the status, any new sugestions?

As I commented before, the PR (still) has renamed rules (the `imports-blacklist` rule renamed as `imports-blocklist`) 
Changing the rule's name is a breaking change and my understanding was we agreed on not introducing breaking changes in this first phase of refactoring. 
mfederowicz: > > @chavacava what is the status, any new sugestions?
> 
> As I commented before, the PR (still) has renamed rules (the `imports-blacklist` rule renamed as `imports-blocklist`) Changing the rule's name is a breaking change and my understanding was we agreed on not introducing breaking changes in this first phase of refactoring.

yes but imports-blocklist is new file not rename old one. I can revert testdata for test/import-blacklist_test.go and testdata/imports-blacklist-original.go but this dont have affect to old code :)
chavacava: > yes but imports-blocklist is new file not rename old one. I can revert testdata for test/import-blacklist_test.go and testdata/imports-blacklist-original.go but this dont have affect to old code :)

Does that mean we will have two rules for the very same check?
mfederowicz: ok @chavacava i mean that I created copy of blacklist rule. Now I reverted tests/testdata for blacklist, and add separated test/testdata for blocklist. In near future we should remove Blacklist rule and add some notes about breaking change. If you (user) use development version of package you must know that changes are part of your live :P

Patch:
diff --git a/config/config.go b/config/config.go
index c4ba0e7..f37f6ef 100644
--- a/config/config.go
+++ b/config/config.go
@@ -56,6 +56,7 @@ var allRules = append([]lint.Rule{
 	&rule.ConstantLogicalExprRule{},
 	&rule.BoolLiteralRule{},
 	&rule.ImportsBlacklistRule{},
+	&rule.ImportsBlocklistRule{},
 	&rule.FunctionResultsLimitRule{},
 	&rule.MaxPublicStructsRule{},
 	&rule.RangeValInClosureRule{},
diff --git a/lint/utils.go b/lint/utils.go
index 28657c6..1efa41f 100644
--- a/lint/utils.go
+++ b/lint/utils.go
@@ -6,7 +6,7 @@ import (
 )
 
 // Name returns a different name if it should be different.
-func Name(name string, whitelist, blacklist []string) (should string) {
+func Name(name string, allowlist, blocklist []string) (should string) {
 	// Fast path for simple cases: "_" and all lowercase.
 	if name == "_" {
 		return name
@@ -57,12 +57,12 @@ func Name(name string, whitelist, blacklist []string) (should string) {
 		// [w,i) is a word.
 		word := string(runes[w:i])
 		ignoreInitWarnings := map[string]bool{}
-		for _, i := range whitelist {
+		for _, i := range allowlist {
 			ignoreInitWarnings[i] = true
 		}
 
 		extraInits := map[string]bool{}
-		for _, i := range blacklist {
+		for _, i := range blocklist {
 			extraInits[i] = true
 		}
 
diff --git a/rule/add-constant.go b/rule/add-constant.go
index f39f224..8618262 100644
--- a/rule/add-constant.go
+++ b/rule/add-constant.go
@@ -18,13 +18,13 @@ const (
 	kindSTRING         = "STRING"
 )
 
-type whiteList map[string]map[string]bool
+type allowList map[string]map[string]bool
 
-func newWhiteList() whiteList {
+func newAllowList() allowList {
 	return map[string]map[string]bool{kindINT: {}, kindFLOAT: {}, kindSTRING: {}}
 }
 
-func (wl whiteList) add(kind, list string) {
+func (wl allowList) add(kind, list string) {
 	elems := strings.Split(list, ",")
 	for _, e := range elems {
 		wl[kind][e] = true
@@ -33,7 +33,7 @@ func (wl whiteList) add(kind, list string) {
 
 // AddConstantRule lints unused params in functions.
 type AddConstantRule struct {
-	whiteList       whiteList
+	allowList       allowList
 	ignoreFunctions []*regexp.Regexp
 	strLitLimit     int
 	sync.Mutex
@@ -53,7 +53,7 @@ func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) []lin
 		onFailure:       onFailure,
 		strLits:         make(map[string]int),
 		strLitLimit:     r.strLitLimit,
-		whiteLst:        r.whiteList,
+		allowList:        r.allowList,
 		ignoreFunctions: r.ignoreFunctions,
 		structTags:      make(map[*ast.BasicLit]struct{}),
 	}
@@ -72,7 +72,7 @@ type lintAddConstantRule struct {
 	onFailure       func(lint.Failure)
 	strLits         map[string]int
 	strLitLimit     int
-	whiteLst        whiteList
+	allowList        allowList
 	ignoreFunctions []*regexp.Regexp
 	structTags      map[*ast.BasicLit]struct{}
 }
@@ -155,7 +155,7 @@ func (w *lintAddConstantRule) isIgnoredFunc(fName string) bool {
 }
 
 func (w *lintAddConstantRule) checkStrLit(n *ast.BasicLit) {
-	if w.whiteLst[kindSTRING][n.Value] {
+	if w.allowList[kindSTRING][n.Value] {
 		return
 	}
 
@@ -175,7 +175,7 @@ func (w *lintAddConstantRule) checkStrLit(n *ast.BasicLit) {
 }
 
 func (w *lintAddConstantRule) checkNumLit(kind string, n *ast.BasicLit) {
-	if w.whiteLst[kind][n.Value] {
+	if w.allowList[kind][n.Value] {
 		return
 	}
 
@@ -196,9 +196,9 @@ func (r *AddConstantRule) configure(arguments lint.Arguments) {
 	r.Lock()
 	defer r.Unlock()
 
-	if r.whiteList == nil {
+	if r.allowList == nil {
 		r.strLitLimit = defaultStrLitLimit
-		r.whiteList = newWhiteList()
+		r.allowList = newAllowList()
 		if len(arguments) > 0 {
 			args, ok := arguments[0].(map[string]any)
 			if !ok {
@@ -223,7 +223,7 @@ func (r *AddConstantRule) configure(arguments lint.Arguments) {
 					if !ok {
 						panic(fmt.Sprintf("Invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v))
 					}
-					r.whiteList.add(kind, list)
+					r.allowList.add(kind, list)
 				case "maxLitCount":
 					sl, ok := v.(string)
 					if !ok {
diff --git a/rule/confusing-naming.go b/rule/confusing-naming.go
index daaf51c..febfd88 100644
--- a/rule/confusing-naming.go
+++ b/rule/confusing-naming.go
@@ -111,7 +111,7 @@ func checkMethodName(holder string, id *ast.Ident, w *lintConfusingNames) {
 		pkgm.methods[holder] = make(map[string]*referenceMethod, 1)
 	}
 
-	// update the black list
+	// update the block list
 	if pkgm.methods[holder] == nil {
 		println("no entry for '", holder, "'")
 	}
diff --git a/rule/imports-blocklist.go b/rule/imports-blocklist.go
new file mode 100644
index 0000000..4310664
--- /dev/null
+++ b/rule/imports-blocklist.go
@@ -0,0 +1,73 @@
+package rule
+
+import (
+	"fmt"
+	"regexp"
+	"sync"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// ImportsBlocklistRule lints given else constructs.
+type ImportsBlocklistRule struct {
+	blocklist []*regexp.Regexp
+	sync.Mutex
+}
+
+var replaceImportRegexp = regexp.MustCompile(`/?\*\*/?`)
+
+func (r *ImportsBlocklistRule) configure(arguments lint.Arguments) {
+	r.Lock()
+	defer r.Unlock()
+
+	if r.blocklist == nil {
+		r.blocklist = make([]*regexp.Regexp, 0)
+
+		for _, arg := range arguments {
+			argStr, ok := arg.(string)
+			if !ok {
+				panic(fmt.Sprintf("Invalid argument to the imports-blocklist rule. Expecting a string, got %T", arg))
+			}
+			regStr, err := regexp.Compile(fmt.Sprintf(`(?m)"%s"$`, replaceImportRegexp.ReplaceAllString(argStr, `(\W|\w)*`)))
+			if err != nil {
+				panic(fmt.Sprintf("Invalid argument to the imports-blocklist rule. Expecting %q to be a valid regular expression, got: %v", argStr, err))
+			}
+			r.blocklist = append(r.blocklist, regStr)
+		}
+	}
+}
+
+func (r *ImportsBlocklistRule) isBlocklisted(path string) bool {
+	for _, regex := range r.blocklist {
+		if regex.MatchString(path) {
+			return true
+		}
+	}
+	return false
+}
+
+// Apply applies the rule to given file.
+func (r *ImportsBlocklistRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	r.configure(arguments)
+
+	var failures []lint.Failure
+
+	for _, is := range file.AST.Imports {
+		path := is.Path
+		if path != nil && r.isBlocklisted(path.Value) {
+			failures = append(failures, lint.Failure{
+				Confidence: 1,
+				Failure:    "should not use the following blocklisted import: " + path.Value,
+				Node:       is,
+				Category:   "imports",
+			})
+		}
+	}
+
+	return failures
+}
+
+// Name returns the rule name.
+func (*ImportsBlocklistRule) Name() string {
+	return "imports-blocklist"
+}
diff --git a/rule/var-naming.go b/rule/var-naming.go
index 8dfba5b..c818e50 100644
--- a/rule/var-naming.go
+++ b/rule/var-naming.go
@@ -19,8 +19,8 @@ var upperCaseConstRE = regexp.MustCompile(`^_?[A-Z][A-Z\d]*(_[A-Z\d]+)*$`)
 // VarNamingRule lints given else constructs.
 type VarNamingRule struct {
 	configured            bool
-	whitelist             []string
-	blacklist             []string
+	allowlist             []string
+	blocklist             []string
 	upperCaseConst        bool // if true - allows to use UPPER_SOME_NAMES for constants
 	skipPackageNameChecks bool
 	sync.Mutex
@@ -35,11 +35,11 @@ func (r *VarNamingRule) configure(arguments lint.Arguments) {
 
 	r.configured = true
 	if len(arguments) >= 1 {
-		r.whitelist = getList(arguments[0], "whitelist")
+		r.allowlist = getList(arguments[0], "whitelist")
 	}
 
 	if len(arguments) >= 2 {
-		r.blacklist = getList(arguments[1], "blacklist")
+		r.blocklist = getList(arguments[1], "blacklist")
 	}
 
 	if len(arguments) >= 3 {
@@ -93,8 +93,8 @@ func (r *VarNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint.
 	walker := lintNames{
 		file:      file,
 		fileAst:   fileAst,
-		whitelist: r.whitelist,
-		blacklist: r.blacklist,
+		allowlist: r.allowlist,
+		blocklist: r.blocklist,
 		onFailure: func(failure lint.Failure) {
 			failures = append(failures, failure)
 		},
@@ -151,7 +151,7 @@ func (w *lintNames) check(id *ast.Ident, thing string) {
 		return
 	}
 
-	should := lint.Name(id.Name, w.whitelist, w.blacklist)
+	should := lint.Name(id.Name, w.allowlist, w.blocklist)
 	if id.Name == should {
 		return
 	}
@@ -177,8 +177,8 @@ type lintNames struct {
 	file           *lint.File
 	fileAst        *ast.File
 	onFailure      func(lint.Failure)
-	whitelist      []string
-	blacklist      []string
+	allowlist      []string
+	blocklist      []string
 	upperCaseConst bool
 }
 


Test patch:
diff --git a/test/import-blocklist_test.go b/test/import-blocklist_test.go
new file mode 100644
index 0000000..bbd8ba4
--- /dev/null
+++ b/test/import-blocklist_test.go
@@ -0,0 +1,34 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+func TestImportsBlocklistOriginal(t *testing.T) {
+	args := []any{"crypto/md5", "crypto/sha1"}
+
+	testRule(t, "imports-blocklist-original", &rule.ImportsBlocklistRule{}, &lint.RuleConfig{
+		Arguments: args,
+	})
+}
+
+func TestImportsBlocklist(t *testing.T) {
+	args := []any{"github.com/full/match", "wildcard/**/between", "wildcard/backward/**", "**/wildcard/forward", "full"}
+
+	testRule(t, "imports-blocklist", &rule.ImportsBlocklistRule{}, &lint.RuleConfig{
+		Arguments: args,
+	})
+}
+
+func BenchmarkImportsBlocklist(b *testing.B) {
+	args := []any{"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-blocklist", &rule.ImportsBlocklistRule{}, &lint.RuleConfig{
+			Arguments: args,
+		})
+	}
+}
diff --git a/testdata/imports-blocklist-original.go b/testdata/imports-blocklist-original.go
new file mode 100644
index 0000000..d5244e8
--- /dev/null
+++ b/testdata/imports-blocklist-original.go
@@ -0,0 +1,8 @@
+package fixtures
+
+import (
+	"crypto/md5"  //  MATCH /should not use the following blocklisted import: "crypto/md5"/
+	"crypto/sha1" // MATCH /should not use the following blocklisted import: "crypto/sha1"/
+	"strings"
+)
+
diff --git a/testdata/imports-blocklist.go b/testdata/imports-blocklist.go
new file mode 100644
index 0000000..baee90b
--- /dev/null
+++ b/testdata/imports-blocklist.go
@@ -0,0 +1,19 @@
+package fixtures
+
+import (
+	"github.com/full/match"  //  MATCH /should not use the following blocklisted import: "github.com/full/match"/
+	"bithub.com/full/match"
+	"github.com/full/matche"
+	"wildcard/between" // MATCH /should not use the following blocklisted import: "wildcard/between"/
+	"wildcard/pkg1/between" // MATCH /should not use the following blocklisted import: "wildcard/pkg1/between"/
+	"wildcard/pkg1/pkg2/between" // MATCH /should not use the following blocklisted import: "wildcard/pkg1/pkg2/between"/
+	"wildcard/backward" // MATCH /should not use the following blocklisted import: "wildcard/backward"/
+	"wildcard/backward/pkg" // MATCH /should not use the following blocklisted import: "wildcard/backward/pkg"/
+	"wildcard/backward/pkg/pkg1" // MATCH /should not use the following blocklisted import: "wildcard/backward/pkg/pkg1"/
+	"wildcard/forward" // MATCH /should not use the following blocklisted import: "wildcard/forward"/
+	"pkg/wildcard/forward" // MATCH /should not use the following blocklisted import: "pkg/wildcard/forward"/
+	"pkg/pkg1/wildcard/forward" // MATCH /should not use the following blocklisted import: "pkg/pkg1/wildcard/forward"/
+	"full" // MATCH /should not use the following blocklisted import: "full"/
+	"github.com/partical/match/fully"
+	"strings"
+)