Make `incompatible_disallow_toplevel_if_statement` default to true.

RELNOTES: Top-level `if` statements are now forbidden.
PiperOrigin-RevId: 165469101

6 files changed, 22 insertions, 105 deletions

diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md
index 8b47c899fd..ac9ab91360 100644
--- a/site/docs/skylark/backward-compatibility.md
+++ b/site/docs/skylark/backward-compatibility.md
@@ -112,7 +112,7 @@ value has a single declaration. This restriction is consistent with the idea
 that global values cannot be redefined.
 
 *   Flag: `--incompatible_disallow_toplevel_if_statement`
-*   Default: `false`
+*   Default: `true`
 
 
 ## Comprehensions variables
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
index a17887feb5..99d1278da9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
@@ -135,7 +135,7 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
 
   @Option(
     name = "incompatible_disallow_toplevel_if_statement",
-    defaultValue = "false",
+    defaultValue = "true",
     category = "incompatible changes",
     documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
     effectTags = {OptionEffectTag.UNKNOWN},
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index 85979f438a..dd6a46af76 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -1329,8 +1329,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
     new SkylarkTest()
         .testEval(
             "d = {1: 'foo', 2: 'bar', 3: 'baz'}\n"
-                + "if len(d) != 3: fail('clear 1')\n"
-                + "if d.clear() != None: fail('clear 2')\n"
+                + "len(d) == 3 or fail('clear 1')\n"
+                + "d.clear() == None or fail('clear 2')\n"
                 + "d",
             "{}");
   }
@@ -1341,12 +1341,12 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .testIfErrorContains(
             "KeyError: 1",
             "d = {1: 'foo', 2: 'bar', 3: 'baz'}\n"
-                + "if len(d) != 3: fail('pop 1')\n"
-                + "if d.pop(2) != 'bar': fail('pop 2')\n"
-                + "if d.pop(3, 'quux') != 'baz': fail('pop 3a')\n"
-                + "if d.pop(3, 'quux') != 'quux': fail('pop 3b')\n"
-                + "if d.pop(1) != 'foo': fail('pop 1')\n"
-                + "if d != {}: fail('pop 0')\n"
+                + "len(d) == 3 or fail('pop 1')\n"
+                + "d.pop(2) == 'bar' or fail('pop 2')\n"
+                + "d.pop(3, 'quux') == 'baz' or fail('pop 3a')\n"
+                + "d.pop(3, 'quux') == 'quux' or fail('pop 3b')\n"
+                + "d.pop(1) == 'foo' or fail('pop 1')\n"
+                + "d == {} or fail('pop 0')\n"
                 + "d.pop(1)");
   }
 
@@ -1356,11 +1356,11 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .testIfErrorContains(
             "popitem(): dictionary is empty",
             "d = {2: 'bar', 3: 'baz', 1: 'foo'}\n"
-                + "if len(d) != 3: fail('popitem 0')\n"
-                + "if d.popitem() != (2, 'bar'): fail('popitem 2')\n"
-                + "if d.popitem() != (3, 'baz'): fail('popitem 3')\n"
-                + "if d.popitem() != (1, 'foo'): fail('popitem 1')\n"
-                + "if d != {}: fail('popitem 4')\n"
+                + "len(d) == 3 or fail('popitem 0')\n"
+                + "d.popitem() == (2, 'bar') or fail('popitem 2')\n"
+                + "d.popitem() == (3, 'baz') or fail('popitem 3')\n"
+                + "d.popitem() == (1, 'foo') or fail('popitem 1')\n"
+                + "d == {} or fail('popitem 4')\n"
                 + "d.popitem()");
   }
 
