Repository: bazelbuild/buildtools
Pull request #911 (2020-09-22 11:25:28)

Allow += on top level lists
The `redefined-variable` check disallows modifying top-level variables, however the `+=` operator on lists doesn't redefine them but just modifies them in-place, that should be allowed.


Patch:
diff --git a/warn/types.go b/warn/types.go
index d1ebbe9..00fa217 100644
--- a/warn/types.go
+++ b/warn/types.go
@@ -36,6 +36,7 @@ const (
 	Int
 	None
 	String
+	List
 )
 
 func (t Type) String() string {
@@ -50,6 +51,7 @@ func (t Type) String() string {
 		"int",
 		"none",
 		"string",
+		"list",
 	}[t]
 }
 
@@ -74,11 +76,15 @@ func detectTypes(f *build.File) map[build.Expr]Type {
 			nodeType = String
 		case *build.DictExpr:
 			nodeType = Dict
+		case *build.ListExpr:
+			nodeType = List
 		case *build.LiteralExpr:
 			nodeType = Int
 		case *build.Comprehension:
 			if node.Curly {
 				nodeType = Dict
+			} else {
+				nodeType = List
 			}
 		case *build.CallExpr:
 			if ident, ok := (node.X).(*build.Ident); ok {
@@ -87,6 +93,8 @@ func detectTypes(f *build.File) map[build.Expr]Type {
 					nodeType = Depset
 				case "dict":
 					nodeType = Dict
+				case "list":
+					nodeType = List
 				}
 			} else if dot, ok := (node.X).(*build.DotExpr); ok {
 				if result[dot.X] == CtxActions && dot.Name == "args" {
diff --git a/warn/warn_control_flow.go b/warn/warn_control_flow.go
index 8aab96e..f9e9202 100644
--- a/warn/warn_control_flow.go
+++ b/warn/warn_control_flow.go
@@ -263,6 +263,7 @@ func redefinedVariableWarning(f *build.File) []*LinterFinding {
 	findings := []*LinterFinding{}
 	definedSymbols := make(map[string]bool)
 
+	types := detectTypes(f)
 	for _, s := range f.Stmt {
 		// look for all assignments in the scope
 		as, ok := s.(*build.AssignExpr)
@@ -273,14 +274,20 @@ func redefinedVariableWarning(f *build.File) []*LinterFinding {
 		if !ok {
 			continue
 		}
-		if definedSymbols[left.Name] {
-			findings = append(findings,
-				makeLinterFinding(as.LHS, fmt.Sprintf(`Variable %q has already been defined. 
-Redefining a global value is discouraged and will be forbidden in the future.
-Consider using a new variable instead.`, left.Name)))
+		if !definedSymbols[left.Name] {
+			definedSymbols[left.Name] = true
+			continue
+		}
+
+		if as.Op == "+=" && (types[as.LHS] == List || types[as.RHS] == List) {
+			// Not a reassignment, just appending to a list
 			continue
 		}
-		definedSymbols[left.Name] = true
+
+		findings = append(findings,
+			makeLinterFinding(as.LHS, fmt.Sprintf(`Variable %q has already been defined. 
+Redefining a global value is discouraged and will be forbidden in the future.
+Consider using a new variable instead.`, left.Name)))
 	}
 	return findings
 }


Test patch:
diff --git a/warn/types_test.go b/warn/types_test.go
index 183dfe8..b1bb8a5 100644
--- a/warn/types_test.go
+++ b/warn/types_test.go
@@ -76,10 +76,10 @@ d = dict:<{}>
 d2 = dict:<{foo: bar}>
 d3 = dict:<dict(**foo)>
 d4 = dict:<{k: v for k, v in foo}>
-dep = depset:<depset(items = [
+dep = depset:<depset(items = list:<[
     string:
,
     dict:<d>,
-])>
+]>)>
 foo = bar
 `)
 }
diff --git a/warn/warn_control_flow_test.go b/warn/warn_control_flow_test.go
index 4c79044..ac3352e 100644
--- a/warn/warn_control_flow_test.go
+++ b/warn/warn_control_flow_test.go
@@ -353,6 +353,7 @@ func TestRedefinedVariable(t *testing.T) {
 	checkFindings(t, "redefined-variable", `
 x = "old_value"
 x = "new_value"
+x[1] = "new"
 cc_library(name = x)`,
 		[]string{":2: Variable \"x\" has already been defined."},
 		scopeEverywhere)
@@ -371,6 +372,44 @@ def bar():
   y = "g"`,
 		[]string{},
 		scopeEverywhere)
+
+	checkFindings(t, "redefined-variable", `
+x = [1, 2, 3]
+y = [a for a in b]
+z = list()
+n = 43
+
+x += something()
+y += something()
+z += something()
+n += something()
+x -= something()`,
+		[]string{
+			":9: Variable \"n\" has already been defined.",
+			":10: Variable \"x\" has already been defined.",
+		},
+		scopeEverywhere)
+
+	checkFindings(t, "redefined-variable", `
+x = [1, 2, 3]
+y = [a for a in b]
+z = list()
+
+a = something()
+b = something()
+c = something()
+d = something()
+e = something()
+
+a += x
+b += y
+c += z
+d += [42]
+e += foo`,
+		[]string{
+			":15: Variable \"e\" has already been defined.",
+		},
+		scopeEverywhere)
 }
 
 func TestWarnUnusedLoad(t *testing.T) {