Flip the flag "--incompatible_static_name_resolution"

This is an incompatible change, but it breaks in a single case: global = 2 def fct(x): if x: global = 3 print(global) Before the change, `global` would refer to either the global variable or the local variable, depending on the value of `x`. After the change, it is either `3` or undefined (runtime error). Fixes #5637 RELNOTES: --incompatible_static_name_resolution is no unable by default PiperOrigin-RevId: 222242205
bazelbuild · Nov 20, 2018 · 3a979f7 · 3a979f7
1 parent 1a8eb1e
commit 3a979f7
Show file tree

Hide file tree

Showing 7 changed files with 19 additions and 10 deletions.
diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md
@@ -542,7 +542,7 @@ for background and examples.
 The proposal is not fully implemented yet.
 
 *   Flag: `--incompatible_static_name_resolution`
-*   Default: `false`
+*   Default: `true`
 *   Tracking issue: [#5637](https://github.com/bazelbuild/bazel/issues/5637)
 
 

diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -478,7 +478,7 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
 
   @Option(
       name = "incompatible_static_name_resolution",
-      defaultValue = "false",
+      defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
       effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
       metadataTags = {

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -231,7 +231,7 @@ public static Builder builderWithDefaults() {
           .incompatibleRangeType(true)
           .incompatibleRemoveNativeGitRepository(true)
           .incompatibleRemoveNativeHttpArchive(true)
-          .incompatibleStaticNameResolution(false)
+          .incompatibleStaticNameResolution(true)
           .incompatibleStricArgumentOrdering(false)
           .incompatibleStringIsNotIterable(false)
           .internalSkylarkFlagTestCanary(false)

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
@@ -217,7 +217,7 @@ public void testVariableIsReferencedBeforeAssignment() throws Exception {
     } catch (EvalExceptionWithStackTrace e) {
       assertThat(e)
           .hasMessageThat()
-          .contains("Variable 'global_var' is referenced before assignment.");
+          .contains("local variable 'global_var' is referenced before assignment.");
     }
   }
 

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -357,8 +357,13 @@ public void testListComprehensionsWithFiltering() throws Exception {
 
   @Test
   public void testListComprehensionDefinitionOrder() throws Exception {
-    newTest().testIfErrorContains("name 'y' is not defined",
-        "[x for x in (1, 2) if y for y in (3, 4)]");
+    new BuildTest()
+        .testIfErrorContains("name 'y' is not defined", "[x for x in (1, 2) if y for y in (3, 4)]");
+
+    new SkylarkTest()
+        .testIfErrorContains(
+            "local variable 'y' is referenced before assignment",
+            "[x for x in (1, 2) if y for y in (3, 4)]");
   }
 
   @Test

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -105,7 +105,8 @@ public void testFunctionDefLocalGlobalScope() throws Exception {
 
   @Test
   public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exception {
-    checkEvalErrorContains("Variable 'a' is referenced before assignment.",
+    checkEvalErrorContains(
+        "local variable 'a' is referenced before assignment.",
         "a = 1",
         "def func():",
         "  b = a",
@@ -116,7 +117,8 @@ public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exce
 
   @Test
   public void testFunctionDefLocalVariableReferencedInCallBeforeAssignment() throws Exception {
-    checkEvalErrorContains("Variable 'a' is referenced before assignment.",
+    checkEvalErrorContains(
+        "local variable 'a' is referenced before assignment.",
         "def dummy(x):",
         "  pass",
         "a = 1",

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -1785,7 +1785,7 @@ public void testFunctionCallOrdering() throws Exception {
   public void testFunctionCallBadOrdering() throws Exception {
     new SkylarkTest()
         .testIfErrorContains(
-            "name 'foo' is not defined",
+            "global variable 'foo' is referenced before assignment.",
             "def func(): return foo() * 2",
             "x = func()",
             "def foo(): return 2");
@@ -2207,7 +2207,9 @@ public void testStructMethodNotDefined() throws Exception {
   @Test
   public void testListComprehensionsDoNotLeakVariables() throws Exception {
     checkEvalErrorContains(
-        "name 'a' is not defined",
+        // TODO(laurentlb): This happens because the variable gets undefined after the list
+        // comprehension. We should do better.
+        "local variable 'a' is referenced before assignment.",
         "def foo():",
         "  a = 10",
         "  b = [a for a in range(3)]",