Skip to content

Commit

Permalink
Flip and delete the flag --incompatible_string_replace_count
Browse files Browse the repository at this point in the history
    Fixes bazelbuild/bazel#11244

    RELNOTES:
      --incompatible_string_replace_count is flipped and removed (#11244)
    PiperOrigin-RevId: 339046399
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent f796ed1 commit fba68de
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 81 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@
* #toStarlarkSemantics}.
* <li>Define a new {@code StarlarkSemantics.Key} or {@code StarlarkSemantics} boolean flag
* identifier.
* <li>Add a line to set the new field in both {@link ConsistencyTest#buildRandomOptions} and
* {@link ConsistencyTest#buildRandomSemantics}.
* <li>Add a line to set the new field in both {@link
* StarlarkSemanticsConsistencyTest#buildRandomOptions} and {@link
* StarlarkSemanticsConsistencyTest#buildRandomSemantics}.
* <li>Update manual documentation in site/docs/skylark/backward-compatibility.md. Also remember
* to update this when flipping a flag's default value.
* <li>Boolean semantic flags can toggle StarlarkMethod-annotated Java methods (or their
Expand All @@ -73,7 +74,8 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
+ "ctx.build_setting_value.")
public boolean experimentalBuildSettingApi;

// TODO(#11437): Delete the special empty string value so that it's on unconditionally.
// TODO(#11437): Implement the flag values listed in the below help string; delete the special
// empty string value so that it's on unconditionally.
@Option(
name = "experimental_builtins_bzl_path",
defaultValue = "",
Expand All @@ -86,23 +88,13 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
+ "intended for Bazel developers, to help when writing @_builtins .bzl code. "
+ "Ordinarily this value is set to \"%install_base%\", which means to use the "
+ "builtins_bzl/ directory located in the install base. However, it can be set to "
+ "the path (relative to the root of the current workspace) of an alternate "
+ "builtins_bzl/ directory, such as one in a Bazel source tree workspace. A literal "
+ "value of \"%workspace%\" is equivalent to the relative package path of "
+ "builtins_bzl/ within a Bazel source tree; this should only be used when running "
+ "Bazel within its own source tree. Finally, a value of the empty string disables "
+ "the builtins injection mechanism entirely.")
+ "the path to the root of a Bazel source tree workspace, in which case the bzl "
+ "sources underneath that workspace are used. If the value is literally "
+ "\"%workspace%\", the root of the current workspace is used; this should only be "
+ "set when running Bazel within its own source tree. Finally, a value of the empty "
+ "string (\"\") disables the builtins injection mechanism entirely.")
public String experimentalBuiltinsBzlPath;

@Option(
name = "experimental_builtins_dummy",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
help = "Enables an internal dummy symbol used to test builtins injection.")
public boolean experimentalBuiltinsDummy;

@Option(
name = "experimental_cc_skylark_api_enabled_packages",
converter = CommaSeparatedOptionListConverter.class,
Expand Down Expand Up @@ -180,7 +172,7 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {

@Option(
name = "incompatible_require_linker_input_cc_api",
defaultValue = "true",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS, OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
Expand Down Expand Up @@ -469,7 +461,7 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {

@Option(
name = "incompatible_run_shell_command_string",
defaultValue = "true",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {
Expand Down Expand Up @@ -533,7 +525,7 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {

@Option(
name = "incompatible_restrict_string_escapes",
defaultValue = "true",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {
Expand All @@ -545,7 +537,7 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {

@Option(
name = "incompatible_linkopts_to_linklibs",
defaultValue = "true",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES},
metadataTags = {
Expand All @@ -557,6 +549,18 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
+ "instead of linkopts to cc_toolchain_config")
public boolean incompatibleLinkoptsToLinklibs;

@Option(
name = "incompatible_objc_provider_remove_compile_info",
defaultValue = "true",
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 ObjcProvider's APIs for compile info/merge_zip will be removed.")
public boolean incompatibleObjcProviderRemoveCompileInfo;

@Option(
name = "incompatible_java_common_parameters",
defaultValue = "false",
Expand Down Expand Up @@ -597,7 +601,6 @@ public StarlarkSemantics toStarlarkSemantics() {
// <== Add new options here in alphabetic order ==>
.setBool(EXPERIMENTAL_ALLOW_TAGS_PROPAGATION, experimentalAllowTagsPropagation)
.set(EXPERIMENTAL_BUILTINS_BZL_PATH, experimentalBuiltinsBzlPath)
.setBool(EXPERIMENTAL_BUILTINS_DUMMY, experimentalBuiltinsDummy)
.set(
EXPERIMENTAL_CC_STARLARK_API_ENABLED_PACKAGES,
ImmutableList.copyOf(experimentalCcStarlarkApiEnabledPackages))
Expand Down Expand Up @@ -648,6 +651,9 @@ public StarlarkSemantics toStarlarkSemantics() {
.setBool(INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API, incompatibleRequireLinkerInputCcApi)
.setBool(INCOMPATIBLE_RESTRICT_STRING_ESCAPES, incompatibleRestrictStringEscapes)
.setBool(INCOMPATIBLE_LINKOPTS_TO_LINKLIBS, incompatibleLinkoptsToLinklibs)
.setBool(
INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO,
incompatibleObjcProviderRemoveCompileInfo)
.set(MAX_COMPUTATION_STEPS, maxComputationSteps)
.build();
return INTERNER.intern(semantics);
Expand All @@ -664,7 +670,6 @@ public StarlarkSemantics toStarlarkSemantics() {
// booleans: the +/- prefix indicates the default value (true/false).
public static final String EXPERIMENTAL_ALLOW_TAGS_PROPAGATION =
"-experimental_allow_tags_propagation";
public static final String EXPERIMENTAL_BUILTINS_DUMMY = "-experimental_builtins_dummy";
public static final String EXPERIMENTAL_CC_SHARED_LIBRARY = "-experimental_cc_shared_library";
public static final String EXPERIMENTAL_DISABLE_EXTERNAL_PACKAGE =
"-experimental_disable_external_package";
Expand Down Expand Up @@ -700,19 +705,21 @@ public StarlarkSemantics toStarlarkSemantics() {
public static final String INCOMPATIBLE_JAVA_COMMON_PARAMETERS =
"-incompatible_java_common_parameters";
public static final String INCOMPATIBLE_LINKOPTS_TO_LINKLIBS =
"+incompatible_linkopts_to_linklibs";
"-incompatible_linkopts_to_linklibs";
public static final String INCOMPATIBLE_NEW_ACTIONS_API = "+incompatible_new_actions_api";
public static final String INCOMPATIBLE_NO_ATTR_LICENSE = "+incompatible_no_attr_license";
public static final String INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT =
"-incompatible_no_implicit_file_export";
public static final String INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM =
"-incompatible_no_rule_outputs_param";
public static final String INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO =
"+incompatible_objc_provider_remove_compile_info";
public static final String INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API =
"+incompatible_require_linker_input_cc_api";
"-incompatible_require_linker_input_cc_api";
public static final String INCOMPATIBLE_RESTRICT_STRING_ESCAPES =
"+incompatible_restrict_string_escapes";
"-incompatible_restrict_string_escapes";
public static final String INCOMPATIBLE_RUN_SHELL_COMMAND_STRING =
"+incompatible_run_shell_command_string";
"-incompatible_run_shell_command_string";
public static final String INCOMPATIBLE_STRUCT_HAS_NO_METHODS =
"-incompatible_struct_has_no_methods";
public static final String INCOMPATIBLE_USE_CC_CONFIGURE_FROM_RULES_CC =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
package net.starlark.java.eval;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
import java.util.HashMap;
import java.util.Map;
import java.util.TreeMap;

/**
* A StarlarkSemantics is an immutable set of optional name/value pairs that affect the dynamic
Expand All @@ -43,15 +43,15 @@
public final class StarlarkSemantics {

/** Returns the empty semantics, in which every option has its default value. */
public static final StarlarkSemantics DEFAULT = new StarlarkSemantics(ImmutableMap.of());
public static final StarlarkSemantics DEFAULT = new StarlarkSemantics(ImmutableSortedMap.of());

// A map entry must be accessed by Key iff its name has no [+-] prefix.
// Key<Boolean> is permitted too.
// The map keys are sorted but we avoid ImmutableSortedMap due to observed inefficiency.
private final ImmutableMap<String, Object> map;
// We use ImmutableSortedMap for the benefit of equals/hashCode/toString.
private final ImmutableSortedMap<String, Object> map;
private final int hashCode;

private StarlarkSemantics(ImmutableMap<String, Object> map) {
private StarlarkSemantics(ImmutableSortedMap<String, Object> map) {
this.map = map;
this.hashCode = map.hashCode();
}
Expand Down Expand Up @@ -98,19 +98,19 @@ public String toString() {
* Returns a new builder that initially holds the same key/value pairs as this StarlarkSemantics.
*/
public Builder toBuilder() {
return new Builder(new TreeMap<>(map));
return new Builder(new HashMap<>(map));
}

/** Returns a new empty builder. */
public static Builder builder() {
return new Builder(new TreeMap<>());
return new Builder(new HashMap<>());
}

/** A Builder is a mutable container used to construct an immutable StarlarkSemantics. */
public static final class Builder {
private final TreeMap<String, Object> map;
private final HashMap<String, Object> map;

private Builder(TreeMap<String, Object> map) {
private Builder(HashMap<String, Object> map) {
this.map = map;
}

Expand Down Expand Up @@ -139,7 +139,7 @@ public Builder setBool(String name, boolean value) {

/** Returns an immutable StarlarkSemantics. */
public StarlarkSemantics build() {
return new StarlarkSemantics(ImmutableMap.copyOf(map));
return new StarlarkSemantics(ImmutableSortedMap.copyOf(map));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ public StarlarkList<String> rsplit(
+ " separator, after). If the input string does not contain the separator, partition"
+ " returns (self, '', '').",
parameters = {@Param(name = "self"), @Param(name = "sep", doc = "The string to split on.")})
public Tuple partition(String self, String sep) throws EvalException {
public Tuple<String> partition(String self, String sep) throws EvalException {
return partitionCommon(self, sep, /*first=*/ true);
}

Expand All @@ -416,15 +416,15 @@ public Tuple partition(String self, String sep) throws EvalException {
+ " separator, after). If the input string does not contain the separator,"
+ " rpartition returns ('', '', self).",
parameters = {@Param(name = "self"), @Param(name = "sep", doc = "The string to split on.")})
public Tuple rpartition(String self, String sep) throws EvalException {
public Tuple<String> rpartition(String self, String sep) throws EvalException {
return partitionCommon(self, sep, /*first=*/ false);
}

// Splits input at the first or last occurrence of the given separator,
// and returns a triple of substrings (before, separator, after).
// If the input does not contain the separator,
// it returns (input, "", "") if first, or ("", "", input), if !first.
private static Tuple partitionCommon(String input, String separator, boolean first)
private static Tuple<String> partitionCommon(String input, String separator, boolean first)
throws EvalException {
if (separator.isEmpty()) {
throw Starlark.errorf("empty separator");
Expand Down Expand Up @@ -941,7 +941,8 @@ private static boolean substringEndsWith(String str, int start, int end, String
extraPositionals = @Param(name = "args", defaultValue = "()", doc = "List of arguments."),
extraKeywords =
@Param(name = "kwargs", defaultValue = "{}", doc = "Dictionary of arguments."))
public String format(String self, Tuple args, Dict<String, Object> kwargs) throws EvalException {
public String format(String self, Tuple<Object> args, Dict<String, Object> kwargs)
throws EvalException {
return new FormatParser().format(self, args, kwargs);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep
"--experimental_disable_external_package=" + rand.nextBoolean(),
"--experimental_sibling_repository_layout=" + rand.nextBoolean(),
"--experimental_builtins_bzl_path=" + rand.nextDouble(),
"--experimental_builtins_dummy=" + rand.nextBoolean(),
"--experimental_cc_skylark_api_enabled_packages="
+ rand.nextDouble()
+ ","
Expand Down Expand Up @@ -149,6 +148,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep
"--incompatible_no_attr_license=" + rand.nextBoolean(),
"--incompatible_no_implicit_file_export=" + rand.nextBoolean(),
"--incompatible_no_rule_outputs_param=" + rand.nextBoolean(),
"--incompatible_objc_provider_remove_compile_info=" + rand.nextBoolean(),
"--incompatible_run_shell_command_string=" + rand.nextBoolean(),
"--incompatible_struct_has_no_methods=" + rand.nextBoolean(),
"--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(),
Expand All @@ -169,7 +169,6 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
.setBool(BuildLanguageOptions.EXPERIMENTAL_DISABLE_EXTERNAL_PACKAGE, rand.nextBoolean())
.setBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT, rand.nextBoolean())
.set(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH, String.valueOf(rand.nextDouble()))
.setBool(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_DUMMY, rand.nextBoolean())
.set(
BuildLanguageOptions.EXPERIMENTAL_CC_STARLARK_API_ENABLED_PACKAGES,
ImmutableList.of(String.valueOf(rand.nextDouble()), String.valueOf(rand.nextDouble())))
Expand Down Expand Up @@ -204,6 +203,8 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
.setBool(BuildLanguageOptions.INCOMPATIBLE_NO_ATTR_LICENSE, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM, rand.nextBoolean())
.setBool(
BuildLanguageOptions.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_RUN_SHELL_COMMAND_STRING, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_STRUCT_HAS_NO_METHODS, rand.nextBoolean())
.setBool(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'")
Expand All @@ -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)");
}
}

0 comments on commit fba68de

Please sign in to comment.