Remove onlyLoadingPhase on objects.

The mechanism was easy to workaround (store the object in a different
variable) and a source of bugs. This affected only 'rule', 'native' and
'attr' objects. It turns out the blacklisting was not useful (native and
attr are already filtered, rule is not a problem).

--
MOS_MIGRATED_REVID=101359277

6 files changed, 2 insertions, 34 deletions

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 2f129501cb..6cdb75fb5c 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
@@ -186,7 +186,6 @@ public class SkylarkRuleClassFunctions {
   @SkylarkSignature(name = "rule", doc =
       "Creates a new rule. Store it in a global value, so that it can be loaded and called "
       + "from BUILD files.",
-      onlyLoadingPhase = true,
       returnType = BaseFunction.class,
       mandatoryPositionals = {
         @Param(name = "implementation", type = BaseFunction.class,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index 0309af2a37..3516bd2d7c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -76,9 +76,6 @@ public abstract class BaseFunction {
   // Documentation for variables, if any
   @Nullable protected List<String> paramDoc;
 
-  // True if this function is only allowed during the Loading Phase
-  protected boolean onlyLoadingPhase;
-
   // The types actually enforced by the Skylark runtime, as opposed to those enforced by the JVM,
   // or those displayed to the user in the documentation.
   @Nullable protected List<SkylarkType> enforcedArgumentTypes;
@@ -119,11 +116,6 @@ public abstract class BaseFunction {
     return signature != null;
   }
 
-  /** Returns true if the function is only available during loading phase */
-  public boolean isOnlyLoadingPhase() {
-    return onlyLoadingPhase;
-  }
-
   /**
    * Creates an unconfigured BaseFunction with the given name.
    *
@@ -489,7 +481,6 @@ public abstract class BaseFunction {
         getName(), annotation, unconfiguredDefaultValues, paramDoc, getEnforcedArgumentTypes());
     this.objectType = annotation.objectType().equals(Object.class)
         ? null : annotation.objectType();
-    this.onlyLoadingPhase = annotation.onlyLoadingPhase();
     configure();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
index 70173cebf9..6b94d325bc 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -294,7 +294,6 @@ public class BuiltinFunction extends BaseFunction {
     this.signature = factory.getSignature();
     this.extraArgs = factory.getExtraArgs();
     this.objectType = factory.getObjectType();
-    this.onlyLoadingPhase = factory.isOnlyLoadingPhase();
     configure();
   }
 
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 ea34b621f2..1c599a8b15 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
@@ -89,14 +89,9 @@ public class Environment {
   protected Map<PathFragment, SkylarkEnvironment> importedExtensions;
 
   /**
-   * A set of disable variables propagating through function calling. This is needed because
+   * A set of disabled namespaces propagating through function calling. This is needed because
    * UserDefinedFunctions lock the definition Environment which should be immutable.
    */
-  protected Set<String> disabledVariables = new HashSet<>();
-
-  /**
-   * A set of disable namespaces propagating through function calling. See disabledVariables.
-   */
   protected Set<Class<?>> disabledNameSpaces = new HashSet<>();
 
   /**
@@ -166,9 +161,6 @@ public class Environment {
    *
    */
   public Object lookup(String varname) throws NoSuchVariableException {
-    if (disabledVariables.contains(varname)) {
-      throw new NoSuchVariableException(varname);
-    }
     Object value = env.get(varname);
     if (value == null) {
       if (parent != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java
index 087bdc448d..99362b6b96 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java
@@ -75,7 +75,6 @@ public class SkylarkEnvironment extends Environment implements Serializable {
       // This should never happen.
       throw new IllegalStateException(e);
     }
-    childEnv.disabledVariables = callerEnv.disabledVariables;
     childEnv.disabledNameSpaces = callerEnv.disabledNameSpaces;
     return childEnv;
   }
@@ -163,9 +162,6 @@ public class SkylarkEnvironment extends Environment implements Serializable {
    */
   @Override
   public Object lookup(String varname) throws NoSuchVariableException {
-    if (disabledVariables.contains(varname)) {
-      throw new NoSuchVariableException(varname);
-    }
     Object value = env.get(varname);
     if (value == null) {
       if (parent != null && parent.hasVariable(varname)) {
@@ -201,22 +197,15 @@ public class SkylarkEnvironment extends Environment implements Serializable {
    * only during the loading phase.
    */
   public void disableOnlyLoadingPhaseObjects() {
-    List<String> objectsToRemove = new ArrayList<>();
     List<Class<?>> modulesToRemove = new ArrayList<>();
     for (Map.Entry<String, Object> entry : env.entrySet()) {
       Object object = entry.getValue();
-      if (object instanceof BaseFunction) {
-        if (((BaseFunction) object).isOnlyLoadingPhase()) {
-          objectsToRemove.add(entry.getKey());
-        }
-      } else if (object.getClass().isAnnotationPresent(SkylarkModule.class)) {
+      if (object.getClass().isAnnotationPresent(SkylarkModule.class)) {
         if (object.getClass().getAnnotation(SkylarkModule.class).onlyLoadingPhase()) {
-          objectsToRemove.add(entry.getKey());
           modulesToRemove.add(entry.getValue().getClass());
         }
       }
     }
-    disabledVariables.addAll(objectsToRemove);
     disabledNameSpaces.addAll(modulesToRemove);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java
index 8329a096af..9c0b5bff8f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java
@@ -52,8 +52,6 @@ public @interface SkylarkSignature {
 
   Class<?> returnType() default Object.class;
 
-  boolean onlyLoadingPhase() default false;
-
   // TODO(bazel-team): determine this way whether to accept mutable Lists
   // boolean mutableLists() default false;