Fix error handling in WorkspaceFile/WorkspaceAST

--
PiperOrigin-RevId: 145818492
MOS_MIGRATED_REVID=145818492

3 files changed, 28 insertions, 25 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 4514c8fd6b..e0116fceb4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -592,7 +592,7 @@ public class PackageFunction implements SkyFunction {
       BuildFileAST buildFileAST,
       Environment env,
       SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining)
-      throws PackageFunctionException, InterruptedException {
+      throws NoSuchPackageException, InterruptedException {
     Preconditions.checkArgument(!packageId.getRepository().isDefault());
 
     ImmutableList<SkylarkImport> imports = buildFileAST.getImports();
@@ -615,8 +615,7 @@ public class PackageFunction implements SkyFunction {
         return null;
       }
     } catch (SkylarkImportFailedException e) {
-      throw new PackageFunctionException(
-          new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+      throw new BuildFileContainsErrorsException(packageId, e.getMessage());
     }
 
     // Look up and load the imports.
@@ -670,11 +669,9 @@ public class PackageFunction implements SkyFunction {
 
       }
     } catch (SkylarkImportFailedException e) {
-      throw new PackageFunctionException(
-          new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+      throw new BuildFileContainsErrorsException(packageId, e.getMessage());
     } catch (InconsistentFilesystemException e) {
-      throw new PackageFunctionException(
-          new NoSuchPackageException(packageId, e.getMessage(), e), Transience.PERSISTENT);
+      throw new NoSuchPackageException(packageId, e.getMessage(), e);
     }
 
     if (valuesMissing) {
@@ -1191,7 +1188,9 @@ public class PackageFunction implements SkyFunction {
                   astAfterPreprocessing.ast,
                   env,
                   skylarkImportLookupFunctionForInlining);
-        } catch (PackageFunctionException | InterruptedException e) {
+        } catch (NoSuchPackageException e) {
+          throw new PackageFunctionException(e, Transience.PERSISTENT);
+        } catch (InterruptedException e) {
           astCache.invalidate(packageId);
           throw e;
         }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java
index 7207787f5f..8cc2d812c5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java
@@ -119,7 +119,10 @@ public class WorkspaceASTFunction implements SkyFunction {
   }
 
   private static final class WorkspaceASTFunctionException extends SkyFunctionException {
-    public WorkspaceASTFunctionException(Exception e, Transience transience) {
+    WorkspaceASTFunctionException(BuildFileContainsErrorsException e, Transience transience) {
+      super(e, transience);
+    }
+    WorkspaceASTFunctionException(IOException e, Transience transience) {
       super(e, transience);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index faacb3c92d..2372793333 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -17,12 +17,12 @@ package com.google.devtools.build.lib.skyframe;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.WorkspaceFactory;
-import com.google.devtools.build.lib.skyframe.PackageFunction.PackageFunctionException;
 import com.google.devtools.build.lib.skyframe.WorkspaceFileValue.WorkspaceFileKey;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment.Extension;
@@ -34,6 +34,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import java.io.IOException;
 
 /**
  * A SkyFunction to parse WORKSPACE files.
@@ -55,13 +56,13 @@ public class WorkspaceFileFunction implements SkyFunction {
   }
 
   @Override
-  public SkyValue compute(SkyKey skyKey, Environment env) throws WorkspaceFileFunctionException,
-      InterruptedException {
+  public SkyValue compute(SkyKey skyKey, Environment env)
+      throws WorkspaceFileFunctionException, InterruptedException {
 
     WorkspaceFileKey key = (WorkspaceFileKey) skyKey.argument();
     RootedPath workspaceRoot = key.getPath();
-    WorkspaceASTValue workspaceASTValue =
-        (WorkspaceASTValue) env.getValue(WorkspaceASTValue.key(workspaceRoot));
+    WorkspaceASTValue workspaceASTValue = (WorkspaceASTValue) env.getValue(
+        WorkspaceASTValue.key(workspaceRoot));
     if (workspaceASTValue == null) {
       return null;
     }
@@ -91,9 +92,8 @@ public class WorkspaceFileFunction implements SkyFunction {
               directories.getEmbeddedBinariesRoot(),
               directories.getWorkspace());
       if (key.getIndex() > 0) {
-        WorkspaceFileValue prevValue =
-            (WorkspaceFileValue)
-                env.getValue(WorkspaceFileValue.key(key.getPath(), key.getIndex() - 1));
+        WorkspaceFileValue prevValue = (WorkspaceFileValue) env.getValue(
+            WorkspaceFileValue.key(key.getPath(), key.getIndex() - 1));
         if (prevValue == null) {
           return null;
         }
@@ -103,16 +103,13 @@ public class WorkspaceFileFunction implements SkyFunction {
         parser.setParent(prevValue.getPackage(), prevValue.getImportMap(), prevValue.getBindings());
       }
       BuildFileAST ast = workspaceASTValue.getASTs().get(key.getIndex());
-      PackageFunction.SkylarkImportResult importResult =
-          PackageFunction.fetchImportsFromBuildFile(
-              repoWorkspace, rootPackage, ast, env, null);
+      PackageFunction.SkylarkImportResult importResult = PackageFunction.fetchImportsFromBuildFile(
+          repoWorkspace, rootPackage, ast, env, null);
       if (importResult == null) {
         return null;
       }
       parser.execute(ast, importResult.importMap);
-    } catch (PackageFunctionException e) {
-      // TODO(jcater): Unwrap the PackageFunctionException and handle the underlying error.
-      // PFE shouldn't be exposed to callers.
+    } catch (NoSuchPackageException e) {
       throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
     } catch (NameConflictException e) {
       throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
@@ -133,11 +130,15 @@ public class WorkspaceFileFunction implements SkyFunction {
   }
 
   private static final class WorkspaceFileFunctionException extends SkyFunctionException {
-    public WorkspaceFileFunctionException(PackageFunctionException e, Transience transience) {
+    WorkspaceFileFunctionException(NoSuchPackageException e, Transience transience) {
       super(e, transience);
     }
 
-    public WorkspaceFileFunctionException(NameConflictException e, Transience transience) {
+    WorkspaceFileFunctionException(NameConflictException e, Transience transience) {
+      super(e, transience);
+    }
+
+    WorkspaceFileFunctionException(IOException e, Transience transience) {
       super(e, transience);
     }
   }