From 594e8588bcd0257c5a3c7e1dd8eae82ce28173b2 Mon Sep 17 00:00:00 2001
From: mstaib <mstaib@google.com>
Date: Wed, 13 Jun 2018 16:21:33 -0700
Subject: When composing RuleTransitionFactories, return something that
 respects equality.

If the output from composeTransitions is one of the input PatchTransitions, return it.
If it's a composed transition, make sure to wrap it in a class which respects equality.

Without this change, running the same RuleTransitionFactory on the same attribute and
target twice will produce two non-equal Dependencies which can't be deduplicated by
OrderedSetMultimap - no lambda is equal to any other instance of the same lambda.

RELNOTES: None.
PiperOrigin-RevId: 200472777
---
 .../config/ComposingRuleTransitionFactory.java     | 45 +++++++++++++++++-----
 1 file changed, 36 insertions(+), 9 deletions(-)

(limited to 'src/main/java/com')

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java
index 51dfa176c4..d0675bb504 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java
@@ -40,15 +40,42 @@ public class ComposingRuleTransitionFactory implements RuleTransitionFactory {
 
   @Override
   public PatchTransition buildTransitionFor(Rule rule) {
-    ConfigurationTransition composedTransition = TransitionResolver.composeTransitions(
+    ConfigurationTransition composed = TransitionResolver.composeTransitions(
         rtf1.buildTransitionFor(rule), rtf2.buildTransitionFor(rule));
-    // Even though we know the composed transition isn't a split (because neither of its children
-    // can be splits), the returned type is a generic ConfigurationTransition. So cast that back to
-    // a PatchTransition here.
-    //
-    // We could alternatively change RuleTransitionFactory's signature to a ConfigurationTransition.
-    // But it's nice to strongly enforce the interface expectation that rule transitions don't
-    // split.
-    return (BuildOptions options) -> Iterables.getOnlyElement(composedTransition.apply(options));
+    if (composed instanceof PatchTransition) {
+      // This is one of the two input transitions. Especially if it's a NoTransition or
+      // HostTransition, we should give it back so it can be specially identified as described
+      // in composeTransitions.
+      return (PatchTransition) composed;
+    } else {
+      // This is a composed transition, but we need a composed transition which is both a
+      // PatchTransition and can be registered as equal to another instance of the same composed
+      // transition.
+      return new AsPatchTransition(composed);
+    }
+  }
+
+  private static final class AsPatchTransition implements PatchTransition {
+    private final ConfigurationTransition wrapped;
+
+    private AsPatchTransition(ConfigurationTransition wrapped) {
+      this.wrapped = wrapped;
+    }
+
+    @Override
+    public BuildOptions patch(BuildOptions options) {
+      return Iterables.getOnlyElement(wrapped.apply(options));
+    }
+
+    @Override
+    public int hashCode() {
+      return wrapped.hashCode();
+    }
+
+    @Override
+    public boolean equals(Object other) {
+      return other instanceof AsPatchTransition
+          && this.wrapped.equals(((AsPatchTransition) other).wrapped);
+    }
   }
 }
-- 
cgit v1.2.3