Fix another only written to unused variable case

warn/warn_control_flow.go

@@ -225,6 +225,16 @@ func noEffectWarning(f *build.File) []*LinterFinding {
 	return findings
 }
 
+var mutatingMethodsReturningNone = map[string]bool{
+	"append": true,
+	"clear":  true,
+	"extend": true,
+	"insert": true,
+	"remove": true,
+	"sort":   true,
+	"update": true,
+}
+
 // extractIdentsFromStmt returns all idents from an AST node representing a
 // single statement that are either defined outside the node and used inside,
 // or defined inside the node and can be used outside.
@@ -235,7 +245,7 @@ func noEffectWarning(f *build.File) []*LinterFinding {
 //
 // Statements that contain other statements (for-loops, if-else blocks) are not
 // traversed inside.
-func extractIdentsFromStmt(stmt build.Expr) (assigned, used map[*build.Ident]bool) {
+func extractIdentsFromStmt(stmt build.Expr, mutatingReceiversCountAsUsed map[string]bool) (assigned, used map[*build.Ident]bool) {
 	// The values for `assigned` are `true` if the warning for the variable should
 	// be suppressed, and `false` otherwise.
 	// It's still important to know that the variable has been assigned in the
@@ -305,6 +315,15 @@ func extractIdentsFromStmt(stmt build.Expr) (assigned, used map[*build.Ident]boo
 					(!allLValuesUnderscored && strings.HasPrefix(lValue.Name, "_"))
 			}
 
+		case *build.CallExpr:
+			if dot, ok := expr.X.(*build.DotExpr); ok && mutatingMethodsReturningNone[dot.Name] {
+				if receiver, ok := dot.X.(*build.Ident); ok {
+					if !mutatingReceiversCountAsUsed[receiver.Name] {
+						blockedNodes[dot.X] = true
+					}
+				}
+			}
+
 		case *build.ForStmt:
 			// Like AssignExpr, ForStmt too has an analogue of LHS and RHS.
 			// Unlike AssignExpr, in this function they may appear only in the root of
@@ -410,6 +429,11 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
 	// Symbols for which the warning should be suppressed
 	suppressedWarnings := make(map[string]bool)
 
+	// Symbols that can represent mutable values received from outside the current
+	// scope. Mutating method calls on these symbols count as usage unless the
+	// symbol has been reassigned.
+	mutatingReceiversCountAsUsed := make(map[string]bool)
+
 	// Symbols from outer scopes that are used in the current scope
 	usedSymbolsFromOuterScope := make(map[string]bool)
 
@@ -445,6 +469,7 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
 				// Function parameters are defined in the current scope.
 				if ident, _ := build.GetParamIdent(param); ident != nil {
 					definedSymbols[ident.Name] = ident
+					mutatingReceiversCountAsUsed[ident.Name] = true
 					if ident.Name == "name" || strings.HasPrefix(ident.Name, "_") || edit.ContainsComments(param, "@unused") {
 						// Don't warn about function arguments if they start with "_"
 						// or explicitly marked with @unused.
@@ -464,7 +489,7 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
 				// RHS is not a statement, but similar traversal rules should be applied
 				// to it. E.g. it may have a comprehension node with its inner scope or
 				// a function call with a keyword parameter.
-				_, used := extractIdentsFromStmt(assign.RHS)
+				_, used := extractIdentsFromStmt(assign.RHS, nil)
 				for ident := range used {
 					// RHS idents in the def statement contains direct references to the outer scope.
 					usedSymbolsFromOuterScope[ident.Name] = true
@@ -479,18 +504,28 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
 			return
 
 		default:
-			assigned, used := extractIdentsFromStmt(expr)
+			assigned, used := extractIdentsFromStmt(expr, mutatingReceiversCountAsUsed)
+			newlyDefinedSymbols := make(map[string]*build.Ident)
 
 			for symbol := range used {
 				usedSymbols[symbol.Name] = true
 			}
 			for symbol, isSuppressed := range assigned {
 				if _, ok := definedSymbols[symbol.Name]; !ok {
 					definedSymbols[symbol.Name] = symbol
+					newlyDefinedSymbols[symbol.Name] = symbol
 					if isSuppressed {
 						suppressedWarnings[symbol.Name] = true
 					}
 				}
+				delete(mutatingReceiversCountAsUsed, symbol.Name)
+			}
+			if forStmt, ok := expr.(*build.ForStmt); ok {
+				for _, lValue := range bzlenv.CollectLValues(forStmt.Vars) {
+					if newlyDefinedSymbols[lValue.Name] == lValue {
+						mutatingReceiversCountAsUsed[lValue.Name] = true
+					}
+				}
 			}
 		}
 		return
@@ -785,7 +820,7 @@ func findUninitializedVariables(stmts []build.Expr, previouslyInitialized map[st
 				// function to retrieve idents from the outer scope that are used inside
 				// the comprehension.
 
-				_, used := extractIdentsFromStmt(expr)
+				_, used := extractIdentsFromStmt(expr, nil)
 				for ident := range used {
 					callbackIfNeeded(ident)
 				}

warn/warn_control_flow_test.go

@@ -697,6 +697,49 @@ sample_macro_with_used_foo()
 `,
 		[]string{},
 		scopeEverywhere)
+
+	checkFindings(t, "unused-variable", `
+def foo(items):
+  items.append(1)
+
+foo([])
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "unused-variable", `
+def foo(passed_in_list_of_lists):
+  for mutatable_list in passed_in_list_of_lists:
+    mutatable_list.append(1)
+
+foo([[], []])
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "unused-variable", `
+def framework_import_impl():
+  used_list = []
+  unused_list = []
+  unused_dictionary = {}
+
+  used_list.extend([1])
+  unused_dictionary.update(_ensure_swiftmodule_is_embedded(swiftmodule))
+  unused_list.extend([
+			x
+      for x in used_list
+      if x > 5
+  ])
+
+  return used_list
+
+framework_import_impl()
+`,
+		[]string{
+			":3: Variable \"unused_list\" is unused.",
+			":4: Variable \"unused_dictionary\" is unused.",
+		},
+		scopeEverywhere)
 }
 
 func TestRedefinedVariable(t *testing.T) {