Repository: bazelbuild/buildtools
Pull request #654 (2019-06-17 11:37:08)

Update warning functions in bazel_warn.go to the new signature
This is the first CL in a series, the rest will be submitted separately.

vladmos: There's just one RuleWarning function here, but more are defined internally, so I've created a wrapper that can easilly convert per-rule to per-file functions, that will help avoid some duplicated logic (i.e. check that a file is a collection of rules (BUILD or WORKSPACE), iterate over top-level statements, check whether they are function calls).

Patch:
diff --git a/warn/warn.go b/warn/warn.go
index 8cfd675..bdd2d6e 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -90,15 +90,19 @@ 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(f *build.File, pkg string, expr build.Expr) *Finding{
-	"positional-args": positionalArgumentsWarning,
-}
+var RuleWarningMap = map[string]func(f *build.File, pkg string, expr build.Expr) *Finding{}
 
 // FileWarningMap lists the warnings that run on the whole file.
 var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
-	"attr-cfg":     attrConfigurationWarning,
-	"attr-license": attrLicenseWarning,
-	"print":        printWarning,
+	"attr-cfg":          attrConfigurationWarning,
+	"attr-license":      attrLicenseWarning,
+	"build-args-kwargs": argsKwargsInBuildFilesWarning,
+	"constant-glob":     constantGlobWarning,
+	"duplicated-name":   duplicatedNameWarning,
+	"native-build":      nativeInBuildFilesWarning,
+	"native-package":    nativePackageWarning,
+	"positional-args":   RuleWarning(positionalArgumentsWarning),
+	"print":             printWarning,
 }
 
 // LegacyFileWarningMap lists the warnings that run on the whole file with legacy interface.
@@ -106,15 +110,12 @@ var LegacyFileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"attr-non-empty":            attrNonEmptyWarning,
 	"attr-output-default":       attrOutputDefaultWarning,
 	"attr-single-file":          attrSingleFileWarning,
-	"build-args-kwargs":         argsKwargsInBuildFilesWarning,
 	"confusing-name":            confusingNameWarning,
-	"constant-glob":             constantGlobWarning,
 	"ctx-actions":               ctxActionsWarning,
 	"ctx-args":                  contextArgsAPIWarning,
 	"depset-iteration":          depsetIterationWarning,
 	"depset-union":              depsetUnionWarning,
 	"dict-concatenation":        dictionaryConcatenationWarning,
-	"duplicated-name":           duplicatedNameWarning,
 	"filetype":                  fileTypeWarning,
 	"function-docstring":        functionDocstringWarning,
 	"function-docstring-header": functionDocstringHeaderWarning,
@@ -129,8 +130,6 @@ var LegacyFileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"module-docstring":          moduleDocstringWarning,
 	"name-conventions":          nameConventionsWarning,
 	"native-android":            nativeAndroidRulesWarning,
-	"native-build":              nativeInBuildFilesWarning,
-	"native-package":            nativePackageWarning,
 	"no-effect":                 noEffectWarning,
 	"out-of-order-load":         outOfOrderLoadWarning,
 	"output-group":              outputGroupWarning,
@@ -154,6 +153,29 @@ var nonDefaultWarnings = map[string]bool{
 	"unsorted-dict-items": true, // dict items should be sorted
 }
 
