diff --git a/warn/warn_control_flow.go b/warn/warn_control_flow.go index 8b244c342..b05fade03 100644 --- a/warn/warn_control_flow.go +++ b/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,7 +504,8 @@ 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 @@ -487,10 +513,19 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li 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) } diff --git a/warn/warn_control_flow_test.go b/warn/warn_control_flow_test.go index 656076523..bc1915c4c 100644 --- a/warn/warn_control_flow_test.go +++ b/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) {