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

2 files changed, 237 insertions, 86 deletions

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 0000000000..2273f7e8c7
--- /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 4613359509..74f6db4f63 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,178 +26,197 @@ import java.io.Serializable;
  * 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 &mdash; in particular, processing BUILD and .bzl files.
+ * between builds will trigger a reevaluation of everything that depends on the Skylark interpreter
+ * &mdash; 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;
+  // <== Add new options here in alphabetic order ==>
 
-  /**
-   * 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",
+    name = "incompatible_bzl_disallow_load_after_statement",
     defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-    effectTags = {OptionEffectTag.UNKNOWN}
+    category = "incompatible changes",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+    help =
+        "If set to true, all `load` must be called at the top of .bzl files, before any other "
+            + "statement."
   )
-  public boolean internalDoNotExportBuiltins;
+  public boolean incompatibleBzlDisallowLoadAfterStatement;
 
   @Option(
-    name = "incompatible_disallow_set_constructor",
+    name = "incompatible_checked_arithmetic",
     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."
+    help = "If set to true, arithmetic operations throw an error in case of overflow/underflow."
   )
-  public boolean incompatibleDisallowSetConstructor;
+  public boolean incompatibleCheckedArithmetic;
 
   @Option(
-    name = "incompatible_disallow_keyword_only_args",
+    name = "incompatible_comprehension_variables_do_not_leak",
     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."
+    help =
+        "If set to true, loop variables in a comprehension shadow any existing variable by "
+            + "the same name. If the existing variable was declared in the same scope that "
+            + "contains the comprehension, then it also becomes inaccessible after the "
+            + " comprehension executes."
   )
-  public boolean incompatibleDisallowKeywordOnlyArgs;
+  public boolean incompatibleComprehensionVariablesDoNotLeak;
 
   @Option(
-    name = "incompatible_list_plus_equals_inplace",
+    name = "incompatible_depset_is_not_iterable",
     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."
+        "If set to true, depset type is not iterable. For loops and functions expecting an "
+            + "iterable will reject depset objects. Use the `.to_list` method to explicitly "
+            + "convert to a list."
   )
-  public boolean incompatibleListPlusEqualsInplace;
+  public boolean incompatibleDepsetIsNotIterable;
 
   @Option(
-    name = "incompatible_disallow_dict_plus",
-    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, the `+` becomes disabled for dicts."
+    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."
   )
-  public boolean incompatibleDisallowDictPlus;
+  public boolean incompatibleDescriptiveStringRepresentations;
 
   @Option(
-    name = "incompatible_bzl_disallow_load_after_statement",
-    defaultValue = "false",
+    name = "incompatible_dict_literal_has_no_duplicates",
+    defaultValue = "true",
     category = "incompatible changes",
     documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
     effectTags = {OptionEffectTag.UNKNOWN},
     metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
-    help =
-        "If set to true, all `load` must be called at the top of .bzl files, before any other "
-            + "statement."
+    help = "If set to true, the dictionary literal syntax doesn't allow duplicated keys."
   )
-  public boolean incompatibleBzlDisallowLoadAfterStatement;
+  public boolean incompatibleDictLiteralHasNoDuplicates;
 
   @Option(
-    name = "incompatible_load_argument_is_label",
+    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 first argument of 'load' statements is a label (not a path). "
-            + "It must start with '//' or ':'."
+    help = "If set to true, the `+` becomes disabled for dicts."
   )
-  public boolean incompatibleLoadArgumentIsLabel;
+  public boolean incompatibleDisallowDictPlus;
 
   @Option(
-    name = "incompatible_disallow_toplevel_if_statement",
+    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, 'if' statements are forbidden at the top-level "
-            + "(outside a function definition)"
+    help = "If set to true, disables the keyword-only argument syntax in function definition."
   )
-  public boolean incompatibleDisallowToplevelIfStatement;
+  public boolean incompatibleDisallowKeywordOnlyArgs;
 
   @Option(
-    name = "incompatible_comprehension_variables_do_not_leak",
+    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, loop variables in a comprehension shadow any existing variable by "
-            + "the same name. If the existing variable was declared in the same scope that "
-            + "contains the comprehension, then it also becomes inaccessible after the "
-            + " comprehension executes."
+    help = "If set to true, disables the deprecated `set` constructor for depsets."
   )
-  public boolean incompatibleComprehensionVariablesDoNotLeak;
+  public boolean incompatibleDisallowSetConstructor;
 
   @Option(
-    name = "incompatible_depset_is_not_iterable",
-    defaultValue = "false",
+    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, depset type is not iterable. For loops and functions expecting an "
-            + "iterable will reject depset objects. Use the `.to_list` method to explicitly "
-            + "convert to a list."
+        "If set to true, 'if' statements are forbidden at the top-level "
+            + "(outside a function definition)"
   )
-  public boolean incompatibleDepsetIsNotIterable;
+  public boolean incompatibleDisallowToplevelIfStatement;
 
   @Option(
-    name = "incompatible_string_is_not_iterable",
+    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, iterating over a string will throw an error. String indexing and `len` "
-            + "are still allowed."
+        "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 incompatibleStringIsNotIterable;
+  public boolean incompatibleListPlusEqualsInplace;
 
   @Option(
-    name = "incompatible_dict_literal_has_no_duplicates",
-    defaultValue = "true",
+    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 dictionary literal syntax doesn't allow duplicated keys."
+    help =
+        "If set to true, the first argument of 'load' statements is a label (not a path). "
+            + "It must start with '//' or ':'."
   )
-  public boolean incompatibleDictLiteralHasNoDuplicates;
+  public boolean incompatibleLoadArgumentIsLabel;
 
   @Option(
       name = "incompatible_new_actions_api",
@@ -212,27 +231,61 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
   public boolean incompatibleNewActionsApi;
 
   @Option(
-    name = "incompatible_checked_arithmetic",
-    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, arithmetic operations throw an error in case of overflow/underflow."
+    help =
+        "If set to true, iterating over a string will throw an error. String indexing and `len` "
+            + "are still allowed."
   )
-  public boolean incompatibleCheckedArithmetic;
+  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 = "incompatible_descriptive_string_representations",
-    defaultValue = "true",
-    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."
+    name = "internal_do_not_export_builtins",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+    effectTags = {OptionEffectTag.UNKNOWN}
   )
-  public boolean incompatibleDescriptiveStringRepresentations;
+  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();
+  }
 }