Repository: bazelbuild/buildtools
Pull request #518 (2019-01-15 18:48:16)

Warn about old-style function implementation function returns
If a function is used as a rule implementation, it's recommended that it doesn't return old style structs.


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index c897f9a..4f62242 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -31,6 +31,7 @@
   * [redefined-variable](#redefined-variable)
   * [repository-name](#repository-name)
   * [return-value](#return-value)
+  * [rule-impl-return](#rule-impl-return)
   * [same-origin-load](#same-origin-load)
   * [string-iteration](#string-iteration)
   * [unreachable](#unreachable)
@@ -587,6 +588,19 @@ know certain parts of the code cannot be reached, add the statement
 
 --------------------------------------------------------------------------------
 
+## <a name="rule-impl-return"></a>Avoid using the legacy provider syntax
+
+  * Category_name: `rule-impl-return`
+  * Automatic fix: no
+
+Returning structs from rule implementation functions is
+<a href="https://docs.bazel.build/versions/master/skylark/rules.html#migrating-from-legacy-providers">deprecated</a>,
+consider using
+<a href="https://docs.bazel.build/versions/master/skylark/rules.html#providers">providers</a>
+or lists of providers instead.
+
+--------------------------------------------------------------------------------
+
 ## <a name="same-origin-load"></a>Same label is used for multiple loads
 
   * Category_name: `same-origin-load`
diff --git a/warn/warn.go b/warn/warn.go
index e751942..1978cbd 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -1130,6 +1130,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"package-on-top":      packageOnTopWarning,
 	"redefined-variable":  redefinedVariableWarning,
 	"repository-name":     repositoryNameWarning,
+	"rule-impl-return":    ruleImplReturnWarning,
 	"same-origin-load":    sameOriginLoadWarning,
 	"string-iteration":    stringIterationWarning,
 	"unreachable":         unreachableStatementWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index 7232e32..3da3b10 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -61,6 +61,18 @@ func getParam(attrs []build.Expr, paramName string) (int, *build.Ident, *build.B
 	return -1, nil, nil
 }
 
+// isFunctionCall checks whether expr is a call of a function with a given name
+func isFunctionCall(expr build.Expr, name string) (*build.CallExpr, bool) {
+	call, ok := expr.(*build.CallExpr)
+	if !ok {
+		return nil, false
+	}
+	if ident, ok := call.X.(*build.Ident); ok && ident.Name == name {
+		return call, true
+	}
+	return nil, false
+}
+
 // globalVariableUsageCheck checks whether there's a usage of a given global variable in the file.
 // It's ok to shadow the name with a local variable and use it.
 func globalVariableUsageCheck(f *build.File, category, global, alternative string, fix bool) []*Finding {
@@ -541,3 +553,54 @@ func attrLicenseWarning(f *build.File, fix bool) []*Finding {
 	})
 	return findings
 }
+
+// ruleImplReturnWarning checks whether a rule implementation function returns an old-style struct
+func ruleImplReturnWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	// iterate over rules and collect rule implementation function names
+	implNames := make(map[string]bool)
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		call, ok := isFunctionCall(expr, "rule")
+		if !ok {
+			return
+		}
+
+		// Try to get the implementaton parameter either by name or as the first argument
+		var impl build.Expr
+		_, _, param := getParam(call.List, "implementation")
+		if param != nil {
+			impl = param.Y
+		} else if len(call.List) > 0 {
+			impl = call.List[0]
+		}
+		if name, ok := impl.(*build.Ident); ok {
+			implNames[name.Name] = true
+		}
+	})
+
+	// iterate over functions
+	for _, stmt := range f.Stmt {
+		def, ok := stmt.(*build.DefStmt)
+		if !ok || !implNames[def.Name] {
+			// either not a function or not used in the file as a rule implementation function
+			continue
+		}
+		// traverse the function and find all of its return statements
+		build.Walk(def, func(expr build.Expr, stack []build.Expr) {
+			ret, ok := expr.(*build.ReturnStmt)
+			if !ok {
+				return
+			}
+			// check whether it returns a struct
+			if _, ok := isFunctionCall(ret.Result, "struct"); ok {
+				start, end := ret.Span()
+				findings = append(findings,
+					makeFinding(f, start, end, "rule-impl-return",
+						`Avoid using the legacy provider syntax.`, true, nil))
+			}
+		})
+	}
+
+	return findings
+}


Test patch:
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index 574766d..a53544c 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -398,3 +398,60 @@ rule(
 		`:4: "attr.license()" is deprecated and shouldn't be used.`,
 	}, scopeEverywhere)
 }
+
+func TestRuleImplReturn(t *testing.T) {
+	checkFindings(t, "rule-impl-return", `
+def _impl(ctx):
+  return struct()
+
+rule(implementation=_impl)
+`, []string{
+		`:2: Avoid using the legacy provider syntax.`,
+	}, scopeEverywhere)
+
+	checkFindings(t, "rule-impl-return", `
+def _impl(ctx):
+  if True:
+    return struct()
+  return
+
+x = rule(_impl, attrs = {})
+`, []string{
+		`:3: Avoid using the legacy provider syntax.`,
+	}, scopeEverywhere)
+
+	checkFindings(t, "rule-impl-return", `
+def _impl(ctx):
+  pass  # no return statements
+
+x = rule(_impl, attrs = {})
+`, []string{}, scopeEverywhere)
+
+	checkFindings(t, "rule-impl-return", `
+def _impl1():  # not used as a rule implementation function
+  return struct()
+
+def _impl2():  # no structs returned
+  if x:
+    return []
+  elif y:
+    return foo()
+  return
+
+x = rule(
+  implementation=_impl2,
+)
+
+rule(
+  _impl3,  # not defined here
+)
+
+rule(
+  _impl1(),  # not an identifier
+)
+
+rule()  # no parameters
+rule(foo = bar)  # no matching parameters
+`, []string{
+	}, scopeEverywhere)
+}