From a05d5b65e41e37ace31b3b66fd030e64e21efbb4 Mon Sep 17 00:00:00 2001
From: John Cater <jcater@google.com>
Date: Mon, 1 Apr 2019 12:36:34 -0700
Subject: [PATCH] Replace ComposingTransition.decomposeTransitions with the
 visitor pattern.

Part of the work on #7814.

Closes #7912.

PiperOrigin-RevId: 241378193
---
 .../transitions/ComposingTransition.java      |  33 +----
 .../transitions/ConfigurationTransition.java  |  11 ++
 .../analysis/skylark/StarlarkTransition.java  | 121 ++++++++++--------
 3 files changed, 87 insertions(+), 78 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
index 76d77aec2e51a2..13ccbf52bfecfe 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
@@ -18,7 +18,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
 
@@ -67,6 +66,13 @@ public String getName() {
     return "(" + transition1.getName() + " + " + transition2.getName() + ")";
   }
 
+  // Override to allow recursive visiting.
+  @Override
+  public <E extends Exception> void visit(Visitor<E> visitor) throws E {
+    this.transition1.visit(visitor);
+    this.transition2.visit(visitor);
+  }
+
   @Override
   public int hashCode() {
     return Objects.hash(transition1, transition2);
@@ -118,29 +124,4 @@ public static ConfigurationTransition of(
   private static boolean isFinal(ConfigurationTransition transition) {
     return transition == NullTransition.INSTANCE || transition.isHostTransition();
   }
-
-  /**
-   * Recursively decompose a composing transition into all the {@link ConfigurationTransition}
-   * instances that it holds.
-   *
-   * @param root {@link ComposingTransition} to decompose
-   */
-  public static ImmutableList<ConfigurationTransition> decomposeTransition(
-      ConfigurationTransition root) {
-    ArrayList<ConfigurationTransition> toBeInspected = new ArrayList<>();
-    ImmutableList.Builder<ConfigurationTransition> transitions = new ImmutableList.Builder<>();
-    toBeInspected.add(root);
-    ConfigurationTransition current;
-    while (!toBeInspected.isEmpty()) {
-      current = toBeInspected.remove(0);
-      if (current instanceof ComposingTransition) {
-        ComposingTransition composingCurrent = (ComposingTransition) current;
-        toBeInspected.addAll(
-            ImmutableList.of(composingCurrent.transition1, composingCurrent.transition2));
-      } else {
-        transitions.add(current);
-      }
-    }
-    return transitions.build();
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
index 3ae4d98e7aaaa3..0437ba36dc138c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
@@ -52,4 +52,15 @@ default boolean isHostTransition() {
   default String getName() {
     return this.getClass().getSimpleName();
   }
+
+  /** Allows the given {@link Visitor} to inspect this transition. */
+  default <E extends Exception> void visit(Visitor<E> visitor) throws E {
+    visitor.accept(this);
+  }
+
+  /** Helper object that can be used to inspect {@link ConfigurationTransition} instances. */
+  @FunctionalInterface
+  interface Visitor<E extends Exception> {
+    void accept(ConfigurationTransition transition) throws E;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java
index 29e7675103abc7..46974c0fbc2a0d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java
@@ -88,12 +88,16 @@ public String getMessage() {
   // TODO(juliexxia): handle the possibility of aliased build settings.
   public static ImmutableSet<SkyKey> getBuildSettingPackageKeys(ConfigurationTransition root) {
     ImmutableSet.Builder<SkyKey> keyBuilder = new ImmutableSet.Builder<>();
-    for (ConfigurationTransition transition : ComposingTransition.decomposeTransition(root)) {
-      if (transition instanceof StarlarkTransition) {
-        for (Label setting : getChangedStarlarkSettings((StarlarkTransition) transition)) {
-          keyBuilder.add(PackageValue.key(setting.getPackageIdentifier()));
-        }
-      }
+    try {
+      root.visit(
+          (StarlarkTransitionVisitor)
+              transition -> {
+                for (Label setting : getChangedStarlarkSettings(transition)) {
+                  keyBuilder.add(PackageValue.key(setting.getPackageIdentifier()));
+                }
+              });
+    } catch (TransitionException e) {
+      // Not actually thrown in the visitor, but declared.
     }
     return keyBuilder.build();
   }
@@ -129,42 +133,41 @@ public static void validate(
       throws TransitionException {
     replayEvents(eventHandler, root);
 
-    List<ConfigurationTransition> transitions = ComposingTransition.decomposeTransition(root);
     // collect settings changed during this transition and their types
     Map<Label, Type<?>> changedSettingToType = Maps.newHashMap();
-    for (ConfigurationTransition transition : transitions) {
-      if (!(transition instanceof StarlarkTransition)) {
-        continue;
-      }
-      StarlarkTransition starlarkTransition = (StarlarkTransition) transition;
-      List<Label> changedSettings = getChangedStarlarkSettings(starlarkTransition);
-      for (Label setting : changedSettings) {
-        Package buildSettingPackage =
-            ((PackageValue)
-                    buildSettingPackages.get(PackageValue.key(setting.getPackageIdentifier())))
-                .getPackage();
-        Target buildSettingTarget;
-        try {
-          buildSettingTarget = buildSettingPackage.getTarget(setting.getName());
-        } catch (NoSuchTargetException e) {
-          throw new TransitionException(e);
-        }
-        if (buildSettingTarget.getAssociatedRule() == null
-            || buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting()
-                == null) {
-          throw new TransitionException(
-              String.format(
-                  "attempting to transition on '%s' which is not a build setting", setting));
-        }
-        changedSettingToType.put(
-            setting,
-            buildSettingTarget
-                .getAssociatedRule()
-                .getRuleClassObject()
-                .getBuildSetting()
-                .getType());
-      }
-    }
+    root.visit(
+        (StarlarkTransitionVisitor)
+            transition -> {
+              List<Label> changedSettings = getChangedStarlarkSettings(transition);
+              for (Label setting : changedSettings) {
+                Package buildSettingPackage =
+                    ((PackageValue)
+                            buildSettingPackages.get(
+                                PackageValue.key(setting.getPackageIdentifier())))
+                        .getPackage();
+                Target buildSettingTarget;
+                try {
+                  buildSettingTarget = buildSettingPackage.getTarget(setting.getName());
+                } catch (NoSuchTargetException e) {
+                  throw new TransitionException(e);
+                }
+                if (buildSettingTarget.getAssociatedRule() == null
+                    || buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting()
+                        == null) {
+                  throw new TransitionException(
+                      String.format(
+                          "attempting to transition on '%s' which is not a build setting",
+                          setting));
+                }
+                changedSettingToType.put(
+                    setting,
+                    buildSettingTarget
+                        .getAssociatedRule()
+                        .getRuleClassObject()
+                        .getBuildSetting()
+                        .getType());
+              }
+            });
 
     // verify changed settings were changed to something reasonable for their type
     for (BuildOptions options : toOptions) {
@@ -193,18 +196,17 @@ private static List<Label> getChangedStarlarkSettings(StarlarkTransition transit
    */
   private static void replayEvents(ExtendedEventHandler eventHandler, ConfigurationTransition root)
       throws TransitionException {
-    List<ConfigurationTransition> transitions = ComposingTransition.decomposeTransition(root);
-    for (ConfigurationTransition transition : transitions) {
-      if (transition instanceof StarlarkTransition) {
-        StarlarkTransition starlarkTransition = (StarlarkTransition) transition;
-        // Replay events and errors and throw if there were errors
-        boolean hasErrors = starlarkTransition.hasErrors();
-        starlarkTransition.replayOn(eventHandler);
-        if (hasErrors) {
-          throw new TransitionException("Errors encountered while applying Starlark transition");
-        }
-      }
-    }
+    root.visit(
+        (StarlarkTransitionVisitor)
+            transition -> {
+              // Replay events and errors and throw if there were errors
+              boolean hasErrors = transition.hasErrors();
+              transition.replayOn(eventHandler);
+              if (hasErrors) {
+                throw new TransitionException(
+                    "Errors encountered while applying Starlark transition");
+              }
+            });
   }
 
   @Override
@@ -224,4 +226,19 @@ public boolean equals(Object object) {
   public int hashCode() {
     return Objects.hashCode(starlarkDefinedConfigTransition);
   }
+
+  @FunctionalInterface
+  // This is only used in this class to handle the cast and the exception
+  @SuppressWarnings("FunctionalInterfaceMethodChanged")
+  private interface StarlarkTransitionVisitor
+      extends ConfigurationTransition.Visitor<TransitionException> {
+    @Override
+    default void accept(ConfigurationTransition transition) throws TransitionException {
+      if (transition instanceof StarlarkTransition) {
+        this.accept((StarlarkTransition) transition);
+      }
+    }
+
+    void accept(StarlarkTransition transition) throws TransitionException;
+  }
 }