Disabled string.partition default parameter and introduce incompatible flag
Related: #8725
Closes #8894.
PiperOrigin-RevId: 259348686
diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md
index 72de9de..8601bb5 100644
--- a/site/docs/skylark/backward-compatibility.md
+++ b/site/docs/skylark/backward-compatibility.md
@@ -18,6 +18,7 @@
* [Dictionary concatenation](#dictionary-concatenation)
* [String escapes](#string-escapes)
* [String.split empty separator](#string.split-empty-separator)
+* [Disable default parameter of String.partition/rpartition](#string.split-empty-separator)
* [Load must appear at top of file](#load-must-appear-at-top-of-file)
* [Depset is no longer iterable](#depset-is-no-longer-iterable)
* [Depset union](#depset-union)
@@ -107,6 +108,15 @@
* Default: `false`
* Tracking issue: [#7355](https://github.com/bazelbuild/bazel/issues/7355)
+### Disable default parameter of String.partition/rpartition
+
+This flag disables the default value `' '` for `sep` parameter of `String.parition`
+and `String.rpartition`.
+
+* Flag: `--incompatible_disable_partition_default_parameter`
+* Default: `false`
+* Tracking issue: [#8725](https://github.com/bazelbuild/bazel/issues/8725)
+
### Load must appear at top of file
Previously, the `load` statement could appear anywhere in a `.bzl` file so long
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 c4d3fe4..8461a68 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
@@ -243,6 +243,20 @@
help = "Unused. Will be removed in future versions of Bazel.")
public boolean incompatibleDisableObjcProviderResources;
+ @Option(
+ name = "incompatible_disable_partition_default_parameter",
+ 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 default value `' '` for the parameter `sep` of `string.partion` and"
+ + " `string.rpartition` will be disabled.")
+ public boolean incompatibleDisablePartitionDefaultParameter;
+
// For Bazel, this flag is a no-op. Bazel doesn't support built-in third party license checking
// (see https://github.com/bazelbuild/bazel/issues/7444).
//
@@ -709,6 +723,8 @@
.incompatibleDisallowSplitEmptySeparator(incompatibleDisallowSplitEmptySeparator)
.incompatibleDisallowDictLookupUnhashableKeys(
incompatibleDisallowDictLookupUnhashableKeys)
+ .incompatibleDisablePartitionDefaultParameter(
+ incompatibleDisablePartitionDefaultParameter)
.incompatibleAllowTagsPropagation(incompatibleAllowTagsPropagation)
.build();
return INTERNER.intern(semantics);
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 a1f7420..8f8567c 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
@@ -212,6 +212,8 @@
public abstract boolean incompatibleDisallowDictLookupUnhashableKeys();
+ public abstract boolean incompatibleDisablePartitionDefaultParameter();
+
public abstract boolean incompatibleAllowTagsPropagation();
@Memoized
@@ -291,6 +293,7 @@
.incompatibleRestrictStringEscapes(false)
.incompatibleDisallowSplitEmptySeparator(false)
.incompatibleDisallowDictLookupUnhashableKeys(false)
+ .incompatibleDisablePartitionDefaultParameter(false)
.incompatibleAllowTagsPropagation(false)
.build();
@@ -390,6 +393,8 @@
public abstract Builder incompatibleDisallowDictLookupUnhashableKeys(boolean value);
+ public abstract Builder incompatibleDisablePartitionDefaultParameter(boolean value);
+
public abstract StarlarkSemantics build();
}
}
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 4a6c6f9..13b194c 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
@@ -432,17 +432,36 @@
@Param(name = "self", type = String.class),
@Param(
name = "sep",
- type = String.class,
+ type = Object.class,
// TODO(cparsons): This parameter should be positional-only.
legacyNamed = true,
- defaultValue = "\" \"",
- doc = "The string to split on, default is space (\" \").")
+ defaultValue = "unbound",
+ doc =
+ "The string to split on, has a default value, space (\" \"), "
+ + "if the flag --incompatible_disable_partition_default_parameter is disabled. "
+ + "Otherwise, a value must be provided.")
},
useEnvironment = true,
useLocation = true)
- public Tuple<String> partition(String self, String sep, Location loc, Environment env)
+ public Tuple<String> partition(String self, Object sep, Location loc, Environment env)
throws EvalException {
- return partitionWrapper(self, sep, true, loc);
+ if (sep == Runtime.UNBOUND) {
+ if (env.getSemantics().incompatibleDisablePartitionDefaultParameter()) {
+ throw new EvalException(
+ loc,
+ "parameter 'sep' has no default value, "
+ + "for call to method partition(sep) of 'string'");
+ } else {
+ sep = " ";
+ }
+ } else if (!(sep instanceof String)) {
+ throw new EvalException(
+ loc,
+ "expected value of type 'string' for parameter"
+ + " 'sep', for call to method partition(sep = unbound) of 'string'");
+ }
+
+ return partitionWrapper(self, (String) sep, true, loc);
}
@SkylarkCallable(
@@ -458,14 +477,32 @@
type = String.class,
// TODO(cparsons): This parameter should be positional-only.
legacyNamed = true,
- defaultValue = "\" \"",
- doc = "The string to split on, default is space (\" \").")
+ defaultValue = "unbound",
+ doc =
+ "The string to split on, has a default value, space (\" \"), "
+ + "if the flag --incompatible_disable_partition_default_parameter is disabled. "
+ + "Otherwise, a value must be provided.")
},
useEnvironment = true,
useLocation = true)
- public Tuple<String> rpartition(String self, String sep, Location loc, Environment env)
+ public Tuple<String> rpartition(String self, Object sep, Location loc, Environment env)
throws EvalException {
- return partitionWrapper(self, sep, false, loc);
+ if (sep == Runtime.UNBOUND) {
+ if (env.getSemantics().incompatibleDisablePartitionDefaultParameter()) {
+ throw new EvalException(
+ loc,
+ "parameter 'sep' has no default value, "
+ + "for call to method partition(sep) of 'string'");
+ } else {
+ sep = " ";
+ }
+ } else if (!(sep instanceof String)) {
+ throw new EvalException(
+ loc,
+ "expected value of type 'string' for parameter"
+ + " 'sep', for call to method partition(sep = unbound) of 'string'");
+ }
+ return partitionWrapper(self, (String) sep, false, loc);
}
/**
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 995e0f7..da1f91c 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
@@ -168,6 +168,7 @@
"--incompatible_string_join_requires_strings=" + rand.nextBoolean(),
"--incompatible_restrict_string_escapes=" + rand.nextBoolean(),
"--incompatible_disallow_dict_lookup_unhashable_keys=" + rand.nextBoolean(),
+ "--incompatible_disable_partition_default_parameter=" + rand.nextBoolean(),
"--internal_skylark_flag_test_canary=" + rand.nextBoolean());
}
@@ -224,6 +225,7 @@
.incompatibleStringJoinRequiresStrings(rand.nextBoolean())
.incompatibleRestrictStringEscapes(rand.nextBoolean())
.incompatibleDisallowDictLookupUnhashableKeys(rand.nextBoolean())
+ .incompatibleDisablePartitionDefaultParameter(rand.nextBoolean())
.internalSkylarkFlagTestCanary(rand.nextBoolean())
.build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 5a913d9..baa8202 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -3076,6 +3076,43 @@
}
@Test
+ public void testPartitionDefaultParameter() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disable_partition_default_parameter=false");
+
+ scratch.file("test/extension.bzl", "y = ['abc'.partition(), 'abc'.rpartition()]");
+
+ scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
+
+ getConfiguredTarget("//test:r");
+ }
+
+ @Test
+ public void testDisabledPartitionDefaultParameter() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disable_partition_default_parameter=true");
+
+ scratch.file("test/extension.bzl", "y = 'abc'.partition()");
+
+ scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
+
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//test:r");
+ assertContainsEvent("parameter 'sep' has no default value");
+ }
+
+ @Test
+ public void testDisabledPartitionDefaultParameter2() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disable_partition_default_parameter=true");
+
+ scratch.file("test/extension.bzl", "y = 'abc'.rpartition()");
+
+ scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
+
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//test:r");
+ assertContainsEvent("parameter 'sep' has no default value");
+ }
+
+ @Test
public void testUnknownStringEscapesForbidden() throws Exception {
setSkylarkSemanticsOptions("--incompatible_restrict_string_escapes=true");
