diff --git a/WARNINGS.md b/WARNINGS.md index 79b75315a..690237cb1 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -1216,14 +1216,6 @@ to [Symbolic Macros](https://bazel.build/extending/macros). + my_macro(name = "foo", env = "bar") ``` -The linter allows the following functions to be called with positional arguments: - - * `load()` - * `vardef()` - * `export_files()` - * `licenses()` - * `print()` - -------------------------------------------------------------------------------- ## `print()` is a debug function and shouldn't be submitted diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel index 225b5fcfa..6408244e7 100644 --- a/warn/BUILD.bazel +++ b/warn/BUILD.bazel @@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "warn", srcs = [ + "macro_analyzer.go", "multifile.go", "types.go", "warn.go", @@ -34,6 +35,7 @@ go_test( name = "warn_test", size = "small", srcs = [ + "macro_analyzer_test.go", "types_test.go", "warn_bazel_api_test.go", "warn_bazel_operation_test.go", @@ -53,6 +55,7 @@ go_test( "//build", "//tables", "//testutils", + "@com_github_google_go_cmp//cmp", ], ) diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index 9d3eb8e3b..16c390af1 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -884,13 +884,7 @@ warnings: { "```diff\n" "- my_macro(\"foo\", \"bar\")\n" "+ my_macro(name = \"foo\", env = \"bar\")\n" - "```\n\n" - "The linter allows the following functions to be called with positional arguments:\n\n" - " * `load()`\n" - " * `vardef()`\n" - " * `export_files()`\n" - " * `licenses()`\n" - " * `print()`" + "```" } warnings: { diff --git a/warn/macro_analyzer.go b/warn/macro_analyzer.go new file mode 100644 index 000000000..e95d07b22 --- /dev/null +++ b/warn/macro_analyzer.go @@ -0,0 +1,302 @@ +/* +Copyright 2025 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Analyzer to determine if a function is a macro (produces targets). + +package warn + +import ( + "fmt" + "strings" + + "github.com/bazelbuild/buildtools/build" + "github.com/bazelbuild/buildtools/labels" +) + +// MacroAnalyzer is an object that analyzes the directed graph of functions calling each other, +// determining whether a function produces targets or not. +type MacroAnalyzer struct { + fileReader *FileReader + cache map[symbolRef]*symbolAnalysisResult +} + +// NewMacroAnalyzer creates and initiates an instance of macroAnalyzer. +func NewMacroAnalyzer(fileReader *FileReader) MacroAnalyzer { + if fileReader == nil { + // If no file reader is provided, a default one is provided which fails on all reads. + // This can still be used if functions are preloaded via cache. + fileReader = NewFileReader(func(_ string) ([]byte, error) { + return nil, fmt.Errorf("tried to read file without file reader") + }) + } + return MacroAnalyzer{ + fileReader: fileReader, + cache: make(map[symbolRef]*symbolAnalysisResult), + } +} + +// MacroAnalyzerReport defines the results of analyzing a function using the MacroAnalyzer. +type MacroAnalyzerReport struct { + SelfDescription string + symbolAnalysis *symbolAnalysisResult +} + +// CanProduceTargets returns true if provided function has any call path which produces a target. +// A function which produces targets is by definition either a rule or a macro. +func (mar *MacroAnalyzerReport) CanProduceTargets() bool { + if mar.symbolAnalysis == nil { + return false + } + return mar.symbolAnalysis.canProduceTargets +} + +// PrintableCallStack returns a user-readable call stack, providing a path for how a function may +// produce targets. +func (mar *MacroAnalyzerReport) PrintableCallStack() string { + if mar.symbolAnalysis == nil { + return "" + } + return strings.Join(mar.symbolAnalysis.callStackFrames, "\n") +} + +// AnalyzeFn analyzes the provided def statement, and returns a report containing whether it produces a target (is a macro) or not. +func (ma *MacroAnalyzer) AnalyzeFn(f *build.File, def *build.DefStmt) (*MacroAnalyzerReport, error) { + ma.fileReader.AddFileToCache(f) + call := symbolCall{symbol: &symbolRef{pkg: f.Pkg, label: f.Label, name: def.Name}, line: exprLine(def)} + report, err := ma.analyzeSymbol(call) + if err != nil { + return nil, err + } + return &MacroAnalyzerReport{ + SelfDescription: call.asCallStackFrame(), + symbolAnalysis: report, + }, nil +} + +// AnalyzeFnCall analyzes a function call to see if it can produce a targets or not. +func (ma *MacroAnalyzer) AnalyzeFnCall(f *build.File, call *build.CallExpr) (*MacroAnalyzerReport, error) { + ma.fileReader.AddFileToCache(f) + if symbolName := callExprToString(call); symbolName != "" { + call := symbolCall{symbol: &symbolRef{pkg: f.Pkg, label: f.Label, name: symbolName}, line: exprLine(call)} + report, err := ma.analyzeSymbol(call) + if err != nil { + return nil, err + } + return &MacroAnalyzerReport{ + SelfDescription: call.asCallStackFrame(), + symbolAnalysis: report, + }, nil + } + return nil, fmt.Errorf("error checking call for being a macro at %s:%d", f.Path, exprLine(call)) +} + +// symbolAnalysisResult stores the result of analyzing a symbolRef. +type symbolAnalysisResult struct { + canProduceTargets bool + callStackFrames []string +} + +// symbolRef represents a symbol in a specific file. +type symbolRef struct { + pkg string + label string + name string +} + +// symbolCall represents a call (by line number) to a symbolRef. +type symbolCall struct { + line int + symbol *symbolRef +} + +func (sc *symbolCall) asCallStackFrame() string { + return fmt.Sprintf("%s:%s:%d %s", sc.symbol.pkg, sc.symbol.label, sc.line, sc.symbol.name) +} + +// traversalNode is an internal structure to keep track of symbol call hierarchies while traversing symbols. +type traversalNode struct { + parent *traversalNode + symbolCall *symbolCall +} + +// analyzeSymbol identifies a given symbol, and traverses its call stack to detect if any downstream calls can generate targets. +func (ma *MacroAnalyzer) analyzeSymbol(sc symbolCall) (*symbolAnalysisResult, error) { + queue := []*traversalNode{{symbolCall: &sc}} + visited := make(map[symbolRef]bool) + + var current *traversalNode + var nodeProducedTarget *traversalNode + + for len(queue) > 0 && nodeProducedTarget == nil { + current, queue = queue[0], queue[1:] + visited[*current.symbolCall.symbol] = true + + if producesTarget(current.symbolCall.symbol) { + nodeProducedTarget = current + } + calls, err := ma.expandSymbol(current.symbolCall.symbol) + if err != nil { + return nil, err + } + for _, call := range calls { + if _, isVisited := visited[*call.symbol]; isVisited { + continue + } + ref := &traversalNode{parent: current, symbolCall: &call} + // adding symbol to front/back of queue depending on whether the file is already loaded or not. + if ma.fileReader.IsCached(call.symbol.pkg, call.symbol.label) { + queue = append([]*traversalNode{ref}, queue...) + } else { + queue = append(queue, ref) + } + } + } + if nodeProducedTarget == nil { + // If no node produced a target, all visited nodes can be cached as non-macros. + for symbol := range visited { + ma.cache[symbol] = &symbolAnalysisResult{canProduceTargets: false} + } + } else { + // If a node produced a target, the call stack above the node can be cached as producing targets. + var callStackFrames []string + node := nodeProducedTarget + for node != nil { + ma.cache[*node.symbolCall.symbol] = &symbolAnalysisResult{canProduceTargets: true, callStackFrames: callStackFrames} + callStackFrames = append([]string{node.symbolCall.asCallStackFrame()}, callStackFrames...) + node = node.parent + } + } + return ma.cache[*sc.symbol], nil +} + +// exprLine returns the start line of an expression +func exprLine(expr build.Expr) int { + start, _ := expr.Span() + return start.Line +} + +// expandSymbol expands the provided symbol, returning a list of other symbols that it references. +// e.g. if the symbol is an alias, the aliased symbol is returned, or if the symbol is a function, the symbols it calls downstream are returned. +func (ma *MacroAnalyzer) expandSymbol(symbol *symbolRef) ([]symbolCall, error) { + f := ma.fileReader.GetFile(symbol.pkg, symbol.label) + if f == nil { + return nil, fmt.Errorf("unable to find file %s:%s", symbol.pkg, symbol.label) + } + + for _, stmt := range f.Stmt { + switch stmt := stmt.(type) { + case *build.AssignExpr: + if lhsIdent, ok := stmt.LHS.(*build.Ident); ok && lhsIdent.Name == symbol.name { + if rhsIdent, ok := stmt.RHS.(*build.Ident); ok { + return []symbolCall{{ + symbol: &symbolRef{pkg: f.Pkg, label: f.Label, name: rhsIdent.Name}, + line: exprLine(stmt), + }}, nil + } + if fnName := callExprToString(stmt.RHS); fnName != "" { + return []symbolCall{{ + symbol: &symbolRef{pkg: f.Pkg, label: f.Label, name: fnName}, + line: exprLine(stmt), + }}, nil + } + } + case *build.DefStmt: + if stmt.Name == symbol.name { + var calls []symbolCall + build.Walk(stmt, func(x build.Expr, _ []build.Expr) { + if fnName := callExprToString(x); fnName != "" { + calls = append(calls, symbolCall{ + symbol: &symbolRef{pkg: f.Pkg, label: f.Label, name: fnName}, + line: exprLine(x), + }) + } + }) + return calls, nil + } + case *build.LoadStmt: + label := labels.ParseRelative(stmt.Module.Value, f.Pkg) + if label.Repository != "" || label.Target == "" { + continue + } + for i, from := range stmt.From { + if stmt.To[i].Name == symbol.name { + return []symbolCall{{ + symbol: &symbolRef{pkg: label.Package, label: label.Target, name: from.Name}, + line: exprLine(stmt), + }}, nil + } + } + } + } + return nil, nil +} + +// callExprToString converts a callExpr to its "symbol name" +func callExprToString(expr build.Expr) string { + call, ok := expr.(*build.CallExpr) + if !ok { + return "" + } + + if fnIdent, ok := call.X.(*build.Ident); ok { + return fnIdent.Name + } + + // call of the format obj.fn(...), ignores call if anything other than ident.fn(). + if fn, ok := call.X.(*build.DotExpr); ok { + if obj, ok := fn.X.(*build.Ident); ok { + return fmt.Sprintf("%s.%s", obj.Name, fn.Name) + } + } + return "" +} + +// native functions which do not produce targets (https://bazel.build/rules/lib/toplevel/native). +var nativeRuleExceptions = map[string]bool{ + "native.existing_rule": true, + "native.existing_rules": true, + "native.exports_files": true, + "native.glob": true, + "native.module_name": true, + "native.module_version": true, + "native.package_default_visibility": true, + "native.package_group": true, + "native.package_name": true, + "native.package_relative_label": true, + "native.repo_name": true, + "native.repository_name": true, + "native.subpackages": true, +} + +// producesTargets returns true if the symbol name is a known generator of a target. +func producesTarget(s *symbolRef) bool { + // Calls to the macro() symbol produce a symbolic macro (https://bazel.build/extending/macros). + if s.name == "macro" { + return true + } + // Calls to the rule() symbol define a rule (https://bazel.build/extending/rules). + if s.name == "rule" { + return true + } + // Calls to native. invokes native rules (except defined list of native helper functions). + // https://bazel.build/rules/lib/toplevel/native + if strings.HasPrefix(s.name, "native.") { + if _, ok := nativeRuleExceptions[s.name]; !ok { + return true + } + } + return false +} diff --git a/warn/macro_analyzer_test.go b/warn/macro_analyzer_test.go new file mode 100644 index 000000000..a6c5314a9 --- /dev/null +++ b/warn/macro_analyzer_test.go @@ -0,0 +1,280 @@ +/* +Copyright 2025 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package warn + +import ( + "fmt" + "testing" + + "github.com/bazelbuild/buildtools/build" + "github.com/google/go-cmp/cmp" +) + +func mustFindDefStatement(f *build.File, name string) *build.DefStmt { + for _, stmt := range f.Stmt { + if def, ok := stmt.(*build.DefStmt); ok { + if def.Name == name { + return def + } + } + } + panic(fmt.Sprintf("unable to find def statement matching %q", name)) +} + +func mustFindCallExpression(f *build.File, name string) *build.CallExpr { + for _, stmt := range f.Stmt { + if call, ok := stmt.(*build.CallExpr); ok { + if fnIdent, ok := call.X.(*build.Ident); ok && fnIdent.Name == name { + return call + } + } + } + panic(fmt.Sprintf("unable to find call expression matching %q", name)) +} + +func fileReaderWithFiles(fileContents map[string]string) *FileReader { + return NewFileReader(func(filename string) ([]byte, error) { + return []byte(fileContents[filename]), nil + }) +} + +func TestAnalyzeFn(t *testing.T) { + defaultFilename := "BUILD" + defaultPackage := "//package/path" + defaultFilepath := fmt.Sprintf("%s/%s", defaultPackage, defaultFilename) + tests := []struct { + name string + fileContents map[string]string + wantCanProduceTargets bool + wantStackTrace string + }{ + { + name: "non_macro", + fileContents: map[string]string{ + defaultFilepath: ` +def other_function(): + pass + +def test_symbol(): + other_function() +`, + }, + wantCanProduceTargets: false, + wantStackTrace: "", + }, + { + name: "with_infinite_recursion", + fileContents: map[string]string{ + defaultFilepath: ` +def first_function(): + test_symbol() + +def test_symbol(): + first_function() +`, + }, + wantCanProduceTargets: false, + wantStackTrace: "", + }, + { + name: "macro_within_single_file", + fileContents: map[string]string{ + defaultFilepath: ` +macro_def = macro() + +def test_symbol(): + macro_def() +`, + }, + wantCanProduceTargets: true, + wantStackTrace: `//package/path:BUILD:5 macro_def +//package/path:BUILD:2 macro`, + }, + { + name: "macro_through_load_statement", + fileContents: map[string]string{ + "package/other_path/file.bzl": ` +imported_rule = rule() +`, + defaultFilepath: ` +load("//package/other_path:file.bzl", "imported_rule") + +def test_symbol(): + imported_rule() +`, + }, + wantCanProduceTargets: true, + wantStackTrace: `//package/path:BUILD:5 imported_rule +package/other_path:file.bzl:2 imported_rule +package/other_path:file.bzl:2 rule`, + }, + { + name: "with_load_statements_prioritizes_local_statements", + fileContents: map[string]string{ + "package/other_path/file.bzl": ` +imported_rule = rule() +`, + defaultFilepath: ` +load("//package/other_path:file.bzl", "imported_rule") + +def test_symbol(): + imported_rule() + native.cc_library() +`, + }, + wantCanProduceTargets: true, + wantStackTrace: `//package/path:BUILD:6 native.cc_library`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ma := NewMacroAnalyzer(fileReaderWithFiles(tc.fileContents)) + f := ma.fileReader.GetFile(defaultPackage, defaultFilename) + macroDef := mustFindDefStatement(f, "test_symbol") + + report, err := ma.AnalyzeFn(f, macroDef) + + if err != nil { + t.Errorf("Got unexpected error %s", err) + } + if diff := cmp.Diff(tc.wantCanProduceTargets, report.CanProduceTargets()); diff != "" { + t.Errorf("AnalyzeFn.CanProduceTargets returned unexpected diff %s", diff) + } + if diff := cmp.Diff(tc.wantStackTrace, report.PrintableCallStack()); diff != "" { + t.Errorf("AnalyzeFn.PrintableCallStack returned unexpected diff %s", diff) + } + }) + } +} + +func TestAnalyzeFnCall(t *testing.T) { + defaultFilename := "BUILD" + defaultPackage := "//package/path" + defaultFilepath := fmt.Sprintf("%s/%s", defaultPackage, defaultFilename) + tests := []struct { + name string + fileContents map[string]string + wantCanProduceTargets bool + wantStackTrace string + }{ + { + name: "non_macro", + fileContents: map[string]string{ + defaultFilepath: ` +def test_symbol(): + pass + +test_symbol() +`, + }, + wantCanProduceTargets: false, + wantStackTrace: "", + }, + { + name: "with_infinite_recursion", + fileContents: map[string]string{ + defaultFilepath: ` +def test_symbol(): + second_function() + +def second_function(): + test_symbol() + +test_symbol() +`, + }, + wantCanProduceTargets: false, + wantStackTrace: "", + }, + { + name: "macro_within_single_file", + fileContents: map[string]string{ + defaultFilepath: ` +macro_def = macro() + +def test_symbol(): + macro_def() + +test_symbol() +`, + }, + wantCanProduceTargets: true, + wantStackTrace: `//package/path:BUILD:5 macro_def +//package/path:BUILD:2 macro`, + }, + { + name: "macro_through_load_statement", + fileContents: map[string]string{ + "package/other_path/file.bzl": ` +imported_rule = rule() +`, + defaultFilepath: ` +load("//package/other_path:file.bzl", "imported_rule") + +def test_symbol(): + imported_rule() + +test_symbol() +`, + }, + wantCanProduceTargets: true, + wantStackTrace: `//package/path:BUILD:5 imported_rule +package/other_path:file.bzl:2 imported_rule +package/other_path:file.bzl:2 rule`, + }, + { + name: "with_load_statements_prioritizes_local_statements", + fileContents: map[string]string{ + "package/other_path/file.bzl": ` +imported_rule = rule() +`, + defaultFilepath: ` +load("//package/other_path:file.bzl", "imported_rule") + +def test_symbol(): + imported_rule() + macro() + +test_symbol() +`, + }, + wantCanProduceTargets: true, + wantStackTrace: `//package/path:BUILD:6 macro`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ma := NewMacroAnalyzer(fileReaderWithFiles(tc.fileContents)) + f := ma.fileReader.GetFile(defaultPackage, defaultFilename) + call := mustFindCallExpression(f, "test_symbol") + + report, err := ma.AnalyzeFnCall(f, call) + + if err != nil { + t.Errorf("Got unexpected error %s", err) + } + if diff := cmp.Diff(tc.wantCanProduceTargets, report.CanProduceTargets()); diff != "" { + t.Errorf("AnalyzeFn.CanProduceTargets returned unexpected diff %s", diff) + } + if diff := cmp.Diff(tc.wantStackTrace, report.PrintableCallStack()); diff != "" { + t.Errorf("AnalyzeFn.PrintableCallStack returned unexpected diff %s", diff) + } + }) + } +} diff --git a/warn/multifile.go b/warn/multifile.go index 73f5af4e5..174da4eaa 100644 --- a/warn/multifile.go +++ b/warn/multifile.go @@ -38,6 +38,23 @@ func NewFileReader(readFile func(string) ([]byte, error)) *FileReader { } } +// AddFileToCache adds the provided file to the filereader cache. +func (fr *FileReader) AddFileToCache(f *build.File) { + if f != nil { + fr.cache[f.Path] = f + } +} + +// IsCached returns true if the file is present in the cache. +func (fr *FileReader) IsCached(pkg, label string) bool { + filename := label + if pkg != "" { + filename = pkg + "/" + label + } + _, contains := fr.cache[filename] + return contains +} + // retrieveFile reads a Starlark file using only the readFile method // (without using the cache). func (fr *FileReader) retrieveFile(filename string) *build.File { @@ -72,6 +89,6 @@ func (fr *FileReader) GetFile(pkg, label string) *build.File { file.Pkg = pkg file.Label = label } - fr.cache[filename] = file + fr.AddFileToCache(file) return file } diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go index be6000cd1..02f6fd8c5 100644 --- a/warn/warn_bazel.go +++ b/warn/warn_bazel.go @@ -175,26 +175,28 @@ func positionalArgumentsWarning(f *build.File, fileReader *FileReader) (findings if f.Type != build.TypeBuild { return nil } - macroAnalyzer := newMacroAnalyzer(fileReader) - macroAnalyzer.files[f.Pkg+":"+f.Label] = analyzeFile(f) + macroAnalyzer := NewMacroAnalyzer(fileReader) 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 { + report, err := macroAnalyzer.AnalyzeFnCall(f, fnCall) + if err != nil { + // TODO: Analysis errors are simply ignored as buildifier does not currently handle errors. return } - - if macroAnalyzer.IsRuleOrMacro(function{pkg: f.Pkg, filename: f.Label, name: fnIdent.Name}).isRuleOrMacro { + if report.CanProduceTargets() { 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))) +Found call to rule or macro %q with positional arguments. +The function was considered a macro as it may produce targets via calls: +%s +%s`, + report.SelfDescription, report.SelfDescription, report.PrintableCallStack()))) return } } diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go index 9201cea7d..ec089a823 100644 --- a/warn/warn_bazel_test.go +++ b/warn/warn_bazel_test.go @@ -124,14 +124,43 @@ 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.`, + `:6: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax. +Found call to rule or macro "test/package:BUILD:6 my_macro" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:6 my_macro +test/package:BUILD:1 macro`, + `:7: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax. +Found call to rule or macro "test/package:BUILD:7 my_rule" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:7 my_rule +test/package:BUILD:2 rule`, }, scopeBuild) } +func TestPositionalArgumentsWithNestedCalls(t *testing.T) { + checkFindings(t, "positional-args", ` +def macro3(foo, *args, **kwargs): + macro2() + +def macro2(foo, *, name): + macro1() + +def macro1(foo, name, bar): + native.java_library() + +macro3("foo") +`, + []string{`:10: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax. +Found call to rule or macro "test/package:BUILD:10 macro3" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:10 macro3 +test/package:BUILD:2 macro2 +test/package:BUILD:5 macro1 +test/package:BUILD:8 native.java_library`}, + scopeBuild) +} + func TestPositionalArgumentsWarnsWhenCalledInNestedContexts(t *testing.T) { checkFindings(t, "positional-args", ` my_macro = macro() @@ -149,13 +178,25 @@ other_function(foo = my_function(x)) `, []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.`, +Found call to rule or macro "test/package:BUILD:6 my_macro" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:6 my_macro +test/package:BUILD:1 macro`, + `:7: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax. +Found call to rule or macro "test/package:BUILD:7 my_rule" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:7 my_rule +test/package:BUILD:2 rule`, + `:10: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax. +Found call to rule or macro "test/package:BUILD:10 my_macro" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:10 my_macro +test/package:BUILD:1 macro`, + `:11: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax. +Found call to rule or macro "test/package:BUILD:11 my_rule" with positional arguments. +The function was considered a macro as it may produce targets via calls: +test/package:BUILD:11 my_rule +test/package:BUILD:2 rule`, }, scopeBuild) @@ -233,7 +274,7 @@ filegroup( ) some_rule( - arg1 = "normal/path/file.txt", + arg1 = "normal/path/file.txt", arg2 = "/external/repo/file.py", arg3 = ["file1.txt", "/external/another/file.cc"], )`, diff --git a/warn/warn_macro.go b/warn/warn_macro.go index f77c2f3f5..46befa7d1 100644 --- a/warn/warn_macro.go +++ b/warn/warn_macro.go @@ -23,31 +23,8 @@ import ( "strings" "github.com/bazelbuild/buildtools/build" - "github.com/bazelbuild/buildtools/labels" ) -// Internal constant that represents the native module -const nativeModule = "" - -// function represents a function identifier, which is a pair (module name, function name). -type function struct { - pkg string // package where the function is defined - filename string // name of a .bzl file relative to the package - name string // original name of the function -} - -func (f function) label() string { - return f.pkg + ":" + f.filename -} - -// 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 { @@ -59,251 +36,12 @@ func acceptsNameArgument(def *build.DefStmt) bool { return false } -// fileData represents information about rules and functions extracted from a file -type fileData struct { - 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 -// in another file and loaded -func resolveExternal(fn function, externalSymbols map[string]function) function { - if external, ok := externalSymbols[fn.name]; ok { - return external - } - 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, pkg, filename string, externalSymbols map[string]function) 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{pkg, 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{} - } - - 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 - for _, stmt := range f.Stmt { - load, ok := stmt.(*build.LoadStmt) - if !ok { - continue - } - label := labels.ParseRelative(load.Module.Value, f.Pkg) - if label.Repository != "" || label.Target == "" { - continue - } - for i, from := range load.From { - report.loadedSymbols[load.To[i].Name] = function{label.Package, label.Target, from.Name} - } - } - - 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.Pkg, f.Label, rhsIdent.Name}, report.loadedSymbols) - continue - } - - call, ok := stmt.RHS.(*build.CallExpr) - if !ok { - continue - } - if ident, ok := call.X.(*build.Ident); ok { - if ident.Name == "rule" || ident.Name == "macro" { - report.rulesOrMacros[lhsIdent.Name] = true - continue - } - } - case *build.DefStmt: - report.functions[stmt.Name] = getFunCalls(stmt, f.Pkg, f.Label, report.loadedSymbols) - default: - continue - } - } - return report -} - -// functionReport represents the analysis result of a function -type functionReport struct { - 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, -// 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(pkg, label string) fileData { - filename := pkg + ":" + label - 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(pkg, label) - 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) IsRuleOrMacro(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", "package_name", - "repository_name", "exports_files": - // Not a rule - default: - report.isRuleOrMacro = true - } - return - } - - fileData := ma.getFileData(fn.pkg, fn.filename) - - // Check whether fn.name is an alias for another function - if alias, ok := fileData.aliases[fn.name]; ok { - if ma.IsRuleOrMacro(alias).isRuleOrMacro { - report.isRuleOrMacro = true - } - return - } - - // 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 { - 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. - var knownFunCalls, newFunCalls []funCall - for _, fc := range funCalls { - if _, ok := ma.files[fc.function.pkg+":"+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.IsRuleOrMacro(fc.function).isRuleOrMacro { - report.isRuleOrMacro = 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.Pkg+":"+f.Label] = analyzeFile(f) + macroAnalyzer := NewMacroAnalyzer(fileReader) findings := []*LinterFinding{} for _, stmt := range f.Stmt { @@ -316,24 +54,27 @@ func unnamedMacroWarning(f *build.File, fileReader *FileReader) []*LinterFinding continue } - report := macroAnalyzer.IsRuleOrMacro(function{f.Pkg, f.Label, def.Name}) - if !report.isRuleOrMacro { + report, err := macroAnalyzer.AnalyzeFn(f, def) + if err != nil { + // TODO: Analysis errors are simply ignored as buildifier does not currently handle errors. continue } - msg := fmt.Sprintf(`The macro %q should have a keyword argument called "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. + if !report.CanProduceTargets() { + continue + } + msg := fmt.Sprintf(`The macro %q should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +%s By convention, every public macro needs a "name" argument (even if it doesn't use it). This is important for tooling and automation. - * If this function is a helper function that's not supposed to be used outside of this file, - please make it private (e.g. rename it to "_%s"). - * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, report.fc.nameAlias, report.fc.line, def.Name) - } +* If this function is a helper function that's not supposed to be used outside of this file, + please make it private (e.g. rename it to "_%s"). +* Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, + def.Name, + report.PrintableCallStack(), + def.Name) finding := makeLinterFinding(def, msg) finding.End = def.ColonPos findings = append(findings, finding) diff --git a/warn/warn_macro_test.go b/warn/warn_macro_test.go index 13c6ce3ae..b2e856931 100644 --- a/warn/warn_macro_test.go +++ b/warn/warn_macro_test.go @@ -24,7 +24,7 @@ load(":foo.bzl", "foo") my_rule = rule() -def macro(x): +def a_macro(x): foo() my_rule(name = x) @@ -43,12 +43,13 @@ def another_macro(x): [native.cc_library() for i in x] `, []string{ - `5: The macro "macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "my_rule" on line 7.`, - `19: The macro "another_macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "native.cc_library" on line 21.`, + `:5: The macro "a_macro" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:7 my_rule +test/package:test_file.bzl:3 rule`, + `:19: The macro "another_macro" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:21 native.cc_library`, }, scopeBzl) @@ -70,32 +71,22 @@ def macro3(foo, *args, **kwargs): checkFindings(t, "unnamed-macro", ` my_rule = rule() -def macro(name): +def a_macro(name): my_rule(name = name) -alias = macro +alias = a_macro def bad_macro(): for x in y: alias(x) `, []string{ - `8: The macro "bad_macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "alias" on line 10.`, - }, - scopeBzl) - - checkFindings(t, "unnamed-macro", ` -symbolic_macro = macro() - -def bad_macro(): - symbolic_macro(x) -`, - []string{ - `3: The macro "bad_macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "symbolic_macro" on line 4.`, + `:8: The macro "bad_macro" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:10 alias +test/package:test_file.bzl:6 a_macro +test/package:test_file.bzl:4 my_rule +test/package:test_file.bzl:1 rule`, }, scopeBzl) @@ -115,18 +106,25 @@ def macro4(): my_rule() `, []string{ - `3: The macro "macro1" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "my_rule" on line 4.`, - `6: The macro "macro2" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "macro1" on line 7`, - `9: The macro "macro3" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "macro2" on line 10.`, - `12: The macro "macro4" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "my_rule" on line 13.`, + `:3: The macro "macro1" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:4 my_rule +test/package:test_file.bzl:1 rule`, + `:6: The macro "macro2" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:7 macro1 +test/package:test_file.bzl:4 my_rule +test/package:test_file.bzl:1 rule`, + `:9: The macro "macro3" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:10 macro2 +test/package:test_file.bzl:7 macro1 +test/package:test_file.bzl:4 my_rule +test/package:test_file.bzl:1 rule`, + `:12: The macro "macro4" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:13 my_rule +test/package:test_file.bzl:1 rule`, }, scopeBzl) } @@ -137,15 +135,15 @@ func TestUnnamedMacroRecursion(t *testing.T) { checkFindings(t, "unnamed-macro", ` my_rule = rule() -def macro(): - macro() +def a_macro(): + a_macro() `, []string{}, scopeBzl) checkFindings(t, "unnamed-macro", ` my_rule = rule() -def macro(): - macro() +def a_macro(): + a_macro() `, []string{}, scopeBzl) checkFindings(t, "unnamed-macro", ` @@ -172,12 +170,15 @@ def bar(): my_rule() `, []string{ - `3: The macro "foo" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "bar" on line 4.`, - `6: The macro "bar" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "my_rule" on line 8.`, + `:3: The macro "foo" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:4 bar +test/package:test_file.bzl:8 my_rule +test/package:test_file.bzl:1 rule`, + `:6: The macro "bar" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:8 my_rule +test/package:test_file.bzl:1 rule`, }, scopeBzl) } @@ -226,36 +227,48 @@ def not_macro(x): f() `, []string{ - `4: The macro "macro1" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "abc" on line 5. + `:4: The macro "macro1" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:5 abc +test/package:subdir1/foo.bzl:1 bar +test/package:subdir1/foo.bzl:6 foo +test/package:subdir1/foo.bzl:3 native.foo_binary By convention, every public macro needs a "name" argument (even if it doesn't use it). This is important for tooling and automation. - * If this function is a helper function that's not supposed to be used outside of this file, - please make it private (e.g. rename it to "_macro1"). - * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, - `7: The macro "macro2" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "baz" on line 8. +* If this function is a helper function that's not supposed to be used outside of this file, + please make it private (e.g. rename it to "_macro1"). +* Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, + `:7: The macro "macro2" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:8 baz +test/package:subdir2/baz.bzl:2 baz +test/package:subdir2/baz.bzl:7 bar +test/package:subdir1/foo.bzl:2 bar +test/package:subdir1/foo.bzl:6 foo +test/package:subdir1/foo.bzl:3 native.foo_binary By convention, every public macro needs a "name" argument (even if it doesn't use it). This is important for tooling and automation. - * If this function is a helper function that's not supposed to be used outside of this file, - please make it private (e.g. rename it to "_macro2"). - * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, - `10: The macro "macro3" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "qux" on line 11. +* If this function is a helper function that's not supposed to be used outside of this file, + please make it private (e.g. rename it to "_macro2"). +* Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, + `:10: The macro "macro3" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:11 qux +test/package:subdir2/baz.bzl:2 qux +test/package:subdir2/baz.bzl:10 your_rule +test/package:subdir1/foo.bzl:2 my_rule +test/package:subdir1/foo.bzl:8 rule By convention, every public macro needs a "name" argument (even if it doesn't use it). This is important for tooling and automation. - * If this function is a helper function that's not supposed to be used outside of this file, - please make it private (e.g. rename it to "_macro3"). - * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, +* If this function is a helper function that's not supposed to be used outside of this file, + please make it private (e.g. rename it to "_macro3"). +* Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, }, scopeBzl) } @@ -294,14 +307,18 @@ def qux(): load(":foo.bzl", "foo", "baz") load(":bar.bzl", quux = "qux") -def macro(): +def a_macro(): foo() baz() quux() `, []string{ - `4: The macro "macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "quux" on line 7.`, + `:4: The macro "a_macro" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:7 quux +test/package:bar.bzl:2 qux +test/package:bar.bzl:10 quuux +test/package:foo.bzl:2 qux +test/package:foo.bzl:11 native.cc_library`, }, scopeBzl) } @@ -320,12 +337,16 @@ load(":foo.bzl", bar = "foo") my_rule = rule() -def macro(): +def a_macro(): bar() `, []string{ - `5: The macro "macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "bar" on line 6.`, + `:5: The macro "a_macro" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:6 bar +test/package:foo.bzl:1 foo +test/package:foo.bzl:5 some_rule +test/package:test_file.bzl:2 my_rule +test/package:test_file.bzl:3 rule`, }, scopeBzl) } @@ -362,18 +383,23 @@ def macro4(): r = rule() `, []string{ - `6: The macro "macro1" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "a" on line 7.`, - `9: The macro "macro2" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "native.cc_library" on line 11.`, - `13: The macro "macro3" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "a" on line 15.`, - `17: The macro "macro4" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "r" on line 19.`, + `:6: The macro "macro1" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:7 a +:a.bzl:1 a +:a.bzl:1 rule`, + `:9: The macro "macro2" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:11 native.cc_library`, + `:13: The macro "macro3" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:15 a +:a.bzl:1 a +:a.bzl:1 rule`, + `:17: The macro "macro4" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:19 r +test/package:test_file.bzl:21 rule`, }, scopeBzl) if len(fileReaderRequests) == 1 && fileReaderRequests[0] == "a.bzl" { @@ -405,9 +431,13 @@ def macro1(): def macro2(name): baz() `, []string{ - `5: The macro "macro1" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "baz" on line 6.`, + `:5: The macro "macro1" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:6 baz +test/package:test_file.bzl:3 bar +test/package:bar.bzl:1 bar +test/package:bar.bzl:4 my_rule +test/package:bar.bzl:2 rule`, }, scopeBzl) } @@ -418,13 +448,15 @@ my_rule = rule() def _not_macro(x): my_rule(name = x) -def macro(x): +def a_macro(x): _not_macro(x) `, []string{ - `6: The macro "macro" should have a keyword argument called "name". - -It is considered a macro because it calls a rule or another macro "_not_macro" on line 7.`, + `:6: The macro "a_macro" should have a keyword argument called "name". +It is considered a macro as it may produce targets via calls: +test/package:test_file.bzl:7 _not_macro +test/package:test_file.bzl:4 my_rule +test/package:test_file.bzl:1 rule`, }, scopeBzl) } @@ -433,7 +465,7 @@ func TestUnnamedMacroTypeAnnotation(t *testing.T) { checkFindings(t, "unnamed-macro", ` my_rule = rule() -def macro(name: string): +def a_macro(name: string): my_rule(name) `, []string{}, @@ -442,7 +474,7 @@ def macro(name: string): checkFindings(t, "unnamed-macro", ` my_rule = rule() -def macro(name: string = "default"): +def a_macro(name: string = "default"): my_rule(name) `, []string{},