cquery: more precise results

[gregestren]

Description of the feature request:

cquery only implicitly shows deps from aspects and toolchain resolution.

For example, assume //tt:parent depends on //tt:child and that dependency adds "my_aspect" which has an implicit dep on //tt:aspect_dep. Querying this produces:

$ bazel cquery 'somepath(//tt:parent, //tt:aspect_dep)'
//tt:parent (e022623)
//tt:aspect_dep (e022623)

This result includes my_aspect's dep, but misleadingly makes it look like //tt:parent's direct dep.

With the proposed change you'd get:

$ bazel cquery 'somepath(//tt:parent, //tt:aspect_dep)'
//tt:parent (e022623)
//tt:defs.bzl%_my_aspect of //tt:child (e022623)
//tt:aspect_dep (e022623)

Toolchains are a similar story.

---

The current behavior is no accident. We explicitly added the current implicit support to at least guarantee cquery doesn't under-eport dependencies. The problem with the full solution is it's a much more invasive change into the Java code that powers cquery and aquery.

I prototyped code to do this, which is why I'm writing this issue now. But it'll take some careful planning and tolerance for reliability issues (crashes in unexpected places) to fully land this extension and know we've properly captured every call sequence.

What underlying problem are you trying to solve with this feature?

1. Help users understand where their dependencies come from (i.e. "why does foo depend on bar? I don't see any reference to bar in foo's deps")
2. Help devs accurately diagnose build structure
3. Improve cquery accuracy. In particular, rpaths works horribly with the current logic (it shows forward deps caused by aspects even though rpaths should only show backward deps). This would allow rpaths and possibly other query functions to work correctly

Which operating system are you running Bazel on?

n/a

What is the output of bazel info release?

n/a

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

No response

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

No response

Have you found anything relevant by searching the web?

• Accessing aspects from cquery Starlark output #13866
• Querying reverse dependencies of a cc_proto_library returns wrong results #14799

No response

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

I think this update has to roll out over multiple PRs and behind a feature flag. It's really hard to audit every call point into the affected code: the full combination of <cquery flags> x <output formats> x <query expressions>. Current code assumes every node is a ConfiguredTarget, and makes assumptions accordingly. The essence of this fix is to generalize that to Aspect and Toolchain.

There's also some API challenge:

//tt:defs.bzl%_my_aspect of //tt:child is not a label. So $ bazel cquery 'somepath(//tt:parent, //tt:defs.bzl%_my_aspect of //tt:child)' triggers a syntax error.

We also need to consider appropriate syntax for cquery's various output formats.

[gregestren]

@katre FYI.

I also appreciate input from anyone else if this is interesting to you. Since I think the change is involved I don't think it's something we can just write up a PR for without having a clearer sense of the value offered.

[gregestren]

Proposal: More accurate cquery

[gregestren]

FYI stnma7e is taking a shot at this.

[cpsauer]

Thanks so much for the heads, Greg :)

[gregestren]

For reference: https://bazel-review.googlesource.com/c/bazel/+/234613.

[EdbertChan]

@gregestren Hopefully this gets upstreamed for 7.0!

[gregestren]

I think 7.1 is more likely? But noted. This is an invasive enough change that we'll really want to gate it behind a flag for a while since there's real risk of bugs in all the different ways you can invoke a query.

While we're not doing this review on Github, the author updated me today that he'll respond to my feedback this week and we'll mutually try to get the current core change checked in as soon as possible (the longer it lingers the greater the chance of disruptive merge conflicts).

[gregestren]

And to be clear, for the immediate moment we're focusing on aspects. Toolchain deps will have to be their own extension to this work. But the work is intended to be a baseline for both.

[gregestren]

I'm happy to report 81e1333 just landed at head.

This means you can add --experimental_explicit_aspects to your cquery and see more accurate behavior as described at More accurate cquery.

This is flag-gated because it's only been tested against basic deps and rdeps, with the default output format. I assume somepath (and maybe allpahs) should also work well. Other functions... who knows? And --output=* will likely have bugs.

Note that while 81e1333 only enables aspects, most of the change generalizes cquery's node type from a ConfiguredTarget to a new CqueryNode abstraction. So we can easily follow up with other non-configured target dependencies. Particularly toolchain resolution.

When we have more confidence we've rooted out the bugs we can de-flag and enable by default. Feedback and testing welcome.

Thanks again @stnm
.

[gregestren]

P1 -> P2: less bandwidth right now to follow up on polishing 81e1333 .

[sluongng]

Any plan on cherry-picking that commit to 7.2?

It would let folks try out the new feature sooner and provide feedback earlier (rather than 1 year from now).

[gregestren]

Would love to. It's a somewhat invasive code change though, so merge conflicts could be worse than normal. Let's give it a try.

[gregestren]

@bazel-io fork 7.2.0

[keertk]

Is this still on track for 7.2? We're aiming to create the first RC on 5/13.

[gregestren]

