Use Labels, rather than PathFragments, to represent Skylark loads internally. The load location for a Skylark Aspect is specified via a PathFragment, for consistency with current non-Aspect Skylark loads.

This should be a semantics-preserving change for users. In a subsequent CL, I'll change the Skylark syntax to allow load statements to use labels as well as paths, with the goal of eventually deprecating the latter.

Also:

- Removed the hack for handling relative loads in the prelude file.

- Refactored some redundant functionality in PackageFunction and SkylarkImportLookupFunction for handling loads.

- Removed the ability to put the BUILD file for the package containing a Skylark file under a different package root than the Skylark file itself. This functionality isn't currently used and is inconsistent with Blaze's handling of the package path elsewhere.

- Added BUILD files to a number of tests that load Skylark files; this is consistent with the requirement that all Skylark files need to be part of some package.

- Changed the constants used to set the location of the prelude file from paths to labels.

--
MOS_MIGRATED_REVID=107741568

1 files changed, 56 insertions, 128 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index e5e4771d9f..c030cccb15 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -14,10 +14,8 @@
 
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
-import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.syntax.Runtime;
@@ -32,113 +30,79 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
 import java.io.IOException;
-import java.util.List;
-import java.util.concurrent.atomic.AtomicReference;
 
 import javax.annotation.Nullable;
 
 /**
- * A SkyFunction for {@link ASTFileLookupValue}s. Tries to locate a file and load it as a
- * syntax tree and cache the resulting {@link BuildFileAST}. If the file doesn't exist
- * the function doesn't fail but returns a specific NO_FILE ASTLookupValue.
+ * A SkyFunction for {@link ASTFileLookupValue}s.
+ *
+ * <p> Given a {@link Label} referencing a Skylark file, loads it as a syntax tree
+ * ({@link BuildFileAST}). The Label must be absolute, and must not reference the special
+ * {@code external} package. If the file (or the package containing it) doesn't exist, the
+ * function doesn't fail, but instead returns a specific {@code NO_FILE} {@link ASTFileLookupValue}.
  */
 public class ASTFileLookupFunction implements SkyFunction {
 
-  private abstract static class FileLookupResult {
-    /** Returns whether the file lookup was successful. */
-    public abstract boolean lookupSuccessful();
-
-    /** If {@code lookupSuccessful()}, returns the {@link RootedPath} to the file. */
-    public abstract RootedPath rootedPath();
-
-    /** If {@code lookupSuccessful()}, returns the file's size, in bytes. */
-    public abstract long fileSize();
-
-    static FileLookupResult noFile() {
-      return UnsuccessfulFileResult.INSTANCE;
-    }
-
-    static FileLookupResult file(RootedPath rootedPath, long fileSize) {
-      return new SuccessfulFileResult(rootedPath, fileSize);
-    }
-
-    private static class SuccessfulFileResult extends FileLookupResult {
-      private final RootedPath rootedPath;
-      private final long fileSize;
-
-      private SuccessfulFileResult(RootedPath rootedPath, long fileSize) {
-        this.rootedPath = rootedPath;
-        this.fileSize = fileSize;
-      }
-
-      @Override
-      public boolean lookupSuccessful() {
-        return true;
-      }
-
-      @Override
-      public RootedPath rootedPath() {
-        return rootedPath;
-      }
-
-      @Override
-      public long fileSize() {
-        return fileSize;
-      }
-    }
-
-    private static class UnsuccessfulFileResult extends FileLookupResult {
-      private static final UnsuccessfulFileResult INSTANCE = new UnsuccessfulFileResult();
-      private UnsuccessfulFileResult() {
-      }
-
-      @Override
-      public boolean lookupSuccessful() {
-        return false;
-      }
-
-      @Override
-      public RootedPath rootedPath() {
-        throw new IllegalStateException("unsuccessful lookup");
-      }
-
-      @Override
-      public long fileSize() {
-        throw new IllegalStateException("unsuccessful lookup");
-      }
-    }
-  }
-
-  private final AtomicReference<PathPackageLocator> pkgLocator;
   private final RuleClassProvider ruleClassProvider;
 
-  public ASTFileLookupFunction(AtomicReference<PathPackageLocator> pkgLocator,
-      RuleClassProvider ruleClassProvider) {
-    this.pkgLocator = pkgLocator;
+  public ASTFileLookupFunction(RuleClassProvider ruleClassProvider) {
     this.ruleClassProvider = ruleClassProvider;
   }
 
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException,
       InterruptedException {
-    PackageIdentifier key = (PackageIdentifier) skyKey.argument();
-    PathFragment astFilePathFragment = key.getPackageFragment();
-    FileLookupResult lookupResult = getASTFile(env, key);
-    if (lookupResult == null) {
+    Label fileLabel = (Label) skyKey.argument();
+    PathFragment filePathFragment = fileLabel.toPathFragment();
+
+    //
+    // Determine whether the package designated by fileLabel exists.
+    //
+    SkyKey pkgSkyKey = PackageLookupValue.key(fileLabel.getPackageIdentifier());
+    PackageLookupValue pkgLookupValue = null;
+    pkgLookupValue = (PackageLookupValue) env.getValue(pkgSkyKey);
+    if (pkgLookupValue == null) {
       return null;
     }
-    if (!lookupResult.lookupSuccessful()) {
-      return ASTFileLookupValue.noFile();
+    if (!pkgLookupValue.packageExists()) {
+      return ASTFileLookupValue.forBadPackage(fileLabel, pkgLookupValue.getErrorMsg());
     }
+
+    //
+    // Determine whether the file designated by fileLabel exists.
+    //
+    Path packageRoot = pkgLookupValue.getRoot();
+    RootedPath rootedPath = RootedPath.toRootedPath(packageRoot, filePathFragment);
+    SkyKey fileSkyKey = FileValue.key(rootedPath);
+    FileValue fileValue = null;
+    try {
+      fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class,
+          FileSymlinkException.class, InconsistentFilesystemException.class);
+    } catch (IOException | FileSymlinkException e) {
+      throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
+          Transience.PERSISTENT);
+    } catch (InconsistentFilesystemException e) {
+      throw new ASTLookupFunctionException(e, Transience.PERSISTENT);
+    }
+    if (fileValue == null) {
+      return null;
+    }
+    if (!fileValue.isFile()) {
+      return ASTFileLookupValue.forBadFile(fileLabel);
+    }
+
+    //
+    // Both the package and the file exist; load the file and parse it as an AST.
+    //
     BuildFileAST ast = null;
-    Path path = lookupResult.rootedPath().asPath();
-    long fileSize = lookupResult.fileSize();
-    // Skylark files end with bzl.
-    boolean parseAsSkylark = astFilePathFragment.getPathString().endsWith(".bzl");
+    Path path = rootedPath.asPath();
+    // Skylark files end with bzl
+    boolean parseAsSkylark = filePathFragment.getPathString().endsWith(".bzl");
     try {
+      long astFileSize = fileValue.getSize();
       if (parseAsSkylark) {
         try (Mutability mutability = Mutability.create("validate")) {
-            ast = BuildFileAST.parseSkylarkFile(path, fileSize, env.getListener(),
+            ast = BuildFileAST.parseSkylarkFile(path, astFileSize, env.getListener(),
                 new ValidationEnvironment(
                     ruleClassProvider.createSkylarkRuleClassEnvironment(
                         mutability,
@@ -150,50 +114,14 @@ public class ASTFileLookupFunction implements SkyFunction {
                     .setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE)));
         }
       } else {
-        ast = BuildFileAST.parseBuildFile(path, fileSize, env.getListener(), false);
+        ast = BuildFileAST.parseBuildFile(path, astFileSize, env.getListener(), false);
       }
     } catch (IOException e) {
-        throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(
-            e.getMessage()), Transience.TRANSIENT);
-    }
-    return ASTFileLookupValue.withFile(ast);
-  }
-
-  private FileLookupResult getASTFile(Environment env, PackageIdentifier key)
-      throws ASTLookupFunctionException {
-    List<Path> candidateRoots;
-    if (!key.getRepository().isDefault()) {
-      RepositoryValue repository =
-          (RepositoryValue) env.getValue(RepositoryValue.key(key.getRepository()));
-      if (repository == null) {
-        return null;
-      }
-
-      candidateRoots = ImmutableList.of(repository.getPath());
-    } else {
-      candidateRoots = pkgLocator.get().getPathEntries();
+      throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
+          Transience.TRANSIENT);
     }
 
-    for (Path root : candidateRoots) {
-      RootedPath rootedPath = RootedPath.toRootedPath(root, key.getPackageFragment());
-      FileValue fileValue;
-      try {
-        fileValue = (FileValue) env.getValueOrThrow(FileValue.key(rootedPath),
-            IOException.class, FileSymlinkException.class, InconsistentFilesystemException.class);
-      } catch (IOException | FileSymlinkException e) {
-        throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(
-            e.getMessage()), Transience.PERSISTENT);
-      } catch (InconsistentFilesystemException e) {
-        throw new ASTLookupFunctionException(e, Transience.PERSISTENT);
-      }
-      if (fileValue == null) {
-        return null;
-      }
-      if (fileValue.isFile()) {
-        return FileLookupResult.file(rootedPath, fileValue.getSize());
-      }
-    }
-    return FileLookupResult.noFile();
+    return ASTFileLookupValue.withFile(ast);
   }
 
   @Nullable