From 755579d07d7a7e232adfc86568a99f312c2d262d Mon Sep 17 00:00:00 2001
From: Googler <messa@google.com>
Date: Fri, 20 Oct 2023 17:25:09 -0700
Subject: [PATCH] During aspect evaluation only validate the aspect
 dependencies

Since there can be conflict between the main aspect attributes names and the names of the rule and the base aspects attributes, revalidating rule owned dependencies while evaluating an aspect on the rule can cause incorrect errors. At the same time, dependencies of base aspects as well as the rule itself are already checked when they are evaluated.

PiperOrigin-RevId: 575355459
Change-Id: I1813de4aca2227d3781c4ab8ff2bf4d05b55d051
---
 .../build/lib/analysis/RuleContext.java       | 21 +++++++++++--------
 .../build/lib/analysis/VisibilityTest.java    |  4 ----
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index acc3808795eb4a..dd8c19e8fb2da3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -1711,17 +1711,20 @@ private ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> createTar
           continue;
         }
 
-        if (attribute.isSilentRuleClassFilter()) {
-          Predicate<String> filter = attribute.getAllowedRuleClassPredicate();
-          for (ConfiguredTargetAndData configuredTarget : entry.getValue()) {
-            if (filter.apply(configuredTarget.getRuleClass())) {
+        Predicate<String> filter =
+            attribute.isSilentRuleClassFilter()
+                ? attribute.getAllowedRuleClassPredicate()
+                : Predicates.<String>alwaysTrue();
+
+        for (ConfiguredTargetAndData configuredTarget : entry.getValue()) {
+          if (filter.apply(configuredTarget.getRuleClass())) {
+            if (aspects.isEmpty()
+                || getMainAspect().getAspectClass().equals(entry.getKey().getOwningAspect())) {
+              // During aspects evaluation, only validate the dependencies of the main aspect.
+              // Dependencies of base aspects as well as the rule itself are checked when they
+              // are evaluated.
               validateDirectPrerequisite(attribute, configuredTarget);
-              mapBuilder.put(entry.getKey(), configuredTarget);
             }
-          }
-        } else {
-          for (ConfiguredTargetAndData configuredTarget : entry.getValue()) {
-            validateDirectPrerequisite(attribute, configuredTarget);
             mapBuilder.put(entry.getKey(), configuredTarget);
           }
         }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java
index c725d6bbaa54cf..7a688adeb1cea1 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java
@@ -18,7 +18,6 @@
 import static org.junit.Assert.assertThrows;
 
 import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -274,9 +273,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exc
     assertThat(hasErrors(getConfiguredTarget("//foo:target_with_aspects"))).isFalse();
   }
 
-  @Ignore(
-      "TODO(b/206127051): The aspects path implicit dependencies with same name are incorrectly"
-          + " merged, enable this test when this is fixed.")
   @Test
   public void testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision()
       throws Exception {