+// RuleWarning is a wrapper that converts a per-rule function to a per-file function. It also doesn't
+// run on .bzl of default files.
+func RuleWarning(ruleWarning func(call *build.CallExpr) []*LinterFinding) func(f *build.File) []*LinterFinding {
+	return func(f *build.File) []*LinterFinding {
+		if f.Type != build.TypeBuild && f.Type != build.TypeWorkspace {
+			return nil
+		}
+		findings := []*LinterFinding{}
+		for _, stmt := range f.Stmt {
+			switch stmt := stmt.(type) {
+			case *build.CallExpr:
+				findings = append(findings, ruleWarning(stmt)...)
+			case *build.Comprehension:
+				// Rules are often called within list comprehensions, e.g. [my_rule(foo) for foo in bar]
+				if call, ok := stmt.Body.(*build.CallExpr); ok {
+					findings = append(findings, ruleWarning(call)...)
+				}
+			}
+		}
+		return findings
+	}
+}
+
 // DisabledWarning checks if the warning was disabled by a comment.
 // The comment format is buildozer: disable=<warning>
 func DisabledWarning(f *build.File, findingLine int, warning string) bool {
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index bb155e7..a9d9bd0 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -7,24 +7,27 @@ import (
 	"strings"
 
 	"github.com/bazelbuild/buildtools/build"
-	"github.com/bazelbuild/buildtools/edit"
 )
 
-func constantGlobWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
-
+func constantGlobWarning(f *build.File) []*LinterFinding {
 	if f.Type == build.TypeDefault {
 		// Only applicable to Bazel files
-		return findings
+		return nil
 	}
 
-	edit.EditFunction(f, "glob", func(call *build.CallExpr, stk []build.Expr) build.Expr {
-		if len(call.List) == 0 {
-			return nil
+	findings := []*LinterFinding{}
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		call, ok := expr.(*build.CallExpr)
+		if !ok || len(call.List) == 0 {
+			return
+		}
+		ident, ok := (call.X).(*build.Ident)
+		if !ok || ident.Name != "glob" {
+			return
 		}
 		patterns, ok := call.List[0].(*build.ListExpr)
 		if !ok {
-			return nil
+			return
 		}
 		for _, expr := range patterns.List {
 			str, ok := expr.(*build.StringExpr)
@@ -32,60 +35,47 @@ func constantGlobWarning(f *build.File, fix bool) []*Finding {
 				continue
 			}
 			if !strings.Contains(str.Value, "*") {
-				start, end := str.Span()
-				findings = append(findings, makeFinding(f, start, end, "constant-glob",
-					"Glob pattern `"+str.Value+"` has no wildcard ('*'). "+
-						"Constant patterns can be error-prone, move the file outside the glob.", true, nil))
-				return nil // at most one warning per glob
+				message := fmt.Sprintf(
+					`Glob pattern %q has no wildcard ('*'). Constant patterns can be error-prone, move the file outside the glob.`, str.Value)
+				findings = append(findings, makeLinterFinding(expr, message))
+				return // at most one warning per glob
 			}
 		}
-		return nil
 	})
 	return findings
 }
 
-func nativeInBuildFilesWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
-
+func nativeInBuildFilesWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeBuild {
-		return findings
+		return nil
 	}
 
-	build.Edit(f, func(expr build.Expr, stack []build.Expr) build.Expr {
+	findings := []*LinterFinding{}
+	build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) {
 		// Search for `native.xxx` nodes
-		dot, ok := expr.(*build.DotExpr)
+		dot, ok := (*expr).(*build.DotExpr)
 		if !ok {
-			return nil
+			return
 		}
 		ident, ok := dot.X.(*build.Ident)
 		if !ok || ident.Name != "native" {
-			return nil
+			return
 		}
 
-		if fix {
-			start, _ := dot.Span()
-			return &build.Ident{
-				Name:    dot.Name,
-				NamePos: start,
-			}
-		}
-		start, end := expr.Span()
 		findings = append(findings,
-			makeFinding(f, start, end, "native-build",
-				`The "native" module shouldn't be used in BUILD files, its members are available as global symbols.`, true, nil))
-
-		return nil
+			makeLinterFinding(ident,
+				`The "native" module shouldn't be used in BUILD files, its members are available as global symbols.`,
+				LinterReplacement{expr, &build.Ident{Name: dot.Name}}))
 	})
 	return findings
 }
 
-func nativePackageWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
-
+func nativePackageWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeBzl {
-		return findings
+		return nil
 	}
 
+	findings := []*LinterFinding{}
 	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
 		// Search for `native.package()` nodes
 		call, ok := expr.(*build.CallExpr)
@@ -101,22 +91,21 @@ func nativePackageWarning(f *build.File, fix bool) []*Finding {
 			return
 		}
 
-		start, end := expr.Span()
 		findings = append(findings,
-			makeFinding(f, start, end, "native-package",
-				`"native.package()" shouldn't be used in .bzl files.`, true, nil))
+			makeLinterFinding(call, `"native.package()" shouldn't be used in .bzl files.`))
 	})
 	return findings
 }
 
