feature: new rule inefficient-map-lookup (#1491)

README.md

@@ -560,6 +560,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`imports-blocklist`](./RULES_DESCRIPTIONS.md#imports-blocklist)   | []string | Disallows importing the specified packages                     |    no    |  no   |
 | [`increment-decrement`](./RULES_DESCRIPTIONS.md#increment-decrement) |  n/a   | Use `i++` and `i--` instead of `i += 1` and `i -= 1`.            |   yes    |  no   |
 | [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)   |  []string   | Prevents redundant else statements.                              |   yes    |  no   |
+| [`inefficient-map-lookup`](./RULES_DESCRIPTIONS.md#inefficient-map-lookup)   |  n/a  | Spots iterative searches for a key in a map           |   no    |  yes   |
 | [`line-length-limit`](./RULES_DESCRIPTIONS.md#line-length-limit)   | int (defaults to 80) | Specifies the maximum number of characters in a line             |    no    |  no   |
 | [`max-control-nesting`](./RULES_DESCRIPTIONS.md#max-control-nesting) |  int (defaults to 5)  | Sets restriction for maximum nesting of control structures. |    no    |  no   |
 | [`max-public-structs`](./RULES_DESCRIPTIONS.md#max-public-structs)  |  int (defaults to 5)  | The maximum number of public structs in a file.                  |    no    |  no   |

RULES_DESCRIPTIONS.md

@@ -56,6 +56,7 @@ List of all available rules.
 - [imports-blocklist](#imports-blocklist)
 - [increment-decrement](#increment-decrement)
 - [indent-error-flow](#indent-error-flow)
+- [inefficient-map-lookup](#inefficient-map-lookup)
 - [line-length-limit](#line-length-limit)
 - [max-control-nesting](#max-control-nesting)
 - [max-public-structs](#max-public-structs)
@@ -865,6 +866,50 @@ arguments = ["preserveScope"]
 arguments = ["preserve-scope"]
 ```
 
+## inefficient-map-lookup
+
+_Description_: This rule identifies code that iteratively searches for a key in a map.
+
+This inefficiency is usually introduced when refactoring code from using a slice to a map.
+For example if during refactoring the `elements` slice is transformed into a map.
+
+```diff
+-       elements             []string
++       elements             map[string]float64
+```
+
+and then a loop over `elements` is changed in an obvious but inefficient way:
+
+```diff
+-       for _, e := range elements {
++       for e := range elements {
+                if e == someStaticValue {
+                        // do something
+                }
+        }
+```
+
+Example:
+
+```go
+aMap := map[string]bool{}{}
+aValue := false
+
+// Inefficient map lookup
+for k := range aMap {
+  if k == aValue {
+    // do something
+  }
+}
+
+// Simpler and more efficient version
+if _, ok := aMap[aValue]; ok {
+  // do something
+}
+```
+
+_Configuration_: N/A
+
 ## line-length-limit
 
 _Description_: Warns in the presence of code lines longer than a configured maximum.

config/config.go

@@ -113,6 +113,7 @@ var allRules = append([]lint.Rule{
 	&rule.PackageDirectoryMismatchRule{},
 	&rule.UseWaitGroupGoRule{},
 	&rule.UnsecureURLSchemeRule{},
+	&rule.InefficientMapLookupRule{},
 }, defaultRules...)
 
 // allFormatters is a list of all available formatters to output the linting results.

rule/inefficient_map_lookup.go

@@ -0,0 +1,169 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"strings"
+
+	"github.com/mgechev/revive/internal/astutils"
+	"github.com/mgechev/revive/lint"
+)
+
+// InefficientMapLookupRule spots potential inefficient map lookups.
+type InefficientMapLookupRule struct{}
+
+// Apply applies the rule to given file.
+func (*InefficientMapLookupRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := &lintInefficientMapLookup{
+		file:      file,
+		onFailure: onFailure,
+	}
+
+	if err := file.Pkg.TypeCheck(); err != nil {
+		return []lint.Failure{
+			lint.NewInternalFailure(fmt.Sprintf("Unable to type check file %q: %v", file.Name, err)),
+		}
+	}
+
+	// Iterate over declarations looking for function declarations
+	for _, decl := range file.AST.Decls {
+		fn, ok := decl.(*ast.FuncDecl)
+		if !ok {
+			continue // not a function
+		}
+
+		if fn.Body == nil {
+			continue // external (no-Go) function
+		}
+
+		// Analyze the function body
+		ast.Walk(w, fn.Body)
+	}
+
+	return failures
+}
+
+// Name returns the rule name.
+func (*InefficientMapLookupRule) Name() string {
+	return "inefficient-map-lookup"
+}
+
+type lintInefficientMapLookup struct {
+	file      *lint.File
+	onFailure func(lint.Failure)
+}
+
+func (w *lintInefficientMapLookup) Visit(node ast.Node) ast.Visitor {
+	// Only interested in blocks of statements
+	block, ok := node.(*ast.BlockStmt)
+	if !ok {
+		return w // not a block of statements
+	}
+
+	w.analyzeBlock(block)
+
+	return w
+}
+
+// analyzeBlock searches AST subtrees with the following form
+//
+//	for <key> := range <map> {
+//		if <key> == <something> {
+//		   ...
+//		}
+func (w *lintInefficientMapLookup) analyzeBlock(b *ast.BlockStmt) {
+	for _, stmt := range b.List {
+		if !w.isRangeOverMapKey(stmt) {
+			continue
+		}
+
+		rangeOverMap := stmt.(*ast.RangeStmt)
+		key := rangeOverMap.Key.(*ast.Ident)
+
+		// Here we have identified a range over the keys of a map
+		// Let's check if the range body is
+		// { if <key> == <something> { ... } }
+		// or
+		// { if <key> != <something> { continue } ... }
+		if !isKeyLookup(key.Name, rangeOverMap.Body) {
+			continue
+		}
+
+		w.onFailure(lint.Failure{
+			Confidence: 1,
+			Node:       rangeOverMap,
+			Category:   lint.FailureCategoryCodeStyle,
+			Failure:    "inefficient lookup of map key",
+		})
+	}
+}
+
+func isKeyLookup(keyName string, blockStmt *ast.BlockStmt) bool {
+	blockLen := len(blockStmt.List)
+	if blockLen == 0 {
+		return false // empty
+	}
+
+	firstStmt := blockStmt.List[0]
+	ifStmt, ok := firstStmt.(*ast.IfStmt)
+	if !ok {
+		return false // the first statement of the body is not an if
+	}
+
+	binExp, ok := ifStmt.Cond.(*ast.BinaryExpr)
+	if !ok {
+		return false // the if condition is not a binary expression
+	}
+
+	if !astutils.IsIdent(binExp.X, keyName) {
+		return false // the if condition is not <key> <bin-op> <LHS>
+	}
+
+	switch binExp.Op {
+	case token.EQL:
+		// if key == ... should be the single statement in the block
+		return blockLen == 1
+
+	case token.NEQ:
+		// if key != ...
+		ifBodyStmts := ifStmt.Body.List
+		if len(ifBodyStmts) < 1 {
+			return false // if key != ... { /* empty */ }
+		}
+
+		branchStmt, ok := ifBodyStmts[0].(*ast.BranchStmt)
+		if !ok || branchStmt.Tok != token.CONTINUE {
+			return false // if key != ... { <not a continue> }
+		}
+
+		return true
+	}
+
+	return false
+}
+
+func (w *lintInefficientMapLookup) isRangeOverMapKey(stmt ast.Stmt) bool {
+	rangeStmt, ok := stmt.(*ast.RangeStmt)
+	if !ok {
+		return false // not a range
+	}
+
+	// Check if we range only on key
+	// for key := range ...
+	// for key, _ := range ...
+	hasValueVariable := rangeStmt.Value != nil && !astutils.IsIdent(rangeStmt.Value, "_")
+	if hasValueVariable {
+		return false // range over both key and value
+	}
+
+	// Check if we range over a map
+	t := w.file.Pkg.TypeOf(rangeStmt.X)
+	return t != nil && strings.HasPrefix(t.String(), "map[")
+}

test/inefficient_map_lookup_test.go

@@ -0,0 +1,12 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+func TestInefficientMapLookup(t *testing.T) {
+	testRule(t, "inefficient_map_lookup", &rule.InefficientMapLookupRule{}, &lint.RuleConfig{})
+}

testdata/inefficient_map_lookup.go

@@ -0,0 +1,63 @@
+package fixtures
+
+import "fmt"
+
+func inefficientMapLookup() {
+	type aS struct {
+		TagIDs map[int]string
+	}
+	a := aS{}
+	someStaticValue := 1
+	// use case from issue #1447
+	for id := range a.TagIDs { // MATCH /inefficient lookup of map key/
+		if id == someStaticValue {
+			return
+		}
+	}
+
+	for key, _ := range a.TagIDs { // MATCH /inefficient lookup of map key/
+		if key == someStaticValue {
+			return
+		}
+	}
+
+	for key, _ := range a.TagIDs { // MATCH /inefficient lookup of map key/
+		if key != someStaticValue {
+			continue
+		}
+		fmt.Println(key)
+	}
+
+	// do not match if the loop body contains more than
+	// just an if statement on the map key
+	aMap := map[int]int{}
+	for k := range aMap {
+		fmt.Println(k)
+		if k == 1 {
+			return
+		}
+	}
+
+	for k := range aMap {
+		if k == 1 {
+			return
+		}
+		fmt.Println(k)
+	}
+
+	// do not match on ranges over types other than maps
+	slice := []int{}
+	for i, _ := range slice {
+		if i == 1 {
+			fmt.Print(i)
+		}
+	}
+
+	for key, _ := range a.TagIDs {
+		if key != someStaticValue { // do not match if the loop body does more than just continuing
+			fmt.Println(key)
+			continue
+		}
+		fmt.Println(key)
+	}
+}