From 5ee7a35140bd63233151d712ccaf4ee2368f1947 Mon Sep 17 00:00:00 2001 From: Luca Di Grazia Date: Sun, 4 Sep 2022 21:37:47 +0200 Subject: [PATCH] Delete Environment.hasVariable It's virtually unused, as callers generally want to know what is the value. https://github.com/bazelbuild/bazel/issues/5637 RELNOTES: None. PiperOrigin-RevId: 210401091 --- .../build/lib/syntax/Environment.java | 47 ++++++---- .../build/lib/syntax/EnvironmentTest.java | 90 ++++++++++--------- 2 files changed, 82 insertions(+), 55 deletions(-) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/Environment.java index 387f6ef8c0b..888642e9567 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/Environment.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/Environment.java @@ -470,7 +470,7 @@ private static final class Continuation { @Nullable final Continuation continuation; /** The lexical Frame of the caller. */ - final Frame lexicalFrame; + final LexicalFrame lexicalFrame; /** The global Frame of the caller. */ final GlobalFrame globalFrame; @@ -482,7 +482,7 @@ private static final class Continuation { @Nullable Continuation continuation, BaseFunction function, @Nullable FuncallExpression caller, - Frame lexicalFrame, + LexicalFrame lexicalFrame, GlobalFrame globalFrame, @Nullable LinkedHashSet knownGlobalVariables) { this.continuation = continuation; @@ -655,7 +655,7 @@ public int hashCode() { * Static Frame for lexical variables that are always looked up in the current Environment * or for the definition Environment of the function currently being evaluated. */ - private Frame lexicalFrame; + private LexicalFrame lexicalFrame; /** * Static Frame for global variables; either the current lexical Frame if evaluation is currently @@ -728,7 +728,7 @@ public int hashCode() { */ void enterScope( BaseFunction function, - Frame lexical, + LexicalFrame lexical, @Nullable FuncallExpression caller, GlobalFrame globals) { continuation = @@ -782,7 +782,7 @@ public void checkLoadingPhase(String symbol, Location loc) throws EvalException * as opposed to inside the body of a function. */ boolean isGlobal() { - return lexicalFrame instanceof GlobalFrame; + return lexicalFrame == null; } @Override @@ -791,6 +791,11 @@ public Mutability mutability() { return dynamicFrame.mutability(); } + /** Returns the current Frame, in which variable side-effects happen. */ + private Frame currentFrame() { + return isGlobal() ? globalFrame : lexicalFrame; + } + /** Returns the global variables for the Environment (not including dynamic bindings). */ public GlobalFrame getGlobals() { return globalFrame; @@ -858,7 +863,6 @@ private Environment( @Nullable String fileContentHashCode, Phase phase, @Nullable Label callerLabel) { - this.lexicalFrame = Preconditions.checkNotNull(globalFrame); this.globalFrame = Preconditions.checkNotNull(globalFrame); this.dynamicFrame = Preconditions.checkNotNull(dynamicFrame); Preconditions.checkArgument(!globalFrame.mutability().isFrozen()); @@ -986,11 +990,21 @@ public Label getCallerLabel() { * @return this Environment, in fluid style */ public Environment setupDynamic(String varname, Object value) { - if (lookup(varname) != null) { + if (dynamicFrame.get(varname) != null) { throw new AssertionError( String.format("Trying to bind dynamic variable '%s' but it is already bound", varname)); } + if (lexicalFrame != null && lexicalFrame.get(varname) != null) { + throw new AssertionError( + String.format("Trying to bind dynamic variable '%s' but it is already bound lexically", + varname)); + } + if (globalFrame.get(varname) != null) { + throw new AssertionError( + String.format("Trying to bind dynamic variable '%s' but it is already bound globally", + varname)); + } try { dynamicFrame.put(this, varname, value); } catch (MutabilityException e) { @@ -1007,7 +1021,7 @@ public Environment setupDynamic(String varname, Object value) { /** Remove variable from local bindings. */ void removeLocalBinding(String varname) { try { - lexicalFrame.remove(this, varname); + currentFrame().remove(this, varname); } catch (MutabilityException e) { throw new AssertionError(e); } @@ -1034,7 +1048,7 @@ public Environment update(String varname, Object value) throws EvalException { null, String.format("Trying to update read-only global variable '%s'", varname)); } try { - lexicalFrame.put(this, varname, Preconditions.checkNotNull(value)); + currentFrame().put(this, varname, Preconditions.checkNotNull(value)); } catch (MutabilityException e) { // Note that since at this time we don't accept the global keyword, and don't have closures, // end users should never be able to mutate a frozen Environment, and a MutabilityException @@ -1082,9 +1096,11 @@ public Environment setupOverride(String varname, Object value) { */ public Object lookup(String varname) { // Lexical frame takes precedence, then globals, then dynamics. - Object lexicalValue = lexicalFrame.get(varname); - if (lexicalValue != null) { - return lexicalValue; + if (lexicalFrame != null) { + Object lexicalValue = lexicalFrame.get(varname); + if (lexicalValue != null) { + return lexicalValue; + } } Object globalValue = globalFrame.get(varname); Object dynamicValue = dynamicFrame.get(varname); @@ -1122,8 +1138,9 @@ public void handleEvent(Event event) { */ public Set getVariableNames() { LinkedHashSet vars = new LinkedHashSet<>(); - vars.addAll(lexicalFrame.getTransitiveBindings().keySet()); - // No-op when globalFrame = lexicalFrame + if (lexicalFrame != null) { + vars.addAll(lexicalFrame.getTransitiveBindings().keySet()); + } vars.addAll(globalFrame.getTransitiveBindings().keySet()); vars.addAll(dynamicFrame.getTransitiveBindings().keySet()); return vars; @@ -1173,7 +1190,7 @@ public ImmutableList listFrames(Location currentLocation) { ImmutableList.Builder frameListBuilder = ImmutableList.builder(); Continuation currentContinuation = continuation; - Frame currentFrame = lexicalFrame; + Frame currentFrame = currentFrame(); // if there's a continuation then the current frame is a lexical frame while (currentContinuation != null) { diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java index 1b4d0eb4567..73f66fb1aab 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java @@ -26,7 +26,9 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Tests of Environment. */ +/** + * Tests of Environment. + */ @RunWith(JUnit4.class) public class EnvironmentTest extends EvaluationTestCase { @@ -93,8 +95,7 @@ public void testBuilderRequiresSemantics() throws Exception { try (Mutability mut = Mutability.create("test")) { IllegalArgumentException expected = assertThrows(IllegalArgumentException.class, () -> Environment.builder(mut).build()); - assertThat(expected) - .hasMessageThat() + assertThat(expected).hasMessageThat() .contains("must call either setSemantics or useDefaultSemantics"); } } @@ -113,13 +114,12 @@ public void testGetVariableNames() throws Exception { .update("wiz", 3); } try (Mutability mut = Mutability.create("inner")) { - innerEnv = - Environment.builder(mut) - .useDefaultSemantics() - .setGlobals(outerEnv.getGlobals()) - .build() - .update("foo", "bat") - .update("quux", 42); + innerEnv = Environment.builder(mut) + .useDefaultSemantics() + .setGlobals(outerEnv.getGlobals()) + .build() + .update("foo", "bat") + .update("quux", 42); } assertThat(outerEnv.getVariableNames()) @@ -245,17 +245,20 @@ public void testFrozen() throws Exception { } @Test - public void testBuiltinsCanBeShadowed() throws Exception { - Environment env = - newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true") - .setup("special_var", 42); - BuildFileAST.eval(env, "special_var = 41"); - assertThat(env.lookup("special_var")).isEqualTo(41); - } + public void testReadOnly() throws Exception { + Environment env = newSkylarkEnvironment() + .setup("special_var", 42) + .update("global_var", 666); + + // We don't even get a runtime exception trying to modify these, + // because we get compile-time exceptions even before we reach runtime! + try { + BuildFileAST.eval(env, "special_var = 41"); + throw new AssertionError("failed to fail"); + } catch (EvalException e) { + assertThat(e).hasMessageThat().contains("Variable special_var is read only"); + } - @Test - public void testVariableIsReferencedBeforeAssignment() throws Exception { - Environment env = newSkylarkEnvironment().update("global_var", 666); try { BuildFileAST.eval(env, "def foo(x): x += global_var; global_var = 36; return x", "foo(1)"); throw new AssertionError("failed to fail"); @@ -271,25 +274,29 @@ public void testVariableIsReferencedBeforeAssignment() throws Exception { @Test public void testVarOrderDeterminism() throws Exception { Mutability parentMutability = Mutability.create("parent env"); - Environment parentEnv = Environment.builder(parentMutability).useDefaultSemantics().build(); + Environment parentEnv = Environment.builder(parentMutability) + .useDefaultSemantics() + .build(); parentEnv.update("a", 1); parentEnv.update("c", 2); parentEnv.update("b", 3); Environment.GlobalFrame parentFrame = parentEnv.getGlobals(); parentMutability.freeze(); Mutability mutability = Mutability.create("testing"); - Environment env = - Environment.builder(mutability).useDefaultSemantics().setGlobals(parentFrame).build(); + Environment env = Environment.builder(mutability) + .useDefaultSemantics() + .setGlobals(parentFrame) + .build(); env.update("x", 4); env.update("z", 5); env.update("y", 6); // The order just has to be deterministic, but for definiteness this test spells out the exact // order returned by the implementation: parent frame before current environment, and bindings // within a frame ordered by when they were added. - assertThat(env.getVariableNames()).containsExactly("a", "c", "b", "x", "z", "y").inOrder(); + assertThat(env.getVariableNames()) + .containsExactly("a", "c", "b", "x", "z", "y").inOrder(); assertThat(env.getGlobals().getTransitiveBindings()) - .containsExactly("a", 1, "c", 2, "b", 3, "x", 4, "z", 5, "y", 6) - .inOrder(); + .containsExactly("a", 1, "c", 2, "b", 3, "x", 4, "z", 5, "y", 6).inOrder(); } @Test @@ -299,30 +306,33 @@ public void testTransitiveHashCodeDeterminism() throws Exception { Extension a = new Extension(ImmutableMap.of(), "a123"); Extension b = new Extension(ImmutableMap.of(), "b456"); Extension c = new Extension(ImmutableMap.of(), "c789"); - Environment env1 = - Environment.builder(Mutability.create("testing1")) - .useDefaultSemantics() - .setImportedExtensions(ImmutableMap.of("a", a, "b", b, "c", c)) - .setFileContentHashCode("z") - .build(); - Environment env2 = - Environment.builder(Mutability.create("testing2")) - .useDefaultSemantics() - .setImportedExtensions(ImmutableMap.of("c", c, "b", b, "a", a)) - .setFileContentHashCode("z") - .build(); + Environment env1 = Environment.builder(Mutability.create("testing1")) + .useDefaultSemantics() + .setImportedExtensions(ImmutableMap.of("a", a, "b", b, "c", c)) + .setFileContentHashCode("z") + .build(); + Environment env2 = Environment.builder(Mutability.create("testing2")) + .useDefaultSemantics() + .setImportedExtensions(ImmutableMap.of("c", c, "b", b, "a", a)) + .setFileContentHashCode("z") + .build(); assertThat(env1.getTransitiveContentHashCode()).isEqualTo(env2.getTransitiveContentHashCode()); } @Test public void testExtensionEqualityDebugging_RhsIsNull() { - assertCheckStateFailsWithMessage(new Extension(ImmutableMap.of(), "abc"), null, "got a null"); + assertCheckStateFailsWithMessage( + new Extension(ImmutableMap.of(), "abc"), + null, + "got a null"); } @Test public void testExtensionEqualityDebugging_RhsHasBadType() { assertCheckStateFailsWithMessage( - new Extension(ImmutableMap.of(), "abc"), 5, "got a java.lang.Integer"); + new Extension(ImmutableMap.of(), "abc"), + 5, + "got a java.lang.Integer"); } @Test