Mark starlark_doc_extract as non-experimental

The --experimental_enable_starlark_doc_extract flag is now a no-op. RELNOTES: Enable starlark_doc_extract - a native rule for Starlark documentation extraction. This rule is intended mainly for internal use by Stardoc. PiperOrigin-RevId: 543478993 Change-Id: I9c9bd64afafe38fb28b37e114e299a6e0feb681a
bazelbuild · Jun 26, 2023 · fa65782 · fa65782
1 parent 5ede15c
commit fa65782
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 86 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
@@ -377,6 +377,15 @@ public static class BuildGraveyardOptions extends OptionsBase {
         effectTags = {OptionEffectTag.EXECUTION},
         help = "Deprecated no-op.")
     public boolean collectLocalSandboxExecutionStatistics;
+
+    @Option(
+        name = "experimental_enable_starlark_doc_extract",
+        defaultValue = "true",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+        metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+        help = "Deprecated no-op.")
+    public boolean enableBzlDocDump;
   }
 
   /** This is where deprecated Bazel-specific options only used by the build command go to die. */

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/GenericRules.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/GenericRules.java
@@ -50,7 +50,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
     GenQueryRule.register(builder);
     builder.addRuleDefinition(new LabelBuildSettingRule());
     builder.addRuleDefinition(new LabelBuildFlagRule());
