Fix skydoc following of nontrivial relative labels.

Previously, only trivial relative paths (within the same package) were handled correctly. Now paths such as ":foo/bar.bzl" are handled appropriately.

RELNOTES: None.
PiperOrigin-RevId: 206237161

12 files changed, 152 insertions, 48 deletions

diff --git a/src/main/java/com/google/devtools/build/skydoc/BUILD b/src/main/java/com/google/devtools/build/skydoc/BUILD
index b0357f2140..d5b485fe7e 100644
--- a/src/main/java/com/google/devtools/build/skydoc/BUILD
+++ b/src/main/java/com/google/devtools/build/skydoc/BUILD
@@ -43,6 +43,7 @@ java_library(
     deps = [
         "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:syntax",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/apple",
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 01fff20892..2a0bf3d475 100644
--- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
+++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
@@ -18,6 +18,8 @@ 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.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 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;
@@ -38,7 +40,6 @@ import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkImport;
-import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skydoc.fakebuildapi.FakeActionsInfoProvider;
 import com.google.devtools.build.skydoc.fakebuildapi.FakeBuildApiGlobals;
 import com.google.devtools.build.skydoc.fakebuildapi.FakeDefaultInfoProvider;
@@ -67,6 +68,7 @@ import com.google.devtools.build.skydoc.rendering.MarkdownRenderer;
 import com.google.devtools.build.skydoc.rendering.RuleInfo;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -88,7 +90,7 @@ import java.util.stream.Collectors;
  *
  * <p>Usage:</p>
  * <pre>
- *   skydoc {target_skylark_file} {output_file} [symbol_name]...
+ *   skydoc {target_skylark_file_label} {output_file} [symbol_name]...
  * </pre>
  * <p>
  *   Generates documentation for all exported symbols of the target skylark file that are
@@ -107,22 +109,25 @@ public class SkydocMain {
     this.fileAccessor = fileAccessor;
   }
 
-  public static void main(String[] args) throws IOException, InterruptedException {
+  public static void main(String[] args)
+      throws IOException, InterruptedException, LabelSyntaxException {
     if (args.length < 2) {
       throw new IllegalArgumentException("Expected two or more arguments. Usage:\n"
-          + "{skydoc_bin} {target_skylark_file} {output_file} [symbol_names]...");
+          + "{skydoc_bin} {target_skylark_file_label} {output_file} [symbol_names]...");
     }
 
-    String bzlPath = args[0];
+    String targetFileLabelString = args[0];
     String outputPath = args[1];
-    ImmutableSet<String> symbolNames = getSymbolNames(args);
 
-    Path path = Paths.get(bzlPath);
+    Label targetFileLabel =
+        Label.parseAbsolute(targetFileLabelString, ImmutableMap.of());
+    ImmutableSet<String> symbolNames = getSymbolNames(args);
 
     ImmutableMap.Builder<String, RuleInfo> ruleInfoMap = ImmutableMap.builder();
     ImmutableList.Builder<RuleInfo> unknownNamedRules = ImmutableList.builder();
 
-    new SkydocMain(new FilesystemFileAccessor()).eval(path, ruleInfoMap, unknownNamedRules);
+    new SkydocMain(new FilesystemFileAccessor())
+        .eval(targetFileLabel, ruleInfoMap, unknownNamedRules);
 
     MarkdownRenderer renderer = new MarkdownRenderer();
 
@@ -175,7 +180,7 @@ public class SkydocMain {
    * 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 label the label 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'
@@ -184,16 +189,13 @@ public class SkydocMain {
    * @throws InterruptedException if evaluation is interrupted
    */
   public Environment eval(
-      Path path,
+      Label label,
       ImmutableMap.Builder<String, RuleInfo> ruleInfoMap,
       ImmutableList.Builder<RuleInfo> unknownNamedRules)
-      throws InterruptedException, IOException {
-    if (path.isAbsolute()) {
-      path = Paths.get(PathFragment.create(path.toString()).toRelative().getPathString());
-    }
+      throws InterruptedException, IOException, LabelSyntaxException {
 
     List<RuleInfo> ruleInfoList = new ArrayList<>();
-    Environment env = recursiveEval(path, ruleInfoList);
+    Environment env = recursiveEval(label, ruleInfoList);
 
     Map<BaseFunction, RuleInfo> ruleFunctions = ruleInfoList.stream()
         .collect(Collectors.toMap(
@@ -220,14 +222,15 @@ public class SkydocMain {
    * 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 label the label 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 {
+      Label label, List<RuleInfo> ruleInfoList)
+      throws InterruptedException, IOException, LabelSyntaxException {
+    Path path = Paths.get(label.toPathFragment().toString());
     if (pending.contains(path)) {
       throw new IllegalStateException("cycle with " + path);
     } else if (loaded.containsKey(path)) {
@@ -240,11 +243,17 @@ public class SkydocMain {
 
     Map<String, Extension> imports = new HashMap<>();
     for (SkylarkImport anImport : buildFileAST.getImports()) {
-      Path importPath = fromPathFragment(path, anImport.asPathFragment());
-
-      Environment importEnv = recursiveEval(importPath, ruleInfoList);
-
-      imports.put(anImport.getImportString(), new Extension(importEnv));
+      Label relativeLabel = label.getRelative(anImport.getImportString());
+      Path importPath = Paths.get(relativeLabel.toPathFragment().toString());
+
+      try {
+        Environment importEnv = recursiveEval(relativeLabel, ruleInfoList);
+        imports.put(anImport.getImportString(), new Extension(importEnv));
+      } catch (NoSuchFileException noSuchFileException) {
+        throw new IllegalStateException(
+            String.format("File %s imported '%s', yet %s was not found.",
+                path, anImport.getImportString(), importPath));
+      }
     }
 
     Environment env = evalSkylarkBody(buildFileAST, imports, ruleInfoList);
@@ -255,12 +264,6 @@ public class SkydocMain {
     return env;
   }
 
-  private static Path fromPathFragment(Path fromPath, PathFragment pathFragment) {
-    return pathFragment.isAbsolute()
-        ? Paths.get(pathFragment.toRelative().getPathString())
-        : fromPath.resolveSibling(pathFragment.getPathString());
-  }
-
   /**
    * Evaluates the AST from a single skylark file, given the already-resolved imports.
    */
diff --git a/src/test/java/com/google/devtools/build/skydoc/BUILD b/src/test/java/com/google/devtools/build/skydoc/BUILD
index f36e9dbdc8..1df4b52d0e 100644
--- a/src/test/java/com/google/devtools/build/skydoc/BUILD
+++ b/src/test/java/com/google/devtools/build/skydoc/BUILD
@@ -8,7 +8,9 @@ package(
 filegroup(
     name = "srcs",
     testonly = 0,
-    srcs = glob(["**"]),
+    srcs = glob(["**"]) + [
+        "//src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test:srcs",
+    ],
     visibility = ["//src:__pkg__"],
 )
 
@@ -20,6 +22,7 @@ java_test(
     visibility = ["//devtools/blaze/main:__pkg__"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib:syntax",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/skydoc:skydoc_lib",
         "//src/main/java/com/google/devtools/build/skydoc/fakebuildapi",
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 0047e820b4..f9f4d6203e 100644
--- a/src/test/java/com/google/devtools/build/skydoc/SkydocTest.java
+++ b/src/test/java/com/google/devtools/build/skydoc/SkydocTest.java
@@ -20,6 +20,7 @@ import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -27,7 +28,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skydoc.fakebuildapi.FakeDescriptor.Type;
 import com.google.devtools.build.skydoc.rendering.RuleInfo;
 import java.io.IOException;
-import java.nio.file.Paths;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.stream.Collectors;
@@ -79,7 +79,7 @@ public final class SkydocTest extends SkylarkTestCase {
     ImmutableList.Builder<RuleInfo> unexportedRuleInfos = ImmutableList.builder();
 
     skydocMain.eval(
-        Paths.get("/test/test.bzl"),
+        Label.parseAbsoluteUnchecked("//test:test.bzl"),
         ruleInfoMap,
         unexportedRuleInfos);
     Map<String, RuleInfo> ruleInfos = ruleInfoMap.build();
@@ -140,7 +140,7 @@ public final class SkydocTest extends SkylarkTestCase {
     ImmutableList.Builder<RuleInfo> unexportedRuleInfos = ImmutableList.builder();
 
     skydocMain.eval(
-        Paths.get("/test/test.bzl"),
+        Label.parseAbsoluteUnchecked("//test:test.bzl"),
         ruleInfoMap,
         unexportedRuleInfos);
 
@@ -191,7 +191,7 @@ public final class SkydocTest extends SkylarkTestCase {
     ImmutableMap.Builder<String, RuleInfo> ruleInfoMapBuilder = ImmutableMap.builder();
 
     skydocMain.eval(
-        Paths.get("/test/main.bzl"),
+        Label.parseAbsoluteUnchecked("//test:main.bzl"),
         ruleInfoMapBuilder,
         ImmutableList.builder());
 
@@ -228,7 +228,7 @@ public final class SkydocTest extends SkylarkTestCase {
     IllegalStateException expected =
         assertThrows(IllegalStateException.class,
             () -> skydocMain.eval(
-                Paths.get("/test/main.bzl"),
+                Label.parseAbsoluteUnchecked("//test:main.bzl"),
                 ruleInfoMapBuilder,
                 ImmutableList.builder()));
 
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 455b45d859..b2d657edb2 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
@@ -39,12 +39,17 @@ def skydoc_test(name, input_file, golden_file, skydoc, deps = [], whitelisted_sy
           will be generated.
     """
     output_golden_file = "%s_output.txt" % name
+
+    # Skydoc requires an absolute input file label to both load the target file and
+    # track what its target is for the purpose of resolving relative labels.
+    abs_input_file_label = str(Label("//%s" % native.package_name()).relative(input_file))
+
     native.sh_test(
         name = "%s_e2e_test" % name,
         srcs = ["skydoc_e2e_test_runner.sh"],
         args = [
             "$(location %s)" % skydoc,
-            "$(location %s)" % input_file,
+            abs_input_file_label,
             "$(location %s)" % golden_file,
         ] + whitelisted_symbols,
         data = [
@@ -60,8 +65,9 @@ def skydoc_test(name, input_file, golden_file, skydoc, deps = [], whitelisted_sy
             input_file,
         ] + deps,
         outs = [output_golden_file],
+        heuristic_label_expansion = 0,
         cmd = "$(location %s) " % skydoc +
-              "$(location %s) $(location %s) " % (input_file, output_golden_file) +
+              "%s $(location %s) " % (abs_input_file_label, output_golden_file) +
               " ".join(whitelisted_symbols),
         tools = [skydoc],
     )
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
index 3ec3530279..1b7ff1aae6 100644
--- 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
@@ -1,6 +1,8 @@
-load(":dep.bzl",
-     "my_rule_impl",
-     dep_rule = "my_rule")
+load(
+    ":testdata/filter_rules_test/dep.bzl",
+    "my_rule_impl",
+    dep_rule = "my_rule",
+)
 
 def my_rule_impl(ctx):
     return struct()
@@ -9,8 +11,12 @@ 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),
+        "first": attr.label(
+            mandatory = True,
+            doc = "first 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/multiple_files_test/dep.bzl b/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/dep.bzl
index f7c5503309..129c1dd70c 100644
--- a/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/dep.bzl
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/dep.bzl
@@ -1,4 +1,4 @@
-load(":inner_dep.bzl", "inner_rule_impl", "prep_work")
+load(":testdata/multiple_files_test/inner_dep.bzl", "inner_rule_impl", "prep_work")
 
 prep_work()
 
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/input.bzl b/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/input.bzl
index efd5ed460c..2c73da21d5 100644
--- a/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/input.bzl
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/multiple_files_test/input.bzl
@@ -1,11 +1,15 @@
-load(":dep.bzl", "my_rule_impl")
+load(":testdata/multiple_files_test/dep.bzl", "my_rule_impl")
 
 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),
+        "first": attr.label(
+            mandatory = True,
+            doc = "first my_rule doc string",
+            allow_files = True,
+            single_file = True,
+        ),
         "second": attr.string_dict(mandatory = True),
     },
 )
@@ -14,8 +18,12 @@ other_rule = rule(
     implementation = my_rule_impl,
     doc = "This is another rule.",
     attrs = {
-        "third": attr.label(mandatory = True, doc = "third other_rule doc string",
-                            allow_files = True, single_file = True),
+        "third": attr.label(
+            mandatory = True,
+            doc = "third other_rule doc string",
+            allow_files = True,
+            single_file = True,
+        ),
         "fourth": attr.string_dict(mandatory = True),
     },
 )
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/BUILD b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/BUILD
new file mode 100644
index 0000000000..97c2e468d2
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/BUILD
@@ -0,0 +1,14 @@
+filegroup(
+    name = "srcs",
+    testonly = 0,
+    srcs = glob(["**"]),
+    visibility = ["//src:__subpackages__"],
+)
+
+exports_files(
+    [
+        "dep.bzl",
+        "golden.txt",
+        "input.bzl",
+    ],
+)
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/dep.bzl b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/dep.bzl
new file mode 100644
index 0000000000..db2cb089ac
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/dep.bzl
@@ -0,0 +1,3 @@
+def my_rule_impl(ctx):
+    _ignore = [ctx]
+    return struct()
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/golden.txt b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/golden.txt
new file mode 100644
index 0000000000..72b0116063
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/golden.txt
@@ -0,0 +1,45 @@
+<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>
+
+
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/input.bzl b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/input.bzl
new file mode 100644
index 0000000000..f0e989f3e9
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/same_level_file_test/input.bzl
@@ -0,0 +1,15 @@
+load(":dep.bzl", "my_rule_impl")
+
+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),
+    },
+)