From d0151acaeb7252af48ec60475c8f647c9b13fa02 Mon Sep 17 00:00:00 2001 From: laurentlb Date: Mon, 26 Oct 2020 08:48:53 -0700 Subject: [PATCH] Flip and delete the flag --incompatible_string_replace_count Fixes https://github.com/bazelbuild/bazel/issues/11244 RELNOTES: --incompatible_string_replace_count is flipped and removed (#11244) PiperOrigin-RevId: 339046399 --- .../semantics/BuildLanguageOptions.java | 16 ------- .../starlark/java/eval/StarlarkSemantics.java | 4 -- .../net/starlark/java/eval/StringModule.java | 41 +++-------------- .../packages/semantics/ConsistencyTest.java | 2 - .../starlark/java/eval/StringModuleTest.java | 45 ++++--------------- 5 files changed, 16 insertions(+), 92 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index fe79b59893a2d0..02b310e772f6fc 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -471,20 +471,6 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable { 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, 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. */ @Option( name = "internal_starlark_flag_test_canary", @@ -650,8 +636,6 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool(INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT, incompatibleNoImplicitFileExport) .setBool(INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM, incompatibleNoRuleOutputsParam) .setBool(INCOMPATIBLE_RUN_SHELL_COMMAND_STRING, incompatibleRunShellCommandString) - .setBool( - StarlarkSemantics.INCOMPATIBLE_STRING_REPLACE_COUNT, incompatibleStringReplaceCount) .setBool(INCOMPATIBLE_STRUCT_HAS_NO_METHODS, incompatibleStructHasNoMethods) .setBool( INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION, diff --git a/src/main/java/net/starlark/java/eval/StarlarkSemantics.java b/src/main/java/net/starlark/java/eval/StarlarkSemantics.java index 5ab80deda41227..b0704194ce22da 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkSemantics.java +++ b/src/main/java/net/starlark/java/eval/StarlarkSemantics.java @@ -205,10 +205,6 @@ public String toString() { // -- semantics options affecting the Starlark interpreter itself -- - /** Ignore negative n in string.replace(count=n), and treat n=None as an error. */ - public static final String INCOMPATIBLE_STRING_REPLACE_COUNT = - "-incompatible_string_replace_count"; - /** Change the behavior of 'print' statements. Used in tests to verify flag propagation. */ public static final String PRINT_TEST_MARKER = "-print_test_marker"; } diff --git a/src/main/java/net/starlark/java/eval/StringModule.java b/src/main/java/net/starlark/java/eval/StringModule.java index 35230d5ae03d1d..f611ca1c7c2ad5 100644 --- a/src/main/java/net/starlark/java/eval/StringModule.java +++ b/src/main/java/net/starlark/java/eval/StringModule.java @@ -275,45 +275,18 @@ public String strip(String self, Object charsOrNone) { @Param(name = "new", doc = "The string to replace with."), @Param( name = "count", - allowedTypes = { - @ParamType(type = StarlarkInt.class), - @ParamType( - type = NoneType.class), // TODO(#11244): remove once incompatible flag is deleted. - }, - defaultValue = "unbound", + defaultValue = "-1", doc = - "The maximum number of replacements. If omitted, there is no limit." - + "

