Repository: bazelbuild/buildtools
Pull request #436 (2018-11-16 13:21:01)

Implement a warning for the default attribute of attr.output


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 3dfdfc0..2732c73 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -545,6 +545,16 @@ modify the semantics of a BUILD file and makes it hard to maintain.
 
 --------------------------------------------------------------------------------
 
+## <a name="attr-output-default"></a>The `default` parameter for `attr.output()`is deprecated
+
+  * Category_name: `attr-output-default`
+  * Flag in Bazel: [`--incompatible_no_output_attr_default`](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#disable-default-parameter-of-output-attributes)
+  * Automatic fix: no
+
+The `default` parameter of `attr.output()` is bug-prone, as two targets of the same rule would be
+unable to exist in the same package under default behavior. Use Starlark macros to specify defaults
+for these attributes instead.
+
 ## <a name="attr-license"></a>`attr.license()` is deprecated and shouldn't be used
 
   * Category_name: `attr-license`
diff --git a/warn/warn.go b/warn/warn.go
index 5d42831..0881a26 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -795,35 +795,36 @@ var RuleWarningMap = map[string]func(f *build.File, pkg string, expr build.Expr)
 
 // FileWarningMap lists the warnings that run on the whole file.
 var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
-	"args-order":         argumentsOrderWarning,
-	"attr-cfg":           attrConfigurationWarning,
-	"attr-non-empty":     attrNonEmptyWarning,
-	"attr-license":       attrLicenseWarning,
-	"attr-single-file":   attrSingleFileWarning,
-	"constant-glob":      constantGlobWarning,
-	"ctx-actions":        ctxActionsWarning,
-	"ctx-args":           contextArgsAPIWarning,
-	"depset-iteration":   depsetIterationWarning,
-	"depset-union":       depsetUnionWarning,
-	"dict-concatenation": dictionaryConcatenationWarning,
-	"duplicated-name":    duplicatedNameWarning,
-	"filetype":           fileTypeWarning,
-	"git-repository":     nativeGitRepositoryWarning,
-	"http-archive":       nativeHTTPArchiveWarning,
-	"integer-division":   integerDivisionWarning,
-	"load":               unusedLoadWarning,
-	"load-on-top":        loadOnTopWarning,
-	"native-build":       nativeInBuildFilesWarning,
-	"native-package":     nativePackageWarning,
-	"no-effect":          noEffectWarning,
-	"output-group":       outputGroupWarning,
-	"package-name":       packageNameWarning,
-	"package-on-top":     packageOnTopWarning,
-	"redefined-variable": redefinedVariableWarning,
-	"repository-name":    repositoryNameWarning,
-	"same-origin-load":   sameOriginLoadWarning,
-	"string-iteration":   stringIterationWarning,
-	"unused-variable":    unusedVariableWarning,
+	"args-order":          argumentsOrderWarning,
+	"attr-cfg":            attrConfigurationWarning,
+	"attr-license":        attrLicenseWarning,
+	"attr-non-empty":      attrNonEmptyWarning,
+	"attr-output-default": attrOutputDefaultWarning,
+	"attr-single-file":    attrSingleFileWarning,
+	"constant-glob":       constantGlobWarning,
+	"ctx-actions":         ctxActionsWarning,
+	"ctx-args":            contextArgsAPIWarning,
+	"depset-iteration":    depsetIterationWarning,
+	"depset-union":        depsetUnionWarning,
+	"dict-concatenation":  dictionaryConcatenationWarning,
+	"duplicated-name":     duplicatedNameWarning,
+	"filetype":            fileTypeWarning,
+	"git-repository":      nativeGitRepositoryWarning,
+	"http-archive":        nativeHTTPArchiveWarning,
+	"integer-division":    integerDivisionWarning,
+	"load":                unusedLoadWarning,
+	"load-on-top":         loadOnTopWarning,
+	"native-build":        nativeInBuildFilesWarning,
+	"native-package":      nativePackageWarning,
+	"no-effect":           noEffectWarning,
+	"output-group":        outputGroupWarning,
+	"package-name":        packageNameWarning,
+	"package-on-top":      packageOnTopWarning,
+	"redefined-variable":  redefinedVariableWarning,
+	"repository-name":     repositoryNameWarning,
+	"same-origin-load":    sameOriginLoadWarning,
+	"string-iteration":    stringIterationWarning,
+	"unused-variable":     unusedVariableWarning,
 }
 
 // DisabledWarning checks if the warning was disabled by a comment.
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index f0ba0ed..4277185 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -489,6 +489,34 @@ func contextArgsAPIWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func attrOutputDefaultWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		// Find nodes that match the following pattern: attr.output(..., default = ...)
+		call, ok := expr.(*build.CallExpr)
+		if !ok {
+			return
+		}
+		dot, ok := (call.X).(*build.DotExpr)
+		if !ok || dot.Name != "output" {
+			return
+		}
+		base, ok := dot.X.(*build.Ident)
+		if !ok || base.Name != "attr" {
+			return
+		}
+		_, _, param := getParam(call.List, "default")
+		if param == nil {
+			return
+		}
+		start, end := param.Span()
+		findings = append(findings,
+			makeFinding(f, start, end, "attr-output-default",
+				`The "default" parameter for attr.output() is deprecated.`, true, nil))
+	})
+	return findings
+}
+
 func attrLicenseWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 	build.Walk(f, func(expr build.Expr, stack []build.Expr) {


Test patch:
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index 6e80e14..574766d 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -368,6 +368,22 @@ def impl(ctx):
 		scopeEverywhere)
 }
 
+func TestAttrOutputDefault(t *testing.T) {
+	checkFindings(t, "attr-output-default", `
+rule(
+  attrs = {
+      "foo": attr.output(default="foo"),
+      "bar": attr.output(not_default="foo"),
+      "baz": attr.string(default="foo"),
+  }
+)
+`,
+		[]string{
+			`:3: The "default" parameter for attr.output() is deprecated.`,
+		},
+		scopeEverywhere)
+}
+
 func TestAttrLicense(t *testing.T) {
 	checkFindings(t, "attr-license", `
 rule(