Cleanup in ValidationEnvironment wrt to user-defined functions

--
MOS_MIGRATED_REVID=91216722

4 files changed, 4 insertions, 62 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
index 57239f3dbf..256abb8468 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.syntax.SkylarkType.SkylarkFunctionType;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -79,8 +78,7 @@ public class FunctionDefStatement extends Statement {
 
   @Override
   void validate(final ValidationEnvironment env) throws EvalException {
-    SkylarkFunctionType type = SkylarkFunctionType.of(ident.getName());
-    ValidationEnvironment localEnv = new ValidationEnvironment(env, type);
+    ValidationEnvironment localEnv = new ValidationEnvironment(env);
     FunctionSignature sig = args.getSignature();
     FunctionSignature.Shape shape = sig.getShape();
     ImmutableList<String> names = sig.getNames();
@@ -108,8 +106,6 @@ public class FunctionDefStatement extends Statement {
     for (Statement stmts : statements) {
       stmts.validate(localEnv);
     }
-    env.updateFunction(ident.getName(), type, getLocation());
-    // Register a dummy return value with an incompatible type if there was no return statement.
-    type.setReturnType(SkylarkType.NONE, getLocation());
+    env.declare(ident.getName(), getLocation());
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index 0ecf5823cb..6956c97437 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -63,7 +63,7 @@ public class ReturnStatement extends Statement {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    if (env.getCurrentFunction() == null) {
+    if (env.isTopLevel()) {
       throw new EvalException(getLocation(), "Return statements must be inside a function");
     }
     returnExpression.validate(env);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index 78ab975b72..01fb872a0e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -510,11 +510,9 @@ public abstract class SkylarkType {
   /**
    * A class representing the type of a Skylark function.
    */
-  // TODO(bazel-team): move the side-effect out of the type object, into the validation environment?
   public static final class SkylarkFunctionType extends SkylarkType {
     private final String name;
     @Nullable private SkylarkType returnType;
-    @Nullable private Location returnTypeLoc;
 
     @Override public SkylarkType intersectWith(SkylarkType other) {
       // This gives the wrong result if both return types are incompatibly updated later!
@@ -563,24 +561,6 @@ public abstract class SkylarkType {
     public SkylarkType getReturnType() {
       return returnType;
     }
-
-    /**
-     * Sets the return type of the function type if it's compatible with the existing return type.
-     */
-    public void setReturnType(SkylarkType newReturnType, Location newLoc) throws EvalException {
-      if (returnType == null) {
-        returnType = newReturnType;
-        returnTypeLoc = newLoc;
-      } else if (newReturnType != SkylarkType.NONE) {
-        // At validation-time, we allow NONE in any type, just like null in Java.
-        SkylarkType intersectionType =
-            returnType.infer(newReturnType, "return type of " + name, newLoc, returnTypeLoc);
-        if (!returnType.equals(intersectionType)) {
-          returnType = intersectionType;
-          returnTypeLoc = newLoc;
-        }
-      }
-    }
   }
 
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index 366cf6e78b..2bbdbbb584 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.syntax;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.syntax.SkylarkType.SkylarkFunctionType;
 
 import java.util.HashMap;
 import java.util.HashSet;
@@ -45,9 +44,6 @@ public class ValidationEnvironment {
   // branches of if-else statements.
   private Stack<Set<String>> futureReadOnlyVariables = new Stack<>();
 
-  // The function we are currently validating.
-  private SkylarkFunctionType currentFunction;
-
   // Whether this validation environment is not modified therefore clonable or not.
   private boolean clonable;
 
@@ -85,11 +81,10 @@ public class ValidationEnvironment {
   /**
    * Creates a local ValidationEnvironment to validate user defined function bodies.
    */
-  public ValidationEnvironment(ValidationEnvironment parent, SkylarkFunctionType currentFunction) {
+  public ValidationEnvironment(ValidationEnvironment parent) {
     // Don't copy readOnlyVariables: Variables may shadow global values.
     this.parent = parent;
     this.variableTypes = new HashMap<String, SkylarkType>();
-    this.currentFunction = currentFunction;
     this.clonable = false;
   }
 
@@ -132,40 +127,11 @@ public class ValidationEnvironment {
         || topLevel().variableTypes.containsKey(varname);
   }
 
-  /**
-   * Returns the type of the existing variable.
-   */
-  public SkylarkType getVartype(String varname) {
-    SkylarkType type = variableTypes.get(varname);
-    if (type == null && parent != null) {
-      type = parent.getVartype(varname);
-    }
-    return Preconditions.checkNotNull(type,
-        String.format("Variable %s is not found in the validation environment", varname));
-  }
-
-  public SkylarkFunctionType getCurrentFunction() {
-    return currentFunction;
-  }
-
   private ValidationEnvironment topLevel() {
     return Preconditions.checkNotNull(parent == null ? this : parent);
   }
 
   /**
-   * Adds a user defined function to the validation environment is not exists.
-   */
-  public void updateFunction(String name, SkylarkFunctionType type, Location loc)
-      throws EvalException {
-    checkReadonly(name, loc);
-    if (variableTypes.containsKey(name)) {
-      throw new EvalException(loc, "function " + name + " already exists");
-    }
-    variableTypes.put(name, type);
-    clonable = false;
-  }
-
-  /**
    * Starts a session with temporarily disabled readonly checking for variables between branches.
    * This is useful to validate control flows like if-else when we know that certain parts of the
    * code cannot both be executed.