Repository: bazelbuild/buildtools
Pull request #404 (2018-10-30 17:09:09)

Warn and fix for the output_group field


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 9d7473d..565350d 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -283,3 +283,10 @@ instead.
 The function `FileType` is [deprecated](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#filetype-is-deprecated).
 Instead of using it as an argument to the [`rule` function](https://docs.bazel.build/versions/master/skylark/lib/globals.html#rule)
 just use a list of strings.
+
+--------------------------------------------------------------------------------
+
+## <a name="output-group"></a>`ctx.attr.dep.output_group` is deprecated
+
+The `output_group` field of a target is [deprecated](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#disable-output-group-field-on-target)
+in favor of the [`OutputGroupInfo` provider](https://docs.bazel.build/versions/master/skylark/lib/OutputGroupInfo.html).
diff --git a/build/print.go b/build/print.go
index fc371df..cb964a1 100644
--- a/build/print.go
+++ b/build/print.go
@@ -276,6 +276,17 @@ func isFunctionDefinition(x Expr) bool {
 	}
 }
 
+// isDifferentLines reports whether two positions belong to different lines.
+// If one of the positions is null (Line == 0), it's not a real position but probably an indicator
+// of manually inserted node. Return false in this case
+func isDifferentLines(p1, p2 Position) bool {
+	if p1.Line == 0 || p2.Line == 0 {
+		return false
+	}
+	return p1.Line != p2.Line
+}
+
+
 // Expression formatting.
 
 // The expression formatter must introduce parentheses to force the
@@ -420,7 +431,7 @@ func (p *printer) expr(v Expr, outerPrec int) {
 		addParen(precSuffix)
 		p.expr(v.X, precSuffix)
 		_, xEnd := v.X.Span()
-		isMultiline := v.NamePos.Line > xEnd.Line
+		isMultiline := isDifferentLines(v.NamePos, xEnd)
 		if isMultiline {
 			p.margin += listIndentation
 			p.breakline()
diff --git a/warn/warn.go b/warn/warn.go
index 3d9f22d..27a3abd 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -450,6 +450,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"load":               unusedLoadWarning,
 	"load-on-top":        loadOnTopWarning,
 	"no-effect":          noEffectWarning,
+	"output-group":       outputGroupWarning,
 	"package-name":       packageNameWarning,
 	"package-on-top":     packageOnTopWarning,
 	"redefined-variable": redefinedVariableWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index 68a5dc1..c85df50 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -295,3 +295,39 @@ func packageNameWarning(f *build.File, fix bool) []*Finding {
 func repositoryNameWarning(f *build.File, fix bool) []*Finding {
 	return globalVariableUsageCheck(f, "repository-name", "REPOSITORY_NAME", "native.repository_name()", fix)
 }
+
+func outputGroupWarning(f* build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+	build.Edit(f, func(expr build.Expr, stack []build.Expr) build.Expr {
+		// Find nodes that match the following pattern: ctx.attr.xxx.output_group
+		outputGroup, ok := (expr).(*build.DotExpr)
+		if !ok || outputGroup.Name != "output_group"{
+			return nil
+		}
+		dep, ok := (outputGroup.X).(*build.DotExpr)
+		if !ok {
+			return nil
+		}
+		attr, ok := (dep.X).(*build.DotExpr)
+		if !ok || attr.Name != "attr" {
+			return nil
+		}
+		ctx, ok := (attr.X).(*build.Ident)
+		if !ok || ctx.Name != "ctx" {
+			return nil
+		}
+		if !fix {
+			start, end := outputGroup.Span()
+			findings = append(findings,
+				makeFinding(f, start, end, "output-group",
+					"\"ctx.attr.dep.output_group\" is deprecated in favor of \"ctx.attr.dep[OutputGroupInfo]\".", true, nil))
+			return nil
+		}
+		// Replace `xxx.output_group` with `xxx[OutputGroupInfo]`
+		return &build.IndexExpr{
+			X: dep,
+			Y: &build.Ident{Name: "OutputGroupInfo"},
+		}
+	})
+	return findings
+}
\ No newline at end of file


Test patch:
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index 9cc83f4..667685c 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -192,7 +192,7 @@ foo(a = REPOSITORY_NAME)
 `, []string{}, false)
 }
 
-func TestFilTypeNameWarning(t *testing.T) {
+func TestFileTypeNameWarning(t *testing.T) {
 	checkFindings(t, "filetype", `
 rule1(types=FileType([".cc", ".h"]))
 rule2(types=FileType(types=[".cc", ".h"]))
@@ -219,3 +219,17 @@ rule1(types=FileType([".cc", ".h"]))
 rule2(types=FileType(types=[".cc", ".h"]))
 `, []string{}, false)
 }
+
+func TestOutputGroupWarning(t *testing.T) {
+	checkFindingsAndFix(t, "output-group", `
+def _impl(ctx):
+    bin = ctx.attr.my_dep.output_group.bin
+`, `
+def _impl(ctx):
+    bin = ctx.attr.my_dep[OutputGroupInfo].bin
+`,
+		[]string{
+			":2: \"ctx.attr.dep.output_group\" is deprecated in favor of \"ctx.attr.dep[OutputGroupInfo]\".",
+		},
+		false)
+}
\ No newline at end of file