Update symbolic macro stardoc protos for visibility, finalizers, and …

…attribute inheritance

Symbolic macros have a mandatory visibility attribute - we need to document it.

To support attribute inheritance in documentation generation, it suffices to
extract non-Starlark-defined attributes in macros, and expose the fact that
they are non-Starlark-defined - so that a documentation renderer can provide
a link to bazel.build common attributes for the natively defined common
attributes that don't have a doc string.

Take the opportunity to refactor addDocumentableAttributes() into a better
shape (taking an explicit argument for the implicitly-added attributes), and
fix the long-standing bug that Stardoc has been inserting a non-existent
"name" attribute for aspects (which was partly obscured by the old
addDocumentableAttributes() implementation).

RELNOTES: Fix starlark_doc_extract proto output for symbolic macro visibility,
attribute inheritance, and rule finalizers; and remove non-existent "name"
attribute from starlark_doc_extract output for aspects.
PiperOrigin-RevId: 700811908
Change-Id: I5486919b9e2af98a7d08f952f8075216c3c60bfd

src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java

@@ -572,7 +572,7 @@ private static class RuleClassInfoFormatter {
     private final ExtractorContext extractorContext =
         ExtractorContext.builder()
             .labelRenderer(LabelRenderer.DEFAULT)
-            .extractNonStarlarkAttrs(true)
+            .extractNativelyDefinedAttrs(true)
             .build();
 
     /**

src/main/java/com/google/devtools/build/lib/starlarkdocextract/AttributeInfoExtractor.java

@@ -23,35 +23,14 @@
 import com.google.devtools.build.lib.packages.Types;
 import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeInfo;
 import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeType;
+import java.util.Map;
 import java.util.Optional;
 import java.util.function.Consumer;
 import net.starlark.java.eval.Starlark.InvalidStarlarkValueException;
 
 /** Starlark API documentation extractor for a rule, macro, or aspect attribute. */
 @VisibleForTesting
 public final class AttributeInfoExtractor {
-
-  @VisibleForTesting
-  public static final AttributeInfo IMPLICIT_NAME_ATTRIBUTE_INFO =
-      AttributeInfo.newBuilder()
-          .setName("name")
-          .setType(AttributeType.NAME)
-          .setMandatory(true)
-          .setDocString("A unique name for this target.")
-          .build();
-
-  @VisibleForTesting
-  public static final AttributeInfo IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO =
-      AttributeInfo.newBuilder()
-          .setName("name")
-          .setType(AttributeType.NAME)
-          .setMandatory(true)
-          .setDocString(
-              "A unique name for this macro instance. Normally, this is also the name for the"
-                  + " macro's main or only target. The names of any other targets that this macro"
-                  + " might create will be this name with a string suffix.")
-          .build();
-
   @VisibleForTesting public static final String UNREPRESENTABLE_VALUE = "<unrepresentable value>";
 
   static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attribute) {
@@ -64,6 +43,9 @@ static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attr
     if (!attribute.isConfigurable()) {
       builder.setNonconfigurable(true);
     }
+    if (!attribute.starlarkDefined()) {
+      builder.setNativelyDefined(true);
+    }
     for (ImmutableSet<StarlarkProviderIdentifier> providerGroup :
         attribute.getRequiredProviders().getStarlarkProviders()) {
       // TODO(b/290788853): it is meaningless to require a provider on an attribute of a
@@ -83,14 +65,24 @@ static AttributeInfo buildAttributeInfo(ExtractorContext context, Attribute attr
     return builder.build();
   }
 
+  /**
+   * Adds {@code implicitAttributeInfos}, followed by documentable attributes from {@code
+   * attributes}.
+   */
   static void addDocumentableAttributes(
-      ExtractorContext context, Iterable<Attribute> attributes, Consumer<AttributeInfo> builder) {
+      ExtractorContext context,
+      Map<String, AttributeInfo> implicitAttributeInfos,
+      Iterable<Attribute> attributes,
+      Consumer<AttributeInfo> builder) {
+    // Inject implicit attributes first.
+    for (AttributeInfo implicitAttributeInfo : implicitAttributeInfos.values()) {
+      builder.accept(implicitAttributeInfo);
+    }
     for (Attribute attribute : attributes) {
-      if (attribute.getName().equals(IMPLICIT_NAME_ATTRIBUTE_INFO.getName())) {
-        // We inject our own IMPLICIT_NAME_ATTRIBUTE_INFO with better documentation.
+      if (implicitAttributeInfos.containsKey(attribute.getName())) {
         continue;
       }
-      if ((attribute.starlarkDefined() || context.extractNonStarlarkAttrs())
+      if ((attribute.starlarkDefined() || context.extractNativelyDefinedAttrs())
           && attribute.isDocumented()
           && ExtractorContext.isPublicName(attribute.getPublicName())) {
         builder.accept(buildAttributeInfo(context, attribute));

src/main/java/com/google/devtools/build/lib/starlarkdocextract/ExtractorContext.java

@@ -28,7 +28,7 @@
 public record ExtractorContext(
     LabelRenderer labelRenderer,
     ImmutableMap<StarlarkProvider.Key, String> providerQualifiedNames,
-    boolean extractNonStarlarkAttrs) {
+    boolean extractNativelyDefinedAttrs) {
 
   public ExtractorContext {
     checkNotNull(labelRenderer, "labelRenderer cannot be null.");
@@ -39,7 +39,14 @@ public record ExtractorContext(
   public static Builder builder() {
     return new AutoBuilder_ExtractorContext_Builder()
         .providerQualifiedNames(ImmutableMap.of())
-        .extractNonStarlarkAttrs(false);
+        .extractNativelyDefinedAttrs(false);
+  }
+
+  public Builder toBuilder() {
+    return builder()
+        .labelRenderer(labelRenderer)
+        .providerQualifiedNames(providerQualifiedNames)
+        .extractNativelyDefinedAttrs(extractNativelyDefinedAttrs);
   }
 
   /** Builder for {@link ExtractorContext}. */
@@ -50,7 +57,7 @@ public interface Builder {
     Builder providerQualifiedNames(
         ImmutableMap<StarlarkProvider.Key, String> providerQualifiedNames);
 
-    Builder extractNonStarlarkAttrs(boolean extractNonStarlarkAttrs);
+    Builder extractNativelyDefinedAttrs(boolean extractNativelyDefinedAttrs);
 
     ExtractorContext build();
   }

src/main/java/com/google/devtools/build/lib/starlarkdocextract/ModuleInfoExtractor.java

@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.starlarkdocextract;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.MacroFunction;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.StarlarkRuleFunction;
@@ -55,14 +54,41 @@ public final class ModuleInfoExtractor {
   private final LabelRenderer labelRenderer;
 
   @VisibleForTesting
-  public static final ImmutableList<AttributeInfo> IMPLICIT_REPOSITORY_RULE_ATTRIBUTES =
-      ImmutableList.of(
+  public static final ImmutableMap<String, AttributeInfo> IMPLICIT_MACRO_ATTRIBUTES =
+      ImmutableMap.of(
+          "name",
+          AttributeInfo.newBuilder()
+              .setName("name")
+              .setType(AttributeType.NAME)
+              .setMandatory(true)
+              .setDocString(
+                  "A unique name for this macro instance. Normally, this is also the name for the"
+                      + " macro's main or only target. The names of any other targets that this"
+                      + " macro might create will be this name with a string suffix.")
+              .build(),
+          "visibility",
+          AttributeInfo.newBuilder()
+              .setName("visibility")
+              .setType(AttributeType.LABEL_LIST)
+              .setMandatory(false)
+              .setDocString(
+                  "The visibility to be passed to this macro's exported targets. It always"
+                      + " implicitly includes the location where this macro is instantiated, so"
+                      + " this attribute only needs to be explicitly set if you want the macro's"
+                      + " targets to be additionally visible somewhere else.")
+              .build());
+
+  @VisibleForTesting
+  public static final ImmutableMap<String, AttributeInfo> IMPLICIT_REPOSITORY_RULE_ATTRIBUTES =
+      ImmutableMap.of(
+          "name",
           AttributeInfo.newBuilder()
               .setName("name")
               .setType(AttributeType.NAME)
               .setMandatory(true)
               .setDocString("A unique name for this repository.")
               .build(),
+          "repo_mapping",
           AttributeInfo.newBuilder()
               .setName("repo_mapping")
               .setType(AttributeType.STRING_DICT)
@@ -346,10 +372,19 @@ protected void visitMacroFunction(String qualifiedName, MacroFunction macroFunct
       macroFunction.getDocumentation().ifPresent(macroInfoBuilder::setDocString);
 
       MacroClass macroClass = macroFunction.getMacroClass();
-      // inject the name attribute; addDocumentableAttributes skips non-Starlark-defined attributes.
-      macroInfoBuilder.addAttribute(AttributeInfoExtractor.IMPLICIT_MACRO_NAME_ATTRIBUTE_INFO);
+      if (macroClass.isFinalizer()) {
+        macroInfoBuilder.setFinalizer(true);
+      }
+      // For symbolic macros, always extract non-Starlark attributes (to support inherit_attrs).
+      ExtractorContext contextForImplicitMacroAttributes =
+          context.extractNativelyDefinedAttrs()
+              ? context
+              : context.toBuilder().extractNativelyDefinedAttrs(true).build();
       AttributeInfoExtractor.addDocumentableAttributes(
-          context, macroClass.getAttributes().values(), macroInfoBuilder::addAttribute);
+          contextForImplicitMacroAttributes,
+          IMPLICIT_MACRO_ATTRIBUTES,
+          macroClass.getAttributes().values(),
+          macroInfoBuilder::addAttribute);
 
       moduleInfoBuilder.addMacroInfo(macroInfoBuilder);
     }
@@ -419,10 +454,8 @@ protected void visitAspect(String qualifiedName, StarlarkDefinedAspect aspect)
           aspectInfoBuilder.addAspectAttribute(aspectAttribute);
         }
       }
-      aspectInfoBuilder.addAttribute(
-          AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first
       AttributeInfoExtractor.addDocumentableAttributes(
-          context, aspect.getAttributes(), aspectInfoBuilder::addAttribute);
+          context, ImmutableMap.of(), aspect.getAttributes(), aspectInfoBuilder::addAttribute);
       moduleInfoBuilder.addAspectInfo(aspectInfoBuilder);
     }
 
@@ -446,7 +479,10 @@ protected void visitModuleExtension(String qualifiedName, ModuleExtension module
         tagClassInfoBuilder.setTagName(entry.getKey());
         entry.getValue().doc().ifPresent(tagClassInfoBuilder::setDocString);
         AttributeInfoExtractor.addDocumentableAttributes(
-            context, entry.getValue().attributes(), tagClassInfoBuilder::addAttribute);
+            context,
+            ImmutableMap.of(),
+            entry.getValue().attributes(),
+            tagClassInfoBuilder::addAttribute);
         moduleExtensionInfoBuilder.addTagClass(tagClassInfoBuilder);
       }
       moduleInfoBuilder.addModuleExtensionInfo(moduleExtensionInfoBuilder);
@@ -460,15 +496,15 @@ protected void visitRepositoryRule(
       repositoryRuleInfoBuilder.setRuleName(qualifiedName);
       repositoryRuleFunction.getDocumentation().ifPresent(repositoryRuleInfoBuilder::setDocString);
       RuleClass ruleClass = repositoryRuleFunction.getRuleClass();
-      repositoryRuleInfoBuilder
-          .setOriginKey(
-              OriginKey.newBuilder()
-                  .setName(ruleClass.getName())
-                  .setFile(
-                      context.labelRenderer().render(repositoryRuleFunction.getExtensionLabel())))
-          .addAllAttribute(IMPLICIT_REPOSITORY_RULE_ATTRIBUTES);
+      repositoryRuleInfoBuilder.setOriginKey(
+          OriginKey.newBuilder()
+              .setName(ruleClass.getName())
+              .setFile(context.labelRenderer().render(repositoryRuleFunction.getExtensionLabel())));
       AttributeInfoExtractor.addDocumentableAttributes(
-          context, ruleClass.getAttributes(), repositoryRuleInfoBuilder::addAttribute);
+          context,
+          IMPLICIT_REPOSITORY_RULE_ATTRIBUTES,
+          ruleClass.getAttributes(),
+          repositoryRuleInfoBuilder::addAttribute);
       if (ruleClass.hasAttr("$environ", Types.STRING_LIST)) {
         repositoryRuleInfoBuilder.addAllEnviron(
             Types.STRING_LIST.cast(ruleClass.getAttributeByName("$environ").getDefaultValue(null)));

src/main/java/com/google/devtools/build/lib/starlarkdocextract/RuleInfoExtractor.java

@@ -14,18 +14,33 @@
 
 package com.google.devtools.build.lib.starlarkdocextract;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
 import com.google.devtools.build.lib.packages.Type;
+import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeInfo;
+import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.AttributeType;
 import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.OriginKey;
 import com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.RuleInfo;
 
 /** API documentation extractor for a rule. */
 public final class RuleInfoExtractor {
 
+  @VisibleForTesting
+  public static final ImmutableMap<String, AttributeInfo> IMPLICIT_RULE_ATTRIBUTES =
+      ImmutableMap.of(
+          "name",
+          AttributeInfo.newBuilder()
+              .setName("name")
+              .setType(AttributeType.NAME)
+              .setMandatory(true)
+              .setDocString("A unique name for this target.")
+              .build());
+
   /**
    * Extracts API documentation for a rule in the form of a {@link RuleInfo} proto.
    *
@@ -72,10 +87,11 @@ public static RuleInfo buildRuleInfo(
       ruleInfoBuilder.setExecutable(true);
     }
 
-    ruleInfoBuilder.addAttribute(
-        AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO); // name comes first
     AttributeInfoExtractor.addDocumentableAttributes(
-        context, ruleClass.getAttributes(), ruleInfoBuilder::addAttribute);
+        context,
+        IMPLICIT_RULE_ATTRIBUTES,
+        ruleClass.getAttributes(),
+        ruleInfoBuilder::addAttribute);
     ImmutableSet<StarlarkProviderIdentifier> advertisedProviders =
         ruleClass.getAdvertisedProviders().getStarlarkProviders();
     if (!advertisedProviders.isEmpty()) {

src/main/protobuf/stardoc_output.proto

@@ -126,6 +126,9 @@ message MacroInfo {
   // The module where and the name under which the macro was originally
   // declared.
   OriginKey origin_key = 4;
+
+  // True if this macro is a rule finalizer.
+  bool finalizer = 5;
 }
 
 // Representation of a Starlark rule, repository rule, or module extension tag
@@ -161,6 +164,9 @@ message AttributeInfo {
 
   // If true, the attribute is non-configurable.
   bool nonconfigurable = 7;
+
+  // If true, the attribute is defined in Bazel's native code, not in Starlark.
+  bool natively_defined = 8;
 }
 
 // Representation of a set of providers.

src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD

@@ -31,6 +31,7 @@ java_library(
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
         "//src/test/java/com/google/devtools/build/lib/testutil",
+        "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
         "@com_google_protobuf//:protobuf_java",

src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java

@@ -16,10 +16,11 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
-import static com.google.devtools.build.lib.starlarkdocextract.AttributeInfoExtractor.IMPLICIT_NAME_ATTRIBUTE_INFO;
+import static com.google.devtools.build.lib.starlarkdocextract.RuleInfoExtractor.IMPLICIT_RULE_ATTRIBUTES;
 import static com.google.devtools.build.lib.starlarkdocextract.StardocOutputProtos.FunctionParamRole.PARAM_ROLE_ORDINARY;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -355,20 +356,27 @@ def my_macro():
             OriginKey.newBuilder().setName("my_rule").setFile("//:origin.bzl").build());
 
     assertThat(moduleInfo.getRuleInfo(0).getAttributeList())
-        .containsExactly(
-            IMPLICIT_NAME_ATTRIBUTE_INFO,
-            AttributeInfo.newBuilder()
-                .setName("a")
-                .setType(AttributeType.LABEL)
-                .setDefaultValue("None")
-                .addProviderNameGroup(
-                    ProviderNameGroup.newBuilder()
-                        .addProviderName("namespace.RenamedInfo")
-                        .addProviderName("other_namespace.RenamedOtherInfo")
-                        .addOriginKey(
-                            OriginKey.newBuilder().setName("MyInfo").setFile("//:origin.bzl"))
-                        .addOriginKey(
-                            OriginKey.newBuilder().setName("MyOtherInfo").setFile("//:origin.bzl")))
+        .isEqualTo(
+            ImmutableList.builder()
+                .addAll(IMPLICIT_RULE_ATTRIBUTES.values())
+                .add(
+                    AttributeInfo.newBuilder()
+                        .setName("a")
+                        .setType(AttributeType.LABEL)
+                        .setDefaultValue("None")
+                        .addProviderNameGroup(
+                            ProviderNameGroup.newBuilder()
+                                .addProviderName("namespace.RenamedInfo")
+                                .addProviderName("other_namespace.RenamedOtherInfo")
+                                .addOriginKey(
+                                    OriginKey.newBuilder()
+                                        .setName("MyInfo")
+                                        .setFile("//:origin.bzl"))
+                                .addOriginKey(
+                                    OriginKey.newBuilder()
+                                        .setName("MyOtherInfo")
+                                        .setFile("//:origin.bzl")))
+                        .build())
                 .build());
     assertThat(moduleInfo.getRuleInfo(0).getAdvertisedProviders())
         .isEqualTo(
@@ -955,7 +963,7 @@ def _impl(repository_ctx):
                 .setOriginKey(
                     OriginKey.newBuilder().setName("my_repo_rule").setFile("//:dep.bzl").build())
                 .setDocString("My repository rule\n\nWith details")
-                .addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES)
+                .addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES.values())
                 .addAttribute(
                     AttributeInfo.newBuilder()
                         .setName("a")