Updating positional-args check to be more comprehensive (#1393)

- Previous implementation just assumed that macros were called at the root of a BUILD file, and that all calls were to macros/rules except for explicit allowlist. Updating the check to more precisely validate whether something is a rule/macro, and checking all call expressions (including ones nested in dicts/attrs).
- Also updating IsMacro implementation to correctly support symblic macro definitions (`my_macro = macro(...)`)

Co-authored-by: Tim Malmström <[email protected]>

Author: oreflow

WARNINGS.md

@@ -1205,10 +1205,11 @@ The linter allows the following to be before `package()`:
   * Automatic fix: no
   * [Suppress the warning](#suppress): `# buildifier: disable=positional-args`
 
-All top level calls (except for some built-ins) should use keyword args over
-positional arguments. Positional arguments can cause subtle errors if the order
-is switched or if an argument is removed. Keyword args also greatly improve
-readability.
+All macro and rule calls should use keyword args over positional arguments.
+Positional arguments can cause subtle errors if the order is switched or if
+an argument is removed. Keyword args also greatly improve readability.
+Additionally, positional arguments prevent migration from [Legacy Macros](https://bazel.build/extending/legacy-macros)
+to [Symbolic Macros](https://bazel.build/extending/macros).
 
 ```diff
 - my_macro("foo", "bar")

warn/docs/warnings.textproto

@@ -876,10 +876,11 @@ warnings: {
   name: "positional-args"
   header: "Keyword arguments should be used over positional arguments"
   description:
-    "All top level calls (except for some built-ins) should use keyword args over\n"
-    "positional arguments. Positional arguments can cause subtle errors if the order\n"
-    "is switched or if an argument is removed. Keyword args also greatly improve\n"
-    "readability.\n\n"
+    "All macro and rule calls should use keyword args over positional arguments.\n"
+    "Positional arguments can cause subtle errors if the order is switched or if\n"
+    "an argument is removed. Keyword args also greatly improve readability.\n"
+    "Additionally, positional arguments prevent migration from [Legacy Macros](https://bazel.build/extending/legacy-macros)\n"
+    "to [Symbolic Macros](https://bazel.build/extending/macros).\n\n"
     "```diff\n"
     "- my_macro(\"foo\", \"bar\")\n"
     "+ my_macro(name = \"foo\", env = \"bar\")\n"

warn/warn.go

@@ -112,9 +112,7 @@ func makeLinterFinding(node build.Expr, message string, replacement ...LinterRep
 
 // RuleWarningMap lists the warnings that run on a single rule.
 // These warnings run only on BUILD files (not bzl files).
-var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFinding{
-	"positional-args": positionalArgumentsWarning,
-}
+var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFinding{}
 
 // FileWarningMap lists the warnings that run on the whole file.
 var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
@@ -219,6 +217,7 @@ var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader)
 	"native-sh-binary":                   NativeShellRulesWarning("sh_binary"),
 	"native-sh-library":                  NativeShellRulesWarning("sh_library"),
 	"native-sh-test":                     NativeShellRulesWarning("sh_test"),
+	"positional-args":                    positionalArgumentsWarning,
 	"unnamed-macro":                      unnamedMacroWarning,
 }
 

warn/warn_bazel.go

@@ -25,14 +25,6 @@ import (
 	"github.com/bazelbuild/buildtools/build"
 )
 
-var functionsWithPositionalArguments = map[string]bool{
-	"exports_files":       true,
-	"licenses":            true,
-	"print":               true,
-	"register_toolchains": true,
-	"vardef":              true,
-}
-
 func constantGlobPatternWarning(patterns *build.ListExpr) []*LinterFinding {
 	findings := []*LinterFinding{}
 	for _, expr := range patterns.List {
@@ -179,17 +171,37 @@ func duplicatedNameWarning(f *build.File) []*LinterFinding {
 	return findings
 }
 
-func positionalArgumentsWarning(call *build.CallExpr, pkg string) *LinterFinding {
-	if id, ok := call.X.(*build.Ident); !ok || functionsWithPositionalArguments[id.Name] {
+func positionalArgumentsWarning(f *build.File, fileReader *FileReader) (findings []*LinterFinding) {
+	if f.Type != build.TypeBuild {
 		return nil
 	}
-	for _, arg := range call.List {
-		if _, ok := arg.(*build.AssignExpr); ok || arg == nil {
-			continue
-		}
-		return makeLinterFinding(arg, "All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.")
+	macroAnalyzer := newMacroAnalyzer(fileReader)
+	macroAnalyzer.files[f.Pkg+":"+f.Label] = analyzeFile(f)
+
+	for _, expr := range f.Stmt {
+		build.Walk(expr, func(x build.Expr, _ []build.Expr) {
+			if fnCall, ok := x.(*build.CallExpr); ok {
+				fnIdent, ok := fnCall.X.(*build.Ident)
+				if !ok {
+					return
+				}
+
+				if macroAnalyzer.IsRuleOrMacro(function{pkg: f.Pkg, filename: f.Label, name: fnIdent.Name}).isRuleOrMacro {
+					for _, arg := range fnCall.List {
+						if _, ok := arg.(*build.AssignExpr); ok || arg == nil {
+							continue
+						}
+						findings = append(findings, makeLinterFinding(fnCall, fmt.Sprintf(
+							`All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro %q with positional arguments.`,
+							fnIdent.Name)))
+						return
+					}
+				}
+			}
+		})
 	}
-	return nil
+	return
 }
 
 func argsKwargsInBuildFilesWarning(f *build.File) []*LinterFinding {
@@ -257,7 +269,6 @@ func externalPathWarning(f *build.File) []*LinterFinding {
 		if !ok {
 			return
 		}
-		
 		// Warn for "/external/" but not if "//" appears in the string (main repository paths)
 		if strings.Contains(stringExpr.Value, "/external/") && !strings.Contains(stringExpr.Value, "//") {
 			findings = append(findings,
@@ -280,7 +291,6 @@ func canonicalRepositoryWarning(f *build.File) []*LinterFinding {
 		if !ok {
 			return
 		}
-		
 		if strings.Contains(stringExpr.Value, "@@") {
 			findings = append(findings,
 				makeLinterFinding(stringExpr, `String contains "@@" which indicates a canonical repository name reference that should be avoided.`))

warn/warn_bazel_test.go

@@ -87,18 +87,22 @@ exports_files(["foo.txt"])
 		scopeBuild|scopeWorkspace)
 }
 
-func TestPositionalArguments(t *testing.T) {
+func TestPositionalArgumentsDoesNotWarnForNamedArguments(t *testing.T) {
 	checkFindings(t, "positional-args", `
+my_macro = macro()
+my_rule = rule()
+def my_function(foo):
+  pass
+
 my_macro(foo = "bar")
-my_macro("foo", "bar")
-my_macro(foo = bar(x))
-[my_macro(foo) for foo in bar]`,
-		[]string{
-			":2: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.",
-			":4: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.",
-		},
+my_rule(foo = "bar")
+my_function(foo = "bar")
+`,
+		[]string{},
 		scopeBuild)
+}
 
+func TestPositionalArgumentsDoesNotWarnForAllowlistedFunctions(t *testing.T) {
 	checkFindings(t, "positional-args", `
 register_toolchains(
 	"//foo",
@@ -108,6 +112,55 @@ register_toolchains(
 		scopeBuild)
 }
 
+func TestPositionalArgumentsWarnsForPositionalMacrosOrRuleCalls(t *testing.T) {
+	checkFindings(t, "positional-args", `
+my_macro = macro()
+my_rule = rule()
+def my_function(foo):
+  pass
+
+my_macro("foo", "bar")
+my_rule("foo", "bar")
+my_function("foo", "bar")
+`,
+		[]string{
+			`6: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro "my_macro" with positional arguments.`,
+			`7: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro "my_rule" with positional arguments.`,
+		},
+		scopeBuild)
+}
+
+func TestPositionalArgumentsWarnsWhenCalledInNestedContexts(t *testing.T) {
+	checkFindings(t, "positional-args", `
+my_macro = macro()
+my_rule = rule()
+def my_function(foo):
+  pass
+
+other_function(foo = my_macro(x))
+other_function(foo = my_rule(x))
+other_function(foo = my_function(x))
+
+[my_macro(foo) for foo in bar]
+[my_rule(foo) for foo in bar]
+[my_function(foo) for foo in bar]
+`,
+		[]string{
+			`6: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro "my_macro" with positional arguments.`,
+			`7: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro "my_rule" with positional arguments.`,
+			`10: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro "my_macro" with positional arguments.`,
+			`11: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax.
+Found call to rule or macro "my_rule" with positional arguments.`,
+		},
+		scopeBuild)
+
+}
+
 func TestKwargsInBuildFilesWarning(t *testing.T) {
 	checkFindings(t, "build-args-kwargs", `
 cc_library(

warn/warn_macro.go

@@ -53,17 +53,18 @@ type funCall struct {
 func acceptsNameArgument(def *build.DefStmt) bool {
 	for _, param := range def.Params {
 		if name, op := build.GetParamName(param); name == "name" || op == "**" {
-  		return true
+			return true
 		}
 	}
 	return false
 }
 
 // fileData represents information about rules and functions extracted from a file
 type fileData struct {
-	rules     map[string]bool               // all rules defined in the file
-	functions map[string]map[string]funCall // outer map: all functions defined in the file, inner map: all distinct function calls from the given function
-	aliases   map[string]function           // all top-level aliases (e.g. `foo = bar`).
+	loadedSymbols map[string]function           // Symbols loaded from other files.
+	rulesOrMacros map[string]bool               // all rules or macros defined in the file.
+	functions     map[string]map[string]funCall // outer map: all functions defined in the file, inner map: all distinct function calls from the given function
+	aliases       map[string]function           // all top-level aliases (e.g. `foo = bar`).
 }
 
 // resolvesExternal takes a local function definition and replaces it with an external one if it's been defined
@@ -123,8 +124,14 @@ func analyzeFile(f *build.File) fileData {
 		return fileData{}
 	}
 
+	report := fileData{
+		loadedSymbols: make(map[string]function),
+		rulesOrMacros: make(map[string]bool),
+		functions:     make(map[string]map[string]funCall),
+		aliases:       make(map[string]function),
+	}
+
 	// Collect loaded symbols
-	externalSymbols := make(map[string]function)
 	for _, stmt := range f.Stmt {
 		load, ok := stmt.(*build.LoadStmt)
 		if !ok {
@@ -135,15 +142,10 @@ func analyzeFile(f *build.File) fileData {
 			continue
 		}
 		for i, from := range load.From {
-			externalSymbols[load.To[i].Name] = function{label.Package, label.Target, from.Name}
+			report.loadedSymbols[load.To[i].Name] = function{label.Package, label.Target, from.Name}
 		}
 	}
 
-	report := fileData{
-		rules:     make(map[string]bool),
-		functions: make(map[string]map[string]funCall),
-		aliases:   make(map[string]function),
-	}
 	for _, stmt := range f.Stmt {
 		switch stmt := stmt.(type) {
 		case *build.AssignExpr:
@@ -153,21 +155,22 @@ func analyzeFile(f *build.File) fileData {
 				continue
 			}
 			if rhsIdent, ok := stmt.RHS.(*build.Ident); ok {
-				report.aliases[lhsIdent.Name] = resolveExternal(function{f.Pkg, f.Label, rhsIdent.Name}, externalSymbols)
+				report.aliases[lhsIdent.Name] = resolveExternal(function{f.Pkg, f.Label, rhsIdent.Name}, report.loadedSymbols)
 				continue
 			}
 
 			call, ok := stmt.RHS.(*build.CallExpr)
 			if !ok {
 				continue
 			}
-			ident, ok := call.X.(*build.Ident)
-			if !ok || ident.Name != "rule" {
-				continue
+			if ident, ok := call.X.(*build.Ident); ok {
+				if ident.Name == "rule" || ident.Name == "macro" {
+					report.rulesOrMacros[lhsIdent.Name] = true
+					continue
+				}
 			}
-			report.rules[lhsIdent.Name] = true
 		case *build.DefStmt:
-			report.functions[stmt.Name] = getFunCalls(stmt, f.Pkg, f.Label, externalSymbols)
+			report.functions[stmt.Name] = getFunCalls(stmt, f.Pkg, f.Label, report.loadedSymbols)
 		default:
 			continue
 		}
@@ -177,8 +180,8 @@ func analyzeFile(f *build.File) fileData {
 
 // functionReport represents the analysis result of a function
 type functionReport struct {
-	isMacro bool     // whether the function is a macro (or a rule)
-	fc      *funCall // a call to the rule or another macro
+	isRuleOrMacro bool     // whether the function is a macro (or a rule)
+	fc            *funCall // a call to the rule or another macro
 }
 
 // macroAnalyzer is an object that analyzes the directed graph of functions calling each other,
@@ -208,7 +211,7 @@ func (ma macroAnalyzer) getFileData(pkg, label string) fileData {
 }
 
 // IsMacro is a public function that checks whether the given function is a macro
-func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
+func (ma macroAnalyzer) IsRuleOrMacro(fn function) (report functionReport) {
 	// Check the cache first
 	if cached, ok := ma.cache[fn]; ok {
 		return cached
@@ -228,7 +231,7 @@ func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
 			"repository_name", "exports_files":
 			// Not a rule
 		default:
-			report.isMacro = true
+			report.isRuleOrMacro = true
 		}
 		return
 	}
@@ -237,18 +240,26 @@ func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
 
 	// Check whether fn.name is an alias for another function
 	if alias, ok := fileData.aliases[fn.name]; ok {
-		if ma.IsMacro(alias).isMacro {
-			report.isMacro = true
+		if ma.IsRuleOrMacro(alias).isRuleOrMacro {
+			report.isRuleOrMacro = true
 		}
 		return
 	}
 
-	// Check whether fn.name is a rule
-	if fileData.rules[fn.name] {
-		report.isMacro = true
+	// Check whether fn.name is a rule or macro
+	if fileData.rulesOrMacros[fn.name] {
+		report.isRuleOrMacro = true
 		return
 	}
 
+	// Check whether fn.name is a loaded symbol from another file
+	if externalFn, ok := fileData.loadedSymbols[fn.name]; ok {
+		if ma.IsRuleOrMacro(externalFn).isRuleOrMacro {
+			report.isRuleOrMacro = true
+			return
+		}
+	}
+
 	// Check whether fn.name is an ordinary function
 	funCalls, ok := fileData.functions[fn.name]
 	if !ok {
@@ -267,8 +278,8 @@ func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
 	}
 
 	for _, fc := range append(knownFunCalls, newFunCalls...) {
-		if ma.IsMacro(fc.function).isMacro {
-			report.isMacro = true
+		if ma.IsRuleOrMacro(fc.function).isRuleOrMacro {
+			report.isRuleOrMacro = true
 			report.fc = &fc
 			return
 		}
@@ -305,8 +316,8 @@ func unnamedMacroWarning(f *build.File, fileReader *FileReader) []*LinterFinding
 			continue
 		}
 
-		report := macroAnalyzer.IsMacro(function{f.Pkg, f.Label, def.Name})
-		if !report.isMacro {
+		report := macroAnalyzer.IsRuleOrMacro(function{f.Pkg, f.Label, def.Name})
+		if !report.isRuleOrMacro {
 			continue
 		}
 		msg := fmt.Sprintf(`The macro %q should have a keyword argument called "name".`, def.Name)