Repository: bazelbuild/buildtools
Pull request #910 (2020-09-21 17:15:45)

Update the warning message for unnamed macros

laurentlb: You could also expand the documentation (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#unnamed-macro). For example, add examples.

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 614b933..ea01d7c 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -961,16 +961,21 @@ can potentially be empty.
 
 --------------------------------------------------------------------------------
 
-## <a name="unnamed-macro"></a>By convention the macro should have a keyword argument called "name".
+## <a name="unnamed-macro"></a>The macro should have a keyword argument called "name".
 
   * Category name: `unnamed-macro`
   * Automatic fix: no
 
-By convention all macro functions should have a keyword argument called "name".
+By convention all macro functions should have a keyword argument called `name`
+(even if they don't use it). This is important for tooling and automation.
 
 A macro is a function that calls a rule (either directly or indirectly by calling other
 macros).
 
+If this function is a helper function that's not supposed to be used outside of its file,
+please make it private (rename it so that the name starts with `_`), this will
+prevent loading the function from BUILD files and suppress the warning.
+
 --------------------------------------------------------------------------------
 
 ## <a name="unreachable"></a>The statement is unreachable
diff --git a/warn/warn_macro.go b/warn/warn_macro.go
index dcb7d65..2bc4a99 100644
--- a/warn/warn_macro.go
+++ b/warn/warn_macro.go
@@ -320,12 +320,19 @@ func unnamedMacroWarning(f *build.File, fileReader *FileReader) []*LinterFinding
 		if !report.isMacro {
 			continue
 		}
-		msg := fmt.Sprintf(`By convention %q should have a keyword argument called "name".`, def.Name)
+		msg := fmt.Sprintf(`The macro %q should have a keyword argument called "name".`, def.Name)
 		if report.fc != nil {
 			// fc shouldn't be nil because that's the only node that can be found inside a function.
 			msg += fmt.Sprintf(`
 
-It is considered a macro because it calls a rule or another macro %q on line %d.`, report.fc.nameAlias, report.fc.line)
+It is considered a macro because it calls a rule or another macro %q on line %d.
+
+By convention, every public macro needs a "name" argument (even if it doesn't use it).
+This is important for tooling and automation.
+
+  * If this function is a helper function that's not supposed to be used outside of this file,
+    please make it private (e.g. rename it to "_%s").
+  * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`, report.fc.nameAlias, report.fc.line, def.Name)
 		}
 		finding := makeLinterFinding(def, msg)
 		finding.End = def.ColonPos


Test patch:
diff --git a/warn/warn_macro_test.go b/warn/warn_macro_test.go
index b3bb4dd..010cef0 100644
--- a/warn/warn_macro_test.go
+++ b/warn/warn_macro_test.go
@@ -43,10 +43,10 @@ def another_macro(x):
   [native.cc_library() for i in x]
 `,
 		[]string{
-			`5: By convention "macro" should have a keyword argument called "name".
+			`5: The macro "macro" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "my_rule" on line 7.`,
-			`19: By convention "another_macro" should have a keyword argument called "name".
+			`19: The macro "another_macro" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "native.cc_library" on line 21.`,
 		},
@@ -80,7 +80,7 @@ def bad_macro():
     alias(x)
 `,
 		[]string{
-			`8: By convention "bad_macro" should have a keyword argument called "name".
+			`8: The macro "bad_macro" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "alias" on line 10.`,
 		},
@@ -102,16 +102,16 @@ def macro4():
   my_rule()
 `,
 		[]string{
-			`3: By convention "macro1" should have a keyword argument called "name".
+			`3: The macro "macro1" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "my_rule" on line 4.`,
-			`6: By convention "macro2" should have a keyword argument called "name".
+			`6: The macro "macro2" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "macro1" on line 7`,
-			`9: By convention "macro3" should have a keyword argument called "name".
+			`9: The macro "macro3" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "macro2" on line 10.`,
-			`12: By convention "macro4" should have a keyword argument called "name".
+			`12: The macro "macro4" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "my_rule" on line 13.`,
 		},
@@ -159,10 +159,10 @@ def bar():
   my_rule()
 `,
 		[]string{
-			`3: By convention "foo" should have a keyword argument called "name".
+			`3: The macro "foo" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "bar" on line 4.`,
-			`6: By convention "bar" should have a keyword argument called "name".
+			`6: The macro "bar" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "my_rule" on line 8.`,
 		},
@@ -213,15 +213,36 @@ def not_macro(x):
   f()
 `,
 		[]string{
-			`4: By convention "macro1" should have a keyword argument called "name".
+			`4: The macro "macro1" should have a keyword argument called "name".
 
-It is considered a macro because it calls a rule or another macro "abc" on line 5.`,
-			`7: By convention "macro2" should have a keyword argument called "name".
+It is considered a macro because it calls a rule or another macro "abc" on line 5.
 
-It is considered a macro because it calls a rule or another macro "baz" on line 8.`,
-			`10: By convention "macro3" should have a keyword argument called "name".
+By convention, every public macro needs a "name" argument (even if it doesn't use it).
+This is important for tooling and automation.
 
-It is considered a macro because it calls a rule or another macro "qux" on line 11.`,
+  * If this function is a helper function that's not supposed to be used outside of this file,
+    please make it private (e.g. rename it to "_macro1").
+  * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`,
+			`7: The macro "macro2" should have a keyword argument called "name".
+
+It is considered a macro because it calls a rule or another macro "baz" on line 8.
+
+By convention, every public macro needs a "name" argument (even if it doesn't use it).
+This is important for tooling and automation.
+
+  * If this function is a helper function that's not supposed to be used outside of this file,
+    please make it private (e.g. rename it to "_macro2").
+  * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`,
+			`10: The macro "macro3" should have a keyword argument called "name".
+
+It is considered a macro because it calls a rule or another macro "qux" on line 11.
+
+By convention, every public macro needs a "name" argument (even if it doesn't use it).
+This is important for tooling and automation.
+
+  * If this function is a helper function that's not supposed to be used outside of this file,
+    please make it private (e.g. rename it to "_macro3").
+  * Otherwise, add a "name" argument. If possible, use that name when calling other macros/rules.`,
 		},
 		scopeBzl)
 }
@@ -265,7 +286,7 @@ def macro():
   baz()
   quux()
 `, []string{
-		`4: By convention "macro" should have a keyword argument called "name".
+		`4: The macro "macro" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "quux" on line 7.`,
 	}, scopeBzl)
@@ -289,7 +310,7 @@ my_rule = rule()
 def macro():
   bar()
 `, []string{
-		`5: By convention "macro" should have a keyword argument called "name".
+		`5: The macro "macro" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "bar" on line 6.`,
 	}, scopeBzl)
@@ -328,16 +349,16 @@ def macro4():
 
 r = rule()
 `, []string{
-		`6: By convention "macro1" should have a keyword argument called "name".
+		`6: The macro "macro1" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "a" on line 7.`,
-		`9: By convention "macro2" should have a keyword argument called "name".
+		`9: The macro "macro2" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "native.cc_library" on line 11.`,
-		`13: By convention "macro3" should have a keyword argument called "name".
+		`13: The macro "macro3" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "a" on line 15.`,
-		`17: By convention "macro4" should have a keyword argument called "name".
+		`17: The macro "macro4" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "r" on line 19.`,
 	}, scopeBzl)
@@ -371,7 +392,7 @@ def macro1():
 def macro2(name):
   baz()
 `, []string{
-		`5: By convention "macro1" should have a keyword argument called "name".
+		`5: The macro "macro1" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "baz" on line 6.`,
 	}, scopeBzl)
@@ -388,7 +409,7 @@ def macro(x):
   _not_macro(x)
 `,
 		[]string{
-			`6: By convention "macro" should have a keyword argument called "name".
+			`6: The macro "macro" should have a keyword argument called "name".
 
 It is considered a macro because it calls a rule or another macro "_not_macro" on line 7.`,
 		},