Remove isSkylark and eval from Environment.

--
MOS_MIGRATED_REVID=140371603

10 files changed, 49 insertions, 75 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index 837bad3b44..53db2a2aeb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -727,7 +727,6 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
       Map<String, Extension> importMap) {
     Environment env =
         Environment.builder(mutability)
-            .setSkylark()
             .setGlobals(globals)
             .setEventHandler(eventHandler)
             .setFileContentHashCode(astFileContentHashCode)
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
index f02f33a673..3fd7c2009e 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -93,7 +93,6 @@ public class SkylarkRepositoryFunction extends RepositoryFunction {
       com.google.devtools.build.lib.syntax.Environment buildEnv =
           com.google.devtools.build.lib.syntax.Environment.builder(mutability)
               .setGlobals(rule.getRuleClassObject().getRuleDefinitionEnvironment().getGlobals())
-              .setSkylark()
               .setEventHandler(env.getListener())
               .build();
       SkylarkRepositoryContext skylarkRepositoryContext = new SkylarkRepositoryContext(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java
index 3f8158a8c3..1a66a3331c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java
@@ -56,8 +56,7 @@ public final class SkylarkModules {
 
   private static Environment.Frame createGlobals(List<Class<?>> modules) {
     try (Mutability mutability = Mutability.create("SkylarkModules")) {
-      Environment env =
-          Environment.builder(mutability).setSkylark().setGlobals(BazelLibrary.GLOBALS).build();
+      Environment env = Environment.builder(mutability).setGlobals(BazelLibrary.GLOBALS).build();
       for (Class<?> moduleClass : modules) {
         Runtime.registerModuleGlobals(env, moduleClass);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
index 84f6a5f359..f78e96d731 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetBuilder.java
@@ -75,7 +75,6 @@ public final class SkylarkRuleConfiguredTargetBuilder {
       SkylarkRuleContext skylarkRuleContext = new SkylarkRuleContext(ruleContext,
           null);
       Environment env = Environment.builder(mutability)
-          .setSkylark()
           .setCallerLabel(ruleContext.getLabel())
           .setGlobals(
               ruleContext.getRule().getRuleClassObject().getRuleDefinitionEnvironment().getGlobals())
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index 7a058fd5eb..f078c3a154 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -59,7 +59,6 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory {
       }
       Environment env =
           Environment.builder(mutability)
-              .setSkylark()
               .setGlobals(skylarkAspect.getFuncallEnv().getGlobals())
               .setEventHandler(ruleContext.getAnalysisEnvironment().getEventHandler())
               .build(); // NB: loading phase functions are not available: this is analysis already,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
index 473c54c301..d0efaca097 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
@@ -339,6 +339,14 @@ public class BuildFileAST extends ASTNode {
     return last;
   }
 
+  public static Object eval(Environment env, String... input)
+      throws EvalException, InterruptedException {
+    BuildFileAST ast = parseSkylarkString(env.getEventHandler(), input);
+    ValidationEnvironment valid = new ValidationEnvironment(env);
+    valid.validateAst(ast.getStatements(), env.getEventHandler());
+    return ast.eval(env);
+  }
+
   /**
    * Returns a hash code calculated from the string content of the source file of this AST.
    */
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 5ff9a81519..11515ef7d2 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
@@ -36,37 +36,37 @@ import java.util.TreeSet;
 import javax.annotation.Nullable;
 
 /**
- * An Environment is the main entry point to evaluating code in the BUILD language or Skylark.
- * It embodies all the state that is required to evaluate such code,
- * except for the current instruction pointer, which is an {@link ASTNode}
- * whose {@link Statement#exec exec} or {@link Expression#eval eval} method is invoked with
- * this Environment, in a straightforward direct-style AST-walking interpreter.
- * {@link Continuation}-s are explicitly represented, but only partly, with another part being
- * implicit in a series of try-catch statements, to maintain the direct style. One notable trick
- * is how a {@link UserDefinedFunction} implements returning values as the function catching a
- * {@link ReturnStatement.ReturnException} thrown by a {@link ReturnStatement} in the body.
+ * An Environment is the main entry point to evaluating code in the BUILD language or Skylark. It
+ * embodies all the state that is required to evaluate such code, except for the current instruction
+ * pointer, which is an {@link ASTNode} whose {@link Statement#exec exec} or {@link Expression#eval
+ * eval} method is invoked with this Environment, in a straightforward direct-style AST-walking
+ * interpreter. {@link Continuation}-s are explicitly represented, but only partly, with another
+ * part being implicit in a series of try-catch statements, to maintain the direct style. One
+ * notable trick is how a {@link UserDefinedFunction} implements returning values as the function
+ * catching a {@link ReturnStatement.ReturnException} thrown by a {@link ReturnStatement} in the
+ * body.
  *
  * <p>Every Environment has a {@link Mutability} field, and must be used within a function that
- * creates and closes this {@link Mutability} with the try-with-resource pattern.
- * This {@link Mutability} is also used when initializing mutable objects within that Environment;
- * when closed at the end of the computation freezes the Environment and all those objects that
- * then become forever immutable. The pattern enforces the discipline that there should be no
- * dangling mutable Environment, or concurrency between interacting Environment-s.
- * It is also an error to try to mutate an Environment and its objects from another Environment,
- * before the {@link Mutability} is closed.
+ * creates and closes this {@link Mutability} with the try-with-resource pattern. This {@link
+ * Mutability} is also used when initializing mutable objects within that Environment; when closed
+ * at the end of the computation freezes the Environment and all those objects that then become
+ * forever immutable. The pattern enforces the discipline that there should be no dangling mutable
+ * Environment, or concurrency between interacting Environment-s. It is also an error to try to
+ * mutate an Environment and its objects from another Environment, before the {@link Mutability} is
+ * closed.
  *
- * <p>One creates an Environment using the {@link #builder} function, then
- * populates it with {@link #setup}, {@link #setupDynamic} and sometimes {@link #setupOverride},
- * before to evaluate code in it with {@link #eval}, or with {@link BuildFileAST#exec}
- * (where the AST was obtained by passing a {@link ValidationEnvironment} constructed from the
- * Environment to {@link BuildFileAST#parseBuildFile} or {@link BuildFileAST#parseSkylarkFile}).
- * When the computation is over, the frozen Environment can still be queried with {@link #lookup}.
+ * <p>One creates an Environment using the {@link #builder} function, then populates it with {@link
+ * #setup}, {@link #setupDynamic} and sometimes {@link #setupOverride}, before to evaluate code in
+ * it with {@link BuildFileAST#eval}, or with {@link BuildFileAST#exec} (where the AST was obtained
+ * by passing a {@link ValidationEnvironment} constructed from the Environment to {@link
+ * BuildFileAST#parseBuildFile} or {@link BuildFileAST#parseSkylarkFile}). When the computation is
+ * over, the frozen Environment can still be queried with {@link #lookup}.
  *
  * <p>Final fields of an Environment represent its dynamic state, i.e. state that remains the same
- * throughout a given evaluation context, and don't change with source code location,
- * while mutable fields embody its static state, that change with source code location.
- * The seeming paradox is that the words "dynamic" and "static" refer to the point of view
- * of the source code, and here we have a dual point of view.
+ * throughout a given evaluation context, and don't change with source code location, while mutable
+ * fields embody its static state, that change with source code location. The seeming paradox is
+ * that the words "dynamic" and "static" refer to the point of view of the source code, and here we
+ * have a dual point of view.
  */
 public final class Environment implements Freezable {
 
@@ -313,12 +313,6 @@ public final class Environment implements Freezable {
   private final Map<String, Extension> importedExtensions;
 
   /**
-   * Is this Environment being executed in Skylark context?
-   * TODO(laurentlb): Remove from Environment
-   */
-  private boolean isSkylark;
-
-  /**
    * Is this Environment being executed during the loading phase? Many builtin functions are only
    * enabled during the loading phase, and check this flag.
    * TODO(laurentlb): Remove from Environment
@@ -467,7 +461,6 @@ public final class Environment implements Freezable {
    * @param dynamicFrame a frame for the dynamic Environment
    * @param eventHandler an EventHandler for warnings, errors, etc
    * @param importedExtensions Extension-s from which to import bindings with load()
-   * @param isSkylark true if in Skylark context
    * @param fileContentHashCode a hash for the source file being evaluated, if any
    * @param phase the current phase
    * @param callerLabel the label this environment came from
@@ -477,7 +470,6 @@ public final class Environment implements Freezable {
       Frame dynamicFrame,
       EventHandler eventHandler,
       Map<String, Extension> importedExtensions,
-      boolean isSkylark,
       @Nullable String fileContentHashCode,
       Phase phase,
       @Nullable Label callerLabel) {
@@ -487,7 +479,6 @@ public final class Environment implements Freezable {
     Preconditions.checkArgument(dynamicFrame.mutability().isMutable());
     this.eventHandler = eventHandler;
     this.importedExtensions = importedExtensions;
-    this.isSkylark = isSkylark;
     this.phase = phase;
     this.callerLabel = callerLabel;
     this.transitiveHashCode =
@@ -499,7 +490,6 @@ public final class Environment implements Freezable {
    */
   public static class Builder {
     private final Mutability mutability;
-    private boolean isSkylark = false;
     private Phase phase = Phase.ANALYSIS;
     @Nullable private Frame parent;
     @Nullable private EventHandler eventHandler;
@@ -511,10 +501,11 @@ public final class Environment implements Freezable {
       this.mutability = mutability;
     }
 
-    /** Enables Skylark for code read in this Environment. */
+    /**
+     * Obsolete, doesn't do anything.
+     * TODO(laurentlb): To be removed once call-sites have been updated
+     */
     public Builder setSkylark() {
-      Preconditions.checkState(!isSkylark);
-      isSkylark = true;
       return this;
     }
 
@@ -568,7 +559,6 @@ public final class Environment implements Freezable {
           dynamicFrame,
           eventHandler,
           importedExtensions,
-          isSkylark,
           fileContentHashCode,
           phase,
           label);
@@ -861,22 +851,4 @@ public final class Environment implements Freezable {
             !EventKind.ERRORS_AND_WARNINGS.contains(event.getKind()), event);
       }
     };
-
-  /**
-   * Evaluates code some String input without a supporting file.
-   * TODO(laurentlb): Remove from Environment
-   * @param input a list of lines of code to evaluate
-   * @return the value of the last statement if it's an Expression or else null
-   */
-  @Nullable public Object eval(String... input) throws EvalException, InterruptedException {
-    BuildFileAST ast;
-    if (isSkylark) {
-      ast = BuildFileAST.parseSkylarkString(eventHandler, input);
-      ValidationEnvironment valid = new ValidationEnvironment(this);
-      valid.validateAst(ast.getStatements(), eventHandler);
-    } else {
-      ast = BuildFileAST.parseBuildString(eventHandler, input);
-    }
-    return ast.eval(this);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java
index ba88b8b024..6eb95ff727 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallbackFunction.java
@@ -40,7 +40,6 @@ public class SkylarkCallbackFunction {
       throws EvalException, InterruptedException {
     try (Mutability mutability = Mutability.create("callback %s", callback)) {
       Environment env = Environment.builder(mutability)
-          .setSkylark()
           .setEventHandler(funcallEnv.getEventHandler())
           .setGlobals(funcallEnv.getGlobals())
           .build();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
index 228ef85e12..add8601855 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
@@ -175,13 +175,13 @@ public class SkylarkSignatureProcessor {
       return Runtime.NONE;
     } else {
       try (Mutability mutability = Mutability.create("initialization")) {
-        return Environment.builder(mutability)
-            .setSkylark()
-            .setGlobals(Environment.CONSTANTS_ONLY)
-            .setEventHandler(Environment.FAIL_FAST_HANDLER)
-            .build()
-            .update("unbound", Runtime.UNBOUND)
-            .eval(param.defaultValue());
+        Environment env =
+            Environment.builder(mutability)
+                .setGlobals(Environment.CONSTANTS_ONLY)
+                .setEventHandler(Environment.FAIL_FAST_HANDLER)
+                .build()
+                .update("unbound", Runtime.UNBOUND);
+        return BuildFileAST.eval(env, param.defaultValue());
       } catch (Exception e) {
         throw new RuntimeException(String.format(
             "Exception while processing @SkylarkSignature.Param %s, default value %s",
diff --git a/src/main/java/com/google/devtools/skylark/Skylark.java b/src/main/java/com/google/devtools/skylark/Skylark.java
index 12f9a16cc5..c0c0e69824 100644
--- a/src/main/java/com/google/devtools/skylark/Skylark.java
+++ b/src/main/java/com/google/devtools/skylark/Skylark.java
@@ -15,6 +15,7 @@ package com.google.devtools.skylark;
 
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.Mutability;
 import com.google.devtools.build.lib.syntax.Printer;
@@ -44,7 +45,6 @@ class Skylark {
   private final Mutability mutability = Mutability.create("interpreter");
   private final Environment env =
       Environment.builder(mutability)
-          .setSkylark()
           .setGlobals(Environment.DEFAULT_GLOBALS)
           .setEventHandler(PRINT_HANDLER)
           .build();
@@ -74,7 +74,7 @@ class Skylark {
     String input;
     while ((input = prompt()) != null) {
       try {
-        Object result = env.eval(input);
+        Object result = BuildFileAST.eval(env, input);
         if (result != null) {
           System.out.println(Printer.repr(result));
         }
@@ -86,7 +86,7 @@ class Skylark {
 
   public static void main(String[] args) {
     if (args.length == 0) {
-       new Skylark().readEvalPrintLoop();
+      new Skylark().readEvalPrintLoop();
     } else {
       System.err.println("no argument expected");
       System.exit(1);