'required_providers' for an aspect does not always work together with the 'aspects' requirement on rule attributes

[martis42]

Description of the bug:

I want to create an aspect which is only meaningful for C++ targets. To ensure my aspect is only invoked on such targets, I added required_providers = [CcInfo],.

This works as expected when invoke the aspect through the CLI.
However, it fails if the aspect is invoked by a rule on a cc_binary target.
Given the aspect documentation and considering that cc_binary does provide CcInfo, this seems like a bug to me.

Which category does this issue belong to?

C++/Objective-C Rules, Core

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Given below example

• bazel build --aspects=//:aspect.bzl%my_aspect //:lib succeeds
• bazel build --aspects=//:aspect.bzl%my_aspect //:main succeeds
• bazel build //:my_aspect_rule_lib succeeds and yields as expected

  DEBUG: <some_path>/aspect.bzl:4:10: <target //:lib, keys:[CcInfo, InstrumentedFilesInfo, 
  OutputGroupInfo]>
  DEBUG: <some_path>/aspect.bzl:21:14: 1
  

• bazel build //:my_aspect_rule_main fails with

  ERROR: <some_path>/BUILD:18:15: in my_aspect_rule rule //:my_aspect_rule_main: 
  Traceback (most recent call last):
  	File "<some_path>/aspect.bzl", line 21, column 18, in _my_aspect_rule_impl
  		print(dep[FileCountInfo].count)
  Error: <target //:main> (rule 'cc_binary') doesn't contain declared provider 'FileCountInfo'
  

---

BUILD file

load("//:aspect.bzl", "my_aspect_rule")

cc_library(
    name = "lib",
    srcs = ["lib.h"],  # empty file, content irrelevant
)

cc_binary(
    name = "main",
    srcs = ["main.cpp"],  # simple main returning 0
)

my_aspect_rule(
    name = "my_aspect_rule_lib",
    deps = [":lib"],
)

my_aspect_rule(
    name = "my_aspect_rule_main",
    deps = [":main"],
)

aspect.bzl

FileCountInfo = provider(fields = {"count": "number of files"})

def _my_aspect_impl(target, ctx):
    print(target)

    count = 0
    for src in ctx.rule.attr.srcs:
        for f in src.files.to_list():
            count = count + 1

    return [FileCountInfo(count = count)]

my_aspect = aspect(
    implementation = _my_aspect_impl,
    required_providers = [CcInfo],
    attr_aspects = [],
)

def _my_aspect_rule_impl(ctx):
    for dep in ctx.attr.deps:
        print(dep[FileCountInfo].count)

my_aspect_rule = rule(
    implementation = _my_aspect_rule_impl,
    attrs = {"deps": attr.label_list(aspects = [my_aspect])},
)

Which operating system are you running Bazel on?

Linux Mint 21.1 Cinnamon

What is the output of bazel info release?

release 6.3.2

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

As far as I can tell this is no regression. Tested this with various Bazel versions in the range 5.1.0 to latest 7.0.0-pre.20230917.3 and all show the same behavior.

However, with Bazel 5.0.0 bazel build //:my_aspect_rule_lib fails as well (invoking the aspect via CLI remains working). This indicates between 5.0.0 and 5.1.0 something was changed/fixed, but cc_binary was forgotten.

Have you found anything relevant by searching the web?

No

Any other information, logs, or outputs that you want to share?

There is a workaround to this.
One can add the following to the beginning of the aspect implementation to achieve the desired behavior.

if not CcInfo in target:
   return []

[comius]

cc @mai93, can you triage?

[mai93]

This happened because cc_library declares CcInfo in its provides list: https://github.com/bazelbuild/bazel/blob/master/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl#L618 while cc_binary does not: https://github.com/bazelbuild/bazel/blob/master/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl#L914

and the aspect's required_providers checks the providers list advertised by the rule via provides. However, from the discussion in #18721 it was decided that cc_binary and cc_test should not advertise CcInfo.

[stagnation]

Interesting background, thanks! This use-case and "Add unique provider for shell rules "#18331 shows that we have a different need for providers too, a way to filter what target to analyze aspects for. Which does not make a large distinction between library targets, which you can typically depend on through the provider, and binary and test targets. And the aspect needs to analyze binaries and tests just as well.

So we would like to say "this is c, or shell or python code", and have some aspects that only operate on that language's main rule set. Most aspects just need that, which is a glorified kind filter. And then FFI uses are typically filtered away, clang-tidy does not care for rust rules that can be compiled into a c binary. But an aspect that looks for symbol collisions in a binary's transitive closure would follow the larger "can be compiled together" semantics and happily analyze rust libraries too. But in neither case would we want to analyze python rules, for instance, which does not have CcInfo.

