Change some core semantics of skydoc.

- Change Skydoc to only document exported symbols in the target file, instead of all exported symbols in the transitive dependencies of the target file. This circumvents a prior error scenario where main.bzl could depend on dep.bzl, and both export a rule named ?my_rule?, which would result in a conflict.
- Offer the option to specify whitelisted symbols for which to output documentation, allowing a user to only request documentation for a subset of rules defined in a given file. This allows more granular control of documentation layout.

RELNOTES: None.
PiperOrigin-RevId: 204161197

9 files changed, 252 insertions, 44 deletions

diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
index 7f664737ac..2f0a7f4932 100644
--- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
+++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.skydoc;
 import com.google.common.base.Functions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skylarkbuildapi.TopLevelBootstrap;
 import com.google.devtools.build.lib.skylarkbuildapi.android.AndroidBootstrap;
@@ -87,8 +88,13 @@ import java.util.stream.Collectors;
  *
  * <p>Usage:</p>
  * <pre>
- *   skydoc {target_skylark_file} {output_file}
+ *   skydoc {target_skylark_file} {output_file} [symbol_name]...
  * </pre>
+ * <p>
+ *   Generates documentation for all exported symbols of the target skylark file that are
+ *   specified in the list of symbol names. If no symbol names are supplied, outputs documentation
+ *   for all exported symbols in the target skylark file.
+ * </p>
  */
 public class SkydocMain {
 
@@ -102,40 +108,58 @@ public class SkydocMain {
   }
 
   public static void main(String[] args) throws IOException, InterruptedException {
-    if (args.length != 2) {
-      throw new IllegalArgumentException("Expected two arguments. Usage:\n"
-          + "{skydoc_bin} {target_skylark_file} {output_file}");
+    if (args.length < 2) {
+      throw new IllegalArgumentException("Expected two or more arguments. Usage:\n"
+          + "{skydoc_bin} {target_skylark_file} {output_file} [symbol_names]...");
     }
 
     String bzlPath = args[0];
     String outputPath = args[1];
+    ImmutableSet<String> symbolNames = getSymbolNames(args);
 
     Path path = Paths.get(bzlPath);
 
     ImmutableMap.Builder<String, RuleInfo> ruleInfoMap = ImmutableMap.builder();
-    ImmutableList.Builder<RuleInfo> unexportedRuleInfos = ImmutableList.builder();
+    ImmutableList.Builder<RuleInfo> unknownNamedRules = ImmutableList.builder();
 
-    new SkydocMain(new FilesystemFileAccessor()).eval(path, ruleInfoMap, unexportedRuleInfos);
+    new SkydocMain(new FilesystemFileAccessor()).eval(path, ruleInfoMap, unknownNamedRules);
 
     MarkdownRenderer renderer = new MarkdownRenderer();
 
-    try (PrintWriter printWriter = new PrintWriter(outputPath, "UTF-8")) {
-      printRuleInfos(printWriter, renderer, ruleInfoMap.build(), unexportedRuleInfos.build());
+    if (symbolNames.isEmpty()) {
+      try (PrintWriter printWriter = new PrintWriter(outputPath, "UTF-8")) {
+        printRuleInfos(printWriter, renderer, ruleInfoMap.build(), unknownNamedRules.build());
+      }
+    } else {
+      Map<String, RuleInfo> filteredRuleInfos = ImmutableMap.copyOf(
+          ruleInfoMap.build().entrySet().stream()
+              .filter(entry -> symbolNames.contains(entry.getKey()))
+              .collect(Collectors.toList()));
+      try (PrintWriter printWriter = new PrintWriter(outputPath, "UTF-8")) {
+        printRuleInfos(printWriter, renderer, filteredRuleInfos, ImmutableList.of());
+      }
+    }
+  }
+
+  private static ImmutableSet<String> getSymbolNames(String[] args) {
+    ImmutableSet.Builder<String> symbolNameSet = ImmutableSet.builder();
+    for (int argi = 2; argi < args.length; argi++) {
+      symbolNameSet.add(args[argi]);
     }
+    return symbolNameSet.build();
   }
 
-  // TODO(cparsons): Improve output (markdown or HTML).
   private static void printRuleInfos(
       PrintWriter printWriter,
       MarkdownRenderer renderer,
       Map<String, RuleInfo> ruleInfos,
-      List<RuleInfo> unexportedRuleInfos) throws IOException {
+      List<RuleInfo> unknownNamedRules) throws IOException {
     for (Entry<String, RuleInfo> ruleInfoEntry : ruleInfos.entrySet()) {
       printRuleInfo(printWriter, renderer, ruleInfoEntry.getKey(), ruleInfoEntry.getValue());
       printWriter.println();
     }
-    for (RuleInfo unexportedRuleInfo : unexportedRuleInfos) {
-      printRuleInfo(printWriter, renderer, "<unknown name>", unexportedRuleInfo);
+    for (RuleInfo unknownNamedRule : unknownNamedRules) {
+      printRuleInfo(printWriter, renderer, "<unknown name>", unknownNamedRule);
       printWriter.println();
     }
   }
@@ -148,20 +172,57 @@ public class SkydocMain {
 
   /**
    * Evaluates/interprets the skylark file at a given path and its transitive skylark dependencies
-   * using a fake build API and collects information about all rule definitions made in those files.
+   * using a fake build API and collects information about all rule definitions made in the
+   * root skylark file.
    *
    * @param path the path of the skylark file to evaluate
    * @param ruleInfoMap a map builder to be populated with rule definition information for
    *     named rules. Keys are exported names of rules, and values are their {@link RuleInfo}
    *     rule descriptions. For example, 'my_rule = rule(...)' has key 'my_rule'
-   * @param unexportedRuleInfos a list builder to be populated with rule definition information
+   * @param unknownNamedRules a list builder to be populated with rule definition information
    *     for rules which were not exported as top level symbols
    * @throws InterruptedException if evaluation is interrupted
    */
   public Environment eval(
       Path path,
       ImmutableMap.Builder<String, RuleInfo> ruleInfoMap,
-      ImmutableList.Builder<RuleInfo> unexportedRuleInfos)
+      ImmutableList.Builder<RuleInfo> unknownNamedRules)
+      throws InterruptedException, IOException {
+    List<RuleInfo> ruleInfoList = new ArrayList<>();
+    Environment env = recursiveEval(path, ruleInfoList);
+
+    Map<BaseFunction, RuleInfo> ruleFunctions = ruleInfoList.stream()
+        .collect(Collectors.toMap(
+            RuleInfo::getIdentifierFunction,
+            Functions.identity()));
+
+    ImmutableSet.Builder<RuleInfo> handledRuleDefinitions = ImmutableSet.builder();
+    for (Entry<String, Object> envEntry : env.getGlobals().getBindings().entrySet()) {
+      if (ruleFunctions.containsKey(envEntry.getValue())) {
+        RuleInfo ruleInfo = ruleFunctions.get(envEntry.getValue());
+        ruleInfoMap.put(envEntry.getKey(), ruleInfo);
+        handledRuleDefinitions.add(ruleInfo);
+      }
+    }
+
+    unknownNamedRules.addAll(ruleFunctions.values().stream()
+        .filter(ruleInfo -> !handledRuleDefinitions.build().contains(ruleInfo)).iterator());
+
+    return env;
+  }
+
+  /**
+   * Recursively evaluates/interprets the skylark file at a given path and its transitive skylark
+   * dependencies using a fake build API and collects information about all rule definitions made
+   * in those files.
+   *
+   * @param path the path of the skylark file to evaluate
+   * @param ruleInfoList a collection of all rule definitions made so far (using rule()); this
+   *     method will add to this list as it evaluates additional files
+   * @throws InterruptedException if evaluation is interrupted
+   */
+  private Environment recursiveEval(
+      Path path, List<RuleInfo> ruleInfoList)
       throws InterruptedException, IOException {
     if (pending.contains(path)) {
       throw new IllegalStateException("cycle with " + path);
@@ -177,12 +238,12 @@ public class SkydocMain {
     for (SkylarkImport anImport : buildFileAST.getImports()) {
       Path importPath = fromPathFragment(path, anImport.asPathFragment());
 
-      Environment importEnv = eval(importPath, ruleInfoMap, unexportedRuleInfos);
+      Environment importEnv = recursiveEval(importPath, ruleInfoList);
 
       imports.put(anImport.getImportString(), new Extension(importEnv));
     }
 
-    Environment env = evalSkylarkBody(buildFileAST, imports, ruleInfoMap, unexportedRuleInfos);
+    Environment env = evalSkylarkBody(buildFileAST, imports, ruleInfoList);
 
     pending.remove(path);
     env.mutability().freeze();
@@ -202,9 +263,7 @@ public class SkydocMain {
   private Environment evalSkylarkBody(
       BuildFileAST buildFileAST,
       Map<String, Extension> imports,
-      ImmutableMap.Builder<String, RuleInfo> ruleInfoMap,
-      ImmutableList.Builder<RuleInfo> unexportedRuleInfos) throws InterruptedException {
-    List<RuleInfo> ruleInfoList = new ArrayList<>();
+      List<RuleInfo> ruleInfoList) throws InterruptedException {
 
     Environment env = createEnvironment(
         eventHandler,
@@ -217,19 +276,6 @@ public class SkydocMain {
 
     env.mutability().freeze();
 
-    Map<BaseFunction, RuleInfo> ruleFunctions = ruleInfoList.stream()
-        .collect(Collectors.toMap(
-            RuleInfo::getIdentifierFunction,
-            Functions.identity()));
-
-    for (Entry<String, Object> envEntry : env.getGlobals().getBindings().entrySet()) {
-      if (ruleFunctions.containsKey(envEntry.getValue())) {
-        ruleInfoMap.put(envEntry.getKey(), ruleFunctions.get(envEntry.getValue()));
-        ruleFunctions.remove(envEntry.getValue());
-      }
-    }
-    unexportedRuleInfos.addAll(ruleFunctions.values());
-
     return env;
   }
 
@@ -240,8 +286,6 @@ public class SkydocMain {
    *     information will be added
    */
   private static GlobalFrame globalFrame(List<RuleInfo> ruleInfoList) {
-    // TODO(cparsons): Complete the Fake Build API stubs. For example, implement provider(),
-    // and include the other bootstraps.
     TopLevelBootstrap topLevelBootstrap =
         new TopLevelBootstrap(new FakeBuildApiGlobals(),
             new FakeSkylarkAttrApi(),
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
index 4260032ad8..e75a483829 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
@@ -123,8 +123,10 @@ public class FakeSkylarkRuleFunctionsApi implements SkylarkRuleFunctionsApi<File
    */
   private static class RuleDefinitionIdentifier extends BaseFunction {
 
+    private static int idCounter = 0;
+
     public RuleDefinitionIdentifier() {
-      super("RuleDefinitionIdentifier");
+      super("RuleDefinitionIdentifier" + idCounter++);
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/skydoc/BUILD b/src/test/java/com/google/devtools/build/skydoc/BUILD
index 31b365d394..f36e9dbdc8 100644
--- a/src/test/java/com/google/devtools/build/skydoc/BUILD
+++ b/src/test/java/com/google/devtools/build/skydoc/BUILD
@@ -105,3 +105,17 @@ skydoc_test(
     input_file = "testdata/attribute_types_test/input.bzl",
     skydoc = "//src/main/java/com/google/devtools/build/skydoc",
 )
+
+skydoc_test(
+    name = "filter_rules_test",
+    golden_file = "testdata/filter_rules_test/golden.txt",
+    input_file = "testdata/filter_rules_test/input.bzl",
+    skydoc = "//src/main/java/com/google/devtools/build/skydoc",
+    whitelisted_symbols = [
+        "my_rule",
+        "whitelisted_dep_rule",
+    ],
+    deps = [
+        "testdata/filter_rules_test/dep.bzl",
+    ],
+)
diff --git a/src/test/java/com/google/devtools/build/skydoc/SkydocTest.java b/src/test/java/com/google/devtools/build/skydoc/SkydocTest.java
index 5bf7a95704..185547d580 100644
--- a/src/test/java/com/google/devtools/build/skydoc/SkydocTest.java
+++ b/src/test/java/com/google/devtools/build/skydoc/SkydocTest.java
@@ -168,7 +168,10 @@ public final class SkydocTest extends SkylarkTestCase {
         "load('//lib:rule_impl.bzl', 'rule_impl')",
         "load(':docstring.bzl', 'doc_string')",
         "",
-        "some_var = 1",
+        "_hidden_rule = rule(",
+        "    doc = doc_string,",
+        "    implementation = rule_impl,",
+        ")",
         "",
         "dep_rule = rule(",
         "    doc = doc_string,",
@@ -178,7 +181,7 @@ public final class SkydocTest extends SkylarkTestCase {
     scratch.file(
         "/test/main.bzl",
         "load('//lib:rule_impl.bzl', 'rule_impl')",
-        "load('//deps/foo:dep_rule.bzl', 'some_var')",
+        "load('//deps/foo:dep_rule.bzl', 'dep_rule')",
         "",
         "main_rule = rule(",
         "    doc = 'Main rule',",
@@ -194,6 +197,8 @@ public final class SkydocTest extends SkylarkTestCase {
 
     Map<String, RuleInfo> ruleInfoMap = ruleInfoMapBuilder.build();
 
+    // dep_rule is available here, even though it was not defined in main.bzl, because it is
+    // imported in main.bzl. Thus, it's a top-level symbol in main.bzl.
     assertThat(ruleInfoMap.keySet()).containsExactly("main_rule", "dep_rule");
     assertThat(ruleInfoMap.get("main_rule").getDocString()).isEqualTo("Main rule");
     assertThat(ruleInfoMap.get("dep_rule").getDocString()).isEqualTo("Dep rule");
diff --git a/src/test/java/com/google/devtools/build/skydoc/skydoc_e2e_test_runner.sh b/src/test/java/com/google/devtools/build/skydoc/skydoc_e2e_test_runner.sh
index 8a9a1c7010..ac40e9abde 100755
--- a/src/test/java/com/google/devtools/build/skydoc/skydoc_e2e_test_runner.sh
+++ b/src/test/java/com/google/devtools/build/skydoc/skydoc_e2e_test_runner.sh
@@ -19,13 +19,16 @@
 set -u
 
 skydoc_bin=$1
-input_file=$2
-golden_file=$3
+shift 1
+input_file=$1
+shift 1
+golden_file=$1
+shift 1
 
 actual_file="${TEST_TMPDIR}/actual"
 
 set -e
-${skydoc_bin} ${input_file} ${actual_file}
+${skydoc_bin} ${input_file} ${actual_file} $@
 set +e
 
 DIFF="$(diff ${actual_file} ${golden_file})"
diff --git a/src/test/java/com/google/devtools/build/skydoc/skydoc_test.bzl b/src/test/java/com/google/devtools/build/skydoc/skydoc_test.bzl
index c63cedaa08..455b45d859 100644
--- a/src/test/java/com/google/devtools/build/skydoc/skydoc_test.bzl
+++ b/src/test/java/com/google/devtools/build/skydoc/skydoc_test.bzl
@@ -20,7 +20,7 @@
 #    the golden file if changes are made to skydoc.
 """Convenience macro for skydoc tests."""
 
-def skydoc_test(name, input_file, golden_file, skydoc, deps = []):
+def skydoc_test(name, input_file, golden_file, skydoc, deps = [], whitelisted_symbols = []):
     """Creates a test target and golden-file regeneration target for skydoc testing.
 
     The test target is named "{name}_e2e_test".
@@ -34,6 +34,9 @@ def skydoc_test(name, input_file, golden_file, skydoc, deps = []):
           is run on the input file.
       skydoc: The label string of the skydoc binary.
       deps: A list of label strings of skylark file dependencies of the input_file.
+      whitelisted_symbols: A list of strings representing top-level symbols in the input file
+          to generate documentation for. If empty, documentation for all top-level symbols
+          will be generated.
     """
     output_golden_file = "%s_output.txt" % name
     native.sh_test(
@@ -43,7 +46,7 @@ def skydoc_test(name, input_file, golden_file, skydoc, deps = []):
             "$(location %s)" % skydoc,
             "$(location %s)" % input_file,
             "$(location %s)" % golden_file,
-        ],
+        ] + whitelisted_symbols,
         data = [
             input_file,
             golden_file,
@@ -58,6 +61,7 @@ def skydoc_test(name, input_file, golden_file, skydoc, deps = []):
         ] + deps,
         outs = [output_golden_file],
         cmd = "$(location %s) " % skydoc +
-              "$(location %s) $(location %s)" % (input_file, output_golden_file),
+              "$(location %s) $(location %s) " % (input_file, output_golden_file) +
+              " ".join(whitelisted_symbols),
         tools = [skydoc],
     )
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/dep.bzl b/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/dep.bzl
new file mode 100644
index 0000000000..8213e0d794
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/dep.bzl
@@ -0,0 +1,12 @@
+def my_rule_impl(ctx):
+    return struct()
+
+my_rule = rule(
+    implementation = my_rule_impl,
+    doc = "This is the dep rule. It does stuff.",
+    attrs = {
+        "first": attr.label(mandatory = True, doc = "dep's my_rule doc string",
+                            allow_files = True, single_file = True),
+        "second": attr.string_dict(mandatory = True),
+    },
+)
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/golden.txt b/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/golden.txt
new file mode 100644
index 0000000000..457c1b7ef9
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/golden.txt
@@ -0,0 +1,90 @@
+<a name="#my_rule"></a>
+## my_rule
+
+<pre>
+my_rule(name, first, second)
+</pre>
+
+This is my rule. It does stuff.
+
+### Attributes
+
+<table class="params-table">
+  <colgroup>
+    <col class="col-param" />
+    <col class="col-description" />
+  </colgroup>
+  <tbody>
+    <tr id="#my_rule_name">
+      <td><code>name</code></td>
+      <td>
+        String; required
+        <p>
+          A unique name for this rule.
+        </p>
+      </td>
+    </tr>
+    <tr id="#my_rule_first">
+      <td><code>first</code></td>
+      <td>
+        Label; required
+        <p>
+          first my_rule doc string
+        </p>
+      </td>
+    </tr>
+    <tr id="#my_rule_second">
+      <td><code>second</code></td>
+      <td>
+        Dictionary: String -> String; required
+      </td>
+    </tr>
+  </tbody>
+</table>
+
+
+<a name="#whitelisted_dep_rule"></a>
+## whitelisted_dep_rule
+
+<pre>
+whitelisted_dep_rule(name, first, second)
+</pre>
+
+This is the dep rule. It does stuff.
+
+### Attributes
+
+<table class="params-table">
+  <colgroup>
+    <col class="col-param" />
+    <col class="col-description" />
+  </colgroup>
+  <tbody>
+    <tr id="#whitelisted_dep_rule_name">
+      <td><code>name</code></td>
+      <td>
+        String; required
+        <p>
+          A unique name for this rule.
+        </p>
+      </td>
+    </tr>
+    <tr id="#whitelisted_dep_rule_first">
+      <td><code>first</code></td>
+      <td>
+        Label; required
+        <p>
+          dep's my_rule doc string
+        </p>
+      </td>
+    </tr>
+    <tr id="#whitelisted_dep_rule_second">
+      <td><code>second</code></td>
+      <td>
+        Dictionary: String -> String; required
+      </td>
+    </tr>
+  </tbody>
+</table>
+
+
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/input.bzl b/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/input.bzl
new file mode 100644
index 0000000000..3ec3530279
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/filter_rules_test/input.bzl
@@ -0,0 +1,34 @@
+load(":dep.bzl",
+     "my_rule_impl",
+     dep_rule = "my_rule")
+
+def my_rule_impl(ctx):
+    return struct()
+
+my_rule = rule(
+    implementation = my_rule_impl,
+    doc = "This is my rule. It does stuff.",
+    attrs = {
+        "first": attr.label(mandatory = True, doc = "first my_rule doc string",
+                            allow_files = True, single_file = True),
+        "second": attr.string_dict(mandatory = True),
+    },
+)
+
+other_rule = rule(
+    implementation = my_rule_impl,
+    doc = "This is another rule.",
+    attrs = {
+        "test": attr.string_dict(mandatory = True),
+    },
+)
+
+whitelisted_dep_rule = dep_rule
+
+yet_another_rule = rule(
+    implementation = my_rule_impl,
+    doc = "This is yet another rule",
+    attrs = {
+        "test": attr.string_dict(mandatory = True),
+    },
+)