[9.1.0] Fix visibility for implicit deps of parent rules (https://github.com/bazelbuild/bazel/pull/28627)#28688
Conversation
The visibility of a default value of an implicit dep in an extended rule should be checked relative to the definition of the rule that introduced it, which may not be the child rule. Fixes bazelbuild#28618 Closes bazelbuild#28627. PiperOrigin-RevId: 871303947 Change-Id: I0027e277dc9f01396fa4674297d2a73e1e9d257e
There was a problem hiding this comment.
Code Review
This pull request aims to fix a visibility issue with implicit dependencies in extended Starlark rules. The core logic change to walk up the parent rule chain to find the original definer of an attribute is correct for handling inherited private attributes. However, this new logic is placed under a condition that will not trigger for private Starlark attributes (which start with _), making the fix ineffective for its intended purpose. I've left a critical comment explaining the issue on the new code block.
| // Rule extensions can't override private attributes, so we can just walk up the chain to | ||
| // find the rule class that actually introduced the attribute and thus its default. | ||
| var owningRule = rule.getRuleClassObject(); | ||
| RuleClass parent; | ||
| while ((parent = owningRule.getStarlarkParent()) != null | ||
| && parent.getAttributeProvider().getAttributeByNameMaybe(attrName) != null) { | ||
| owningRule = parent; | ||
| } | ||
| state.owningRule = owningRule; |
There was a problem hiding this comment.
This logic to find the original defining rule class is correct for handling inherited private attributes in Starlark. However, it is placed inside an if (isImplicitDep) block where the condition will not be met for private Starlark attributes.
The isImplicitDep check at line 189 is based on attribute.isImplicit(), which only returns true for attribute names starting with $. Private Starlark attributes start with _, so isImplicit() will be false for them. This means this new logic will not execute for the very case it is intended to fix, as demonstrated by the _tool attribute in the new test.
To fix this, this logic block should be moved, or the conditional at line 189 should be updated to also check for private Starlark attributes (e.g., attribute.getName().startsWith("_")). Since line 189 is not in the diff, I recommend moving this block to a new else if that correctly handles private Starlark attributes.
faaa1b4
The visibility of a default value of an implicit dep in an extended rule should be checked relative to the definition of the rule that introduced it, which may not be the child rule.
Fixes #28618
Closes #28627.
PiperOrigin-RevId: 871303947
Change-Id: I0027e277dc9f01396fa4674297d2a73e1e9d257e
Commit a16489f