Oh, I didn't know who'd do the cherrypick but I'm happy to.

[keertk]

We can cherrypick. Wanted to make sure it's good to go since the issue isn't closed yet.
Is 81e1333 the only commit that's required?

[gregestren]

Yes. But it's such a big change I wouldn't be surprised if there are merge issues. Let me know if you see any.

[iancha1992]

Yes. But it's such a big change I wouldn't be surprised if there are merge issues. Let me know if you see any.

@gregestren Yup. Looks like there is a bunch of conflicts I cannot resolve. Could you please take a look?

cc: @bazelbuild/triage

[gregestren]

Give me a few days but I'll take a look.

[Wyverald]

Any progress here? We're aiming to cut RC1 next Monday.

[gregestren]

Giving this a shot today.

[gregestren]

Turns out this is already in 7.2.0: #21776 (comment). Experiment away - nothing further we need to do.

[matts1]

Looking at the documentation, this feature is supposed to decide whether to include aspect-generated-actions in the output. However, I think there's another use case here - accessing providers created by the aspect in cquery.

The use case that I was trying to do for this is that if I run cquery on the following target:

cc_binary(
    name = "foo",
    deps = ["//path/to:rust_library"],
)

Then I want cquery to attach a RustAnalyzerInfo object to :foo. This can then be picked up on by bazel. At the moment we use bazel build instead of bazel cquery, which means that if any bazel target fails to build, then we can't run rust-analyzer.

I tried it at head, where this feature should be landed, and it didn't work. Here's how to reproduce, in case that's a bug:

• Clone rules_rust
• cd examples/bzlmod/hello_world

USE_BAZEL_VERSION=9cf8dc6b65f59f954765b9e255810bee43464488 \
    bazel cquery \
    --experimental_explicit_aspects \
    "--aspects=@rules_rust//rust:defs.bzl%rust_analyzer_aspect" \
    "//bazel/rust/examples/prost:proto_test"
    --output=starlark --starlark:expr='providers(target).keys()'

The output contains:

INFO: Invocation ID: 2dff41ea-1efc-4e21-aeeb-b4480d44a22e
DEBUG: /usr/local/google/home/msta/.cache/bazel/_bazel_msta/271d6d3f5c91f5c775f0cfd39e345e6d/external/rules_rust~/rust/private/rust_analyzer.bzl:74:10: RUST ANALYZER ASPPECT STARTED FOR @@_main~crate_repositories~vendor__anyhow-1.0.77//:anyhow_bs
DEBUG: /usr/local/google/home/msta/.cache/bazel/_bazel_msta/271d6d3f5c91f5c775f0cfd39e345e6d/external/rules_rust~/rust/private/rust_analyzer.bzl:74:10: RUST ANALYZER ASPPECT STARTED FOR @@_main~crate_repositories~vendor__anyhow-1.0.77//:anyhow
DEBUG: /usr/local/google/home/msta/.cache/bazel/_bazel_msta/271d6d3f5c91f5c775f0cfd39e345e6d/external/rules_rust~/rust/private/rust_analyzer.bzl:146:10: RUST ANALYZER ASPECT RETURNING RustAnalyzerInfo FOR @@_main~crate_repositories~vendor__anyhow-1.0.77//:anyhow
DEBUG: /usr/local/google/home/msta/.cache/bazel/_bazel_msta/271d6d3f5c91f5c775f0cfd39e345e6d/external/rules_rust~/rust/private/rust_analyzer.bzl:74:10: RUST ANALYZER ASPPECT STARTED FOR @@//:hello_world_vendored
DEBUG: /usr/local/google/home/msta/.cache/bazel/_bazel_msta/271d6d3f5c91f5c775f0cfd39e345e6d/external/rules_rust~/rust/private/rust_analyzer.bzl:146:10: RUST ANALYZER ASPECT RETURNING RustAnalyzerInfo FOR @@//:hello_world_vendored
INFO: Analyzed target //:hello_world_vendored (34 packages loaded, 417 targets configured).
INFO: Found 1 target...
["LicenseInfo", "InstrumentedFilesInfo", "@@rules_rust~//rust/private:providers.bzl%CrateInfo", "@@rules_rust~//rust/private:providers.bzl%DepInfo", "RunEnvironmentInfo", "FileProvider", "FilesToRunProvider", "OutputGroupInfo"]
INFO: Elapsed time: 0.600s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions
INFO: Analyzed target //:hello_world_vendored (12 packages loaded, 198 targets configured).
INFO: Found 1 target...
["LicenseInfo", "InstrumentedFilesInfo", "@@rules_rust~//rust/private:providers.bzl%CrateInfo", "@@rules_rust~//rust/private:providers.bzl%DepInfo", "RunEnvironmentInfo", "FileProvider", "FilesToRunProvider", "OutputGroupInfo"]
INFO: Elapsed time: 46.007s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions

As you can see from the print statement, the aspect does successfully return a RustAnalyzerInfo object, but it doesn't show up in cquery.