@@ -1379,11 +1379,11 @@ public class MethodLibraryTest extends EvaluationTestCase {
     new SkylarkTest()
         .testEval(
             "d = {2: 'bar', 1: 'foo'}\n"
-                + "if len(d) != 2: fail('setdefault 0')\n"
-                + "if d.setdefault(1, 'a') != 'foo': fail('setdefault 1')\n"
-                + "if d.setdefault(2) != 'bar': fail('setdefault 2')\n"
-                + "if d.setdefault(3) != None: fail('setdefault 3')\n"
-                + "if d.setdefault(4, 'b') != 'b': fail('setdefault 4')\n"
+                + "len(d) == 2 or fail('setdefault 0')\n"
+                + "d.setdefault(1, 'a') == 'foo' or fail('setdefault 1')\n"
+                + "d.setdefault(2) == 'bar' or fail('setdefault 2')\n"
+                + "d.setdefault(3) == None or fail('setdefault 3')\n"
+                + "d.setdefault(4, 'b') == 'b' or fail('setdefault 4')\n"
                 + "d",
             "{1: 'foo', 2: 'bar', 3: None, 4: 'b'}");
   }
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
index 86e7964041..374ac78531 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -1193,14 +1193,6 @@ public class SkylarkEvaluationTest extends EvaluationTest {
   }
 
   @Test
-  public void testTopLevelDict() throws Exception {
-    new SkylarkTest().setUp("if 1:",
-      "  v = 'a'",
-      "else:",
-      "  v = 'b'").testLookup("v", "a");
-  }
-
-  @Test
   public void testUserFunctionKeywordArgs() throws Exception {
     new SkylarkTest().setUp("def foo(a, b, c):",
         "  return a + b + c", "s = foo(1, c=2, b=3)")
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
index df518ca67d..98695c56e0 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkListTest.java
@@ -216,21 +216,13 @@ public class SkylarkListTest extends EvaluationTestCase {
 
   @Test
   public void testConcatListNotEmpty() throws Exception {
-    eval("l = [1, 2] + [3, 4]",
-        "if l:",
-        "  v = 1",
-        "else:",
-        "  v = 0");
+    eval("l = [1, 2] + [3, 4]", "v = 1 if l else 0");
     assertThat(lookup("v")).isEqualTo(1);
   }
 
   @Test
   public void testConcatListEmpty() throws Exception {
-    eval("l = [] + []",
-        "if l:",
-        "  v = 1",
-        "else:",
-        "  v = 0");
+    eval("l = [] + []", "v = 1 if l else 0");
     assertThat(lookup("v")).isEqualTo(0);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
index 27b84423b6..014d3be53a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -175,73 +175,6 @@ public class ValidationTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testReadOnlyWorksForSimpleBranching() {
-    parse("if 1:", "  v = 'a'", "else:", "  v = 'b'");
-  }
-
-  @Test
-  public void testReadOnlyWorksForNestedBranching() {
-    parse(
-        "if 1:",
-        "  if 0:",
-        "    v = 'a'",
-        "  else:",
-        "    v = 'b'",
-        "else:",
-        "  if 0:",
-        "    v = 'c'",
-        "  else:",
-        "    v = 'd'\n");
-  }
-
-  @Test
-  public void testReadOnlyWorksForDifferentLevelBranches() {
-    checkError("Variable v is read only", "if 1:", "  if 1:", "    v = 'a'", "  v = 'b'\n");
-  }
-
-  @Test
-  public void testReadOnlyWorksWithinSimpleBranch() {
-    checkError(
-        "Variable v is read only", "if 1:", "  v = 'a'", "else:", "  v = 'b'", "  v = 'c'\n");
-  }
-
-  @Test
-  public void testReadOnlyWorksWithinNestedBranch() {
-    checkError(
-        "Variable v is read only",
-        "if 1:",
-        "  v = 'a'",
-        "else:",
-        "  if 1:",
-        "    v = 'b'",
-        "  else:",
-        "    v = 'c'",
-        "    v = 'd'\n");
-  }
-
-  @Test
-  public void testReadOnlyWorksAfterSimpleBranch() {
-    checkError("Variable v is read only", "if 1:", "  v = 'a'", "else:", "  w = 'a'", "v = 'b'");
-  }
-
-  @Test
-  public void testReadOnlyWorksAfterNestedBranch() {
-    checkError("Variable v is read only", "if 1:", "  if 1:", "    v = 'a'", "v = 'b'");
-  }
-
-  @Test
-  public void testReadOnlyWorksAfterNestedBranch2() {
-    checkError(
-        "Variable v is read only",
-        "if 1:",
-        "  v = 'a'",
-        "else:",
-        "  if 0:",
-        "    w = 1",
-        "v = 'b'\n");
-  }
-
-  @Test
   public void testModulesReadOnlyInFuncDefBody() {
     parse("def func():", "  cmd_helper = depset()");
   }