[8.0.0] Explicitly ban attribute inheritance from unexported rules/ma…

…cros

It did not and cannot work; I was misled by an incorrect test. (So take the
opportunity to fix the tests.)

Working towards bazelbuild#24066

Cherry-pick of
bazelbuild@372980c

PiperOrigin-RevId: 696954211
Change-Id: Ia205fe4e6686d51248c1389e1cf7b826650908e8

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -418,7 +418,7 @@ public MacroFunctionApi macro(
       Attribute attr = descriptor.build(attrName);
       builder.addAttribute(attr);
     }
-    for (Attribute attr : getInheritedAttrs(inheritAttrs)) {
+    for (Attribute attr : getAttrsOf(inheritAttrs)) {
       String attrName = attr.getName();
       if (attr.isPublic()
           // isDocumented() is false only for generator_* magic attrs (for which isPublic() is true)
@@ -443,20 +443,31 @@ public MacroFunctionApi macro(
         getBzlKeyToken(thread, "Macros"));
   }
 
-  private static ImmutableList<Attribute> getInheritedAttrs(Object inheritAttrs)
-      throws EvalException {
-    if (inheritAttrs == Starlark.NONE) {
+  private static ImmutableList<Attribute> getAttrsOf(Object inheritAttrsArg) throws EvalException {
+    if (inheritAttrsArg == Starlark.NONE) {
       return ImmutableList.of();
-    } else if (inheritAttrs instanceof RuleFunction ruleFunction) {
+    } else if (inheritAttrsArg instanceof RuleFunction ruleFunction) {
+      verifyInheritAttrsArgExportedIfExportable(ruleFunction);
       return ruleFunction.getRuleClass().getAttributes();
-    } else if (inheritAttrs instanceof MacroFunction macroFunction) {
+    } else if (inheritAttrsArg instanceof MacroFunction macroFunction) {
+      verifyInheritAttrsArgExportedIfExportable(macroFunction);
       return macroFunction.getMacroClass().getAttributes().values().asList();
-    } else if (inheritAttrs.equals(COMMON_ATTRIBUTES_NAME)) {
+    } else if (inheritAttrsArg.equals(COMMON_ATTRIBUTES_NAME)) {
       return baseRule.getAttributes();
     }
     throw Starlark.errorf(
         "Invalid 'inherit_attrs' value %s; expected a rule, a macro, or \"common\"",
-        Starlark.repr(inheritAttrs));
+        Starlark.repr(inheritAttrsArg));
+  }
+
+  private static void verifyInheritAttrsArgExportedIfExportable(Object inheritAttrsArg)
+      throws EvalException {
+    // Note that the value of 'inherit_attrs' can be non-exportable (e.g. native rule).
+    if (inheritAttrsArg instanceof StarlarkExportable exportable && !exportable.isExported()) {
+      throw Starlark.errorf(
+          "Invalid 'inherit_attrs' value: a rule or macro callable must be assigned to a global"
+              + " variable in a .bzl file before it can be inherited from");
+    }
   }
 
   private static Symbol<BzlLoadValue.Key> getBzlKeyToken(StarlarkThread thread, String onBehalfOf) {

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java

@@ -277,6 +277,10 @@ site of the rule. Such attributes can be assigned a default value (as in
 <a href="/reference/be/common-definitions#common-attributes">common rule attribute definitions</a>
 used by all Starlark rules.
 
+<p>Note that if the return value of <code>rule()</code> or <code>macro()</code> was not assigned to
+a global variable in a .bzl file, then such a value has not been registered as a rule or macro
+symbol, and therefore cannot be used for <code>inherit_attrs</code>.
+
 <p>By convention, a macro should pass inherited, non-overridden attributes unchanged to the "main"
 rule or macro symbol which the macro is wrapping. Typically, most inherited attributes will not have
 a parameter in the implementation function's parameter list, and will simply be passed via

src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java

@@ -1183,15 +1183,18 @@ def _impl(name, visibility, attr_using_schema_default, attr_using_hardcoded_nonn
     assertContainsEvent(
         """
 attr_using_schema_default is select({Label("//common:some_configsetting"): None, \
-Label("//conditions:default"): None})""");
+Label("//conditions:default"): None})\
+""");
     assertContainsEvent(
         """
 attr_using_hardcoded_nonnull_default is select({Label("//common:some_configsetting"): None, \
-Label("//conditions:default"): None})""");
+Label("//conditions:default"): None})\
+""");
     assertContainsEvent(
         """
 attr_using_hardcoded_null_default is select({Label("//common:some_configsetting"): None, \
-Label("//conditions:default"): None})""");
+Label("//conditions:default"): None})\
+""");
   }
 
   @Test
@@ -1813,29 +1816,24 @@ def _impl(name, visibility, **kwargs):
   public void inheritAttrs_fromExportedStarlarkRule() throws Exception {
     setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
     scratch.file(
-        "pkg/my_rule.bzl",
+        "pkg/my_macro.bzl",
         """
         def _my_rule_impl(ctx):
             pass
 
-        my_rule = rule(
+        _my_rule = rule(
             implementation = _my_rule_impl,
             attrs = {
                 "srcs": attr.label_list(),
             },
         )
-        """);
-    scratch.file(
-        "pkg/my_macro.bzl",
-        """
-        load(":my_rule.bzl", "my_rule")
 
         def _my_macro_impl(name, visibility, **kwargs):
-            my_rule(name = name + "_my_rule", visibility = visibility, **kwargs)
+            _my_rule(name = name + "_my_rule", visibility = visibility, **kwargs)
 
         my_macro = macro(
             implementation = _my_macro_impl,
-            inherit_attrs = my_rule,
+            inherit_attrs = _my_rule,
         )
         """);
     scratch.file(
@@ -1853,27 +1851,29 @@ def _my_macro_impl(name, visibility, **kwargs):
   }
 
   @Test
-  public void inheritAttrs_fromUnexportedStarlarkRule() throws Exception {
+  public void inheritAttrs_fromUnexportedStarlarkRule_fails() throws Exception {
     setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
     scratch.file(
         "pkg/my_macro.bzl",
         """
         def _my_rule_impl(ctx):
             pass
 
-        _my_rule = rule(
-            implementation = _my_rule_impl,
-            attrs = {
-                "srcs": attr.label_list(),
-            },
+        _unexported = struct(
+            rule = rule(
+                implementation = _my_rule_impl,
+                attrs = {
+                    "srcs": attr.label_list(),
+                },
+            ),
         )
 
         def _my_macro_impl(name, visibility, **kwargs):
-            _my_rule(name = name + "_my_rule", visibility = visibility, **kwargs)
+            pass
 
         my_macro = macro(
             implementation = _my_macro_impl,
-            inherit_attrs = _my_rule,
+            inherit_attrs = _unexported.rule,
         )
         """);
     scratch.file(
@@ -1882,41 +1882,35 @@ def _my_macro_impl(name, visibility, **kwargs):
         load(":my_macro.bzl", "my_macro")
         my_macro(name = "abc")
         """);
-    Package pkg = getPackage("pkg");
-    assertPackageNotInError(pkg);
-    assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet())
-        .containsAtLeast("srcs", "tags");
-    assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet())
-        .containsNoneOf("generator_name", "generator_location", "generator_function");
+    reporter.removeHandler(failFastHandler);
+    assertThat(getPackage("pkg")).isNull();
+    assertContainsEvent(
+        "a rule or macro callable must be assigned to a global variable in a .bzl file before it"
+            + " can be inherited from");
   }
 
   @Test
   public void inheritAttrs_fromExportedMacro() throws Exception {
     setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
     scratch.file(
-        "pkg/other_macro.bzl",
+        "pkg/my_macro.bzl",
         """
         def _other_macro_impl(name, visibility, **kwargs):
             pass
 
-        other_macro = macro(
+        _other_macro = macro(
             implementation = _other_macro_impl,
             attrs = {
                 "srcs": attr.label_list(),
             },
         )
-        """);
-    scratch.file(
-        "pkg/my_macro.bzl",
-        """
-        load(":other_macro.bzl", "other_macro")
 
         def _my_macro_impl(name, visibility, **kwargs):
-            other_macro(name = name + "_other_macro", visibility = visibility, **kwargs)
+            _other_macro(name = name + "_other_macro", visibility = visibility, **kwargs)
 
         my_macro = macro(
             implementation = _my_macro_impl,
-            inherit_attrs = other_macro,
+            inherit_attrs = _other_macro,
         )
         """);
     scratch.file(
@@ -1932,27 +1926,29 @@ def _my_macro_impl(name, visibility, **kwargs):
   }
 
   @Test
-  public void inheritAttrs_fromUnexportedMacro() throws Exception {
+  public void inheritAttrs_fromUnexportedMacro_fails() throws Exception {
     setBuildLanguageOptions("--experimental_enable_macro_inherit_attrs");
     scratch.file(
         "pkg/my_macro.bzl",
         """
         def _other_macro_impl(name, visibility, **kwargs):
             pass
 
-        _other_macro = macro(
-            implementation = _other_macro_impl,
-            attrs = {
-                "srcs": attr.label_list(),
-            },
+        _unexported = struct(
+            macro = macro(
+                implementation = _other_macro_impl,
+                attrs = {
+                    "srcs": attr.label_list(),
+                },
+            ),
         )
 
         def _my_macro_impl(name, visibility, **kwargs):
-            _other_macro(name = name + "_other_macro", visibility = visibility, **kwargs)
+            pass
 
         my_macro = macro(
             implementation = _my_macro_impl,
-            inherit_attrs = _other_macro,
+            inherit_attrs = _unexported.macro,
         )
         """);
     scratch.file(
@@ -1961,9 +1957,10 @@ def _my_macro_impl(name, visibility, **kwargs):
         load(":my_macro.bzl", "my_macro")
         my_macro(name = "abc")
         """);
-    Package pkg = getPackage("pkg");
-    assertPackageNotInError(pkg);
-    assertThat(getMacroById(pkg, "abc:1").getMacroClass().getAttributes().keySet())
-        .containsExactly("name", "visibility", "srcs");
+    reporter.removeHandler(failFastHandler);
+    assertThat(getPackage("pkg")).isNull();
+    assertContainsEvent(
+        "a rule or macro callable must be assigned to a global variable in a .bzl file before it"
+            + " can be inherited from");
   }
 }