Add a python_version attribute to replace default_python_version

The new attribute is guarded by --experimental_better_python_version_mixing, which gets checked in the analysis phase.

When both the new attribute and old one are present, the new one takes precedence. The migration procedure is to just rename `default_python_version` to `python_version`.

Minor PyCommon cleanup: d1fb6dd made initCommon() redundant, so this CL merges it into PyCommon's constructor and eliminates some unneeded attr-reading helpers.

Work toward #6583.

RELNOTES: None
PiperOrigin-RevId: 226427679

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.DEFAULT_PYTHON_VERSION_TRANSITION)
+        .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION)
         .add(
             attr("$zipper", LABEL)
                 .cfg(HostTransition.INSTANCE)

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

@@ -164,14 +164,28 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
           all of its <code>deps</code>.
           Valid values are <code>"PY2"</code> (default) or <code>"PY3"</code>.
           Python 3 support is experimental.
+          <p>If both this attribute and <code>python_version</code> are supplied,
+          <code>default_python_version</code> will be ignored.
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .add(
-              attr("default_python_version", STRING)
+              attr(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, STRING)
                   .value(PythonVersion.DEFAULT_TARGET_VALUE.toString())
                   .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS))
                   .nonconfigurable(
-                      "read by PythonUtils.getNewPythonVersion, which doesn't have access"
-                          + " to configuration keys"))
+                      "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
+                          + " to the configuration"))
+          /* <!-- #BLAZE_RULE($base_py_binary).ATTRIBUTE(python_version) -->
+          An experimental replacement for <code>default_python_version</code>. Only available when
+          <code>--experimental_better_python_version_mixing</code> is enabled. If both this and
+          <code>default_python_version</code> are supplied, the latter will be ignored.
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+          .add(
+              attr(PyCommon.PYTHON_VERSION_ATTRIBUTE, STRING)
+                  .value(PythonVersion.DEFAULT_TARGET_VALUE.toString())
+                  .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS))
+                  .nonconfigurable(
+                      "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
+                          + " to the configuration"))
           /* <!-- #BLAZE_RULE($base_py_binary).ATTRIBUTE(srcs) -->
           The list of source files that are processed to create the target.
           This includes all your checked-in code and any

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.DEFAULT_PYTHON_VERSION_TRANSITION)
+        .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION)
         .add(
             attr("$zipper", LABEL)
                 .cfg(HostTransition.INSTANCE)

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

@@ -44,7 +44,6 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory {
   public ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException, ActionConflictException {
     PyCommon common = new PyCommon(ruleContext);
-    common.initCommon(common.getDefaultPythonVersion());
 
     RuleConfiguredTargetBuilder builder = init(ruleContext, createSemantics(), common);
     if (builder == null) {

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

@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.StructImpl;
@@ -70,6 +71,9 @@ public final class PyCommon {
   public static final String IS_USING_SHARED_LIBRARY = "uses_shared_libraries";
   public static final String IMPORTS = "imports";
 
+  public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version";
+  public static final String PYTHON_VERSION_ATTRIBUTE = "python_version";
+
   private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() {
     @Override
     public void collectMetadataArtifacts(Iterable<Artifact> artifacts,
@@ -82,23 +86,21 @@ public void collectMetadataArtifacts(Iterable<Artifact> artifacts,
 
   private Artifact executable = null;
 
-  private NestedSet<Artifact> transitivePythonSources;
+  private final NestedSet<Artifact> transitivePythonSources;
 
-  private PythonVersion sourcesVersion;
-  private PythonVersion version = null;
+  private final PythonVersion sourcesVersion;
+  private final PythonVersion version;
   private Map<PathFragment, Artifact> convertedFiles;
 
   private NestedSet<Artifact> filesToBuild = null;
 
   public PyCommon(RuleContext ruleContext) {
     this.ruleContext = ruleContext;
-  }
-
-  public void initCommon(PythonVersion defaultVersion) {
     this.sourcesVersion = getSrcsVersionAttr(ruleContext);
     this.version = ruleContext.getFragment(PythonConfiguration.class).getPythonVersion();
     this.transitivePythonSources = collectTransitivePythonSources();
     checkSourceIsCompatible(this.version, this.sourcesVersion, ruleContext.getLabel());
+    validatePythonVersionAttr(ruleContext);
   }
 
   public PythonVersion getVersion() {
@@ -189,16 +191,6 @@ public static StructImpl createSourceProvider(
         "No such attribute '%s'");
   }
 
-  /**
-   * Returns the parsed value of the "default_python_version" attribute, or null if it is not
-   * defined for this rule class.
-   */
-  public PythonVersion getDefaultPythonVersion() {
-    return ruleContext.getRule().isAttrDefined("default_python_version", Type.STRING)
-        ? getPythonVersionAttr(ruleContext)
-        : null;
-  }
-
   /** Returns the parsed value of the "srcs_version" attribute. */
   private static PythonVersion getSrcsVersionAttr(RuleContext ruleContext) {
     String attrValue = ruleContext.attributes().get("srcs_version", Type.STRING);
@@ -215,19 +207,23 @@ private static PythonVersion getSrcsVersionAttr(RuleContext ruleContext) {
     }
   }
 
