Provide access to the doc string of starlark rule functions

Required for new implementation of Stardoc PiperOrigin-RevId: 512140817 Change-Id: Id5fc0fd2ac993f059440b966db05b6d240300b2c
fweikert · May 25, 2023 · ea571e9 · ea571e9
1 parent dbf8863
commit ea571e9
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 28 deletions.
diff --git a/...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -98,6 +98,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Debug;
 import net.starlark.java.eval.Dict;
@@ -287,7 +288,7 @@ public StarlarkRuleFunction rule(
       Boolean starlarkTestable,
       Sequence<?> toolchains,
       boolean useToolchainTransition,
-      String doc,
+      Object doc,
       Sequence<?> providesArg,
       Sequence<?> execCompatibleWith,
       Object analysisTest,
@@ -309,6 +310,7 @@ public StarlarkRuleFunction rule(
         hostFragments,
         starlarkTestable,
         toolchains,
+        doc,
         providesArg,
         execCompatibleWith,
         analysisTest,
@@ -331,6 +333,7 @@ public static StarlarkRuleFunction createRule(
       Sequence<?> hostFragments,
       boolean starlarkTestable,
       Sequence<?> toolchains,
+      Object doc,
       Sequence<?> providesArg,
       Sequence<?> execCompatibleWith,
       Object analysisTest,
@@ -502,7 +505,12 @@ public static StarlarkRuleFunction createRule(
     }
 
     StarlarkRuleFunction starlarkRuleFunction =
-        new StarlarkRuleFunction(builder, type, attributes, thread.getCallerLocation());
+        new StarlarkRuleFunction(
+            builder,
+            type,
+            attributes,
+            thread.getCallerLocation(),
+            Starlark.toJavaOptional(doc, String.class));
     // 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.
@@ -750,6 +758,7 @@ public static final class StarlarkRuleFunction implements StarlarkExportable, Ru
     private final RuleClassType type;
     private ImmutableList<Pair<String, StarlarkAttrModule.Descriptor>> attributes;
     private final Location definitionLocation;
+    @Nullable private final String documentation;
     private Label starlarkLabel;
 
     // TODO(adonovan): merge {Starlark,Builtin}RuleFunction and RuleClass,
@@ -760,18 +769,28 @@ public StarlarkRuleFunction(
         RuleClass.Builder builder,
         RuleClassType type,
         ImmutableList<Pair<String, StarlarkAttrModule.Descriptor>> attributes,
-        Location definitionLocation) {
+        Location definitionLocation,
+        Optional<String> documentation) {
       this.builder = builder;
       this.type = type;
       this.attributes = attributes;
       this.definitionLocation = definitionLocation;
+      this.documentation = documentation.orElse(null);
     }
 
     @Override
     public String getName() {
       return ruleClass != null ? ruleClass.getName() : "unexported rule";
     }
 
+    /**
+     * Returns the value of the doc parameter passed to {@code rule()} in Starlark, or an empty
+     * Optional if a doc string was not provided.
+     */
+    public Optional<String> getDocumentation() {
+      return Optional.ofNullable(documentation);
+    }
+
     @Override
     public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwargs)
         throws EvalException, InterruptedException {

diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java
@@ -74,23 +74,24 @@ public void analysisTest(
     StarlarkRuleFunction starlarkRuleFunction =
         StarlarkRuleClassFunctions.createRule(
             implementation,
-            /*test=*/ true,
+            /* test= */ true,
             attrs,
-            /*implicitOutputs=*/ Starlark.NONE,
-            /*executable=*/ false,
-            /*outputToGenfiles=*/ false,
-            /*fragments=*/ fragments,
-            /*hostFragments=*/ StarlarkList.empty(),
-            /*starlarkTestable=*/ false,
-            /*toolchains=*/ toolchains,
-            /*providesArg=*/ StarlarkList.empty(),
-            /*execCompatibleWith=*/ StarlarkList.empty(),
-            /*analysisTest=*/ Boolean.TRUE,
-            /*buildSetting=*/ Starlark.NONE,
-            /*cfg=*/ Starlark.NONE,
-            /*execGroups=*/ Starlark.NONE,
-            /*compileOneFiletype=*/ Starlark.NONE,
-            /*name=*/ Starlark.NONE,
+            /* implicitOutputs= */ Starlark.NONE,
+            /* executable= */ false,
+            /* outputToGenfiles= */ false,
+            /* fragments= */ fragments,
+            /* hostFragments= */ StarlarkList.empty(),
+            /* starlarkTestable= */ false,
+            /* toolchains= */ toolchains,
+            /* doc= */ Starlark.NONE,
+            /* providesArg= */ StarlarkList.empty(),
+            /* execCompatibleWith= */ StarlarkList.empty(),
+            /* analysisTest= */ Boolean.TRUE,
+            /* buildSetting= */ Starlark.NONE,
+            /* cfg= */ Starlark.NONE,
+            /* execGroups= */ Starlark.NONE,
+            /* compileOneFiletype= */ Starlark.NONE,
+            /* name= */ Starlark.NONE,
             thread);
 
     // Export the rule

diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java
@@ -362,7 +362,11 @@ Object provider(Object doc, Object fields, Object init, StarlarkThread thread)
         @Param(
             name = "doc",
             named = true,
-            defaultValue = "''",
+            allowedTypes = {
+              @ParamType(type = String.class),
+              @ParamType(type = NoneType.class),
+            },
+            defaultValue = "None",
             doc =
                 "A description of the rule that can be extracted by documentation generating "
                     + "tools."),
@@ -487,7 +491,7 @@ StarlarkCallable rule(
       Boolean starlarkTestable,
       Sequence<?> toolchains,
       boolean useToolchainTransition,
-      String doc,
+      Object doc,
       Sequence<?> providesArg,
       Sequence<?> execCompatibleWith,
       Object analysisTest,

diff --git a/...main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/...main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
@@ -138,7 +138,7 @@ public StarlarkCallable rule(
       Boolean starlarkTestable,
       Sequence<?> toolchains,
       boolean useToolchainTransition,
-      String doc,
+      Object doc,
       Sequence<?> providesArg,
       Sequence<?> execCompatibleWith,
       Object analysisTest,
@@ -165,10 +165,9 @@ public StarlarkCallable rule(
     RuleDefinitionIdentifier functionIdentifier = new RuleDefinitionIdentifier();
 
     // 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);
-    }
+    RuleInfo.Builder ruleInfo = RuleInfo.newBuilder().addAllAttribute(attrInfos);
+    Starlark.toJavaOptional(doc, String.class).ifPresent(ruleInfo::setDocString);
+    Starlark.toJavaOptional(name, String.class).ifPresent(ruleInfo::setRuleName);
     Location loc = thread.getCallerLocation();
     ruleInfoList.add(new RuleInfoWrapper(functionIdentifier, loc, ruleInfo));
 

diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -860,7 +860,16 @@ public void testRuleImplementation() throws Exception {
 
   @Test
   public void testRuleDoc() throws Exception {
-    evalAndExport(ev, "def impl(ctx): return None", "rule1 = rule(impl, doc='foo')");
+    evalAndExport(
+        ev,
+        "def impl(ctx):",
+        "    return None",
+        "documented_rule = rule(impl, doc='My doc string')",
+        "undocumented_rule = rule(impl)");
+    StarlarkRuleFunction documentedRule = (StarlarkRuleFunction) ev.lookup("documented_rule");
+    StarlarkRuleFunction undocumentedRule = (StarlarkRuleFunction) ev.lookup("undocumented_rule");
+    assertThat(documentedRule.getDocumentation()).hasValue("My doc string");
+    assertThat(undocumentedRule.getDocumentation()).isEmpty();
   }
 
   @Test
@@ -1111,7 +1120,8 @@ public void testRuleBadTypeInAdd() throws Exception {
   @Test
   public void testRuleBadTypeForDoc() throws Exception {
     registerDummyStarlarkFunction();
-    ev.checkEvalErrorContains("got value of type 'int', want 'string'", "rule(impl, doc = 1)");
+    ev.checkEvalErrorContains(
+        "got value of type 'int', want 'string or NoneType'", "rule(impl, doc = 1)");
   }
 
   @Test