Repository: bazelbuild/buildtools
Pull request #879 (2020-07-24 14:26:45)

Add warning for calls to `provider` without `doc` or `fields`.

laurentlb: (fyi, @alandonovan, @meisterT)

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index ae8bc89..614b933 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -48,6 +48,7 @@ Warning categories supported by buildifier's linter:
   * [`package-on-top`](#package-on-top)
   * [`positional-args`](#positional-args)
   * [`print`](#print)
+  * [`provider-params`](#provider-params)
   * [`redefined-variable`](#redefined-variable)
   * [`repository-name`](#repository-name)
   * [`return-value`](#return-value)
@@ -796,6 +797,44 @@ may never see the warning.
 
 --------------------------------------------------------------------------------
 
+## <a name="provider-params"></a>Calls to 'provider' should specify a list of fields and a documentation
+
+  * Category name: `provider-params`
+  * Automatic fix: no
+
+Calls to `provider` should specify a documentation string and a list of fields:
+
+```python
+ServerAddressInfo = provider(
+    "The address of an HTTP server. Fields are host (string) and port (int).",
+    fields = ["host", "port"])
+```
+
+Fields should also be documented when needed:
+
+```python
+ServerAddressInfo = provider(
+    "The address of an HTTP server.",
+    fields = {
+        "host": "string, e.g. 'example.com'",
+        "port": "int, a TCP port number",
+    })
+``
+
+Note that specifying a list of fields is a breaking change. It is an error if a
+call to the provider uses undeclared fields. If you cannot declare the list of
+fields, you may explicitly set it to None (and explain why in a comment).
+
+```python
+AllInfo = provider("This provider accepts any field.", fields = None)
+
+NoneInfo = provider("This provider cannot have fields.", fields = [])
+```
+
+See the [documentation for providers](https://docs.bazel.build/versions/master/skylark/lib/globals.html#provider).
+
+--------------------------------------------------------------------------------
+
 ## <a name="redefined-variable"></a>Variable has already been defined
 
   * Category name: `redefined-variable`
diff --git a/warn/warn.go b/warn/warn.go
index 44a884f..446540b 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -144,6 +144,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"package-name":              packageNameWarning,
 	"package-on-top":            packageOnTopWarning,
 	"print":                     printWarning,
+	"provider-params":           providerParamsWarning,
 	"redefined-variable":        redefinedVariableWarning,
 	"repository-name":           repositoryNameWarning,
 	"rule-impl-return":          ruleImplReturnWarning,
diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go
index 801e734..9cea53c 100644
--- a/warn/warn_bazel_api.go
+++ b/warn/warn_bazel_api.go
@@ -1008,3 +1008,43 @@ func keywordPositionalParametersWarning(f *build.File) []*LinterFinding {
 
 	return findings
 }
+
+func providerParamsWarning(f *build.File) []*LinterFinding {
+	if f.Type != build.TypeBzl {
+		return nil
+	}
+
+	var findings []*LinterFinding
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		call, ok := isFunctionCall(expr, "provider")
+		if !ok {
+			return
+		}
+
+		_, _, fields := getParam(call.List, "fields")
+		_, _, doc := getParam(call.List, "doc")
+		// doc can also be the first positional argument
+		hasPositional := false
+		if len(call.List) > 0 {
+			if _, ok := call.List[0].(*build.AssignExpr); !ok {
+				hasPositional = true
+			}
+		}
+		msg := ""
+		if fields == nil {
+			msg = "a list of fields"
+		}
+		if doc == nil && !hasPositional {
+			if msg != "" {
+				msg += " and "
+			}
+			msg += "a documentation"
+		}
+		if msg != "" {
+			findings = append(findings, makeLinterFinding(call,
+				`Calls to 'provider' should provide `+msg+`:\n`+
+					`  provider("description", fields = [...])`))
+		}
+	})
+	return findings
+}


Test patch:
diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go
index c4bad0c..6cc9b2b 100644
--- a/warn/warn_bazel_api_test.go
+++ b/warn/warn_bazel_api_test.go
@@ -2,8 +2,9 @@ package warn
 
 import (
 	"fmt"
-	"github.com/bazelbuild/buildtools/tables"
 	"testing"
+
+	"github.com/bazelbuild/buildtools/tables"
 )
 
 func TestAttrConfigurationWarning(t *testing.T) {
@@ -749,3 +750,16 @@ getattr(
 		`:12: Keyword parameter "default" for "getattr" should be positional.`,
 	}, scopeEverywhere)
 }
+
+func TestProvider(t *testing.T) {
+	checkFindings(t, "provider-params", `provider(doc = "doc", fields = [])`, []string{}, scopeBzl)
+	checkFindings(t, "provider-params", `provider("doc", fields = [])`, []string{}, scopeBzl)
+	checkFindings(t, "provider-params", `provider(fields = None, doc = "doc")`, []string{}, scopeBzl)
+
+	checkFindings(t, "provider-params", `provider(fields = [])`,
+		[]string{`1: Calls to 'provider' should provide a documentation`}, scopeBzl)
+	checkFindings(t, "provider-params", `provider(doc = "doc")`,
+		[]string{`1: Calls to 'provider' should provide a list of fields:`}, scopeBzl)
+	checkFindings(t, "provider-params", `p = provider()`,
+		[]string{`1: Calls to 'provider' should provide a list of fields and a documentation:`}, scopeBzl)
+}