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
keith · Oct 21, 2023 · 755579d · 755579d
1 parent 042f411
commit 755579d
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 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
@@ -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
@@ -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 {