-func duplicatedNameWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func duplicatedNameWarning(f *build.File) []*LinterFinding {
 	if f.Type == build.TypeBzl || f.Type == build.TypeDefault {
 		// Not applicable to .bzl files.
-		return findings
+		return nil
 	}
+
+	findings := []*LinterFinding{}
 	names := make(map[string]int) // map from name to line number
-	msg := `A rule with name "%s" was already found on line %d. ` +
+	msg := `A rule with name %q was already found on line %d. ` +
 		`Even if it's valid for Blaze, this may confuse other tools. ` +
 		`Please rename it and use different names.`
 
@@ -125,13 +114,14 @@ func duplicatedNameWarning(f *build.File, fix bool) []*Finding {
 		if name == "" {
 			continue
 		}
-		start, end := rule.Call.Span()
-		if nameNode := rule.Attr("name"); nameNode != nil {
-			start, end = nameNode.Span()
-		}
+		start, _ := rule.Call.Span()
 		if line, ok := names[name]; ok {
-			findings = append(findings,
-				makeFinding(f, start, end, "duplicated-name", fmt.Sprintf(msg, name, line), true, nil))
+			finding := makeLinterFinding(rule.Call, fmt.Sprintf(msg, name, line))
+			if nameNode := rule.Attr("name"); nameNode != nil {
+				finding.Start, finding.End = nameNode.Span()
+				start = finding.Start
+			}
+			findings = append(findings, finding)
 		} else {
 			names[name] = start.Line
 		}
@@ -139,12 +129,8 @@ func duplicatedNameWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
-func positionalArgumentsWarning(f *build.File, pkg string, stmt build.Expr) *Finding {
+func positionalArgumentsWarning(call *build.CallExpr) []*LinterFinding {
 	msg := "All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax."
-	call, ok := stmt.(*build.CallExpr)
-	if !ok {
-		return nil
-	}
 	if id, ok := call.X.(*build.Ident); !ok || functionsWithPositionalArguments[id.Name] {
 		return nil
 	}
@@ -152,19 +138,17 @@ func positionalArgumentsWarning(f *build.File, pkg string, stmt build.Expr) *Fin
 		if _, ok := arg.(*build.AssignExpr); ok {
 			continue
 		}
-		start, end := arg.Span()
-		return makeFinding(f, start, end, "positional-args", msg, true, nil)
+		return []*LinterFinding{makeLinterFinding(arg, msg)}
 	}
 	return nil
 }
 
-func argsKwargsInBuildFilesWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
-
+func argsKwargsInBuildFilesWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeBuild {
-		return findings
+		return nil
 	}
 
+	findings := []*LinterFinding{}
 	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
 		// Search for function call nodes
 		call, ok := expr.(*build.CallExpr)
@@ -176,16 +160,13 @@ func argsKwargsInBuildFilesWarning(f *build.File, fix bool) []*Finding {
 			if !ok {
 				continue
 			}
-			start, end := param.Span()
 			switch unary.Op {
 			case "*":
 				findings = append(findings,
-					makeFinding(f, start, end, "build-args-kwargs",
-						`*args are not allowed in BUILD files.`, true, nil))
+					makeLinterFinding(param, `*args are not allowed in BUILD files.`))
 			case "**":
 				findings = append(findings,
-					makeFinding(f, start, end, "build-args-kwargs",
-						`**kwargs are not allowed in BUILD files.`, true, nil))
+					makeLinterFinding(param, `**kwargs are not allowed in BUILD files.`))
 			}
 		}
 	})


Test patch:
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index e170610..a04d823 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -12,8 +12,8 @@ cc_library(srcs =
     "test.cpp",
   ])
 )`,
-		[]string{":1: Glob pattern `foo.cc` has no wildcard",
-			":6: Glob pattern `test.cpp` has no wildcard"},
+		[]string{`:1: Glob pattern "foo.cc" has no wildcard`,
+			`:6: Glob pattern "test.cpp" has no wildcard`},
 		scopeBazel)
 }
 
@@ -65,8 +65,13 @@ exports_files(["foo.txt"])
 func TestPositionalArguments(t *testing.T) {
 	checkFindings(t, "positional-args", `
 my_macro(foo = "bar")
-my_macro("foo", "bar")`,
-		[]string{":2: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax."},
+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.",
+		},
 		scopeBuild|scopeWorkspace)
 
 	checkFindings(t, "positional-args", `