Repository: bazelbuild/bazel
Issue: string.replace incorrectly handles negative value for count
Labels: type: bug
P2

### Description of the problem:

According to the [spec](https://github.com/bazelbuild/starlark/blob/master/spec.md#stringreplace), a negative value for `count` in `string.replace` should be ignored and this is also what python and the Go implementation do.
```
# Current implementation
"abc".replace("a", "z", -1)    # "abc"

# Python/Go
"abc".replace("a", "z", -1)    # "zbc"
```

**Side Note:** The parameter `count` of `string.replace` is incorrectly named `maxsplit` in the definition of `replace` [here](https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java#L277).

Issue comments:

Pull request review comments:
I think renaming this would be a breaking change. May need to keep the old name for now.
Yes, it's clearly breaking! The question is if it's a change that's allowed to be made according to the compatability policy, the [Backwards Compatability document](https://docs.bazel.build/versions/master/backward-compatibility.html) does not mention what's allowed and not in these edge cases.

To draw some inspiration from other projects: the [Go1 compatability document](https://golang.org/doc/go1compat) states:

>  If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.

I think that this is an example of that, as the Starlark implementation in Bazel does not satisfy the [spec](https://github.com/bazelbuild/starlark/blob/master/spec.md#stringreplace).

I'll leave it up to you and the team to figure this out, but I do think that it would be nice to allow to make some breaking changes without increasing the major-version number in the future.
 
This is definitely a breaking change which is not allowed by our policy -- apologies this is not clear by the policy doc, but this is definitely going to break some users and cause some pain if we change it.

We should avoid renaming this for now.
The parameter is tagged with `legacyNamed = true`, so, subject to the rollout of https://github.com/bazelbuild/bazel/issues/8147, this parameter will be positional-only in the future. We can then rename the parameter safely for documentation purposes.
2019*
StringModuleTest ?
StringModule ?
Please update:

> "The maximum number of replacements. A negative value is ignored."
Super nit: I feel like `count >= 0` is more readable than `count > -1`.
To clarify, there's two breaking changes: the rename and the behavior for negative values.

The rename is subsumed by #8147, so we should definitely not do any extra work here. You can add a comment reminding us to get back to it:

> // TODO(#8147): rename param to "count" once it's positional-only.

The behavioral change for negatives is also breaking. I feel like it's not very likely people will be broken by it, but it seems like the kind of thing we should guard with a flag. Look for other examples of using incompatible flags in Starlark builtins -- for instance, `incompatibleDisableDepsetItems` in the depset constructor later in this file, and its corresponding definitions in `StarlarkSemantics` and `StarlarkSemanticsOptions`.
That's not a super nit. >=0 simply *is* more readable than > -1.
Ok, let's do this the proper way, by marking the negative value behaviour change as breaking.

Other than adding the flag and the handling of it, is there anything else that needs to be done? What's the process for creating issues with the `incompatible-change` and `breaking-change-*` labels?
When you file the GitHub issue, you can cc me so that I add the labels.
If your change is ready this week, we can include it in Bazel 3.2.

Thanks!
Also: 'None' is not a legal argument for count, according to the Starlark spec (which matches Pythons 2 and 3 and go.starlark.net implementation).
Hm, I'll write a quick followup to change the default value to the internal UNBOUND sentinel and have the incompatible flag also reject the value None.
Thanks. I suggest to:

- Change the default value to unbound
- Raise an error if the argument is None and the incompatible flag is enabled
- After the flag flip, remove noneable=True
@alandonovan points out to me that we actually already flipped *and deleted* `--incompatible_restrict_named_params`, so I'll do the rename as part of the follow-up.

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
index 01769336fe6b85..531801020d11a3 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -508,6 +508,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
       help = "If set to true, the command parameter of actions.run_shell will only accept string")
   public boolean incompatibleRunShellCommandString;
 
+  @Option(
+      name = "incompatible_string_replace_count",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help = "If set to true, a negative count in string.replace() will be ignored")
+  public boolean incompatibleStringReplaceCount;
+
   /** Used in an integration test to confirm that flags are visible to the interpreter. */
   @Option(
       name = "internal_skylark_flag_test_canary",
@@ -652,6 +664,7 @@ public StarlarkSemantics toSkylarkSemantics() {
             .incompatibleNoRuleOutputsParam(incompatibleNoRuleOutputsParam)
             .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs)
             .incompatibleRunShellCommandString(incompatibleRunShellCommandString)
+            .incompatibleStringReplaceCount(incompatibleStringReplaceCount)
             .incompatibleVisibilityPrivateAttributesAtDefinition(
                 incompatibleVisibilityPrivateAttributesAtDefinition)
             .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
index 76c517d50b2fd9..a1d9e2b8dea922 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -257,6 +257,8 @@ boolean isFeatureEnabledBasedOnTogglingFlags(String enablingFlag, String disabli
 
   public abstract boolean incompatibleRunShellCommandString();
 
+  public abstract boolean incompatibleStringReplaceCount();
+
   public abstract boolean incompatibleVisibilityPrivateAttributesAtDefinition();
 
   public abstract boolean internalSkylarkFlagTestCanary();
@@ -343,6 +345,7 @@ public static Builder builderWithDefaults() {
           .incompatibleNoRuleOutputsParam(false)
           .incompatibleNoSupportToolsInActionInputs(true)
           .incompatibleRunShellCommandString(false)
+          .incompatibleStringReplaceCount(false)
           .incompatibleVisibilityPrivateAttributesAtDefinition(false)
           .internalSkylarkFlagTestCanary(false)
           .incompatibleDoNotSplitLinkingCmdline(true)
@@ -422,6 +425,8 @@ public abstract static class Builder {
 
     public abstract Builder incompatibleRunShellCommandString(boolean value);
 
+    public abstract Builder incompatibleStringReplaceCount(boolean value);
+
     public abstract Builder incompatibleVisibilityPrivateAttributesAtDefinition(boolean value);
 
     public abstract Builder internalSkylarkFlagTestCanary(boolean value);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java b/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java
index cf7ac6ccd24eb2..8373c0b97743f0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java
@@ -274,21 +274,32 @@ public String strip(String self, Object charsOrNone) {
             type = String.class,
             doc = "The string to replace with."),
         @Param(
+            // TODO(#8147): rename param to "count" once it's positional-only.
             name = "maxsplit",
             type = Integer.class,
             noneable = true,
             defaultValue = "None",
-            doc = "The maximum number of replacements.")
-      })
-  public String replace(String self, String oldString, String newString, Object maxSplitO)
+            doc = "The maximum number of replacements. A negative value is ignored (if --incompatible_string_replace_count is true).")
+      },
+      useStarlarkThread = true)
+  public String replace(String self, String oldString, String newString, Object count, StarlarkThread thread)
       throws EvalException {
-    int maxSplit = Integer.MAX_VALUE;
-    if (maxSplitO != Starlark.NONE) {
-      maxSplit = Math.max(0, (Integer) maxSplitO);
+    int maxReplaces = Integer.MAX_VALUE;
+
+    StarlarkSemantics semantics = thread.getSemantics();
+    if (semantics.incompatibleStringReplaceCount()) {
+      if (count != Starlark.NONE && (Integer) count >= 0) {
+        maxReplaces = (Integer) count;
+      }
+    } else {
+      if (count != Starlark.NONE) {
+        maxReplaces = Math.max(0, (Integer) count);
+      }
     }
+
     StringBuilder sb = new StringBuilder();
     int start = 0;
-    for (int i = 0; i < maxSplit; i++) {
+    for (int i = 0; i < maxReplaces; i++) {
       if (oldString.isEmpty()) {
         sb.append(newString);
         if (start < self.length()) {


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index 3539c6f7946283..1aeb40851b419e 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -156,6 +156,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_no_rule_outputs_param=" + rand.nextBoolean(),
         "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(),
         "--incompatible_run_shell_command_string=" + rand.nextBoolean(),
+        "--incompatible_string_replace_count=" + rand.nextBoolean(),
         "--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(),
         "--incompatible_require_linker_input_cc_api=" + rand.nextBoolean(),
         "--incompatible_restrict_string_escapes=" + rand.nextBoolean(),
@@ -207,6 +208,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleNoRuleOutputsParam(rand.nextBoolean())
         .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean())
         .incompatibleRunShellCommandString(rand.nextBoolean())
+        .incompatibleStringReplaceCount(rand.nextBoolean())
         .incompatibleVisibilityPrivateAttributesAtDefinition(rand.nextBoolean())
         .incompatibleRequireLinkerInputCcApi(rand.nextBoolean())
         .incompatibleRestrictStringEscapes(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StringModuleTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StringModuleTest.java
new file mode 100644
index 00000000000000..203aa75a4da40b
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StringModuleTest.java
@@ -0,0 +1,79 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.syntax;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
+import java.util.Arrays;
+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.Test;
+
+/**
+ * Tests for SkylarkStringModule.
+ */
+@RunWith(Parameterized.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.
+      new Scenario(flag)
+        .testEval("'banana'.replace('a', 'o')", "'bonono'")
+        .testEval("'banana'.replace('a', 'o', 2)", "'bonona'")
+        .testEval("'banana'.replace('a', 'o', 0)", "'banana'")
+        .testEval("'banana'.replace('a', 'e')", "'benene'")
+        .testEval("'banana'.replace('a', '$()')", "'b$()n$()n$()'")
+        .testEval("'banana'.replace('a', '$')", "'b$n$n$'")
+        .testEval("'b$()n$()n$()'.replace('$()', '$($())')", "'b$($())n$($())n$($())'")
+        .testEval("'banana'.replace('a', 'e', 2)", "'benena'")
+        .testEval("'banana'.replace('a', 'e', 0)", "'banana'")
+        .testEval("'banana'.replace('', '-')", "'-b-a-n-a-n-a-'")
+        .testEval("'banana'.replace('', '-', 2)", "'-b-anana'")
+        .testEval("'banana'.replace('', '-', 0)", "'banana'")
+        .testEval("'banana'.replace('', '')", "'banana'")
+        .testEval("'banana'.replace('a', '')", "'bnn'")
+        .testEval("'banana'.replace('a', '', 2)", "'bnna'");
+  }
+
+  @Test
+  public void testReplaceIncompatibleFlag() throws Exception {
+      // Test the scenario that changes with the incompatible flag
+      new Scenario("--incompatible_string_replace_count=false")
+        .testEval("'banana'.replace('a', 'o', -2)", "'banana'")
+        .testEval("'banana'.replace('a', 'e', -1)", "'banana'")
+        .testEval("'banana'.replace('a', 'e', -10)", "'banana'")
+        .testEval("'banana'.replace('', '-', -2)", "'banana'");
+
+      new Scenario("--incompatible_string_replace_count=true")
+        .testEval("'banana'.replace('a', 'o', -2)", "'bonono'")
+        .testEval("'banana'.replace('a', 'e', -1)", "'benene'")
+        .testEval("'banana'.replace('a', 'e', -10)", "'benene'")
+        .testEval("'banana'.replace('', '-', -2)", "'-b-a-n-a-n-a-'");
+  }
+}
diff --git a/src/test/starlark/testdata/string_misc.sky b/src/test/starlark/testdata/string_misc.sky
index ea3aa9d141d5b3..3d426f377571fc 100644
--- a/src/test/starlark/testdata/string_misc.sky
+++ b/src/test/starlark/testdata/string_misc.sky
@@ -43,17 +43,6 @@ assert_eq('b\\n\\n\\'.replace('\\', '$()'), "b$()n$()n$()")
 
 assert_eq('banana'.replace('a', 'e', 2), "benena")
 assert_eq('banana'.replace('a', 'e', 0), "banana")
-assert_eq('banana'.replace('a', 'e', -1), "banana")
-assert_eq('banana'.replace('a', 'e', -10), "banana")
-
-assert_eq('banana'.replace('', '-'), "-b-a-n-a-n-a-")
-assert_eq('banana'.replace('', '-', -2), "banana")
-assert_eq('banana'.replace('', '-', 2), "-b-anana")
-assert_eq('banana'.replace('', '-', 0), "banana")
-
-assert_eq('banana'.replace('', ''), "banana")
-assert_eq('banana'.replace('a', ''), "bnn")
-assert_eq('banana'.replace('a', '', 2), "bnna")
 
 # index, rindex
 assert_eq('banana'.index('na'), 2)