Substituted NoSuchVariableException with manual checks for performance purposes

--
MOS_MIGRATED_REVID=129869968

7 files changed, 44 insertions, 80 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index df26e80295..9c1a5c8a0c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -44,7 +44,6 @@ import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.ClassObject;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.Environment.Extension;
-import com.google.devtools.build.lib.syntax.Environment.NoSuchVariableException;
 import com.google.devtools.build.lib.syntax.Environment.Phase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
@@ -1191,15 +1190,15 @@ public final class PackageFactory {
    */
   public static PackageContext getContext(Environment env, FuncallExpression ast)
       throws EvalException {
-    try {
-      return (PackageContext) env.lookup(PKG_CONTEXT);
-    } catch (NoSuchVariableException e) {
+    PackageContext value = (PackageContext) env.lookup(PKG_CONTEXT);
+    if (value == null) {
       // if PKG_CONTEXT is missing, we're not called from a BUILD file. This happens if someone
       // uses native.some_func() in the wrong place.
       throw new EvalException(ast.getLocation(),
           "The native module cannot be accessed from here. "
           + "Wrap the function in a macro and call it from a BUILD file");
     }
+    return value;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index 9433807410..8b6f05230a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -73,7 +73,6 @@ import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.ClassObject;
 import com.google.devtools.build.lib.syntax.Environment;
-import com.google.devtools.build.lib.syntax.Environment.NoSuchVariableException;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
@@ -525,6 +524,11 @@ public class SkylarkRuleClassFunctions {
           new BuildLangTypedAttributeValuesMap((Map<String, Object>) args[0]);
       try {
         PackageContext pkgContext = (PackageContext) env.lookup(PackageFactory.PKG_CONTEXT);
+        if (pkgContext == null) {
+          throw new EvalException(ast.getLocation(),
+              "Cannot instantiate a rule when loading a .bzl file. Rules can only called from "
+              + "a BUILD file (possibly via a macro).");
+        }
         return RuleFactory.createAndAddRule(
             pkgContext,
             ruleClass,
@@ -534,11 +538,6 @@ public class SkylarkRuleClassFunctions {
             pkgContext.getAttributeContainerFactory().apply(ruleClass));
       } catch (InvalidRuleException | NameConflictException e) {
         throw new EvalException(ast.getLocation(), e.getMessage());
-      } catch (NoSuchVariableException e) {
-        // Thrown when trying to get PackageContext.
-        throw new EvalException(ast.getLocation(),
-            "Cannot instantiate a rule when loading a .bzl file. Rules can only called from "
-            + "a BUILD file (possibly via a macro).");
       }
     }
 
@@ -578,11 +577,9 @@ public class SkylarkRuleClassFunctions {
 
     // Export aspects first since rules can depend on aspects.
     for (String name : globalNames) {
-      Object value;
-      try {
-        value = env.lookup(name);
-      } catch (NoSuchVariableException e) {
-        throw new AssertionError(e);
+      Object value = env.lookup(name);
+      if (name == null) {
+        throw new AssertionError(String.format("No such variable: '%s'", name));
       }
       if (value instanceof SkylarkAspect) {
         SkylarkAspect skylarkAspect = (SkylarkAspect) value;
@@ -593,16 +590,15 @@ public class SkylarkRuleClassFunctions {
     }
 
     for (String name : globalNames) {
-      try {
-        Object value = env.lookup(name);
-        if (value instanceof RuleFunction) {
-          RuleFunction function = (RuleFunction) value;
-          if (function.skylarkLabel == null) {
-            function.export(skylarkLabel, name);
-          }
+      Object value = env.lookup(name);
+      if (value == null) {
+        throw new AssertionError(String.format("No such variable: '%s'", name));
+      }
+      if (value instanceof RuleFunction) {
+        RuleFunction function = (RuleFunction) value;
+        if (function.skylarkLabel == null) {
+          function.export(skylarkLabel, name);
         }
-      } catch (NoSuchVariableException e) {
-        throw new AssertionError(e);
       }
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index e9d7bd93dd..ec504a06ec 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -699,12 +699,7 @@ public final class Environment implements Freezable {
   }
 
   private boolean hasVariable(String varname) {
-    try {
-      lookup(varname);
-      return true;
-    } catch (NoSuchVariableException e) {
-      return false;
-    }
+    return lookup(varname) != null;
   }
 
   /**
@@ -737,10 +732,9 @@ public final class Environment implements Freezable {
   }
 
   /**
-   * @return the value from the environment whose name is "varname".
-   * @throws NoSuchVariableException if the variable is not defined in the Environment.
+   * @return the value from the environment whose name is "varname" if it exists, otherwise null.
    */
-  public Object lookup(String varname) throws NoSuchVariableException {
+  public Object lookup(String varname) {
     // Which Frame to lookup first doesn't matter because update prevents clashes.
     if (lexicalFrame != null) {
       Object lexicalValue = lexicalFrame.get(varname);
@@ -751,7 +745,7 @@ public final class Environment implements Freezable {
     Object globalValue = globalFrame.get(varname);
     Object dynamicValue = dynamicFrame.get(varname);
     if (globalValue == null && dynamicValue == null) {
-      throw new NoSuchVariableException(varname);
+      return null;
     }
     if (knownGlobalVariables != null) {
       knownGlobalVariables.add(varname);
@@ -768,11 +762,11 @@ public final class Environment implements Freezable {
    */
   public Object lookup(String varname, Object defaultValue) {
     Preconditions.checkState(!isSkylark);
-    try {
-      return lookup(varname);
-    } catch (NoSuchVariableException e) {
-      return defaultValue;
+    Object value = lookup(varname);
+    if (value != null) {
+      return value;
     }
+    return defaultValue;
   }
 
   /**
@@ -818,23 +812,6 @@ public final class Environment implements Freezable {
   }
 
   /**
-   * An Exception thrown when an attempt is made to lookup a non-existent
-   * variable in the Environment.
-   */
-  public static class NoSuchVariableException extends Exception {
-    private final String variable;
-    NoSuchVariableException(String variable) {
-      super(null, null, false, false /* don't fillInStackTrace() */);
-      this.variable = variable;
-    }
-
-    @Override
-    public String getMessage() {
-      return "no such variable: " + variable;
-    }
-  }
-
-  /**
    * An Exception thrown when an attempt is made to import a symbol from a file
    * that was not properly loaded.
    */
@@ -842,12 +819,16 @@ public final class Environment implements Freezable {
     LoadFailedException(String importString) {
       super(String.format("file '%s' was not correctly loaded. "
               + "Make sure the 'load' statement appears in the global scope in your file",
-              importString));
+          importString));
+    }
+
+    LoadFailedException(String importString, String symbolString) {
+      super(String.format("file '%s' does not contain symbol '%s'", importString, symbolString));
     }
   }
 
   void importSymbol(String importString, Identifier symbol, String nameInLoadedFile)
-      throws NoSuchVariableException, LoadFailedException {
+      throws LoadFailedException {
     Preconditions.checkState(isGlobal()); // loading is only allowed at global scope.
 
     if (!importedExtensions.containsKey(importString)) {
@@ -856,10 +837,8 @@ public final class Environment implements Freezable {
 
     Extension ext = importedExtensions.get(importString);
 
-    // TODO(bazel-team): Throw a LoadFailedException instead, with an appropriate message.
-    // Throwing a NoSuchVariableException is backward compatible, but backward.
     if (!ext.containsKey(nameInLoadedFile)) {
-      throw new NoSuchVariableException(nameInLoadedFile);
+      throw new LoadFailedException(importString, nameInLoadedFile);
     }
 
     Object value = ext.get(nameInLoadedFile);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index 0f1fc18a6f..f953b05d08 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -72,11 +72,11 @@ public final class Identifier extends Expression {
 
   @Override
   Object doEval(Environment env) throws EvalException {
-    try {
-      return env.lookup(name);
-    } catch (Environment.NoSuchVariableException e) {
+    Object value = env.lookup(name);
+    if (value == null) {
       throw createInvalidIdentifierException();
     }
+    return value;
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
index b1e6c1579a..35fb5d1784 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
@@ -82,7 +82,7 @@ public final class LoadStatement extends Statement {
         // The key is the original name that was used to define the symbol
         // in the loaded bzl file.
         env.importSymbol(imp.getImportString(), name, declared.getName());
-      } catch (Environment.NoSuchVariableException | Environment.LoadFailedException e) {
+      } catch (Environment.LoadFailedException e) {
         throw new EvalException(getLocation(), e.getMessage());
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/compiler/Variable.java b/src/main/java/com/google/devtools/build/lib/syntax/compiler/Variable.java
index 41e2e5d32d..39dbe7b987 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/compiler/Variable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/compiler/Variable.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.syntax.compiler;
 
 import com.google.devtools.build.lib.syntax.ASTNode;
 import com.google.devtools.build.lib.syntax.Environment;
-import com.google.devtools.build.lib.syntax.Environment.NoSuchVariableException;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalExceptionWithStackTrace;
 import com.google.devtools.build.lib.syntax.compiler.DebugInfo.AstAccessors;
@@ -107,15 +106,15 @@ public abstract class Variable {
      */
     public static Object lookupUnboundVariable(Environment global, String variable, ASTNode node)
         throws EvalExceptionWithStackTrace {
-      try {
-        return global.lookup(variable);
-      } catch (NoSuchVariableException e) {
+      Object value = global.lookup(variable);
+      if (value == null) {
         throw new EvalExceptionWithStackTrace(
             new EvalException(
                 node.getLocation(),
                 "local variable '" + variable + "' referenced before assignment"),
             node);
       }
+      return value;
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
index 64d293c5f0..3f01b28467 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.syntax;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.Sets;
@@ -39,12 +40,7 @@ public class EnvironmentTest extends EvaluationTestCase {
   // Test the API directly
   @Test
   public void testLookupAndUpdate() throws Exception {
-    try {
-      lookup("foo");
-      fail();
-    } catch (Environment.NoSuchVariableException e) {
-      assertThat(e).hasMessage("no such variable: foo");
-    }
+    assertNull(lookup("foo"));
     update("foo", "bar");
     assertEquals("bar", lookup("foo"));
   }
@@ -67,12 +63,7 @@ public class EnvironmentTest extends EvaluationTestCase {
   // Test assign through interpreter, lookup through API:
   @Test
   public void testAssign() throws Exception {
-    try {
-      lookup("foo");
-      fail();
-    } catch (Environment.NoSuchVariableException e) {
-      assertThat(e).hasMessage("no such variable: foo");
-    }
+    assertNull(lookup("foo"));
     eval("foo = 'bar'");
     assertEquals("bar", lookup("foo"));
   }