Split off SkylarkSemanticsOptions into an immutable class
This is a first step toward making the core Skylark interpreter (the syntax/ directory) not depend on the options parser. Subsequent CLs will replace uses of SkylarkSemanticsOptions within the interpreter with uses of SkylarkSemantics, and move SkylarkSemanticsOptions to the packages/ directory alongside SkylarkSemanticsCodec.
SkylarkSemantics will also replace SkylarkSemanticsOptions as the value that gets passed through Skyframe. This is nice because SkylarkSemantics is strictly immutable, whereas options classes are only kinda-sorta-immutable.
The downside is significantly more redundancy when defining new options. But some of the work is saved by using AutoValue, and there are tests that protect us from dumb mechanical errors. The details are outlined in the javadoc for SkylarkSemanticsOptions and SkylarkSemanticsConsistencyTest.
RELNOTES: None
PiperOrigin-RevId: 171060034
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index b8f6e9c..559bbfc 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -282,11 +282,11 @@
"//src/main/java/com/google/devtools/build/lib:skylarkinterface",
"//src/main/java/com/google/devtools/build/lib:util",
"//src/main/java/com/google/devtools/build/lib/cmdline",
- "//src/main/java/com/google/devtools/build/lib/collect",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/vfs",
+ # TODO(brandjon): Remove dependency of syntax/ on the options parser.
"//src/main/java/com/google/devtools/common/options",
"//third_party:asm",
"//third_party:asm-commons",
@@ -339,6 +339,7 @@
"//src/main/java/com/google/devtools/build/lib/collect",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
+ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
new file mode 100644
index 0000000..8a19ff2
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
@@ -0,0 +1,86 @@
+// Copyright 2017 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.packages;
+
+import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
+import com.google.devtools.build.lib.syntax.SkylarkSemantics;
+import com.google.protobuf.CodedInputStream;
+import com.google.protobuf.CodedOutputStream;
+import java.io.IOException;
+
+/**
+ * Codec for a {@link SkylarkSemantics} object.
+ *
+ * <p>Since the core Skylark interpreter should not depend on the protobuf library, this codec
+ * cannot be nested alongside the {@code SkylarkSemantics} class definition.
+ *
+ * <p>Tests for this codec are in {@link SkylarkSemanticsConsistencyTest}.
+ */
+public final class SkylarkSemanticsCodec implements ObjectCodec<SkylarkSemantics> {
+
+ @Override
+ public Class<SkylarkSemantics> getEncodedClass() {
+ return SkylarkSemantics.class;
+ }
+
+ @Override
+ public void serialize(SkylarkSemantics semantics, CodedOutputStream codedOut)
+ throws SerializationException, IOException {
+ // <== Add new options here in alphabetic order ==>
+ codedOut.writeBoolNoTag(semantics.incompatibleBzlDisallowLoadAfterStatement());
+ codedOut.writeBoolNoTag(semantics.incompatibleCheckedArithmetic());
+ codedOut.writeBoolNoTag(semantics.incompatibleComprehensionVariablesDoNotLeak());
+ codedOut.writeBoolNoTag(semantics.incompatibleDepsetIsNotIterable());
+ codedOut.writeBoolNoTag(semantics.incompatibleDescriptiveStringRepresentations());
+ codedOut.writeBoolNoTag(semantics.incompatibleDictLiteralHasNoDuplicates());
+ codedOut.writeBoolNoTag(semantics.incompatibleDisallowDictPlus());
+ codedOut.writeBoolNoTag(semantics.incompatibleDisallowKeywordOnlyArgs());
+ codedOut.writeBoolNoTag(semantics.incompatibleDisallowSetConstructor());
+ codedOut.writeBoolNoTag(semantics.incompatibleDisallowToplevelIfStatement());
+ codedOut.writeBoolNoTag(semantics.incompatibleListPlusEqualsInplace());
+ codedOut.writeBoolNoTag(semantics.incompatibleLoadArgumentIsLabel());
+ codedOut.writeBoolNoTag(semantics.incompatibleNewActionsApi());
+ codedOut.writeBoolNoTag(semantics.incompatibleStringIsNotIterable());
+ codedOut.writeBoolNoTag(semantics.internalDoNotExportBuiltins());
+ codedOut.writeBoolNoTag(semantics.internalSkylarkFlagTestCanary());
+ }
+
+ @Override
+ public SkylarkSemantics deserialize(CodedInputStream codedIn)
+ throws SerializationException, IOException {
+ SkylarkSemantics.Builder builder = SkylarkSemantics.builder();
+
+ // <== Add new options here in alphabetic order ==>
+ builder.incompatibleBzlDisallowLoadAfterStatement(codedIn.readBool());
+ builder.incompatibleCheckedArithmetic(codedIn.readBool());
+ builder.incompatibleComprehensionVariablesDoNotLeak(codedIn.readBool());
+ builder.incompatibleDepsetIsNotIterable(codedIn.readBool());
+ builder.incompatibleDescriptiveStringRepresentations(codedIn.readBool());
+ builder.incompatibleDictLiteralHasNoDuplicates(codedIn.readBool());
+ builder.incompatibleDisallowDictPlus(codedIn.readBool());
+ builder.incompatibleDisallowKeywordOnlyArgs(codedIn.readBool());
+ builder.incompatibleDisallowSetConstructor(codedIn.readBool());
+ builder.incompatibleDisallowToplevelIfStatement(codedIn.readBool());
+ builder.incompatibleListPlusEqualsInplace(codedIn.readBool());
+ builder.incompatibleLoadArgumentIsLabel(codedIn.readBool());
+ builder.incompatibleNewActionsApi(codedIn.readBool());
+ builder.incompatibleStringIsNotIterable(codedIn.readBool());
+ builder.internalDoNotExportBuiltins(codedIn.readBool());
+ builder.internalSkylarkFlagTestCanary(codedIn.readBool());
+
+ return builder.build();
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
new file mode 100644
index 0000000..2273f7e
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -0,0 +1,98 @@
+// Copyright 2017 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.auto.value.AutoValue;
+
+/**
+ * Options that affect Skylark semantics.
+ *
+ * <p>For descriptions of what these options do, see {@link SkylarkSemanticsOptions}.
+ */
+// TODO(brandjon): User error messages that reference options should maybe be substituted with the
+// option name outside of the core Skylark interpreter?
+// TODO(brandjon): Eventually these should be documented in full here, and SkylarkSemanticsOptions
+// should refer to this class for documentation. But this doesn't play nice with the options
+// parser's annotation mechanism.
+@AutoValue
+public abstract class SkylarkSemantics {
+
+ // <== Add new options here in alphabetic order ==>
+ public abstract boolean incompatibleBzlDisallowLoadAfterStatement();
+ public abstract boolean incompatibleCheckedArithmetic();
+ public abstract boolean incompatibleComprehensionVariablesDoNotLeak();
+ public abstract boolean incompatibleDepsetIsNotIterable();
+ public abstract boolean incompatibleDescriptiveStringRepresentations();
+ public abstract boolean incompatibleDictLiteralHasNoDuplicates();
+ public abstract boolean incompatibleDisallowDictPlus();
+ public abstract boolean incompatibleDisallowKeywordOnlyArgs();
+ public abstract boolean incompatibleDisallowSetConstructor();
+ public abstract boolean incompatibleDisallowToplevelIfStatement();
+ public abstract boolean incompatibleListPlusEqualsInplace();
+ public abstract boolean incompatibleLoadArgumentIsLabel();
+ public abstract boolean incompatibleNewActionsApi();
+ public abstract boolean incompatibleStringIsNotIterable();
+ public abstract boolean internalDoNotExportBuiltins();
+ public abstract boolean internalSkylarkFlagTestCanary();
+
+ public static Builder builder() {
+ return new AutoValue_SkylarkSemantics.Builder();
+ }
+
+ public static final SkylarkSemantics DEFAULT_SEMANTICS = builder()
+ // <== Add new options here in alphabetic order ==>
+ .incompatibleBzlDisallowLoadAfterStatement(false)
+ .incompatibleCheckedArithmetic(true)
+ .incompatibleComprehensionVariablesDoNotLeak(true)
+ .incompatibleDepsetIsNotIterable(false)
+ .incompatibleDescriptiveStringRepresentations(true)
+ .incompatibleDictLiteralHasNoDuplicates(true)
+ .incompatibleDisallowDictPlus(false)
+ .incompatibleDisallowKeywordOnlyArgs(true)
+ .incompatibleDisallowSetConstructor(true)
+ .incompatibleDisallowToplevelIfStatement(true)
+ .incompatibleListPlusEqualsInplace(false)
+ .incompatibleLoadArgumentIsLabel(false)
+ .incompatibleNewActionsApi(false)
+ .incompatibleStringIsNotIterable(false)
+ .internalDoNotExportBuiltins(false)
+ .internalSkylarkFlagTestCanary(false)
+ .build();
+
+ /** Builder for {@link SkylarkSemantics}. All fields are mandatory. */
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ // <== Add new options here in alphabetic order ==>
+ public abstract Builder incompatibleBzlDisallowLoadAfterStatement(boolean value);
+ public abstract Builder incompatibleCheckedArithmetic(boolean value);
+ public abstract Builder incompatibleComprehensionVariablesDoNotLeak(boolean value);
+ public abstract Builder incompatibleDepsetIsNotIterable(boolean value);
+ public abstract Builder incompatibleDescriptiveStringRepresentations(boolean value);
+ public abstract Builder incompatibleDictLiteralHasNoDuplicates(boolean value);
+ public abstract Builder incompatibleDisallowDictPlus(boolean value);
+ public abstract Builder incompatibleDisallowKeywordOnlyArgs(boolean value);
+ public abstract Builder incompatibleDisallowSetConstructor(boolean value);
+ public abstract Builder incompatibleDisallowToplevelIfStatement(boolean value);
+ public abstract Builder incompatibleListPlusEqualsInplace(boolean value);
+ public abstract Builder incompatibleLoadArgumentIsLabel(boolean value);
+ public abstract Builder incompatibleNewActionsApi(boolean value);
+ public abstract Builder incompatibleStringIsNotIterable(boolean value);
+ public abstract Builder internalDoNotExportBuiltins(boolean value);
+ public abstract Builder internalSkylarkFlagTestCanary(boolean value);
+
+ public abstract SkylarkSemantics build();
+ }
+}
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 4613359..74f6db4 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
@@ -26,86 +26,47 @@
* Contains options that affect Skylark's semantics.
*
* <p>These are injected into Skyframe when a new build invocation occurs. Changing these options
- * between builds will trigger a reevaluation of everything that depends on the Skylark
- * interpreter — in particular, processing BUILD and .bzl files.
+ * between builds will trigger a reevaluation of everything that depends on the Skylark interpreter
+ * — in particular, processing BUILD and .bzl files.
*
* <p>Because these options are stored in Skyframe, they must be immutable and serializable, and so
* are subject to the restrictions of {@link UsesOnlyCoreTypes}: No {@link Option#allowMultiple}
* options, and no options with types not handled by the default converters. (Technically all
* options classes are mutable because their fields are public and non-final, but we assume no one
* is manipulating these fields by the time parsing is complete.)
+ *
+ * <p><em>To add a new option, update the following:</em>
+ * <ul>
+ * <li>Add a new abstract method (which is interpreted by {@code AutoValue} as a field) to {@link
+ * SkylarkSemantics} and {@link SkylarkSemantics.Builder}. Set its default value in {@link
+ * SkylarkSemantics#DEFAULT_SEMANTICS}.
+ *
+ * <li>Add a new {@code @Option}-annotated field to this class. The field name and default value
+ * should be the same as in {@link SkylarkSemantics}, and the option name in the annotation
+ * should be that name written in snake_case. Add a line to set the new field in {@link
+ * #toSkylarkSemantics}.
+ *
+ * <li>Add a line to read and write the new field in {@link SkylarkSemanticsCodec#serialize} and
+ * {@link SkylarkSemanticsCodec#deserialize}.
+ *
+ * <li>Add a line to set the new field in both {@link
+ * SkylarkSemanticsOptionsTest#buildRandomOptions} and {@link
+ * SkylarkSemanticsOptions#buildRandomSemantics}.
+ *
+ * <li>Update manual documentation in site/docs/skylark/backward-compatibility.md. Also remember
+ * to update this when flipping a flag's default value.
+ * </ul>
+ * For both readability and correctness, the relative order of the options in all of these locations
+ * must be kept consistent; to make it easy we use alphabetic order. The parts that need updating
+ * are marked with the comment "<== Add new options here in alphabetic order ==>".
*/
+// TODO(brandjon): Do not store these options in Skyframe. Instead store SkylarkSemantics objects.
+// Eliminate use of UsesOnlyCoreTypes, and then we can remove UsesOnlyCoreTypes from the options
+// parser entirely.
@UsesOnlyCoreTypes
public class SkylarkSemanticsOptions extends OptionsBase implements Serializable {
- /** Used in an integration test to confirm that flags are visible to the interpreter. */
- @Option(
- name = "internal_skylark_flag_test_canary",
- defaultValue = "false",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.UNKNOWN}
- )
- public boolean internalSkylarkFlagTestCanary;
-
- /**
- * Used in testing to produce a truly minimalistic Extension object for certain evaluation
- * contexts. This flag is Bazel-specific.
- */
- // TODO(bazel-team): A pending incompatible change will make it so that load()ed and built-in
- // symbols do not get re-exported, making this flag obsolete.
- @Option(
- name = "internal_do_not_export_builtins",
- defaultValue = "false",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.UNKNOWN}
- )
- public boolean internalDoNotExportBuiltins;
-
- @Option(
- name = "incompatible_disallow_set_constructor",
- defaultValue = "true",
- category = "incompatible changes",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.UNKNOWN},
- metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- help = "If set to true, disables the deprecated `set` constructor for depsets."
- )
- public boolean incompatibleDisallowSetConstructor;
-
- @Option(
- name = "incompatible_disallow_keyword_only_args",
- defaultValue = "true",
- category = "incompatible changes",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.UNKNOWN},
- metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- help = "If set to true, disables the keyword-only argument syntax in function definition."
- )
- public boolean incompatibleDisallowKeywordOnlyArgs;
-
- @Option(
- name = "incompatible_list_plus_equals_inplace",
- defaultValue = "false",
- category = "incompatible changes",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.UNKNOWN},
- metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- 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 incompatibleListPlusEqualsInplace;
-
- @Option(
- name = "incompatible_disallow_dict_plus",
- defaultValue = "false",
- category = "incompatible changes",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.UNKNOWN},
- metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- help = "If set to true, the `+` becomes disabled for dicts."
- )
- public boolean incompatibleDisallowDictPlus;
+ // <== Add new options here in alphabetic order ==>
@Option(
name = "incompatible_bzl_disallow_load_after_statement",
@@ -121,30 +82,15 @@
public boolean incompatibleBzlDisallowLoadAfterStatement;
@Option(
- name = "incompatible_load_argument_is_label",
- defaultValue = "false",
- category = "incompatible changes",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.UNKNOWN},
- metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- help =
- "If set to true, the first argument of 'load' statements is a label (not a path). "
- + "It must start with '//' or ':'."
- )
- public boolean incompatibleLoadArgumentIsLabel;
-
- @Option(
- name = "incompatible_disallow_toplevel_if_statement",
+ name = "incompatible_checked_arithmetic",
defaultValue = "true",
category = "incompatible changes",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- help =
- "If set to true, 'if' statements are forbidden at the top-level "
- + "(outside a function definition)"
+ help = "If set to true, arithmetic operations throw an error in case of overflow/underflow."
)
- public boolean incompatibleDisallowToplevelIfStatement;
+ public boolean incompatibleCheckedArithmetic;
@Option(
name = "incompatible_comprehension_variables_do_not_leak",
@@ -176,17 +122,18 @@
public boolean incompatibleDepsetIsNotIterable;
@Option(
- name = "incompatible_string_is_not_iterable",
- defaultValue = "false",
+ name = "incompatible_descriptive_string_representations",
+ defaultValue = "true",
category = "incompatible changes",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help =
- "If set to true, iterating over a string will throw an error. String indexing and `len` "
- + "are still allowed."
+ "If set to true, objects are converted to strings by `str` and `repr` functions using the "
+ + "new style representations that are designed to be more descriptive and not to leak "
+ + "information that's not supposed to be exposed."
)
- public boolean incompatibleStringIsNotIterable;
+ public boolean incompatibleDescriptiveStringRepresentations;
@Option(
name = "incompatible_dict_literal_has_no_duplicates",
@@ -200,6 +147,78 @@
public boolean incompatibleDictLiteralHasNoDuplicates;
@Option(
+ name = "incompatible_disallow_dict_plus",
+ defaultValue = "false",
+ category = "incompatible changes",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+ help = "If set to true, the `+` becomes disabled for dicts."
+ )
+ public boolean incompatibleDisallowDictPlus;
+
+ @Option(
+ name = "incompatible_disallow_keyword_only_args",
+ defaultValue = "true",
+ category = "incompatible changes",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+ help = "If set to true, disables the keyword-only argument syntax in function definition."
+ )
+ public boolean incompatibleDisallowKeywordOnlyArgs;
+
+ @Option(
+ name = "incompatible_disallow_set_constructor",
+ defaultValue = "true",
+ category = "incompatible changes",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+ help = "If set to true, disables the deprecated `set` constructor for depsets."
+ )
+ public boolean incompatibleDisallowSetConstructor;
+
+ @Option(
+ name = "incompatible_disallow_toplevel_if_statement",
+ defaultValue = "true",
+ category = "incompatible changes",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+ help =
+ "If set to true, 'if' statements are forbidden at the top-level "
+ + "(outside a function definition)"
+ )
+ public boolean incompatibleDisallowToplevelIfStatement;
+
+ @Option(
+ name = "incompatible_list_plus_equals_inplace",
+ defaultValue = "false",
+ category = "incompatible changes",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+ 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 incompatibleListPlusEqualsInplace;
+
+ @Option(
+ name = "incompatible_load_argument_is_label",
+ defaultValue = "false",
+ category = "incompatible changes",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+ help =
+ "If set to true, the first argument of 'load' statements is a label (not a path). "
+ + "It must start with '//' or ':'."
+ )
+ public boolean incompatibleLoadArgumentIsLabel;
+
+ @Option(
name = "incompatible_new_actions_api",
defaultValue = "false",
category = "incompatible changes",
@@ -212,27 +231,61 @@
public boolean incompatibleNewActionsApi;
@Option(
- name = "incompatible_checked_arithmetic",
- defaultValue = "true",
- category = "incompatible changes",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.UNKNOWN},
- metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
- help = "If set to true, arithmetic operations throw an error in case of overflow/underflow."
- )
- public boolean incompatibleCheckedArithmetic;
-
- @Option(
- name = "incompatible_descriptive_string_representations",
- defaultValue = "true",
+ name = "incompatible_string_is_not_iterable",
+ defaultValue = "false",
category = "incompatible changes",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help =
- "If set to true, objects are converted to strings by `str` and `repr` functions using the "
- + "new style representations that are designed to be more descriptive and not to leak "
- + "information that's not supposed to be exposed."
+ "If set to true, iterating over a string will throw an error. String indexing and `len` "
+ + "are still allowed."
)
- public boolean incompatibleDescriptiveStringRepresentations;
+ public boolean incompatibleStringIsNotIterable;
+
+ /**
+ * Used in testing to produce a truly minimalistic Extension object for certain evaluation
+ * contexts. This flag is Bazel-specific.
+ */
+ // TODO(bazel-team): A pending incompatible change will make it so that load()ed and built-in
+ // symbols do not get re-exported, making this flag obsolete.
+ @Option(
+ name = "internal_do_not_export_builtins",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.UNKNOWN}
+ )
+ public boolean internalDoNotExportBuiltins;
+
+ /** Used in an integration test to confirm that flags are visible to the interpreter. */
+ @Option(
+ name = "internal_skylark_flag_test_canary",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.UNKNOWN}
+ )
+ public boolean internalSkylarkFlagTestCanary;
+
+ /** Constructs a {@link SkylarkSemantics} object corresponding to this set of option values. */
+ public SkylarkSemantics toSkylarkSemantics() {
+ return SkylarkSemantics.builder()
+ // <== Add new options here in alphabetic order ==>
+ .incompatibleBzlDisallowLoadAfterStatement(incompatibleBzlDisallowLoadAfterStatement)
+ .incompatibleCheckedArithmetic(incompatibleCheckedArithmetic)
+ .incompatibleComprehensionVariablesDoNotLeak(incompatibleComprehensionVariablesDoNotLeak)
+ .incompatibleDepsetIsNotIterable(incompatibleDepsetIsNotIterable)
+ .incompatibleDescriptiveStringRepresentations(incompatibleDescriptiveStringRepresentations)
+ .incompatibleDictLiteralHasNoDuplicates(incompatibleDictLiteralHasNoDuplicates)
+ .incompatibleDisallowDictPlus(incompatibleDisallowDictPlus)
+ .incompatibleDisallowKeywordOnlyArgs(incompatibleDisallowKeywordOnlyArgs)
+ .incompatibleDisallowSetConstructor(incompatibleDisallowSetConstructor)
+ .incompatibleDisallowToplevelIfStatement(incompatibleDisallowToplevelIfStatement)
+ .incompatibleListPlusEqualsInplace(incompatibleListPlusEqualsInplace)
+ .incompatibleLoadArgumentIsLabel(incompatibleLoadArgumentIsLabel)
+ .incompatibleNewActionsApi(incompatibleNewActionsApi)
+ .incompatibleStringIsNotIterable(incompatibleStringIsNotIterable)
+ .internalDoNotExportBuiltins(internalDoNotExportBuiltins)
+ .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary)
+ .build();
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 8795a25..61903f6 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -667,6 +667,7 @@
"//src/main/java/com/google/devtools/build/lib:packages",
"//src/main/java/com/google/devtools/build/lib:skylarkinterface",
"//src/main/java/com/google/devtools/build/lib:util",
+ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
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
new file mode 100644
index 0000000..baaa1b0
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -0,0 +1,160 @@
+// Copyright 2017 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.packages;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.devtools.build.lib.skyframe.serialization.testutils.TestUtils;
+import com.google.devtools.build.lib.syntax.SkylarkSemantics;
+import com.google.devtools.build.lib.syntax.SkylarkSemanticsOptions;
+import com.google.devtools.common.options.Options;
+import com.google.devtools.common.options.OptionsParser;
+import java.util.Arrays;
+import java.util.Random;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for the flow of flags from {@link SkylarkSemanticsOptions} to {@link SkylarkSemantics}, and
+ * to and from {@code SkylarkSemantics}' serialized representation.
+ *
+ * <p>When adding a new option, it is trivial to make a transposition error or a copy/paste error.
+ * These tests guard against such errors. The following possible bugs are considered:
+ * <ul>
+ * <li>If a new option is added to {@code SkylarkSemantics} but not to {@code
+ * SkylarkSemanticsOptions}, or vice versa, then the programmer will either be unable to
+ * implement its behavior, or unable to test it from the command line and add user
+ * documentation. We hope that the programmer notices this on their own.
+ *
+ * <li>If {@link SkylarkSemanticsOptions#toSkylarkSemantics} or {@link
+ * SkylarkSemanticsCodec#deserialize} is not updated to set all fields of {@code
+ * SkylarkSemantics}, then it will fail immediately because all fields of {@link
+ * SkylarkSemantics.Builder} are mandatory.
+ *
+ * <li>To catch a copy/paste error where the wrong field's data is threaded through {@code
+ * toSkylarkSemantics()} or {@code deserialize(...)}, we repeatedly generate matching random
+ * instances of the input and expected output objects.
+ *
+ * <li>The {@link #checkDefaultsMatch} test ensures that there is no divergence between the
+ * default values of the two classes.
+ *
+ * <li>There is no test coverage for failing to update the non-generated webpage documentation.
+ * So don't forget that!
+ * </ul>
+ */
+@RunWith(JUnit4.class)
+public class SkylarkSemanticsConsistencyTest {
+
+ private static final int NUM_RANDOM_TRIALS = 10;
+
+ /**
+ * Checks that a randomly generated {@link SkylarkSemanticsOptions} object can be converted to a
+ * {@link SkylarkSemantics} object with the same field values.
+ */
+ @Test
+ public void optionsToSemantics() throws Exception {
+ for (int i = 0; i < NUM_RANDOM_TRIALS; i++) {
+ long seed = i;
+ SkylarkSemanticsOptions options = buildRandomOptions(new Random(seed));
+ SkylarkSemantics semantics = buildRandomSemantics(new Random(seed));
+ SkylarkSemantics semanticsFromOptions = options.toSkylarkSemantics();
+ assertThat(semanticsFromOptions).isEqualTo(semantics);
+ }
+ }
+
+ /*
+ * Checks that a randomly generated {@link SkylarkSemantics} object can be serialized and
+ * deserialized to an equivalent object.
+ */
+ @Test
+ public void serializationRoundTrip() throws Exception {
+ SkylarkSemanticsCodec codec = new SkylarkSemanticsCodec();
+ for (int i = 0; i < NUM_RANDOM_TRIALS; i++) {
+ SkylarkSemantics semantics = buildRandomSemantics(new Random(i));
+ SkylarkSemantics deserialized =
+ TestUtils.fromBytes(codec, TestUtils.toBytes(codec, semantics));
+ assertThat(deserialized).isEqualTo(semantics);
+ }
+ }
+
+ @Test
+ public void checkDefaultsMatch() {
+ SkylarkSemanticsOptions defaultOptions = Options.getDefaults(SkylarkSemanticsOptions.class);
+ SkylarkSemantics defaultSemantics = SkylarkSemantics.DEFAULT_SEMANTICS;
+ SkylarkSemantics semanticsFromOptions = defaultOptions.toSkylarkSemantics();
+ assertThat(semanticsFromOptions).isEqualTo(defaultSemantics);
+ }
+
+ /**
+ * Constructs a {@link SkylarkSemanticsOptions} object with random fields. Must access {@code
+ * rand} using the same sequence of operations (for the same fields) as {@link
+ * #buildRandomSemantics}.
+ */
+ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Exception {
+ return parseOptions(
+ // <== Add new options here in alphabetic order ==>
+ "--incompatible_bzl_disallow_load_after_statement=" + rand.nextBoolean(),
+ "--incompatible_checked_arithmetic=" + rand.nextBoolean(),
+ "--incompatible_comprehension_variables_do_not_leak=" + rand.nextBoolean(),
+ "--incompatible_depset_is_not_iterable=" + rand.nextBoolean(),
+ "--incompatible_descriptive_string_representations=" + rand.nextBoolean(),
+ "--incompatible_dict_literal_has_no_duplicates=" + rand.nextBoolean(),
+ "--incompatible_disallow_dict_plus=" + rand.nextBoolean(),
+ "--incompatible_disallow_keyword_only_args=" + rand.nextBoolean(),
+ "--incompatible_disallow_set_constructor=" + rand.nextBoolean(),
+ "--incompatible_disallow_toplevel_if_statement=" + rand.nextBoolean(),
+ "--incompatible_list_plus_equals_inplace=" + rand.nextBoolean(),
+ "--incompatible_load_argument_is_label=" + rand.nextBoolean(),
+ "--incompatible_new_actions_api=" + rand.nextBoolean(),
+ "--incompatible_string_is_not_iterable=" + rand.nextBoolean(),
+ "--internal_do_not_export_builtins=" + rand.nextBoolean(),
+ "--internal_skylark_flag_test_canary=" + rand.nextBoolean()
+ );
+ }
+
+ /**
+ * Constructs a {@link SkylarkSemantics} object with random fields. Must access {@code rand} using
+ * the same sequence of operations (for the same fields) as {@link #buildRandomOptions}.
+ */
+ private static SkylarkSemantics buildRandomSemantics(Random rand) {
+ return SkylarkSemantics.builder()
+ // <== Add new options here in alphabetic order ==>
+ .incompatibleBzlDisallowLoadAfterStatement(rand.nextBoolean())
+ .incompatibleCheckedArithmetic(rand.nextBoolean())
+ .incompatibleComprehensionVariablesDoNotLeak(rand.nextBoolean())
+ .incompatibleDepsetIsNotIterable(rand.nextBoolean())
+ .incompatibleDescriptiveStringRepresentations(rand.nextBoolean())
+ .incompatibleDictLiteralHasNoDuplicates(rand.nextBoolean())
+ .incompatibleDisallowDictPlus(rand.nextBoolean())
+ .incompatibleDisallowKeywordOnlyArgs(rand.nextBoolean())
+ .incompatibleDisallowSetConstructor(rand.nextBoolean())
+ .incompatibleDisallowToplevelIfStatement(rand.nextBoolean())
+ .incompatibleListPlusEqualsInplace(rand.nextBoolean())
+ .incompatibleLoadArgumentIsLabel(rand.nextBoolean())
+ .incompatibleNewActionsApi(rand.nextBoolean())
+ .incompatibleStringIsNotIterable(rand.nextBoolean())
+ .internalDoNotExportBuiltins(rand.nextBoolean())
+ .internalSkylarkFlagTestCanary(rand.nextBoolean())
+ .build();
+ }
+
+ private static SkylarkSemanticsOptions parseOptions(String... args) throws Exception {
+ OptionsParser parser = OptionsParser.newOptionsParser(SkylarkSemanticsOptions.class);
+ parser.setAllowResidue(false);
+ parser.parse(Arrays.asList(args));
+ return parser.getOptions(SkylarkSemanticsOptions.class);
+ }
+}
