Add .equals(), .hashCode() to EnableLipoTransition.

This fixes a subtle integration bug between PatchTransition, ConfiguredTargetFunction, and DependencyResolver.

Short story: ConfiguredTargetFunction.getDynamicConfigurations assumes at most one <Attribute, Label, Transition> entry per configured target dep. DependencyResolver.dependentNodeMap faithfully supplies this by returning an OrderedSetMultimap<Attribute, Dependency>, where each Dependency is a <Label, Transition>. OrderedSetMultimap guarantees no <key, value> repeats, but if you have two semantically equal EnableLipoTransition instances that don't satisfy .equals(), deduping fails.

Most PatchTransitions don't have this problem because they're singletons. But each EnableLipoTransition is instantiated with a Rule, so singletons don't work here.

This problem can only happen when a rule has multiple instances of the same C++ dep under the same attribute. But this isn't actually possible for normal attributes like "deps" because they don't allow duplicates (see RuleClass#checkForDuplicateLabels). Only non-LABEL_LIST attributes can trigger this.

PiperOrigin-RevId: 157733284

1 files changed, 24 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java
index 0fae3c07ea..33362b1dac 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java
@@ -21,16 +21,32 @@ import com.google.devtools.build.lib.packages.Attribute;
  * <p>The concept is simple: given the input configuration's build options, the
  * transition does whatever it wants to them and returns the modified result.
  *
- * <p>Implementations must be stateless: the transformation logic cannot use
- * any information from any data besides the input build options.
+ * <p>Implementations must be stateless: the output must exclusively depend on the
+ * input build options and any immutable member fields. Implementations must also override
+ * {@link Object#equals} and {@link Object#hashCode} unless exclusively accessed as
+ * singletons. For example:
  *
- * <p>For performance reasons, the input options are passed in as a <i>reference</i>,
- * not a <i>copy</i>. Transition implementations should <i>always</i> treat these
- * options as immutable, and call
+ * <pre>
+ * public class MyTransition implements PatchTransition {
+ *   public MyTransition INSTANCE = new MyTransition();
+ *
+ *   private MyTransition() {}
+ *
+ *   {@literal @}Override
+ *   public BuildOptions apply(BuildOptions options) {
+ *     BuildOptions toOptions = options.clone();
+ *     // Change some setting on toOptions
+ *     return toOptions;
+ *   }
+ * }
+ * </pre>
+ *
+ * <p>For performance reasons, the input options are passed as a <i>reference</i>, not a
+ * <i>copy</i>. Implementations should <i>always</i> treat these as immutable, and call
  * {@link com.google.devtools.build.lib.analysis.config.BuildOptions#clone}
- * before applying any mutations. Unfortunately,
- * {@link com.google.devtools.build.lib.analysis.config.BuildOptions} does not currently
- * enforce immutability, so care must be taken not to modify the wrong instance.
+ * before making changes. Unfortunately,
+ * {@link com.google.devtools.build.lib.analysis.config.BuildOptions} doesn't currently
+ * enforce immutability. So care must be taken not to modify the wrong instance.
  */
 public interface PatchTransition extends Attribute.Transition {