Skip to content

Commit

Permalink
Delete Environment.hasVariable
Browse files Browse the repository at this point in the history 
    It's virtually unused, as callers generally want to know what is the value.

    bazelbuild/bazel#5637

    RELNOTES: None.
    PiperOrigin-RevId: 210401091
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent f6c0a3e commit 5ee7a35
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 55 deletions.
47 changes: 32 additions & 15 deletions ...Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String> knownGlobalVariables) {
this.continuation = continuation;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -728,7 +728,7 @@ public int hashCode() {
*/
void enterScope(
BaseFunction function,
Frame lexical,
LexicalFrame lexical,
@Nullable FuncallExpression caller,
GlobalFrame globals) {
continuation =
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
}
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1122,8 +1138,9 @@ public void handleEvent(Event event) {
*/
public Set<String> getVariableNames() {
LinkedHashSet<String> 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;
Expand Down Expand Up @@ -1173,7 +1190,7 @@ public ImmutableList<DebugFrame> listFrames(Location currentLocation) {
ImmutableList.Builder<DebugFrame> 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) {
Expand Down
90 changes: 50 additions & 40 deletions .../bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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");
}
}
Expand All @@ -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())
Expand Down Expand Up @@ -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");
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 5ee7a35

Please sign in to comment.