Have --incompatible_string_replace_count reject None counts

This is another change to bring string.replace() in line with the spec. Safe to modify the flag since it has not been released yet.

Eliminated use of parameterization in test suite since it caused test cases that didn't need it to be cloned.

Also add RELNOTES in this CL for the flag.

Follow-up work to #9181.

RELNOTES: Added --incompatible_string_replace_count, to make string.replace() behave as in Python.
PiperOrigin-RevId: 309483323

src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java

@@ -517,7 +517,9 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
         OptionMetadataTag.INCOMPATIBLE_CHANGE,
         OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
       },
-      help = "If set to true, a negative count in string.replace() will be ignored")
+      help =
+          "If set to true, the `count` parameter of string.replace() is changed to behave as in "
+              + "Python: a negative count is ignored, and a None count is an error")
   public boolean incompatibleStringReplaceCount;
 
   /** Used in an integration test to confirm that flags are visible to the interpreter. */

src/main/java/com/google/devtools/build/lib/syntax/StringModule.java

@@ -266,36 +266,45 @@ public String strip(String self, Object charsOrNone) {
         @Param(name = "old", type = String.class, doc = "The string to be replaced."),
         @Param(name = "new", type = String.class, doc = "The string to replace with."),
         @Param(
-            // TODO(#8147): rename param to "count" once it's positional-only.
-            name = "maxsplit",
+            name = "count",
             type = Integer.class,
-            noneable = true,
-            defaultValue = "None",
+            noneable = true, // TODO(#11244): Set false once incompatible flag is deleted.
+            defaultValue = "unbound",
             doc =
-                "The maximum number of replacements. A negative value is ignored if"
-                    + " --incompatible_string_replace_count is true; otherwise a negative value"
-                    + " is treated as 0.")
+                "The maximum number of replacements. If omitted, there is no limit."
+                    + "<p>If <code>--incompatible_string_replace_count</code> is true, a negative "
+                    + "value is ignored (so there's no limit) and a <code>None</code> value is an "
+                    + "error. Otherwise, a negative value is treated as 0 and a <code>None</code> "
+                    + "value is ignored. (See also issue <a "
+                    + "href='https://github.com/bazelbuild/bazel/issues/11244'>#11244</a>.)")
       },
       useStarlarkThread = true)
   public String replace(
-      String self, String oldString, String newString, Object count, StarlarkThread thread)
+      String self, String oldString, String newString, Object countUnchecked, StarlarkThread thread)
       throws EvalException {
-    int maxReplaces = Integer.MAX_VALUE;
+    int count = Integer.MAX_VALUE;
 
     StarlarkSemantics semantics = thread.getSemantics();
     if (semantics.incompatibleStringReplaceCount()) {
-      if (count != Starlark.NONE && (Integer) count >= 0) {
-        maxReplaces = (Integer) count;
+      if (countUnchecked == Starlark.NONE) {
+        throw Starlark.errorf(
+            "Cannot pass a None count to string.replace(); omit the count argument instead. (You "
+                + "can temporarily opt out of this change by setting "
+                + "--incompatible_string_replace_count=false.)");
+      }
+      if (countUnchecked != Starlark.UNBOUND && (Integer) countUnchecked >= 0) {
+        count = (Integer) countUnchecked;
       }
     } else {
-      if (count != Starlark.NONE) {
-        maxReplaces = Math.max(0, (Integer) count);
+      if (countUnchecked != Starlark.UNBOUND && countUnchecked != Starlark.NONE) {
+        // Negative has same effect as 0 below.
+        count = (Integer) countUnchecked;
       }
     }
 
     StringBuilder sb = new StringBuilder();
     int start = 0;
-    for (int i = 0; i < maxReplaces; i++) {
+    for (int i = 0; i < count; i++) {
       if (oldString.isEmpty()) {
         sb.append(newString);
         if (start < self.length()) {

src/test/java/com/google/devtools/build/lib/syntax/StringModuleTest.java

@@ -14,32 +14,18 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
-import java.util.Arrays;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameter;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.runners.JUnit4;
 
-/** Tests for SkylarkStringModule. */
+/** Tests for StringModule. */
 // TODO(bazel-team): Migrate these test cases back to string_misc.sky, once either
 // 1) --incompatible_string_replace_count has been flipped (#11244) and deleted, or 2) the
 // standalone Starlark interpreter and tests gain the ability to run with semantics flags.
-@RunWith(Parameterized.class)
+@RunWith(JUnit4.class)
 public class StringModuleTest extends EvaluationTestCase {
 
-  @Parameters(name = "{index}: flag={0}")
-  public static Iterable<? extends Object> data() {
-    return Arrays.asList(
-        "--incompatible_string_replace_count=false", "--incompatible_string_replace_count=true");
-  }
-
-  @Parameter public String flag;
-
-  @Test
-  public void testReplace() throws Exception {
-    // Test that the behaviour is the same for the basic case both before
-    // and after the incompatible change.
+  private void runReplaceTest(String flag) throws Exception {
     new Scenario(flag)
         .testEval("'banana'.replace('a', 'o')", "'bonono'")
         .testEval("'banana'.replace('a', 'o', 2)", "'bonona'")
@@ -58,6 +44,12 @@ public void testReplace() throws Exception {
         .testEval("'banana'.replace('a', '', 2)", "'bnna'");
   }
 
+  @Test
+  public void testReplaceWithAndWithoutFlag() throws Exception {
+    runReplaceTest("--incompatible_string_replace_count=false");
+    runReplaceTest("--incompatible_string_replace_count=true");
+  }
+
   @Test
   public void testReplaceIncompatibleFlag() throws Exception {
     // Test the scenario that changes with the incompatible flag
@@ -73,4 +65,13 @@ public void testReplaceIncompatibleFlag() throws Exception {
         .testEval("'banana'.replace('a', 'e', -10)", "'benene'")
         .testEval("'banana'.replace('', '-', -2)", "'-b-a-n-a-n-a-'");
   }
+
+  @Test
+  public void testReplaceNoneCount() throws Exception {
+    // Passing None as the max number of replacements is disallowed with the incompatible flag.
+    new Scenario("--incompatible_string_replace_count=false")
+        .testEval("'banana'.replace('a', 'e', None)", "'benene'");
+    new Scenario("--incompatible_string_replace_count=true")
+        .testIfErrorContains("Cannot pass a None count", "'banana'.replace('a', 'e', None)");
+  }
 }