-    StarlarkDocExtractRule.register(builder);
+    builder.addRuleDefinition(new StarlarkDocExtractRule());
 
     try {
       builder.addWorkspaceFilePrefix(

diff --git a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/BUILD
@@ -19,9 +19,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/binary_file_write_action",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
@@ -36,7 +33,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/skydoc/rendering:function_util",
         "//src/main/java/com/google/devtools/build/skydoc/rendering/proto:stardoc_output_java_proto",
         "//src/main/java/com/google/devtools/build/skyframe",
-        "//src/main/java/com/google/devtools/common/options",
         "//src/main/java/net/starlark/java/eval",
         "//third_party:guava",
         "//third_party:jsr305",

diff --git a/...main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java b/...main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java
@@ -346,8 +346,13 @@ protected void visitProvider(String qualifiedName, StarlarkProvider provider) {
       // Note that it's possible that qualifiedName != getDocumentedProviderName() if the same
       // provider symbol is made accessible under more than one qualified name.
       // TODO(b/276733504): if a provider (or any other documentable entity) is made accessible
-      // under two different qualified names, record them in a repeated field inside a single *Info
-      // object, instead of producing a separate *Info object for each alias.
+      // under two different public qualified names, record them in a repeated field inside a single
+      // ProviderInfo (or other ${FOO}Info for documentable entity ${FOO}) message, instead of
+      // producing a separate ${FOO}Info message for each alias. That requires adding an "alias"
+      // field to ${FOO}Info messages (making the existing "${FOO}_name" field repeated would break
+      // existing Stardoc templates). Note that for backwards compatibility,
+      // ProviderNameGroup.provider_name would still need to refer to only the first qualified name
+      // under which a given provider is made accessible by the module.
       providerInfoBuilder.setProviderName(qualifiedName);
       // Record the origin provider key for cross references.
       providerInfoBuilder.setOriginKey(

diff --git a/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java b/src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java
@@ -37,10 +37,6 @@
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.actions.BinaryFileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.analysis.config.BuildOptions;
-import com.google.devtools.build.lib.analysis.config.Fragment;
-import com.google.devtools.build.lib.analysis.config.FragmentOptions;
-import com.google.devtools.build.lib.analysis.config.RequiresOptions;
 import com.google.devtools.build.lib.cmdline.BazelModuleContext;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -56,10 +52,6 @@
 import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
 import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ModuleInfo;
 import com.google.devtools.build.skyframe.SkyFunction;
-import com.google.devtools.common.options.Option;
-import com.google.devtools.common.options.OptionDocumentationCategory;
-import com.google.devtools.common.options.OptionEffectTag;
-import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.protobuf.TextFormat;
 import java.io.IOException;
@@ -78,47 +70,10 @@ public class StarlarkDocExtract implements RuleConfiguredTargetFactory {
   static final SafeImplicitOutputsFunction BINARYPROTO_OUT = fromTemplates("%{name}.binaryproto");
   static final SafeImplicitOutputsFunction TEXTPROTO_OUT = fromTemplates("%{name}.textproto");
 
-  /** Configuration fragment for the {@code starlark_doc_extract} rule. */
-  // TODO(b/276733504): remove once non-experimental.
-  @RequiresOptions(options = {Configuration.Options.class})
-  public static final class Configuration extends Fragment {
-
-    /** Options for the {@code starlark_doc_extract} rule. */
-    public static final class Options extends FragmentOptions {
-      @Option(
-          name = "experimental_enable_starlark_doc_extract",
-          defaultValue = "false",
-          documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-          effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
-          metadataTags = {OptionMetadataTag.EXPERIMENTAL},
-          help = "If set to true, enables the experimental starlark_doc_extract rule.")
-      public boolean experimentalEnableBzlDocDump;
-    }
-
-    private final boolean enabled;
-
-    public Configuration(BuildOptions buildOptions) {
-      enabled = buildOptions.get(Options.class).experimentalEnableBzlDocDump;
-    }
-
-    public boolean enabled() {
-      return enabled;
-    }
-  }
-
   @Override
   @Nullable
   public ConfiguredTarget create(RuleContext ruleContext)
       throws ActionConflictException, InterruptedException, RuleErrorException {
-    if (!ruleContext.getFragment(Configuration.class).enabled()) {
-      RuleErrorException exception =
-          new RuleErrorException(
-              "The experimental starlark_doc_extract rule is disabled; use"
-                  + " --experimental_enable_starlark_doc_extract flag to enable.");
-      ruleContext.ruleError(exception.getMessage());
-      throw exception;
-    }
-
     RepositoryMapping repositoryMapping = getTargetRepositoryMapping(ruleContext);
     Module module = loadModule(ruleContext, repositoryMapping);
     if (module == null) {
@@ -227,15 +182,21 @@ private static String formatDerivedArtifact(
    *
    * @throws RuleErrorException if that is not the case.
    */
+  // TODO(https://github.com/bazelbuild/bazel/issues/18599): to avoid flattening deps, we could use
+  // either (a) a new, native bzl_library-like rule that verifies strict deps, or (b) a new native
+  // aspect that verifies strict deps for the existing bzl_library rule. Ideally, however, we ought
+  // to get rid of the deps attribute (and the need to verify it) altogether; that requires new
+  // dependency machinery for `bazel query` to use the Starlark load graph for collecting the
+  // dependencies of starlark_doc_extract's src.
   private static void verifyModuleDeps(
       RuleContext ruleContext, Module module, RepositoryMapping repositoryMapping)
       throws RuleErrorException {
     // Note attr schema validates that deps are .bzl or .scl files.
     Map<Boolean, ImmutableSet<Artifact>> flattenedDepsPartitionedByIsSource =
         ruleContext.getPrerequisites(DEPS_ATTR).stream()
-            // TODO(https://github.com/bazelbuild/bazel/issues/18599): ideally we should use
-            // StarlarkLibraryInfo here instead of FileProvider#getFilesToBuild; that requires a
-            // native StarlarkLibraryInfo in Bazel.
+            // TODO(https://github.com/bazelbuild/bazel/issues/18599): we are using FileProvider
+            // instead of StarlarkLibraryInfo only because StarlarkLibraryInfo is defined in
+            // bazel_skylib, not natively in Bazel.
             .flatMap(dep -> dep.getProvider(FileProvider.class).getFilesToBuild().toList().stream())
             .collect(partitioningBy(Artifact::isSourceArtifact, toImmutableSet()));
     // bzl_library targets may contain both source artifacts and derived artifacts (e.g. generated

diff --git a/...n/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractRule.java b/...n/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractRule.java
@@ -22,7 +22,6 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
-import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -31,38 +30,79 @@
 
 /** Definition of the {@code starlark_doc_extract} rule. */
 public final class StarlarkDocExtractRule implements RuleDefinition {
-
-  /**
-   * Adds {@code starlark_doc_extract} and its dependencies to the provided configured rule class
-   * builder.
-   */
-  public static void register(ConfiguredRuleClassProvider.Builder builder) {
-    builder.addConfigurationFragment(StarlarkDocExtract.Configuration.class);
-    builder.addRuleDefinition(new StarlarkDocExtractRule());
-  }
-
   @Override
   @Nullable
   public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
-    // TODO(b/276733504): publicly document the rule and its attributes once non-experimental.
     return builder
+        /*<!-- #BLAZE_RULE(starlark_doc_extract).ATTRIBUTE(src) -->
+        A Starlark file from which to extract documentation.
+
+        <p>Note that this must be a file in the source tree; Bazel cannot <code>load()</code>
+        generated files.
+        <!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
         .add(
             attr(StarlarkDocExtract.SRC_ATTR, LABEL)
                 .allowedFileTypes(FileType.of(".bzl"), FileType.of(".scl"))
                 .singleArtifact()
                 .mandatory())
-        // TODO(https://github.com/bazelbuild/bazel/issues/18599): for deps, we ought to set
-        // mandatoryProviders(StarlarkLibraryInfo); that requires a native StarlarkLibraryInfo in
-        // Bazel.
+        /*<!-- #BLAZE_RULE(starlark_doc_extract).ATTRIBUTE(deps) -->
+        A list of targets wrapping the Starlark files which are <code>load()</code>-ed by
+        <code>src</code>. These targets <em>should</em> under normal usage be
+        <a href="https://github.com/bazelbuild/bazel-skylib/blob/main/bzl_library.bzl"><code>bzl_library</code></a>
+        targets, but the <code>starlark_doc_extract</code> rule does not enforce that, and accepts
+        any target which provides Starlark files in its <code>DefaultInfo</code>.
+
+        <p>Note that the wrapped Starlark files must be files in the source tree; Bazel cannot
+        <code>load()</code> generated files.
+        <!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
+        // TODO(https://github.com/bazelbuild/bazel/issues/18599): we cannot set
+        // mandatoryProviders(StarlarkLibraryInfo) because StarlarkLibraryInfo is defined in
+        // bazel_skylib, not natively in Bazel. Ideally, we ought to get rid of the deps attribute
+        // altogether; but that requires new dependency machinery for `bazel query` to use the
+        // Starlark load graph for collecting the dependencies of starlark_doc_extract's src.
         .override(
             attr(StarlarkDocExtract.DEPS_ATTR, LABEL_LIST)
                 .allowedFileTypes(FileType.of(".bzl"), FileType.of(".scl")))
+        /*<!-- #BLAZE_RULE(starlark_doc_extract).ATTRIBUTE(symbol_names) -->
+        An optional list of qualified names of exported functions, rules, providers, or aspects (or
+        structs in which they are nested) for which to extract documentation. Here, a <em>qualified
+        name</em> means the name under which an entity is made available to a user of the module,
+        including any structs in which the entity is nested for namespacing.
+
+        <p><code>starlark_doc_extract</code> emits documentation for an entity if and only if
+        <ol>
+          <li>
+            each component of the entity's qualified name is public (in other words, the first
+            character of each component of the qualified name is alphabetic, not <code>"_"</code>);
+            <em>and</em>
+          </li>
+          <li>
+            <ol>
+              <li>
+                <em>either</em> the <code>symbol_names</code> list is empty (which is the default
+                case), <em>or</em>
+              </li>
+              <li>
+                the entity's qualified name, or the qualified name of a struct in which the entity
+                is nested, is in the <code>symbol_names</code> list.
+              </li>
+            </ol>
+          </li>
+        </ol>
+        <!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
         .add(
             attr(StarlarkDocExtract.SYMBOL_NAMES_ATTR, STRING_LIST)
                 .value(ImmutableList.<String>of()))
+        /*<!-- #BLAZE_RULE(starlark_doc_extract).IMPLICIT_OUTPUTS -->
+        <ul>
+          <li><code><var>name</var>.binaryproto</code> (the default output): A
+            <code>ModuleInfo</code> binary proto.</li>
+          <li><code><var>name</var>.textproto</code> (only built if explicitly requested): the text
+            proto version of <code><var>name</var>.binaryproto</code>.</li>
+        </ul>
+        <!-- #END_BLAZE_RULE.IMPLICIT_OUTPUTS -->*/
         .setImplicitOutputsFunction(
             fromFunctions(StarlarkDocExtract.BINARYPROTO_OUT, StarlarkDocExtract.TEXTPROTO_OUT))
-        .requiresConfigurationFragments(StarlarkDocExtract.Configuration.class)
         .build();
   }
 
@@ -76,3 +116,18 @@ public Metadata getMetadata() {
         .build();
   }
 }
+/*<!-- #BLAZE_RULE (NAME = starlark_doc_extract, FAMILY = General)[GENERIC_RULE] -->
+
+<p><code>starlark_doc_extract()</code> extracts documentation for rules, functions (including
+macros), aspects, and providers defined or re-exported in a given <code>.bzl</code> or
+<code>.scl</code> file. The output of this rule is a <code>ModuleInfo</code> binary proto as defined
+in
+<a href="https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/skydoc/rendering/proto/stardoc_output.proto">stardoc_output.proto</a>
+in the Bazel source tree.
+
+${IMPLICIT_OUTPUTS}
+
+<p>Warning: the output format of this rule is not guaranteed to be stable. It is intended mainly for
+internal use by <a href="https://github.com/bazelbuild/stardoc">Stardoc</a>.
+
+<!-- #END_BLAZE_RULE -->*/
diff --git a/...t/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java b/...t/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
@@ -132,7 +132,6 @@ public void setUpBzlLibrary() throws Exception {
 
   @Test
   public void basicFunctionality() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "foo.bzl", //
         "'''Module doc string'''",
@@ -152,7 +151,6 @@ public void basicFunctionality() throws Exception {
 
   @Test
   public void textprotoOut() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "foo.bzl", //
         "'''Module doc string'''",
@@ -177,7 +175,6 @@ public void textprotoOut() throws Exception {
 
   @Test
   public void sclDialect() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     setBuildLanguageOptions("--experimental_enable_scl_dialect");
     scratch.file(
         "foo.scl", //
@@ -209,7 +206,6 @@ public void sclDialect() throws Exception {
 
   @Test
   public void sourceWithSyntaxError_fails() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "error.bzl", //
         "!!!");
@@ -245,7 +241,6 @@ public void sourceWithSyntaxError_fails() throws Exception {
 
   @Test
   public void symbolNames() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "foo.bzl", //
         "def func1():",
@@ -277,7 +272,6 @@ public void symbolNames() throws Exception {
 
   @Test
   public void originKey() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "origin.bzl", //
         "def my_macro():",
@@ -372,7 +366,6 @@ public void originKey() throws Exception {
   public void originKeyFileAndModuleInfoFileLabels_forBzlFileInBzlmodModule_areDisplayForm()
       throws Exception {
     setBuildLanguageOptions("--enable_bzlmod");
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.overwriteFile("MODULE.bazel", "bazel_dep(name='origin_repo', version='0.1')");
     registry.addModule(
         BzlmodTestUtil.createModuleKey("origin_repo", "0.1"),
@@ -450,7 +443,6 @@ public void originKeyFileAndModuleInfoFileLabels_forBzlFileInBzlmodModule_areDis
 
   @Test
   public void exportNestedFunctionsAndLambdas() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "origin.bzl", //
         "def return_nested():",
@@ -502,7 +494,6 @@ public void exportNestedFunctionsAndLambdas() throws Exception {
 
   @Test
   public void missingBzlLibraryDeps_fails() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file(
         "dep.bzl", //
         "load('//:forgotten_dep_of_dep.bzl', 'g')",
@@ -552,7 +543,6 @@ public void missingBzlLibraryDeps_fails() throws Exception {
 
   @Test
   public void depsWithDerivedFiles_onUnknownLoads_failsAndPrintsDerivedFiles() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file("BUILD");
     scratch.file(
         "pkg/source_file_masked_by_rule_name.bzl", //
@@ -614,7 +604,6 @@ public void depsWithDerivedFiles_onUnknownLoads_failsAndPrintsDerivedFiles() thr
 
   @Test
   public void depsWithDerivedFiles_onNoUnknownLoads_succeeds() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file("BUILD");
     scratch.file(
         "util.bzl",
@@ -667,7 +656,6 @@ public void depsWithDerivedFiles_onNoUnknownLoads_succeeds() throws Exception {
 
   @Test
   public void srcDerivedFile_fails() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file("BUILD");
     scratch.file("pkg/source_file_masked_by_rule_name.bzl");
     scratch.file("pkg/source_file_masked_by_rule_output_name.bzl");
@@ -734,7 +722,6 @@ public void srcDerivedFile_fails() throws Exception {
 
   @Test
   public void srcAlias_resolvesToActual() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file("alias_name.bzl");
     scratch.file("alias_actual.bzl");
     scratch.file(
@@ -754,7 +741,6 @@ public void srcAlias_resolvesToActual() throws Exception {
 
   @Test
   public void srcFilegroup_resolvesToFilegroupSrc() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file("masked_by_filegroup_name.bzl");
     scratch.file("filegroup_src_actual.bzl");
     scratch.file(
@@ -774,7 +760,6 @@ public void srcFilegroup_resolvesToFilegroupSrc() throws Exception {
 
   @Test
   public void srcFilegroup_mustHaveSingleSrc() throws Exception {
-    useConfiguration("--experimental_enable_starlark_doc_extract");
     scratch.file("foo.bzl");
     scratch.file("bar.bzl");
     scratch.file(