-  /** Returns the parsed value of the "default_python_version" attribute. */
-  private static PythonVersion getPythonVersionAttr(RuleContext ruleContext) {
-    String attrValue = ruleContext.attributes().get("default_python_version", Type.STRING);
-    try {
-      return PythonVersion.parseTargetValue(attrValue);
-    } catch (IllegalArgumentException ex) {
-      // Should already have been disallowed in the rule.
+  /**
+   * If the {@code python_version} attribute is defined for {@code ruleContext}, this method reports
+   * an attribute error if the attribute is set explicitly without the new API being enabled (via
+   * {@code --experimental_better_python_version_mixing}).
+   */
+  private static void validatePythonVersionAttr(RuleContext ruleContext) {
+    AttributeMap attrs = ruleContext.attributes();
+    if (!attrs.has(PYTHON_VERSION_ATTRIBUTE, Type.STRING)) {
+      return;
+    }
+    boolean newApiEnabled =
+        ruleContext.getFragment(PythonConfiguration.class).newPyVersionApiEnabled();
+    if (attrs.isAttributeValueExplicitlySpecified(PYTHON_VERSION_ATTRIBUTE) && !newApiEnabled) {
       ruleContext.attributeError(
-          "default_python_version",
-          String.format(
-              "'%s' is not a valid value. Expected one of: %s",
-              attrValue, Joiner.on(", ").join(PythonVersion.TARGET_STRINGS)));
-      return PythonVersion.DEFAULT_TARGET_VALUE;
+          PYTHON_VERSION_ATTRIBUTE,
+          "using the 'python_version' attribute requires the "
+              + "'--experimental_better_python_version_mixing' flag");
     }
   }
 

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

@@ -44,7 +44,6 @@ public ConfiguredTarget create(final RuleContext ruleContext)
       throws InterruptedException, RuleErrorException, ActionConflictException {
     PythonSemantics semantics = createSemantics();
     PyCommon common = new PyCommon(ruleContext);
-    common.initCommon(common.getDefaultPythonVersion());
     common.validatePackageName();
     semantics.validate(ruleContext, common);
 

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

@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.python;
 
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleTransitionFactory;
@@ -25,27 +27,42 @@ public class PyRuleClasses {
   public static final FileType PYTHON_SOURCE = FileType.of(".py", ".py3");
 
   /**
-   * Input for {@link RuleClass.Builder#cfg(RuleTransitionFactory)}: if {@link
-   * PythonOptions#forcePython} is unset, sets the Python version according to the rule's default
-   * Python version. Assumes the rule has the expected attribute for this setting.
+   * A rule transition factory for Python binary rules and other rules that may change the Python
+   * version.
    *
-   * <p>Since this is a configuration transition, this propagates to the rules' transitive deps.
+   * <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 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.
    */
-  public static final RuleTransitionFactory DEFAULT_PYTHON_VERSION_TRANSITION =
+  public static final RuleTransitionFactory PYTHON_VERSION_TRANSITION =
       (rule) -> {
-        String attrDefault = RawAttributeMapper.of(rule).get("default_python_version", Type.STRING);
+        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'");
+
+        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.
-        PythonVersion version = null;
-        // Should be non-null because this transition shouldn't be used on rules without the attr.
-        if (attrDefault != null) {
-          try {
-            version = PythonVersion.parseTargetValue(attrDefault);
-          } catch (IllegalArgumentException ex) {
-            // Parsing error.
-          }
+        // 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);
       };

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

@@ -34,7 +34,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException, ActionConflictException {
     PythonSemantics semantics = createSemantics();
     PyCommon common = new PyCommon(ruleContext);
-    common.initCommon(common.getDefaultPythonVersion());
 
     RuleConfiguredTargetBuilder builder = PyBinary.init(ruleContext, semantics, common);
     if (builder == null) {

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

@@ -41,14 +41,18 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
   private final PythonVersion version;
   private final TriState buildPythonZip;
   private final boolean buildTransitiveRunfilesTrees;
+  // TODO(brandjon): Remove once migration to the new API is complete (#6583).
+  private final boolean newPyVersionApiEnabled;
 
   PythonConfiguration(
       PythonVersion defaultPythonVersion,
       TriState buildPythonZip,
-      boolean buildTransitiveRunfilesTrees) {
+      boolean buildTransitiveRunfilesTrees,
+      boolean newPyVersionApiEnabled) {
     this.version = defaultPythonVersion;
     this.buildPythonZip = buildPythonZip;
     this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees;
+    this.newPyVersionApiEnabled = newPyVersionApiEnabled;
   }
 
   /**
@@ -113,4 +117,12 @@ public boolean buildPythonZip() {
   public boolean buildTransitiveRunfilesTrees() {
     return buildTransitiveRunfilesTrees;
   }
+
+  /**
+   * Returns whether use of {@code --python_version} flag and {@code python_version} attribute is
+   * allowed.
+   */
+  public boolean newPyVersionApiEnabled() {
+    return newPyVersionApiEnabled;
+  }
 }

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

@@ -37,7 +37,8 @@ public PythonConfiguration create(BuildOptions buildOptions)
     return new PythonConfiguration(
         pythonVersion,
         pythonOptions.buildPythonZip,
-        pythonOptions.buildTransitiveRunfilesTrees);
+        pythonOptions.buildTransitiveRunfilesTrees,
+        /*newPyVersionApiEnabled=*/ pythonOptions.experimentalBetterPythonVersionMixing);
   }
 
   @Override