If --incompatible_string_replace_count is true, a negative " - + "value is ignored (so there's no limit) and a None value is an " - + "error. Otherwise, a negative value is treated as 0 and a None " - + "value is ignored. (See also issue #11244.)") + "The maximum number of replacements. If omitted, or if the value is negative, " + + "there is no limit.") }, useStarlarkThread = true) public String replace( - String self, String oldString, String newString, Object countUnchecked, StarlarkThread thread) + String self, String oldString, String newString, StarlarkInt countI, StarlarkThread thread) throws EvalException { - int count = Integer.MAX_VALUE; - - StarlarkSemantics semantics = thread.getSemantics(); - if (semantics.getBool(StarlarkSemantics.INCOMPATIBLE_STRING_REPLACE_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) { - int x = Starlark.toInt(countUnchecked, "count"); - if (x >= 0) { - count = x; - } - } - } else { - if (countUnchecked != Starlark.UNBOUND && countUnchecked != Starlark.NONE) { - // Negative has same effect as 0 below. - count = Starlark.toInt(countUnchecked, "count"); - } + int count = countI.toInt("count"); + if (count < 0) { + count = Integer.MAX_VALUE; } StringBuilder sb = new StringBuilder(); diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index 0889dc01d73b02..de6cd5447d58b3 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -150,7 +150,6 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--incompatible_no_rule_outputs_param=" + rand.nextBoolean(), "--incompatible_objc_provider_remove_compile_info=" + rand.nextBoolean(), "--incompatible_run_shell_command_string=" + rand.nextBoolean(), - "--incompatible_string_replace_count=" + rand.nextBoolean(), "--incompatible_struct_has_no_methods=" + rand.nextBoolean(), "--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(), "--incompatible_require_linker_input_cc_api=" + rand.nextBoolean(), @@ -207,7 +206,6 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .setBool( BuildLanguageOptions.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_RUN_SHELL_COMMAND_STRING, rand.nextBoolean()) - .setBool(StarlarkSemantics.INCOMPATIBLE_STRING_REPLACE_COUNT, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_STRUCT_HAS_NO_METHODS, rand.nextBoolean()) .setBool( BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION, diff --git a/src/test/java/net/starlark/java/eval/StringModuleTest.java b/src/test/java/net/starlark/java/eval/StringModuleTest.java index c38c4bc05a5b14..62c4630a3c7b0d 100644 --- a/src/test/java/net/starlark/java/eval/StringModuleTest.java +++ b/src/test/java/net/starlark/java/eval/StringModuleTest.java @@ -18,16 +18,15 @@ import org.junit.runners.JUnit4; /** 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. +// TODO(bazel-team): Migrate these test cases back to string_misc.sky. @RunWith(JUnit4.class) public class StringModuleTest { private final EvaluationTestCase ev = new EvaluationTestCase(); - private void runReplaceTest(StarlarkSemantics semantics) throws Exception { - ev.new Scenario(semantics) + @Test + public void testReplace() throws Exception { + ev.new Scenario() .testEval("'banana'.replace('a', 'o')", "'bonono'") .testEval("'banana'.replace('a', 'o', 2)", "'bonona'") .testEval("'banana'.replace('a', 'o', 0)", "'banana'") @@ -43,42 +42,16 @@ ev.new Scenario(semantics) .testEval("'banana'.replace('', '')", "'banana'") .testEval("'banana'.replace('a', '')", "'bnn'") .testEval("'banana'.replace('a', '', 2)", "'bnna'"); - } - - @Test - public void testReplaceWithAndWithoutFlag() throws Exception { - runReplaceTest(replaceCount(false)); - runReplaceTest(replaceCount(true)); - } - @Test - public void testReplaceIncompatibleFlag() throws Exception { - // Test the scenario that changes with the incompatible flag - ev.new Scenario(replaceCount(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'"); - - ev.new Scenario(replaceCount(true)) + ev.new Scenario() .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-'"); - } - - @Test - public void testReplaceNoneCount() throws Exception { - // Passing None as the max number of replacements is disallowed with the incompatible flag. - ev.new Scenario(replaceCount(false)).testEval("'banana'.replace('a', 'e', None)", "'benene'"); - ev.new Scenario(replaceCount(true)) - .testIfErrorContains("Cannot pass a None count", "'banana'.replace('a', 'e', None)"); - } - // Returns semantics for --incompatible_string_replace_count=x. - private static final StarlarkSemantics replaceCount(boolean x) { - return StarlarkSemantics.builder() - .setBool(StarlarkSemantics.INCOMPATIBLE_STRING_REPLACE_COUNT, x) - .build(); + ev.new Scenario() + .testIfErrorContains( + "parameter 'count' got value of type 'NoneType', want 'int'", + "'banana'.replace('a', 'e', None)"); } }