@mai93 I dug deeper and found 399beac. This change doesn't seem to change any existing Starlark aspect-on-aspect logic that wasn't already broken due to type mismatches, only native aspects should be able to observe a difference in precedence.

another case that will have a different behaviour with this change is for aspect propagating to its underlying aspect dependency:

## defs.bzl
prov_a = provider()
prov_b = provider()

def _aspect_a_impl(target, ctx):
  print('aspect_a on target {}'.format(target.label))
  if prov_b in target:
    print('\tcan see: ', target[prov_b].val)
  if hasattr(ctx.rule.attr, '_tool') and ctx.rule.attr._tool[prov_a]:
    print('\tcan see: ', ctx.rule.attr._tool[prov_a].val)
  if hasattr(ctx.rule.attr, 'dep') and ctx.rule.attr.dep and ctx.rule.attr.dep[prov_a]:
    print('\tcan see: ', ctx.rule.attr.dep[prov_a].val)
  return [prov_a(val='aspect_a on target {}'.format(target.label))]

aspect_a = aspect(
    implementation = _aspect_a_impl,
    attr_aspects = ['dep', '_tool'],
    required_aspect_providers = [prov_b],
    attrs = {
     '_tool' : attr.label(default = "//:tool_a"),
   },
)

def _aspect_b_impl(target, ctx):
  print('aspect_b on target {}'.format(target.label))
  return [prov_b(val='aspect_b on target {}'.format(target.label))]

aspect_b = aspect(
    implementation = _aspect_b_impl,
    attr_aspects = ['dep', '_tool'],
    attrs = {
     '_tool' : attr.label(default = "//:tool_b"),
    },
    provides = [prov_b],
)

def _rule_impl(ctx):
  pass

r1 = rule(
   implementation = _rule_impl,
   attrs = {
     'dep' : attr.label(aspects=[aspect_b, aspect_a]),
   },
)

r2 = rule(
   implementation = _rule_impl,
   attrs = {
     'dep' : attr.label(),
   },
)

## BUILD
load('defs.bzl', 'r1', 'r2')
r1(
  name = 't1',
  dep = ':t2',
)
r2(
  name = 't2',
  dep = ':t3',
)

r2(
  name = 't3',
)

sh_binary(name = "tool_a", srcs = ["tool.sh"])
sh_binary(name = "tool_b", srcs = ["tool.sh"])

currently when building t1, aspect_a will be applied to _tool_b and it can inspect the result of its application but with this change the value of ctx.rule.attr._tool in aspect_a will be //:tool_a instead of //:tool_b . So a proper separation between the aspects attributes is needed, if this is not blocking #19442 I think we can submit it and flip the incompatible_visibility_private_attributes_at_definition flag, I plan to work on the separation of the aspects implicit dependency.

Thanks for the interesting example! If I understand this correctly, does it mean that ctx.rule contains attributes of underlying aspects in addition to those of the underlying rule? I wasn't aware of that being possible.

@mai93 #19442 does require the current PR. Would you like to block it on implementing proper separation or do you see that as an independent effort? I am fine with both, it's just that our window to flip the flag will close for a somewhat long time when we don't get it into 7.0.0. But the flag is also not blocking anything else, so we are in no particular hurry to flip it.

does it mean that ctx.rule contains attributes of underlying aspects in addition to those of the underlying rule? I wasn't aware of that being possible.

yes, the attributes of the rule and the underlying aspects are grouped under ctx.rule.attr (code).

#19442 does require the current PR

I thought the current PR is only needed for testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision in #19442, if that's the case we can merge #19442 and mark testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision ignored until the problem with same name attributes is fixed.

@mai93 Is this still relevant with your recent work?

I think we can close this, the test testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision is no longer ignored after 755579d