Make --experimental_remove_old_python_version_api not breaking existi…

…ng_rules()

The current way of gating access to the deprecated attribute `default_python_version` is to have the rule check whether or not the attribute was set explicitly (AttributeMap#isAttributeValueExplicitlySpecified), and match that against whether or not the attribute is allowed. However, this breaks the use case of copying a rule definition programmatically via `native.existing_rules()`, because when the definition is copied all the attributes of the new target appear to be explicitly specified.

The solution is to use a sentinel value as the physical default value in the attribute definition. Then the rule just checks for equality with that sentinel. If the configuration permits use of that attribute, then the sentinel is interpreted the same as what the logical default would be.

This requires adding an item to the PythonVersion enum. Ugly, but necessary. We can remove it once the incompatible flag is flipped to remove the old API.

Work toward #6583. Fixes #7071.

RELNOTES: None
PiperOrigin-RevId: 230399577

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryRule.java

@@ -41,7 +41,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
     <!-- #END_BLAZE_RULE.NAME --> */
     return builder
         .requiresConfigurationFragments(PythonConfiguration.class, BazelPythonConfiguration.class)
-        .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION)
+        .cfg(PyRuleClasses.VERSION_TRANSITION)
         .add(
             attr("$zipper", LABEL)
                 .cfg(HostTransition.INSTANCE)

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java

@@ -111,7 +111,7 @@ A string specifying the Python major version(s) that the <code>.py</code> source
           .add(
               attr("srcs_version", STRING)
                   .value(PythonVersion.DEFAULT_SRCS_VALUE.toString())
-                  .allowedValues(new AllowedValueSet(PythonVersion.ALL_STRINGS)))
+                  .allowedValues(new AllowedValueSet(PythonVersion.SRCS_STRINGS)))
           // TODO(brandjon): Consider adding to py_interpreter a .mandatoryNativeProviders() of
           // BazelPyRuntimeProvider. (Add a test case to PythonConfigurationTest for violations
           // of this requirement.)
@@ -170,8 +170,8 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .add(
               attr(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, STRING)
-                  .value(PythonVersion.DEFAULT_TARGET_VALUE.toString())
-                  .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS))
+                  .value(PythonVersion._INTERNAL_SENTINEL.toString())
+                  .allowedValues(PyRuleClasses.TARGET_PYTHON_ATTR_VALUE_SET)
                   .nonconfigurable(
                       "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
                           + " to the configuration"))
@@ -181,8 +181,8 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .add(
               attr(PyCommon.PYTHON_VERSION_ATTRIBUTE, STRING)
-                  .value(PythonVersion.DEFAULT_TARGET_VALUE.toString())
-                  .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS))
+                  .value(PythonVersion._INTERNAL_SENTINEL.toString())
+                  .allowedValues(PyRuleClasses.TARGET_PYTHON_ATTR_VALUE_SET)
                   .nonconfigurable(
                       "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
                           + " to the configuration"))

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyTestRule.java

