diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java index 3a32412f690eb4..5140f572b16e18 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java @@ -66,11 +66,18 @@ * aware that an error in the specification of the {@code @Option} will exercise failure code paths * in the early part of the Bazel server execution. * - *

After the breaking change has been enabled unconditionally, it is recommended (required?) that - * its corresponding incompatible change option be left as a valid no-op option, rather than - * removed. This helps avoid breaking invocations of Bazel upon upgrading to a new release. Just as - * for other options, names of incompatible change options must never be reused for a different - * option. + *

After the breaking change has been enabled by default, it is recommended (required?) that the + * flag stick around for a few releases, to provide users the flexibility to opt out. Even after + * enabling the behavior unconditionally, it can still be useful to keep the flag around as a valid + * no-op so that Bazel invocations are not immediately broken. + * + *

Generally speaking, we should never reuse names for multiple options. Therefore, when choosing + * a name for a new incompatible change, try to describe not just the affected feature, but what the + * change to that feature is. This avoids conflicts in case the feature changes multiple times. For + * example, {@code "--incompatible_depset_constructor"} is ambiguous because it only communicates + * that there is a change to how depsets are constructed, but {@code + * "--incompatible_disallow_set_constructor"} uniquely says that the {@code set} alias for the + * depset constructor is being disallowed. */ // Javadoc can't resolve inner classes. @SuppressWarnings("javadoc") diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java index 70f4b95e1f6195..88275c3e805a86 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java @@ -126,13 +126,13 @@ public SkylarkNestedSet invoke(Object items, String order, Location loc) new BuiltinFunction("set") { public SkylarkNestedSet invoke(Object items, String order, Location loc, Environment env) throws EvalException { - if (env.getSemantics().incompatibleDepsetConstructor) { + if (env.getSemantics().incompatibleDisallowSetConstructor) { throw new EvalException( loc, "The `set` constructor for depsets is deprecated and will be removed. Please use " + "the `depset` constructor instead. You can temporarily enable the " + "deprecated `set` constructor by passing the flag " - + "--incompatible_depset_constructor=false"); + + "--incompatible_disallow_set_constructor=false"); } try { return new SkylarkNestedSet(Order.parse(order), items, loc); diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java index ef8b7f1c5534bf..ea8280ac8af64d 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java @@ -209,7 +209,7 @@ private static Object plus( } if ((lval instanceof MutableList) && (rval instanceof MutableList)) { - if (isAugmented && env.getSemantics().incompatibleListPlusEquals) { + if (isAugmented && env.getSemantics().incompatibleListPlusEqualsInplace) { @SuppressWarnings("unchecked") MutableList list = (MutableList) lval; list.addAll((MutableList) rval, location, env); @@ -219,12 +219,12 @@ private static Object plus( } if (lval instanceof SkylarkDict && rval instanceof SkylarkDict) { - if (env.getSemantics().incompatibleDictPlus) { + if (env.getSemantics().incompatibleDisallowDictPlus) { throw new EvalException( location, "The `+` operator for dicts is deprecated and no longer supported. Please use the " + "`update` method instead. You can temporarily enable the `+` operator by passing " - + "the flag --incompatible_dict_plus=false"); + + "the flag --incompatible_disallow_dict_plus=false"); } return SkylarkDict.plus((SkylarkDict) lval, (SkylarkDict) rval, env); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java index cde876e58a5ed2..201d084f409ab2 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java @@ -51,12 +51,12 @@ void doExec(Environment env) throws EvalException, InterruptedException { } FunctionSignature sig = signature.getSignature(); - if (env.getSemantics().incompatibleKeywordOnlySyntax + if (env.getSemantics().incompatibleDisallowKeywordOnlyArgs && sig.getShape().getMandatoryNamedOnly() > 0) { throw new EvalException( getLocation(), "Keyword-only argument is forbidden. You can temporarily disable this " - + "error using the flag --incompatible_keyword_only_syntax=false"); + + "error using the flag --incompatible_disallow_keyword_only_args=false"); } env.update( diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java index abd56117040315..8653e837079051 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java @@ -44,36 +44,36 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable public boolean skylarkFlagTestCanary; @Option( - name = "incompatible_depset_constructor", + name = "incompatible_disallow_set_constructor", defaultValue = "false", category = "incompatible changes", help = "If set to true, disables the deprecated `set` constructor for depsets." ) - public boolean incompatibleDepsetConstructor; + public boolean incompatibleDisallowSetConstructor; @Option( - name = "incompatible_keyword_only_syntax", + name = "incompatible_disallow_keyword_only_args", defaultValue = "false", category = "incompatible changes", help = "If set to true, disables the keyword-only argument syntax in function definition." ) - public boolean incompatibleKeywordOnlySyntax; + public boolean incompatibleDisallowKeywordOnlyArgs; @Option( - name = "incompatible_list_plus_equals", + name = "incompatible_list_plus_equals_inplace", defaultValue = "false", category = "incompatible changes", help = "If set to true, `+=` on lists works like the `extend` method mutating the original " + "list. Otherwise it copies the original list without mutating it." ) - public boolean incompatibleListPlusEquals; + public boolean incompatibleListPlusEqualsInplace; @Option( - name = "incompatible_dict_plus", + name = "incompatible_disallow_dict_plus", defaultValue = "false", category = "incompatible changes", help = "If set to true, the `+` becomes disabled for dicts." ) - public boolean incompatibleDictPlus; + public boolean incompatibleDisallowDictPlus; } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java index 71d90cdf00584b..258d21943168d7 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java @@ -337,13 +337,13 @@ public void testKwargs() throws Exception { @Test public void testKeywordOnlyIsForbidden() throws Exception { - env = newEnvironmentWithSkylarkOptions("--incompatible_keyword_only_syntax=true"); + env = newEnvironmentWithSkylarkOptions("--incompatible_disallow_keyword_only_args=true"); checkEvalErrorContains("forbidden", "def foo(a, b, *, c): return a + b + c"); } @Test public void testParamAfterStarArgs() throws Exception { - env = newEnvironmentWithSkylarkOptions("--incompatible_keyword_only_syntax=true"); + env = newEnvironmentWithSkylarkOptions("--incompatible_disallow_keyword_only_args=true"); checkEvalErrorContains("forbidden", "def foo(a, *b, c): return a"); } @@ -428,7 +428,7 @@ public void testStarParam() throws Exception { @Test public void testIncompatibleStarParam() throws Exception { - env = newEnvironmentWithSkylarkOptions("--incompatible_keyword_only_syntax=true"); + env = newEnvironmentWithSkylarkOptions("--incompatible_disallow_keyword_only_args=true"); eval("def f(name, value = '1', optional = '2', *rest):", " r = name + value + optional + '|'", " for x in rest: r += x", diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index b87094d13d08ce..b88d972ca051ea 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -988,7 +988,7 @@ public void testDotExpressionOnNonStructObject() throws Exception { @Test public void testPlusEqualsOnListCopying() throws Exception { - new SkylarkTest("--incompatible_list_plus_equals=false") + new SkylarkTest("--incompatible_list_plus_equals_inplace=false") .setUp( "def func():", " l1 = [1, 2]", @@ -1001,7 +1001,7 @@ public void testPlusEqualsOnListCopying() throws Exception { @Test public void testPlusEqualsOnListMutating() throws Exception { - new SkylarkTest("--incompatible_list_plus_equals=true") + new SkylarkTest("--incompatible_list_plus_equals_inplace=true") .setUp( "def func():", " l1 = [1, 2]", @@ -1012,7 +1012,7 @@ public void testPlusEqualsOnListMutating() throws Exception { .testLookup("lists", "([1, 2, 3, 4], [1, 2, 3, 4])"); // The same but with += after an IndexExpression - new SkylarkTest("--incompatible_list_plus_equals=true") + new SkylarkTest("--incompatible_list_plus_equals_inplace=true") .setUp( "def func():", " l = [1, 2]", @@ -1025,7 +1025,7 @@ public void testPlusEqualsOnListMutating() throws Exception { @Test public void testPlusEqualsOnTuple() throws Exception { - new SkylarkTest("--incompatible_list_plus_equals=false") + new SkylarkTest("--incompatible_list_plus_equals_inplace=false") .setUp( "def func():", " t1 = (1, 2)", @@ -1038,8 +1038,9 @@ public void testPlusEqualsOnTuple() throws Exception { SkylarkList.Tuple.of(1, 2, 3, 4) )); - // This behavior should remain the same regardless of the incompatible_list_plus_equals flag - new SkylarkTest("--incompatible_list_plus_equals=true") + // This behavior should remain the same regardless of the + // --incompatible_list_plus_equals_inplace flag + new SkylarkTest("--incompatible_list_plus_equals_inplace=true") .setUp( "def func():", " t1 = (1, 2)", @@ -1065,10 +1066,10 @@ public void testPlusEqualsOnDict() throws Exception { @Test public void testPlusOnDictDeprecated() throws Exception { - new SkylarkTest("--incompatible_dict_plus=true") + new SkylarkTest("--incompatible_disallow_dict_plus=true") .testIfErrorContains( "The `+` operator for dicts is deprecated and no longer supported.", "{1: 2} + {3: 4}"); - new SkylarkTest("--incompatible_dict_plus=true") + new SkylarkTest("--incompatible_disallow_dict_plus=true") .testIfErrorContains( "The `+` operator for dicts is deprecated and no longer supported.", "def func():", diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java index 4a4cc0f961ef59..c42905fa374965 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java @@ -45,7 +45,7 @@ public void testLegacyConstructor() throws Exception { @Test public void testLegacyConstructorDeprecation() throws Exception { - env = newEnvironmentWithSkylarkOptions("--incompatible_depset_constructor=true"); + env = newEnvironmentWithSkylarkOptions("--incompatible_disallow_set_constructor=true"); try { eval("s = set([1, 2, 3], order='postorder')"); Assert.fail("`set` should have failed");