From 00ea8fa9f619dac69ad8e6546908839c8f5c03fa Mon Sep 17 00:00:00 2001 From: Gustav Westling Date: Fri, 1 May 2020 06:49:01 -0700 Subject: [PATCH] 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 --- .../packages/StarlarkSemanticsOptions.java | 13 ++++ .../build/lib/syntax/StarlarkSemantics.java | 5 ++ .../build/lib/syntax/StringModule.java | 39 ++++++---- .../SkylarkSemanticsConsistencyTest.java | 2 + .../build/lib/syntax/StringModuleTest.java | 76 +++++++++++++++++++ src/test/starlark/testdata/string_misc.sky | 4 - 6 files changed, 120 insertions(+), 19 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/syntax/StringModuleTest.java 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 f46ae15f218f8a..32d8bd8ee9f82e 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 @@ public String strip(String self, Object charsOrNone) { + "restricting the number of replacements to maxsplit.", 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 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..dbc7b556d68b71 --- /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 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..d3286ab06b15b2 100644 --- a/src/test/starlark/testdata/string_misc.sky +++ b/src/test/starlark/testdata/string_misc.sky @@ -43,11 +43,7 @@ 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")