From fe644bee95c14d461e0d1e3cccaa8bbcd57bcd8d Mon Sep 17 00:00:00 2001
From: gregce <gregce@google.com>
Date: Mon, 1 Nov 2021 12:12:02 -0700
Subject: [PATCH] Fix cache leak when applying transitions when only a rule's
 attributes change.

Fixes https://github.com/bazelbuild/bazel/issues/13997

PiperOrigin-RevId: 406886706
---
 .../StarlarkRuleTransitionProvider.java       |  9 ++-
 .../StarlarkRuleTransitionProviderTest.java   | 66 +++++++++++++++++++
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
index 847592a7c02f4c..a45e846d4065cb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
@@ -25,7 +25,6 @@
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -71,13 +70,13 @@ public StarlarkDefinedConfigTransition getStarlarkDefinedConfigTransitionForTest
    */
   private static class CacheKey {
     private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;
-    private final Label ruleLabel;
+    private final Rule rule;
     private final int hashCode;
 
     CacheKey(StarlarkDefinedConfigTransition starlarkDefinedConfigTransition, Rule rule) {
       this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
-      this.ruleLabel = rule.getLabel();
-      this.hashCode = Objects.hash(starlarkDefinedConfigTransition, ruleLabel);
+      this.rule = rule;
+      this.hashCode = Objects.hash(starlarkDefinedConfigTransition, rule);
     }
 
     @Override
@@ -90,7 +89,7 @@ public boolean equals(Object other) {
       }
       return (this.starlarkDefinedConfigTransition.equals(
               ((CacheKey) other).starlarkDefinedConfigTransition)
-          && this.ruleLabel.equals(((CacheKey) other).ruleLabel));
+          && this.rule.equals(((CacheKey) other).rule));
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
index 5108994ade527b..53016c32cc4475 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -30,6 +30,9 @@
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import com.google.testing.junit.testparameterinjector.TestParameters;
 import java.util.List;
@@ -1692,4 +1695,67 @@ public void testNoPlatformChange() throws Exception {
                 .platforms)
         .containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_platform"));
   }
+
+  @Test
+  public void testTransitionsStillTriggerWhenOnlyRuleAttributesChange() throws Exception {
+    scratch.file(
+        "test/defs.bzl",
+        "def _transition_impl(settings, attr):",
+        "  return {",
+        "    '//command_line_option:foo': attr.my_attr,",
+        "  }",
+        "_my_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = [],",
+        "  outputs = [",
+        "    '//command_line_option:foo',",
+        "  ]",
+        ")",
+        "def _rule_impl(ctx):",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = _rule_impl,",
+        "  cfg = _my_transition,",
+        "  attrs = {",
+        "    'my_attr': attr.string(),",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  },",
+        ")");
+    writeAllowlistFile();
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'my_rule')",
+        "my_rule(",
+        "  name = 'buildme',",
+        "  my_attr = 'first build',",
+        ")");
+    assertThat(
+            getConfiguration(getConfiguredTarget("//test:buildme"))
+                .getOptions()
+                .get(DummyTestOptions.class)
+                .foo)
+        .isEqualTo("first build");
+
+    scratch.overwriteFile(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'my_rule')",
+        "my_rule(",
+        "  name = 'buildme',",
+        "  my_attr = 'second build',",
+        ")");
+    skyframeExecutor.invalidateFilesUnderPathForTesting(
+        reporter,
+        ModifiedFileSet.builder().modify(PathFragment.create("test/BUILD")).build(),
+        Root.fromPath(rootDirectory));
+
+    assertThat(
+            getConfiguration(getConfiguredTarget("//test:buildme"))
+                .getOptions()
+                .get(DummyTestOptions.class)
+                .foo)
+        .isEqualTo("second build");
+  }
 }