Fix resolving aspect attribute with same name as underlying target attribute

mai93 · copybara-github · commit 93ad5890883b · 2023-07-17T09:31:37.000-07:00
Check `ConfiguredAttributeMapper` only for non-aspect explicit attributes to avoid selecting a rule attribute with same name but different type Fixes: #18901 PiperOrigin-RevId: 548714977 Change-Id: Iaa9148cf21ce28d9414840e7cd874ffaaf9d7bb4
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -379,7 +379,7 @@ private static <T> void resolveAttribute(
     } else if (attribute.isLateBound()) {
       attributeValue =
           type.cast(resolveLateBoundDefault(rule, attributeMap, attribute, ruleConfig));
-    } else if (attributeMap.has(attribute.getName())) {
+    } else if (dependencyKind.getOwningAspect() == null && attributeMap.has(attribute.getName())) {
       // This condition is false for aspect attributes that do not give rise to dependencies because
       // attributes that come from aspects do not appear in attributeMap (see the comment in the
       // case that handles implicit attributes).
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java
@@ -8606,6 +8606,39 @@ public void testAspectRunsTwiceWithDiffBaseAspectsDeps() throws Exception {
             "aspect a on @//test:t4 sees b_provider = aspect b cannot see c_provider");
   }
 
+  @Test
+  public void testAspectWithSameExplicitAttributeNameAsUnderlyingTarget() throws Exception {
+    scratch.file(
+        "test/defs.bzl",
+        "def _a_impl(target, ctx):",
+        "  value = 'x from aspect = {}, x from target = {}'.format(ctx.attr.x, ctx.rule.attr.x)",
+        "  return struct(aspect_result = value)",
+        "a = aspect(",
+        "  implementation = _a_impl,",
+        "  attrs = {",
+        "    'x': attr.string(default = 'xyz')",
+        "  },",
+        ")",
+        "",
+        "def _rule_impl(ctx):",
+        "  pass",
+        "r1 = rule(",
+        "  implementation = _rule_impl,",
+        "  attrs = {",
+        "    'x': attr.int(default = 4)",
+        "  },",
+        ")");
+    scratch.file("test/BUILD", "load('//test:defs.bzl', 'r1')", "r1(name = 't1')");
+
+    AnalysisResult analysisResult = update(ImmutableList.of("//test:defs.bzl%a"), "//test:t1");
+
+    ImmutableMap<AspectKey, ConfiguredAspect> configuredAspects = analysisResult.getAspectsMap();
+    ConfiguredAspect aspectA = getConfiguredAspect(configuredAspects, "a");
+    assertThat(aspectA).isNotNull();
+    String aspectAResult = (String) aspectA.get("aspect_result");
+    assertThat(aspectAResult).isEqualTo("x from aspect = xyz, x from target = 4");
+  }
+
   private ImmutableList<AspectKey> getAspectKeys(String targetLabel, String aspectLabel) {
     return skyframeExecutor.getEvaluator().getDoneValues().entrySet().stream()
         .filter(
