Handle negative count arg in string.replace
Also update the name of the argument to be "count" instead of "maxsplit",
to match the Starlark spec.
This fixes #9181
Closes #9228.
PiperOrigin-RevId: 309400161
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 0176933..5318010 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 @@
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 @@
.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 76c517d..a1d9e2b 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 @@
public abstract boolean incompatibleRunShellCommandString();
+ public abstract boolean incompatibleStringReplaceCount();
+
public abstract boolean incompatibleVisibilityPrivateAttributesAtDefinition();
public abstract boolean internalSkylarkFlagTestCanary();
@@ -343,6 +345,7 @@
.incompatibleNoRuleOutputsParam(false)
.incompatibleNoSupportToolsInActionInputs(true)
.incompatibleRunShellCommandString(false)
+ .incompatibleStringReplaceCount(false)
.incompatibleVisibilityPrivateAttributesAtDefinition(false)
.internalSkylarkFlagTestCanary(false)
.incompatibleDoNotSplitLinkingCmdline(true)
@@ -422,6 +425,8 @@
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 f46ae15..32d8bd8 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
@@ -263,30 +263,39 @@
+ "restricting the number of replacements to <code>maxsplit</code>.",
parameters = {
@Param(name = "self", type = String.class, doc = "This string."),
+ @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(
- 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",
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; otherwise a negative value"
+ + " is treated as 0.")
+ },
+ 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()) {
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 3539c6f..1aeb408 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 @@
"--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 @@
.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 0000000..dbc7b55
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StringModuleTest.java
@@ -0,0 +1,76 @@
+// 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 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;
+
+/** Tests for SkylarkStringModule. */
+// 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)
+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 ea3aa9d..d3286ab 100644
--- a/src/test/starlark/testdata/string_misc.sky
+++ b/src/test/starlark/testdata/string_misc.sky
@@ -43,11 +43,7 @@
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")
