From 81fb6e7bb8a5b9d818d4974e24a79628020c4dd0 Mon Sep 17 00:00:00 2001 From: Laurent Le Brun Date: Mon, 24 Aug 2015 13:33:18 +0000 Subject: [PATCH] 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 --- .../build/lib/rules/SkylarkRuleClassFunctions.java | 1 - .../devtools/build/lib/syntax/BaseFunction.java | 9 --------- .../devtools/build/lib/syntax/BuiltinFunction.java | 1 - .../devtools/build/lib/syntax/Environment.java | 10 +--------- .../build/lib/syntax/SkylarkEnvironment.java | 13 +------------ .../devtools/build/lib/syntax/SkylarkSignature.java | 2 -- 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 2f129501cb658e..6cdb75fb5c13d6 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 @@ private static String attributeToNative(String oldName, Location loc, boolean is @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 0309af2a37a308..3516bd2d7ced33 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 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 enforcedArgumentTypes; @@ -119,11 +116,6 @@ public boolean isConfigured() { 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 void configure(SkylarkSignature annotation) { 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 70173cebf9c71e..6b94d325bc4418 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 void configure(BuiltinFunction.Factory factory) { 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 ea34b621f24400..1c599a8b15045d 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 @@ private NoneType() {} protected Map 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 disabledVariables = new HashSet<>(); - - /** - * A set of disable namespaces propagating through function calling. See disabledVariables. - */ protected Set> disabledNameSpaces = new HashSet<>(); /** @@ -166,9 +161,6 @@ protected boolean hasVariable(String varname) { * */ 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 087bdc448dce9e..99362b6b965b67 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 static SkylarkEnvironment createEnvironmentForFunctionCalling( // This should never happen. throw new IllegalStateException(e); } - childEnv.disabledVariables = callerEnv.disabledVariables; childEnv.disabledNameSpaces = callerEnv.disabledNameSpaces; return childEnv; } @@ -163,9 +162,6 @@ public boolean isSkylarkEnabled() { */ @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 getVariableType(String varname) { * only during the loading phase. */ public void disableOnlyLoadingPhaseObjects() { - List objectsToRemove = new ArrayList<>(); List> modulesToRemove = new ArrayList<>(); for (Map.Entry 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 8329a096aff618..9c0b5bff8f620f 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 @@ Class returnType() default Object.class; - boolean onlyLoadingPhase() default false; - // TODO(bazel-team): determine this way whether to accept mutable Lists // boolean mutableLists() default false;