Bugfix false positive for "unused" variable for nested defs (#1376)

When detecting whether symbols are used or not, variables referenced as argument defaults of the inner def would count as "use" in the inner scope, and hence falsely trigger "unused" warning.

Example:

```
def outer_def(name, foo = "foo"):
    def inner_def(foo = foo):
        print(foo)  # buildifier: disable=print

    inner_def()
```

Unused variable check for the above function would
1. Check outer_def for variable usage
2. Recursively check inner_def for variable usage
  - Detect that `foo` is used
  - Omit `foo` from returned usedSymbolsFromOuterScope since `foo` is a local variable
3. Receive no "usedSymbol" for `foo`
4. Output "unused-variable" warning

If there are RHS idents in a def statement, these idents are always from an outer scope, so these can check the later inner-scope definedSymbols check.

Co-authored-by: Tim Malmström <oreflow@google.com>

Author: oreflow

warn/warn_control_flow.go

@@ -410,6 +410,9 @@ 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 from outer scopes that are used in the current scope
+	usedSymbolsFromOuterScope := make(map[string]bool)
+
 	build.WalkStatements(root, func(expr build.Expr, stack []build.Expr) (err error) {
 		switch expr := expr.(type) {
 		case *build.File:
@@ -463,7 +466,8 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
 				// a function call with a keyword parameter.
 				_, used := extractIdentsFromStmt(assign.RHS)
 				for ident := range used {
-					usedSymbols[ident.Name] = true
+					// RHS idents in the def statement contains direct references to the outer scope.
+					usedSymbolsFromOuterScope[ident.Name] = true
 				}
 			}
 
@@ -497,7 +501,6 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
 	// make the variable with the same name from an outer scope also used.
 	// Collect variables that are used in the current or inner scopes but are not
 	// defined in the current scope.
-	usedSymbolsFromOuterScope := make(map[string]bool)
 	for symbol := range usedSymbols {
 		if _, ok := definedSymbols[symbol]; ok {
 			continue

warn/warn_control_flow_test.go

@@ -668,6 +668,35 @@ bar()
 			":9: Variable \"y\" is unused.",
 		},
 		scopeEverywhere)
+
+	// foo is unused in the outer macro, since the symbol is overloaded, and only
+	// the inner_def "foo" is actually used.
+	checkFindings(t, "unused-variable", `
+def sample_macro_with_unused_foo(name, foo = "foo"):
+  def inner_def(foo = "bar"):
+    print(foo)
+
+  inner_def()
+
+sample_macro_with_unused_foo()
+`,
+		[]string{
+			":1: Variable \"foo\" is unused.",
+		},
+		scopeEverywhere)
+
+	// Foo is referenced as the default value for foo in inner_def, hence it is not unused.
+	checkFindings(t, "unused-variable", `
+def sample_macro_with_used_foo(name, foo = "foo"):
+  def inner_def(bar = foo):
+    print(bar)
+
+  inner_def()
+
+sample_macro_with_used_foo()
+`,
+		[]string{},
+		scopeEverywhere)
 }
 
 func TestRedefinedVariable(t *testing.T) {