Export rule class with 'name' if provided to 'rule()'

https://docs.google.com/document/d/12sC92mq7WasTvDWsm4U782oCegitfi1jbgOnMIGCe-A/edit

PiperOrigin-RevId: 391720461

Author: hvadehra

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

@@ -54,6 +54,8 @@
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.events.EventKind;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.AllowlistChecker;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
@@ -299,6 +301,7 @@ public StarlarkCallable rule(
       Object cfg,
       Object execGroups,
       Object compileOneFiletype,
+      Object name,
       StarlarkThread thread)
       throws EvalException {
     BazelStarlarkContext bazelContext = BazelStarlarkContext.from(thread);
@@ -461,7 +464,36 @@ public StarlarkCallable rule(
       builder.setPreferredDependencyPredicate(FileType.of((String) compileOneFiletype));
     }
 
-    return new StarlarkRuleFunction(builder, type, attributes, thread.getCallerLocation());
+    StarlarkRuleFunction starlarkRuleFunction =
+        new StarlarkRuleFunction(builder, type, attributes, thread.getCallerLocation());
+    // If a name= parameter is supplied (and we're currently initializing a .bzl module), export the
+    // rule immediately under that name; otherwise the rule will be exported by the postAssignHook
+    // set up in BzlLoadFunction.
+    //
+    // Because exporting can raise multiple errors, we need to accumulate them here into a single
+    // EvalException. This is a code smell because any non-ERROR events will be lost, and any
+    // location
+    // information in the events will be overwritten by the location of this rule's definition.
+    // However, this is currently fine because StarlarkRuleFunction#export only creates events that
+    // are ERRORs and that have the rule definition as their location.
+    // TODO(brandjon): Instead of accumulating events here, consider registering the rule in the
+    // BazelStarlarkContext, and exporting such rules after module evaluation in
+    // BzlLoadFunction#execAndExport.
+    if (name != Starlark.NONE && bzlModule != null) {
+      StoredEventHandler handler = new StoredEventHandler();
+      starlarkRuleFunction.export(handler, bzlModule.label(), (String) name);
+      if (handler.hasErrors()) {
+        StringBuilder errors =
+            handler.getEvents().stream()
+                .filter(e -> e.getKind() == EventKind.ERROR)
+                .reduce(
+                    new StringBuilder(),
+                    (sb, ev) -> sb.append("\n").append(ev.getMessage()),
+                    StringBuilder::append);
+        throw Starlark.errorf("Errors in exporting %s: %s", name, errors.toString());
+      }
+    }
+    return starlarkRuleFunction;
   }
 
   /**
@@ -756,6 +788,9 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
     }
 
     /** Export a RuleFunction from a Starlark file with a given name. */
