Repository: bazelbuild/buildtools
Pull request #814 (2020-04-09 17:01:40)

Implement the unnamed macro warning
Warn if a macro function does not accept (either directly or indirectly via **kwargs) a keyword argument called `name`.

A macro is a function that calls a rule (either directly or indirectly via another macro).

The warning prints an example stack trace (statically analyzed) from the current function to the detected rule definition or a native rule call.

The warning tries to not load unnecessary extra files (e.g. if a function calls other functions from already read files, they will be checked before functions from not yet read files), it also doesn't analyze the same function twice.


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 3b053cc..80f2b45 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -55,6 +55,7 @@ Warning categories supported by buildifier's linter:
   * [`same-origin-load`](#same-origin-load)
   * [`string-iteration`](#string-iteration)
   * [`uninitialized`](#uninitialized)
+  * [`unnamed-macro`](#unnamed-macro)
   * [`unreachable`](#unreachable)
   * [`unsorted-dict-items`](#unsorted-dict-items)
   * [`unused-variable`](#unused-variable)
@@ -909,6 +910,18 @@ can potentially be empty.
 
 --------------------------------------------------------------------------------
 
+## <a name="unnamed-macro"></a>Macro function doesn't accept a keyword argument "name"
+
+  * Category name: `unnamed-macro`
+  * Automatic fix: no
+
+By convention all macro functions should have a keyword argument called "name".
+
+A macro is a function that calls a rule (either directly or indirectly by calling other
+macros).
+
+--------------------------------------------------------------------------------
+
 ## <a name="unreachable"></a>The statement is unreachable
 
   * Category name: `unreachable`
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
index ed8d2ac..e628cd8 100644
--- a/warn/BUILD.bazel
+++ b/warn/BUILD.bazel
@@ -13,6 +13,7 @@ go_library(
         "warn_cosmetic.go",
         "warn_deprecated.go",
         "warn_docstring.go",
+        "warn_macro.go",
         "warn_naming.go",
         "warn_operation.go",
         "warn_visibility.go",
@@ -40,6 +41,7 @@ go_test(
         "warn_cosmetic_test.go",
         "warn_deprecated_test.go",
         "warn_docstring_test.go",
+        "warn_macro_test.go",
         "warn_naming_test.go",
         "warn_operation_test.go",
         "warn_test.go",
diff --git a/warn/multifile.go b/warn/multifile.go
index 1fb9481..d8a4036 100644
--- a/warn/multifile.go
+++ b/warn/multifile.go
@@ -1,6 +1,8 @@
 package warn
 
 import (
+	"path"
+
 	"github.com/bazelbuild/buildtools/build"
 )
 
@@ -35,6 +37,7 @@ func (fr *FileReader) retrieveFile(filename string) *build.File {
 		return nil
 	}
 
+	file.Pkg = path.Dir(filename)
 	return file
 }
 
diff --git a/warn/warn.go b/warn/warn.go
index bd5f6fd..e76e6aa 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -159,6 +159,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 // MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.
 var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) []*LinterFinding{
 	"deprecated-function": deprecatedFunctionWarning,
+	"unnamed-macro": unnamedMacroWarning,
 }
 
 // nonDefaultWarnings contains warnings that are enabled by default because they're not applicable
diff --git a/warn/warn_macro.go b/warn/warn_macro.go
new file mode 100644
index 0000000..9f84115
--- /dev/null
+++ b/warn/warn_macro.go
@@ -0,0 +1,318 @@
+// Warnings for using deprecated functions
+
+package warn
+
+import (
+	"fmt"
+
+	"github.com/bazelbuild/buildtools/build"
+)
+
+// Internal constant that represents the native module
+const nativeModule = "<native>"
+
+// function represents a function identifier, which is a pair (module name, function name).
+type function struct {
+	filename string // .bzl file where the function is defined
+	name     string // original name of the function
+}
+
+// funCall represents a call to another function. It contains information of the function itself as well as some
+// information about the environment
+type funCall struct {
+	function
+	nameAlias string // function name alias (it could be loaded with a different name or assigned to a new variable).
+	line      int    // line on which the function is being called
+}
+
+// acceptsNameArgument checks whether a function can accept a named argument called "name",
+// either directly or via **kwargs.
+func acceptsNameArgument(def *build.DefStmt) bool {
+	for _, param := range def.Params {
+		switch param := param.(type) {
+		case *build.Ident:
+			if param.Name == "name" {
+				return true
+			}
+		case *build.AssignExpr:
+			if ident, ok := param.LHS.(*build.Ident); ok && ident.Name == "name" {
+				return true
+			}
+		case *build.UnaryExpr:
+			if param.Op == "**" {
+				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`).
+}
+
+// externalDependency is a reference to a symbol defined in another file
+type externalDependency struct {
+	filename string
+	symbol   string
+}
+
+// resolvesExternal takes a local function definition and replaces it with an external one if it's been defined
+// in another file and loaded
+func resolveExternal(fn function, externalSymbols map[string]externalDependency) function {
+	if external, ok := externalSymbols[fn.name]; ok {
+		return function{filename: external.filename, name: external.symbol}
+	}
+	return fn
+}
+
+// exprLine returns the start line of an expression
+func exprLine(expr build.Expr) int {
+	start, _ := expr.Span()
+	return start.Line
+}
+
+// getFunCalls extracts information about functions that are being called from the given function
+func getFunCalls(def *build.DefStmt, filename string, externalSymbols map[string]externalDependency) map[string]funCall {
+	funCalls := make(map[string]funCall)
+	build.Walk(def, func(expr build.Expr, stack []build.Expr) {
+		call, ok := expr.(*build.CallExpr)
+		if !ok {
+			return
+		}
+		if ident, ok := call.X.(*build.Ident); ok {
+			funCalls[ident.Name] = funCall{
+				function:  resolveExternal(function{filename, ident.Name}, externalSymbols),
+				nameAlias: ident.Name,
+				line:      exprLine(call),
+			}
+			return
+		}
+		dot, ok := call.X.(*build.DotExpr)
+		if !ok {
+			return
+		}
+		if ident, ok := dot.X.(*build.Ident); !ok || ident.Name != "native" {
+			return
+		}
+		name := "native." + dot.Name
+		funCalls[name] = funCall{
+			function: function{
+				name:     dot.Name,
+				filename: nativeModule,
+			},
+			nameAlias: name,
+			line:      exprLine(dot),
+		}
+	})
+	return funCalls
+}
+
+// analyzeFile extracts the information about rules and functions defined in the file
+func analyzeFile(f *build.File) fileData {
+	if f == nil {
+		return fileData{}
+	}
+
+	// Collect loaded symbols
+	externalSymbols := make(map[string]externalDependency)
+	for _, stmt := range f.Stmt {
+		load, ok := stmt.(*build.LoadStmt)
+		if !ok {
+			continue
+		}
+		moduleFile := getPathFromLabel(load.Module.Value, f.Pkg)
+		if moduleFile == "" {
+			continue
+		}
+		for i, from := range load.From {
+			externalSymbols[load.To[i].Name] = externalDependency{moduleFile, 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:
+			// Analyze aliases (`foo = bar`) or rule declarations (`foo = rule(...)`)
+			lhsIdent, ok := stmt.LHS.(*build.Ident)
+			if !ok {
+				continue
+			}
+			if rhsIdent, ok := stmt.RHS.(*build.Ident); ok {
+				report.aliases[lhsIdent.Name] = resolveExternal(function{f.DisplayPath(), rhsIdent.Name}, externalSymbols)
+				continue
+			}
+
+			call, ok := stmt.RHS.(*build.CallExpr)
+			if !ok {
+				continue
+			}
+			ident, ok := call.X.(*build.Ident)
+			if !ok || ident.Name != "rule" {
+				continue
+			}
+			report.rules[lhsIdent.Name] = true
+		case *build.DefStmt:
+			report.functions[stmt.Name] = getFunCalls(stmt, f.DisplayPath(), externalSymbols)
+		default:
+			continue
+		}
+	}
+	return report
+}
+
+// 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
+}
+
+// macroAnalyzer is an object that analyzes the directed graph of functions calling each other,
+// loading other files lazily if necessary.
+type macroAnalyzer struct {
+	fileReader *FileReader
+	files      map[string]fileData
+	cache      map[function]functionReport
+}
+
+// getFileData retrieves a file using the fileReader object and extracts information about functions and rules
+// defined in the file.
+func (ma macroAnalyzer) getFileData(filename string) fileData {
+	if fd, ok := ma.files[filename]; ok {
+		return fd
+	}
+	if ma.fileReader == nil {
+		fd := fileData{}
+		ma.files[filename] = fd
+		return fd
+	}
+	f := ma.fileReader.GetFile(filename)
+	fd := analyzeFile(f)
+	ma.files[filename] = fd
+	return fd
+}
+
+// IsMacro is a public function that checks whether the given function is a macro
+func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
+	// Check the cache first
+	if cached, ok := ma.cache[fn]; ok {
+		return cached
+	}
+	// Write a negative result to the cache before analyzing. This will prevent stack overflow crashes
+	// if the input data contains recursion.
+	ma.cache[fn] = report
+	defer func() {
+		// Update the cache with the actual result
+		ma.cache[fn] = report
+	}()
+
+	// Check for native rules
+	if fn.filename == nativeModule {
+		switch fn.name {
+		case "glob", "existing_rule", "existing_rules":
+			// Not a rule
+		default:
+			report.isMacro = true
+		}
+		return
+	}
+
+	fileData := ma.getFileData(fn.filename)
+
+	// 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
+		}
+		return
+	}
+
+	// Check whether fn.name is a rule
+	if fileData.rules[fn.name] {
+		report.isMacro = true
+		return
+	}
+
+	// Check whether fn.name is an ordinary function
+	funCalls, ok := fileData.functions[fn.name]
+	if !ok {
+		return
+	}
+
+	// Prioritize function calls from already loaded files. If some of the function calls are from the same file
+	// (or another file that has been loaded already), check them first.
+	knownFunCalls := []funCall{}
+	newFunCalls := []funCall{}
+	for _, fc := range funCalls {
+		if _, ok := ma.files[fc.function.filename]; ok || fc.function.filename == nativeModule {
+			knownFunCalls = append(knownFunCalls, fc)
+		} else {
+			newFunCalls = append(newFunCalls, fc)
+		}
+	}
+
+	for _, fc := range append(knownFunCalls, newFunCalls...) {
+		if ma.IsMacro(fc.function).isMacro {
+			report.isMacro = true
+			report.fc = &fc
+			return
+		}
+	}
+
+	return
+}
+
+// newMacroAnalyzer creates and initiates an instance of macroAnalyzer.
+func newMacroAnalyzer(fileReader *FileReader) macroAnalyzer {
+	return macroAnalyzer{
+		fileReader: fileReader,
+		files:      make(map[string]fileData),
+		cache:      make(map[function]functionReport),
+	}
+}
+
+func unnamedMacroWarning(f *build.File, fileReader *FileReader) []*LinterFinding {
+	if f.Type != build.TypeBzl {
+		return nil
+	}
+
+	macroAnalyzer := newMacroAnalyzer(fileReader)
+	macroAnalyzer.files[f.DisplayPath()] = analyzeFile(f)
+
+	findings := []*LinterFinding{}
+	for _, stmt := range f.Stmt {
+		def, ok := stmt.(*build.DefStmt)
+		if !ok {
+			continue
+		}
+
+		if !ok || acceptsNameArgument(def) {
+			continue
+		}
+
+		report := macroAnalyzer.IsMacro(function{f.DisplayPath(), def.Name})
+		if !report.isMacro {
+			continue
+		}
+		msg := fmt.Sprintf(`Macro function %q doesn't accept a keyword argument "name".`, def.Name)
+		if report.fc != nil {
+			// fc shouldn't be nil because that's the only node that can be found inside a function.
+			msg += fmt.Sprintf(`
+
+It is considered a macro because it calls a rule or another macro %q on line %d.`, report.fc.nameAlias, report.fc.line)
+		}
+		finding := makeLinterFinding(def, msg)
+		finding.End = def.ColonPos
+		findings = append(findings, finding)
+	}
+
+	return findings
+}


Test patch:
diff --git a/warn/warn_macro_test.go b/warn/warn_macro_test.go
new file mode 100644
index 0000000..734f427
--- /dev/null
+++ b/warn/warn_macro_test.go
@@ -0,0 +1,361 @@
+package warn
+
+import "testing"
+
+func TestUnnamedMacroNoReaderSameFile(t *testing.T) {
+	checkFindings(t, "unnamed-macro", `
+load(":foo.bzl", "foo")
+
+my_rule = rule()
+
+def macro(x):
+  foo()
+  my_rule(name = x)
+
+def not_macro(x):
+  foo()
+  native.glob()
+  native.existing_rule()
+  native.existing_rules()
+  return my_rule
+
+def another_macro(x):
+  foo()
+  [native.cc_library() for i in x]
+`,
+		[]string{
+			`5: Macro function "macro" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "my_rule" on line 7.`,
+			`16: Macro function "another_macro" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "native.cc_library" on line 18.`,
+		},
+		scopeBzl)
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def macro1(foo, name, bar):
+  my_rule()
+
+def macro2(foo, *, name):
+  my_rule()
+
+def macro3(foo, *args, **kwargs):
+  my_rule()
+`,
+		[]string{},
+		scopeBzl)
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def macro(name):
+  my_rule(name = name)
+
+alias = macro
+
+def bad_macro():
+  for x in y:
+    alias(x)
+`,
+		[]string{
+			`8: Macro function "bad_macro" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "alias" on line 10.`,
+		},
+		scopeBzl)
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def macro1():
+  my_rule(name = x)
+
+def macro2():
+  macro1()
+
+def macro3():
+  macro2()
+
+def macro4():
+  my_rule()
+`,
+		[]string{
+			`3: Macro function "macro1" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "my_rule" on line 4.`,
+			`6: Macro function "macro2" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "macro1" on line 7`,
+			`9: Macro function "macro3" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "macro2" on line 10.`,
+			`12: Macro function "macro4" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "my_rule" on line 13.`,
+		},
+		scopeBzl)
+}
+
+func TestUnnamedMacroRecursion(t *testing.T) {
+	// Recursion is not allowed in Bazel, but shouldn't cause Buildifier to crash
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def macro():
+  macro()
+`, []string{}, scopeBzl)
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def macro():
+  macro()
+`, []string{}, scopeBzl)
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def macro1():
+  macro2()
+
+def macro2():
+  macro3()
+
+def macro3():
+  macro1()
+`, []string{}, scopeBzl)
+
+	checkFindings(t, "unnamed-macro", `
+my_rule = rule()
+
+def foo():
+  bar()
+
+def bar():
+  foo()
+  my_rule()
+`,
+		[]string{
+			`3: Macro function "foo" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "bar" on line 4.`,
+			`6: Macro function "bar" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "my_rule" on line 8.`,
+		},
+		scopeBzl)
+}
+
+func TestUnnamedMacroWithReader(t *testing.T) {
+	defer setUpFileReader(map[string]string{
+		"test/package/foo.bzl": `
+def foo():
+  native.foo_binary()
+
+def bar():
+  foo()
+
+my_rule = rule()
+`,
+		"test/package/baz.bzl": `
+load(":foo.bzl", "bar", your_rule = "my_rule")
+load("//does/not:exist.bzl", "something")
+
+def baz():
+  if False:
+    bar()
+
+def qux():
+  your_rule()
+
+def f():
+  something()
+`,
+	})()
+
+	checkFindings(t, "unnamed-macro", `
+load("//test/package/foo.bzl", abc = "bar")
+load(":baz.bzl", "baz", "qux", "f")
+
+def macro1(surname):
+  abc()
+
+def macro2(surname):
+  baz()
+
+def macro3(surname):
+  qux()
+
+def not_macro(x):
+  f()
+`,
+		[]string{
+			`4: Macro function "macro1" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "abc" on line 5.`,
+			`7: Macro function "macro2" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "baz" on line 8.`,
+			`10: Macro function "macro3" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "qux" on line 11.`,
+		},
+		scopeBzl)
+}
+
+func TestUnnamedMacroRecursionWithReader(t *testing.T) {
+	// Recursion is not allowed in Bazel, but shouldn't cause Buildifier to crash
+
+	defer setUpFileReader(map[string]string{
+		"test/package/foo.bzl": `
+load(":bar.bzl", "bar")
+
+def foo():
+  foo()
+
+def baz():
+  bar()
+
+def qux():
+  native.cc_library()
+
+`,
+		"test/package/bar.bzl": `
+load(":foo.bzl", "foo", "baz", quuux = "qux")
+
+def bar():
+  baz()
+
+def qux():
+  foo()
+  baz()
+  quuux()
+`,
+	})()
+
+	checkFindings(t, "unnamed-macro", `
+load(":foo.bzl", "foo", "baz")
+load(":bar.bzl", quux = "qux")
+
+def macro():
+  foo()
+  baz()
+  quux()
+`, []string{
+		`4: Macro function "macro" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "quux" on line 7.`,
+	}, scopeBzl)
+}
+
+func TestUnnamedMacroLoadCycle(t *testing.T) {
+	defer setUpFileReader(map[string]string{
+		"test/package/foo.bzl": `
+load(":test_file.bzl", some_rule = "my_rule")
+
+def foo():
+  some_rule()
+`,
+	})()
+
+	checkFindings(t, "unnamed-macro", `
+load(":foo.bzl", bar = "foo")
+
+my_rule = rule()
+
+def macro():
+  bar()
+`, []string{
+		`5: Macro function "macro" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "bar" on line 6.`,
+	}, scopeBzl)
+}
+
+func TestUnnamedMacroLoadedFiles(t *testing.T) {
+	// Test that not necessary files are not loaded
+
+	defer setUpFileReader(map[string]string{
+		"a.bzl": "a = rule()",
+		"b.bzl": "b = rule()",
+		"c.bzl": "c = rule()",
+		"d.bzl": "d = rule()",
+	})()
+
+	checkFindings(t, "unnamed-macro", `
+load("//a.bzl", "a")
+load("//b.bzl", "b")
+load("//c.bzl", "c")
+load("//d.bzl", "d")
+
+def macro1():
+  a()  # has to load a.bzl to analyze
+
+def macro2():
+  b()  # can skip b.bzl because there's a native rule
+  native.cc_library()
+
+def macro3():
+  c()  # can skip c.bzl because a.bzl has already been loaded
+  a()
+
+def macro4():
+  d()  # can skip d.bzl because there's a rule or another macro defined in the same file
+  r()
+
+r = rule()
+`, []string{
+		`6: Macro function "macro1" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "a" on line 7.`,
+		`9: Macro function "macro2" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "native.cc_library" on line 11.`,
+		`13: Macro function "macro3" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "a" on line 15.`,
+		`17: Macro function "macro4" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "r" on line 19.`,
+	}, scopeBzl)
+
+	if len(fileReaderRequests) == 1 && fileReaderRequests[0] == "a.bzl" {
+		return
+	}
+	t.Errorf("expected to load only a.bzl, instead loaded %d files: %v", len(fileReaderRequests), fileReaderRequests)
+}
+
+func TestUnnamedMacroAliases(t *testing.T) {
+	// Test that not necessary files are not loaded
+
+	defer setUpFileReader(map[string]string{
+		"test/package/foo.bzl": `
+load(":bar.bzl", _bar = "bar")
+
+bar = _bar`,
+		"test/package/bar.bzl": `
+my_rule = rule()
+
+bar = my_rule`,
+	})()
+
+	checkFindings(t, "unnamed-macro", `
+load(":bar.bzl", "bar")
+
+baz = bar
+
+def macro1():
+  baz()
+
+def macro2(name):
+  baz()
+`, []string{
+		`5: Macro function "macro1" doesn't accept a keyword argument "name".
+
+It is considered a macro because it calls a rule or another macro "baz" on line 6.`,
+	}, scopeBzl)
+}
diff --git a/warn/warn_test.go b/warn/warn_test.go
index f0c1037..ac0f53a 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -23,18 +23,24 @@ const (
 // reset it when it finishes.
 var testFileReader *FileReader
 
+// fileReaderRequests is used by tests to check which files have actually been requested by testFileReader
+var fileReaderRequests []string
+
 func setUpFileReader(data map[string]string) (cleanup func()) {
 	readFile := func(filename string) ([]byte, error) {
+		fileReaderRequests = append(fileReaderRequests, filename)
 		if contents, ok := data[filename]; ok {
 			return []byte(contents), nil
 		}
 		return nil, fmt.Errorf("File not found")
 	}
 	testFileReader = NewFileReader(readFile)
+	fileReaderRequests = nil
 
 	return func() {
 		// Tear down
 		testFileReader = nil
+		fileReaderRequests = nil
 	}
 }