To @martis42: I hope you agree as I wouldn't want to hijack your issue. Thanks for the DWYU code, I have seen similar internal code and thinks it is a great idea, combined with IWYU it really improves the code quality.e

[comius]

cc @oquenchil

There's some disagreement if cc_binary should provide/declare CcInfo or not. I'm advocating that cc_binary should both provide and declare it. At least in the short term. That is because it can compile sources and the aspects need to extract this data the same way as on a cc_library.

The other argument is saying that we shouldn't return CcInfo from cc_binary.

In the long run, we should probably split it - provide 2 things: CcCompileInfo (for data extraction) and CcLibraryInfo (for linking the library into the cc_binary or cc_test). We're not there yet, and we need a migration for this. Probably simplest migration is to say CcInfo is used for data extraction and a new provider is used to define what cc_binary can depend on.

Those are my reasons. Let's not block the users for a long time. Let's allow the users to do what they need now and migrate and clean-up C++ rules behind the stage, ideally without users even noticing it.

[brentleyjones]

In the long run, we should probably split it - provide 2 things: CcCompileInfo (for data extraction) and CcLibraryInfo (for linking the library into the cc_binary or cc_test).

❤️

[martis42]

@mai93
Thanks for your clarification. It seems this is not a bug but right now the desired behavior. However, putting aside if cc_binary will advertise CcInfo or not, I believe the current state is bad.

That it makes a difference if one invokes an aspect via CLI on a target or via indirection through a rule is surprising and inconsistent. If cc_binary is not supposed to advertise CcInfo, then the aspect should also skip cc_binary targets when being invoked via CLI on a cc_binary

[martis42]

@stagnation
Thanks for your kind words regarding DWYU :)

You summarized the underlying problem of my issue very well. I want to create an aspect which runs on all cc_ targets without caring about them being a binary or library.

[mai93]

That it makes a difference if one invokes an aspect via CLI on a target or via indirection through a rule is surprising and inconsistent. If cc_binary is not supposed to advertise CcInfo, then the aspect should also skip cc_binary targets when being invoked via CLI on a cc_binary

That's correct, but fixing this in command line aspects turned out to be an incompatible change so it has been guarded by incompatible_top_level_aspects_require_providers

[buildbreaker2021]

I am still not fully convinced that we need to do this, mostly because of #18721

What are the downsides of the workaround you have mentioned?

What about using ctx.rule.kind? AFAIU you are using CcInfo to identify if the rule is cc_binary or cc_test right?

[comius]

What are the downsides of the workaround you have mentioned?

The downside of workaround is that it propagates the aspect over everything, not just rules that have CcInfo. This uses additional memory and time. The numbers are usually not small in big repositories.

[martis42]

What are the downsides of the workaround you have mentioned?

While the workaround implements the desired behavior, it is still unexpected that the workaround is required. The documentation does not make it clear that required_providers of an aspect is solely working for providers advertised by provides of a rule.

Besides that, there are performance penalties mentioned by @comius.

What about using ctx.rule.kind? AFAIU you are using CcInfo to identify if the rule is cc_binary or cc_test right?

While I am currently limiting the aspect in DWYU to some specific cc_* rules, this is supposed to be an intermediate step. In the end I want to generally work on all all rules providing CcInfo. As long as a rule manages C/C++ source code, provides CcInfo and has (implementation_)deps I can apply my tools logic. This could also work on any third_party rule for C++ besides the official Bazel cc_* rules.

[fmeum]

@bazel-io flag

[iancha1992]

@bazel-io fork 7.0.0

[iancha1992]

@fmeum @mai93 @comius I've tried cherry-picking this to release-7.0.0, but looks like there's a conflict since the make_cc_binary(cc_binary_attrs, **kwargs) function is missing from src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl in the first place from the branch.

cc: @bazelbuild/triage

[mai93]

I think that's because c59739e is not cherry-picked in the release-7.0.0 branch, @comius should it be cherry-picked?

[iancha1992]

@mai93 Thanks. Looks like I'm able to cherry-pick now.
@comius #19986 includes both c59739e and b1d3441. Let us know if c59739e shouldn't be included. Thanks

[comius]

Yes, please cherry-pick c59739e. It's a rollback, I'll need to implement it in a different way.

[iancha1992]

A fix for this issue has been included in Bazel 7.0.0 RC5. Please test out the release candidate and report any issues as soon as possible. Thanks!

[martis42]

@iancha1992 Thanks for the ping. Can confirm that my issue is resolved with Bazel 7.0.0 RC5