commit c3550ed9110fd06b0d852a68fff33de1da3928f7
author Googler <yuyueny@google.com> Fri Apr 14 13:49:27 2023 -0700
committer Copybara-Service <copybara-worker@google.com> Fri Apr 14 13:50:57 2023 -0700
tree 498a40c24862b2b799132240a5b0c023fd7e47b4
parent f505e8398d880a86674f3ba0c627870076e6bd5b

Dirty Package node can still pool intern labels

This change fixes a remaining issue in `LabelPool#getOrWeakIntern`. By changing to get node from graph using `toValue()` method, we are promised to get `PackageValue` even if node is dirty or changed.

PiperOrigin-RevId: 524377579
Change-Id: I69859977751fe4c664828438c3645e21e67c6d9c

src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
index dff42c4..edd9852 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -312,13 +312,16 @@
       PackageValue.Key packageKey = PackageValue.key(packageIdentifier);
 
       InMemoryNodeEntry inMemoryNodeEntry = nodeMap.get(packageKey);
-      if (inMemoryNodeEntry == null || !inMemoryNodeEntry.isDone()) {
-        // If we cannot get the InMemoryNodeEntry, or it is not done, values are not available. So
-        // we can only weak intern sample.
+      if (inMemoryNodeEntry == null) {
+        // If we cannot get the InMemoryNodeEntry, just weak intern the sample.
         return interner.weakIntern(sample);
       }
 
       SkyValue value = inMemoryNodeEntry.toValue();
+      if (value == null) {
+        return interner.weakIntern(sample);
+      }
+
       checkState(value instanceof PackageValue, value);
       ImmutableSortedMap<String, Target> targets = ((PackageValue) value).getPackage().getTargets();
       Target target = targets.get(sample.getName());

src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
index 7ab3395..6716572 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
@@ -119,13 +119,39 @@
   }
 
   @Test
+  public void labelInterner_dirtyPackageStillPoolInternLabel() throws Exception {
+    write("hello/BUILD", "cc_binary(name = 'foo', srcs = ['foo.cc'])");
+    write("hello/foo.cc", "int main() {", "  return 0;", "}");
+    buildTarget("//hello:foo");
+
+    InMemoryGraph graph = skyframeExecutor().getEvaluator().getInMemoryGraph();
+    PackageValue.Key packageKey =
+        PackageValue.key(PackageIdentifier.createInMainRepo(/* name= */ "hello"));
+    NodeEntry nodeEntry = graph.get(/* requestor= */ null, Reason.OTHER, packageKey);
+    assertThat(nodeEntry).isNotNull();
+
+    ImmutableSet<Label> targetLabels =
+        ((PackageValue) nodeEntry.toValue())
+            .getPackage().getTargets().values().stream()
+                .map(TargetApi::getLabel)
+                .collect(toImmutableSet());
+
+    nodeEntry.markDirty(DirtyType.DIRTY);
+
+    // Expect `intern` a duplicate instance to return the canonical one stored in the pool.
+    targetLabels.forEach(
+        l ->
+            assertThat(Label.createUnvalidated(l.getPackageIdentifier(), l.getName()))
+                .isSameInstanceAs(l));
+  }
+
+  @Test
   public void labelInterner_removeDirtyPackageStillWeakInternItsLabels() throws Exception {
     write("hello/BUILD", "cc_binary(name = 'foo', srcs = ['foo.cc'])");
     write("hello/foo.cc", "int main() {", "  return 0;", "}");
     buildTarget("//hello:foo");
 
     InMemoryGraph graph = skyframeExecutor().getEvaluator().getInMemoryGraph();
-
     PackageValue.Key packageKey =
         PackageValue.key(PackageIdentifier.createInMainRepo(/* name= */ "hello"));
     NodeEntry nodeEntry = graph.get(/* requestor= */ null, Reason.OTHER, packageKey);