@@ -39,7 +39,7 @@ public final class BazelPyTestRule implements RuleDefinition {
   public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
     return builder
         .requiresConfigurationFragments(PythonConfiguration.class, BazelPythonConfiguration.class)
-        .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION)
+        .cfg(PyRuleClasses.VERSION_TRANSITION)
         .add(
             attr("$zipper", LABEL)
                 .cfg(HostTransition.INSTANCE)

src/main/java/com/google/devtools/build/lib/packages/Attribute.java

@@ -329,7 +329,7 @@ public static class AllowedValueSet implements PredicateWithMessage<Object> {
 
     private final Set<Object> allowedValues;
 
-    public <T> AllowedValueSet(T... values) {
+    public AllowedValueSet(Object... values) {
       this(Arrays.asList(values));
     }
 

src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java

@@ -93,13 +93,20 @@ public interface AttributeMap {
   @Nullable Attribute getAttributeDefinition(String attrName);
 
   /**
-   * Returns true iff the value of the specified attribute is explicitly set in the BUILD file (as
-   * opposed to its default value). This also returns true if the value from the BUILD file is the
-   * same as the default value.
+   * Returns true iff the specified attribute is explicitly set in the target's definition (as
+   * opposed to being omitted and taking on its default value from the rule definition).
    *
-   * <p>It is probably a good idea to avoid this method in default value and implicit outputs
-   * computation, because it is confusing that setting an attribute to an empty list (for example)
-   * is different from not setting it at all.
+   * <p>Note that this returns true in the case where the attribute is explicitly set to the same
+   * value as its default. Therefore, this method breaks encapsulation in the sense that it
+   * describes *how* a target is defined rather than just *what* its attribute values are.
+   *
+   * <p>CAUTION: It is a good idea to avoid relying on this method if possible. It's confusing to
+   * users that setting an attribute to (for example) an empty list is different from not setting it
+   * at all. It also breaks some use cases, such as programmatically copying a target definition via
+   * {@code native.existing_rules}. Specifically, the Starlark code doing the copying will observe
+   * the attribute on the existing target whether or not it was set explicitly, and then set that
+   * value explicitly on the new target. This can cause the two targets to behave differently, and
+   * can be a difficult bug to track down. (See #7071, b/122596733).
    */
   boolean isAttributeValueExplicitlySpecified(String attributeName);
 

src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java

@@ -65,6 +65,37 @@ public final class PyCommon {
   public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version";
   public static final String PYTHON_VERSION_ATTRIBUTE = "python_version";
 
+  /**
+   * Returns the Python version based on the {@code python_version} and {@code
+   * default_python_version} attributes of the given {@code AttributeMap}.
+   *
+   * <p>It is expected that both attributes are defined, string-typed, and default to {@link
+   * PythonVersion#_INTERNAL_SENTINEL}. The returned version is the value of {@code python_version}
+   * if it is not the sentinel, then {@code default_python_version} if it is not the sentinel, then
+   * finally {@code default}. In all cases the return value is a target version value (either {@code
+   * PY2} or {@code PY3}).
+   *
+   * @throws IllegalArgumentException if the attributes are not present, not string-typed, or not
+   *     parsable as target {@link PythonVersion} values or as the sentinel value; or if {@code
+   *     default} is not a target version value
+   */
+  public static PythonVersion readPythonVersionFromAttributes(
+      AttributeMap attrs, PythonVersion defaultVersion) {
+    Preconditions.checkArgument(defaultVersion.isTargetValue());
+    PythonVersion pythonVersionAttr =
+        PythonVersion.parseTargetOrSentinelValue(attrs.get(PYTHON_VERSION_ATTRIBUTE, Type.STRING));
+    PythonVersion defaultPythonVersionAttr =
+        PythonVersion.parseTargetOrSentinelValue(
+            attrs.get(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING));
+    if (pythonVersionAttr != PythonVersion._INTERNAL_SENTINEL) {
+      return pythonVersionAttr;
+    } else if (defaultPythonVersionAttr != PythonVersion._INTERNAL_SENTINEL) {
+      return defaultPythonVersionAttr;
+    } else {
+      return defaultVersion;
+    }
+  }
+
   private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() {
     @Override
     public void collectMetadataArtifacts(Iterable<Artifact> artifacts,
@@ -153,7 +184,9 @@ public PyCommon(RuleContext ruleContext, PythonSemantics semantics) {
     this.hasPy3OnlySources = initHasPy3OnlySources(ruleContext, this.sourcesVersion);
     this.convertedFiles = makeAndInitConvertedFiles(ruleContext, version, this.sourcesVersion);
     maybeValidateVersionCompatibleWithOwnSourcesAttr();
-    validatePythonVersionAttr();
+    validateTargetPythonVersionAttr(DEFAULT_PYTHON_VERSION_ATTRIBUTE);
+    validateTargetPythonVersionAttr(PYTHON_VERSION_ATTRIBUTE);
+    validateOldVersionAttrNotUsedIfDisabled();
   }
 
   /** Returns the parsed value of the "srcs_version" attribute. */
@@ -167,7 +200,7 @@ private static PythonVersion initSrcsVersionAttr(RuleContext ruleContext) {
           "srcs_version",
           String.format(
               "'%s' is not a valid value. Expected one of: %s",
-              attrValue, Joiner.on(", ").join(PythonVersion.ALL_STRINGS)));
+              attrValue, Joiner.on(", ").join(PythonVersion.SRCS_STRINGS)));
       return PythonVersion.DEFAULT_SRCS_VALUE;
     }
   }
@@ -386,16 +419,49 @@ private void maybeValidateVersionCompatibleWithOwnSourcesAttr() {
     }
   }
 
+  /**
+   * Reports an attribute error if the given target Python version attribute ({@code
+   * default_python_version} or {@code python_version}) cannot be parsed as {@code PY2}, {@code
+   * PY3}, or the sentinel value.
+   *
+   * <p>This *should* be enforced by rule attribute validation ({@link
+   * Attribute.Builder.allowedValues}), but this check is here to fail-fast just in case.
+   */
+  private void validateTargetPythonVersionAttr(String attr) {
+    AttributeMap attrs = ruleContext.attributes();
+    if (!attrs.has(attr, Type.STRING)) {
+      return;
+    }
+    String attrValue = attrs.get(attr, Type.STRING);
+    try {
+      PythonVersion.parseTargetOrSentinelValue(attrValue);
+    } catch (IllegalArgumentException ex) {
+      ruleContext.attributeError(
+          attr,
+          String.format("'%s' is not a valid value. Expected either 'PY2' or 'PY3'", attrValue));
+    }
+  }
+
   /**
    * Reports an attribute error if the {@code default_python_version} attribute is set but
    * disallowed by the configuration.
    */
-  private void validatePythonVersionAttr() {
+  private void validateOldVersionAttrNotUsedIfDisabled() {
     AttributeMap attrs = ruleContext.attributes();
+    if (!attrs.has(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING)) {
+      return;
+    }
+    PythonVersion value;
+    try {
+      value =
+          PythonVersion.parseTargetOrSentinelValue(
+              attrs.get(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING));
+    } catch (IllegalArgumentException e) {
+      // Should be reported by validateTargetPythonVersionAttr(); no action required here.
+      return;
+    }
     PythonConfiguration config = ruleContext.getFragment(PythonConfiguration.class);
-    if (attrs.has(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING)
-        && attrs.isAttributeValueExplicitlySpecified(DEFAULT_PYTHON_VERSION_ATTRIBUTE)
-        && !config.oldPyVersionApiAllowed()) {
+    if (value != PythonVersion._INTERNAL_SENTINEL && !config.oldPyVersionApiAllowed()) {
       ruleContext.attributeError(
           DEFAULT_PYTHON_VERSION_ATTRIBUTE,
           "the 'default_python_version' attribute is disabled by the "

src/main/java/com/google/devtools/build/lib/rules/python/PyRuleClasses.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.rules.python;
 
 import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -27,43 +28,61 @@ public class PyRuleClasses {
   public static final FileType PYTHON_SOURCE = FileType.of(".py", ".py3");
 
   /**
-   * A rule transition factory for Python binary rules and other rules that may change the Python
-   * version.
+   * A value set of the target and sentinel values that doesn't mention the sentinel in error
+   * messages.
+   */
+  public static final AllowedValueSet TARGET_PYTHON_ATTR_VALUE_SET =
+      new AllowedValueSet(PythonVersion.TARGET_AND_SENTINEL_STRINGS) {
+        @Override
+        public String getErrorReason(Object value) {
+          return String.format("has to be one of 'PY2' or 'PY3' instead of '%s'", value);
+        }
+      };
+
+  /**
+   * Returns a rule transition factory for Python binary rules and other rules that may change the
+   * Python version.
    *
-   * <p>This sets the Python version to the value specified by {@code python_version} if it is given
-   * explicitly, or the (possibly default) value of {@code default_python_version} otherwise.
+   * <p>The factory makes a transition to set the Python version to the value specified by the
+   * rule's {@code python_version} attribute if it is given, or otherwise the {@code
+   * default_python_version} attribute if it is given, or otherwise the default value passed into
+   * this function.
    *
-   * <p>The transition throws {@link IllegalArgumentException} if used on a rule whose {@link
-   * RuleClass} does not define both attributes. If both are defined, but the one whose value is to
-   * be read cannot be parsed as a Python version, {@link PythonVersion#DEFAULT_TARGET_VALUE} is
-   * used instead; in this case it is up to the rule's analysis phase (in {@link PyCommon}) to
-   * report an attribute error to the user.
+   * <p>The factory throws {@link IllegalArgumentException} if used on a rule whose {@link
+   * RuleClass} does not define both attributes. If both are defined, but one of their values cannot
+   * be parsed as a Python version, the given default value is used as a fallback instead; in this
+   * case it is up to the rule's analysis phase ({@link PyCommon#validateTargetPythonVersionAttr})
+   * to report an attribute error to the user. This case should be prevented by attribute validation
+   * if the rule is defined correctly.
    */
-  public static final RuleTransitionFactory PYTHON_VERSION_TRANSITION =
-      (rule) -> {
-        AttributeMap attrs = RawAttributeMapper.of(rule);
-        Preconditions.checkArgument(
-            attrs.has(PyCommon.PYTHON_VERSION_ATTRIBUTE, Type.STRING)
-                && attrs.has(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING),
-            "python version transitions require that the RuleClass define both "
-                + "'default_python_version' and 'python_version'");
+  public static RuleTransitionFactory makeVersionTransition(PythonVersion defaultVersion) {
+    return (rule) -> {
+      AttributeMap attrs = RawAttributeMapper.of(rule);
+      // Fail fast if we're used on an ill-defined rule class.
+      Preconditions.checkArgument(
+          attrs.has(PyCommon.PYTHON_VERSION_ATTRIBUTE, Type.STRING)
+              && attrs.has(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING),
+          "python version transitions require that the RuleClass define both "
+              + "'default_python_version' and 'python_version'");
+      // Attribute validation should enforce that the attribute string value is either a target
+      // value ("PY2" or "PY3") or the sentinel value ("_INTERNAL_SENTINEL"). But just in case,
+      // we'll, treat an invalid value as the default value rather than propagate an unchecked
+      // exception in this context. That way the user can at least get a clean error message
+      // instead of a crash.
+      PythonVersion version;
+      try {
+        version = PyCommon.readPythonVersionFromAttributes(attrs, defaultVersion);
+      } catch (IllegalArgumentException ex) {
+        version = defaultVersion;
+      }
+      return new PythonVersionTransition(version);
+    };
+  }
 
-        String versionString =
-            attrs.isAttributeValueExplicitlySpecified(PyCommon.PYTHON_VERSION_ATTRIBUTE)
-                ? attrs.get(PyCommon.PYTHON_VERSION_ATTRIBUTE, Type.STRING)
-                : attrs.get(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING);
-
-        // It should be a target value ("PY2" or "PY3"), and if not that should be caught by
-        // attribute validation. But just in case, we'll treat an invalid value as null (which means
-        // "use the hard-coded default version") rather than propagate an unchecked exception in
-        // this context. That way the user can at least get a clean error message instead of a
-        // crash.
-        PythonVersion version;
-        try {
-          version = PythonVersion.parseTargetValue(versionString);
-        } catch (IllegalArgumentException ex) {
-          version = null;
-        }
-        return new PythonVersionTransition(version);
-      };
+  /**
+   * A Python version transition that sets the version as specified by the target's attributes, with
+   * a default of {@link PythonVersion#DEFAULT_TARGET_VALUE}.
+   */
+  public static final RuleTransitionFactory VERSION_TRANSITION =
+      makeVersionTransition(PythonVersion.DEFAULT_TARGET_VALUE);
 }

src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java

@@ -67,27 +67,47 @@ public enum PythonVersion {
    *
    * <p><i>Deprecated meaning:</i> Indicates code that cannot be processed by 3to2.
    */
-  PY3ONLY;
+  PY3ONLY,
+
+  /**
+   * Internal sentinel value used as the default value of the {@code python_version} and {@code
+   * default_python_version} attributes.
+   *
+   * <p>This should not be referenced by the user. But since we can't actually hide it from Starlark
+   * ({@code native.existing_rules()}) or bazel query, we give it the scary "_internal" prefix
+   * instead.
+   *
+   * <p>The logical meaning of this value is the same as {@link #DEFAULT_TARGET_VALUE}.
+   */
+  _INTERNAL_SENTINEL;
 
   private static ImmutableList<String> convertToStrings(List<PythonVersion> values) {
     return values.stream()
         .map(Functions.toStringFunction())
         .collect(ImmutableList.toImmutableList());
   }
 
-  /** All enum values. */
-  public static final ImmutableList<PythonVersion> ALL_VALUES =
-      ImmutableList.of(PY2, PY3, PY2AND3, PY2ONLY, PY3ONLY);
-
-  /** String names of all enum values. */
-  public static final ImmutableList<String> ALL_STRINGS = convertToStrings(ALL_VALUES);
-
   /** Enum values representing a distinct Python version. */
   public static final ImmutableList<PythonVersion> TARGET_VALUES = ImmutableList.of(PY2, PY3);
 
   /** String names of enum values representing a distinct Python version. */
   public static final ImmutableList<String> TARGET_STRINGS = convertToStrings(TARGET_VALUES);
 
+  /** Target values plus the sentinel value. */
+  public static final ImmutableList<PythonVersion> TARGET_AND_SENTINEL_VALUES =
+      ImmutableList.of(PY2, PY3, _INTERNAL_SENTINEL);
+
+  /** String names of target values plus the sentinel value. */
+  public static final ImmutableList<String> TARGET_AND_SENTINEL_STRINGS =
+      convertToStrings(TARGET_AND_SENTINEL_VALUES);
+
+  /** All values not including the sentinel. */
+  public static final ImmutableList<PythonVersion> SRCS_VALUES =
+      ImmutableList.of(PY2, PY3, PY2AND3, PY2ONLY, PY3ONLY);
+
+  /** String names of all enum values not including the sentinel. */
+  public static final ImmutableList<String> SRCS_STRINGS = convertToStrings(SRCS_VALUES);
+
   /** Enum values that do not imply running a transpiler to convert between versions. */
   public static final ImmutableList<PythonVersion> NON_CONVERSION_VALUES =
       ImmutableList.of(PY2AND3, PY2ONLY, PY3ONLY);
@@ -120,12 +140,31 @@ public static PythonVersion parseTargetValue(String str) {
     return PythonVersion.valueOf(str);
   }
 
+  /**
+   * Converts the string to a target or sentinel {@code PythonVersion} value (case-sensitive).
+   *
+   * @throws IllegalArgumentException if the string is not "PY2", "PY3", or "_INTERNAL_SENTINEL".
+   */
+  public static PythonVersion parseTargetOrSentinelValue(String str) {
+    if (!TARGET_AND_SENTINEL_STRINGS.contains(str)) {
+      // Use the same error message as for parseTargetValue, because the user shouldn't be aware of
+      // the sentinel value.
+      throw new IllegalArgumentException(
+          String.format("'%s' is not a valid Python major version. Expected 'PY2' or 'PY3'.", str));
+    }
+    return PythonVersion.valueOf(str);
+  }
+
   /**
    * Converts the string to a sources {@code PythonVersion} value (case-sensitive).
    *
-   * @throws IllegalArgumentException if the string is not an enum name.
+   * @throws IllegalArgumentException if the string is not an enum name or is the sentinel value.
    */
   public static PythonVersion parseSrcsValue(String str) {
+    if (!SRCS_STRINGS.contains(str)) {
+      throw new IllegalArgumentException(
+          String.format("'%s' is not a valid Python srcs_version value.", str));
+    }
     return PythonVersion.valueOf(str);
   }
 }