+    // To avoid losing event information in the case where the rule was defined with an explicit
+    // name= arg, all events should be created using errorf(). See the comment in rule() above for
+    // details.
     @Override
     public void export(EventHandler handler, Label starlarkLabel, String ruleClassName) {
       Preconditions.checkState(ruleClass == null && builder != null);

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

@@ -352,6 +352,24 @@ public interface StarlarkRuleFunctionsApi<FileApiT extends FileApi> {
             doc =
                 "Used by --compile_one_dependency: if multiple rules consume the specified file, "
                     + "should we choose this rule over others."),
+        @Param(
+            name = "name",
+            named = true,
+            defaultValue = "None",
+            positional = false,
+            doc =
+                "The name of this rule, as understood by Bazel and reported in contexts such as"
+                    + " logging, <code>native.existing_rule(...)[kind]</code>, and <code>bazel"
+                    + " query</code>. Usually this is the same as the Starlark identifier that gets"
+                    + " bound to this rule; for instance a rule called <code>foo_library</code>"
+                    + " would typically be declared as <code>foo_library = rule(...)</code> and"
+                    + " instantiated in a BUILD file as <code>foo_library(...)</code>.<p>If this"
+                    + " parameter is omitted, the rule's name is set to the name of the first"
+                    + " Starlark global variable to be bound to this rule within its declaring .bzl"
+                    + " module. Thus, <code>foo_library = rule(...)</code> need not specify this"
+                    + " parameter if the name is <code>foo_library</code>.<p>Specifying an explicit"
+                    + " name for a rule does not change where you are allowed to instantiate the"
+                    + " rule."),
       },
       useStarlarkThread = true)
   StarlarkCallable rule(
@@ -374,6 +392,7 @@ StarlarkCallable rule(
       Object cfg,
       Object execGroups,
       Object compileOneFiletype,
+      Object name,
       StarlarkThread thread)
       throws EvalException;
 

src/main/java/com/google/devtools/build/skydoc/SkydocMain.java

@@ -279,8 +279,12 @@ public Module eval(
     for (Entry<String, Object> envEntry : sortedBindings.entrySet()) {
       if (ruleFunctions.containsKey(envEntry.getValue())) {
         RuleInfo.Builder ruleInfoBuild = ruleFunctions.get(envEntry.getValue()).getRuleInfo();
-        RuleInfo ruleInfo = ruleInfoBuild.setRuleName(envEntry.getKey()).build();
-        ruleInfoMap.put(envEntry.getKey(), ruleInfo);
+        // Use symbol name as the rule name only if not already set in the call to rule()
+        if ("".equals(ruleInfoBuild.getRuleName())) {
+          ruleInfoBuild.setRuleName(envEntry.getKey());
+        }
+        RuleInfo ruleInfo = ruleInfoBuild.build();
+        ruleInfoMap.put(ruleInfo.getRuleName(), ruleInfo);
       }
       if (providerInfos.containsKey(envEntry.getValue())) {
         ProviderInfo.Builder providerInfoBuild =

src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java

@@ -137,6 +137,7 @@ public StarlarkCallable rule(
       Object cfg,
       Object execGroups,
       Object compileOneFiletype,
+      Object name,
       StarlarkThread thread)
       throws EvalException {
     ImmutableMap.Builder<String, FakeDescriptor> attrsMapBuilder = ImmutableMap.builder();
@@ -154,9 +155,11 @@ public StarlarkCallable rule(
 
     RuleDefinitionIdentifier functionIdentifier = new RuleDefinitionIdentifier();
 
-    // Only the Builder is passed to RuleInfoWrapper as the rule name is not yet available.
+    // Only the Builder is passed to RuleInfoWrapper as the rule name may not be available yet.
     RuleInfo.Builder ruleInfo = RuleInfo.newBuilder().setDocString(doc).addAllAttribute(attrInfos);
-
+    if (name != Starlark.NONE) {
+      ruleInfo.setRuleName((String) name);
+    }
     Location loc = thread.getCallerLocation();
     ruleInfoList.add(new RuleInfoWrapper(functionIdentifier, loc, ruleInfo));
 

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java

@@ -1645,6 +1645,57 @@ public void testExportAliasedName() throws Exception {
     assertThat(fooName).isEqualTo("d");
   }
 
+  @Test
+  public void testExportWithSpecifiedName() throws Exception {
+    evalAndExport(
+        ev, //
+        "def _impl(ctx): pass",
+        "a = rule(implementation = _impl, name = 'r')",
+        "z = a");
+
+    String aName = ((StarlarkRuleFunction) ev.lookup("a")).getRuleClass().getName();
+    assertThat(aName).isEqualTo("r");
+    String zName = ((StarlarkRuleFunction) ev.lookup("z")).getRuleClass().getName();
+    assertThat(zName).isEqualTo("r");
+  }
+
+  @Test
+  public void testExportWithSpecifiedNameFailure() throws Exception {
+    ev.setFailFast(false);
+
+    evalAndExport(
+        ev, //
+        "def _impl(ctx): pass",
+        "rule(implementation = _impl, name = '1a')");
+
+    ev.assertContainsError("Invalid rule name: 1a");
+  }
+
+  @Test
+  public void testExportWithMultipleErrors() throws Exception {
+    ev.setFailFast(false);
+
+    evalAndExport(
+        ev,
+        "def _impl(ctx): pass",
+        "rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'name' : attr.string(),",
+        "    'tags' : attr.string_list(),",
+        "  },",
+        "  name = '1a',",
+        ")");
+
+    ev.assertContainsError(
+        "Error in rule: Errors in exporting 1a: \n"
+            + "cannot add attribute: There is already a built-in attribute 'name' which cannot be"
+            + " overridden.\n"
+            + "cannot add attribute: There is already a built-in attribute 'tags' which cannot be"
+            + " overridden.\n"
+            + "Invalid rule name: 1a");
+  }
+
   @Test
   public void testOutputToGenfiles() throws Exception {
     evalAndExport(ev, "def impl(ctx): pass", "r1 = rule(impl, output_to_genfiles=True)");

src/test/java/com/google/devtools/build/skydoc/SkydocTest.java

@@ -220,6 +220,60 @@ public void testMultipleRuleNames() throws Exception {
     assertThat(ruleInfoMap.build().keySet()).containsExactly("rule_one", "rule_two");
   }
 
+  @Test
+  public void testRuleExportedWithSpecifiedName() throws Exception {
+    scratch.file(
+        "/execroot/io_bazel/test/test.bzl",
+        "def rule_impl(ctx):",
+        "  return []",
+        "",
+        "rule_one = rule(",
+        "    doc = 'Rule one',",
+        "    implementation = rule_impl,",
+        "    name = 'rule_one_exported_name',",
+        ")");
+
+    ImmutableMap.Builder<String, RuleInfo> ruleInfoMap = ImmutableMap.builder();
+
+    skydocMain.eval(
+        StarlarkSemantics.DEFAULT,
+        Label.parseAbsoluteUnchecked("//test:test.bzl"),
+        ruleInfoMap,
+        ImmutableMap.builder(),
+        ImmutableMap.builder(),
+        ImmutableMap.builder(),
+        ImmutableMap.builder());
+
+    assertThat(ruleInfoMap.build().keySet()).containsExactly("rule_one_exported_name");
+  }
+
+  @Test
+  public void testUnassignedRuleNotDocumented() throws Exception {
+    scratch.file(
+        "/execroot/io_bazel/test/test.bzl",
+        "def rule_impl(ctx):",
+        "  return []",
+        "",
+        "rule(",
+        "    doc = 'Undocumented rule',",
+        "    implementation = rule_impl,",
+        "    name = 'rule_exported_name',",
+        ")");
+
+    ImmutableMap.Builder<String, RuleInfo> ruleInfoMap = ImmutableMap.builder();
+
+    skydocMain.eval(
+        StarlarkSemantics.DEFAULT,
+        Label.parseAbsoluteUnchecked("//test:test.bzl"),
+        ruleInfoMap,
+        ImmutableMap.builder(),
+        ImmutableMap.builder(),
+        ImmutableMap.builder(),
+        ImmutableMap.builder());
+
+    assertThat(ruleInfoMap.build().keySet()).isEmpty();
+  }
+
   @Test
   public void testRulesAcrossMultipleFiles() throws Exception {
     scratch.file("/execroot/io_bazel/lib/rule_impl.bzl", "def rule_impl(ctx):", "  return []");