Check providers before analyzing dependencies

[katre]

Description of the feature request:

Currently, required providers and advertised providers are not checked until after dependencies are analyzed, which means that invalid targets can produce errors that obscure the underlying problem of a bad dependency.

If a target's advertised providers were checked earlier against the parent target's required providers, an error could be shown earlier and would be more relevant to users.

Which category does this issue belong to?

Rules API

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

This came up in #22996.

Consider a platform target with an invalid dependency in the constraint_values attribute:

cc_library(name = "lib")

platform(
    name = "plat",
    constraint_values = [
        ":lib",
    ],
)

The :lib dependency is incorrect: the constraint_values attribute requires a ConstraintValueInfo provider that cc_library does not provide.

However, because platform uses NoConfigTransition, when bazel tries to analyze the dependency, analysis fails with a confusing error (after the fix for #22996 is merged): "expected a Starlark exec transition definition, but was null".

A better error would be "The dependency //:lib is not appropriate because it does not provide ConstraintValueInfo"

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

No response

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 HEAD ?

No response

Have you found anything relevant by searching the web?

No response

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

No response

[katre]

CC @comius and @lberki for implementation and ideas, and @tpudlik because it's related to #22996.

[lberki]

I don't have any particular wisdom to add other than "make it so". @mai93 did something similar with aspects: before, aspects were propagated based on the set of providers a configured target had, and she changed it so that they are now propagated according to what they declare.

This required making it an error if a configured target advertises a provider but does not provide it. So in theory, it should be possible to tell DependencyResolver to raise errors based on the advertised providers, which can happen before analyzing the whole transitive closure of a "wrong" dependency.

However, this is complicated by the fact that I think if, say, //a depends on //b, the configured target of //a does not depend on the package of //b. So you either add that Skyframe edge (and pay its RAM cost) or need to pay the cost of the analysis of //b, but then you already have the actual set of providers //b provides.

[mai93]

Although aspects check the declared providers of the target's rule against the aspect required providers, they still do so after the target is configured. We will try to change that to depend on loading the underlying target and checking its rule's declared providers instead. One problem there is aliases as you will need to unwrap the chain of aliases to get to the actual target and its rule's declared providers. I think this will need to be implemented for this solution as well.

Another issue that was brought up for aspects before is that not all language rules that always return a certain provider actually declare it in the rule definition (example: https://github.com/bazelbuild/bazel/issues/19609) so this will cause a lot of breakages when this solution is in place.

[aoeui]

From a skim of #22996, it sounds like this can mostly be addressed by improving the error handling. While it's nice to not fully analyze obviously broken ConfiguredTarget, this breaks a somewhat important encapsulation layer.

We did many quarters of work to encapsulate ConfiguredTargetFunction so that it doesn't require Packages of its dependencies. In addition to saving RAM, this makes it possible, when distributing analysis, to not load Packages across shards, which essentially makes it infeasible for distributed analysis to have good performance.

Are there any other viable approaches here?

[katre]

Even fixing the crash in #22996, there's no case where a cc_library is a valid dependency of a platform: fixing the error handling still gives a confusing error ("Cannot build //whatever:library in the empty configuration") instead of pinpointing the actual problem ("Target //whatever:library is a cc_library, which does not provide ConstraintValueInfo and is not appropriate in platform.constraint_values").

This is a wishlist, not something I think we should actively pursue, but if it were possible it'd shortcut some confusing error messages and reduce the mount of work done before stopping the build due to the error.

[lberki]

Yep, @aoeui and @mai93 are right: it's not particularly nice that reporting the "wrong kind of dependency" error requires transitively analyzing that dependency, but the price to pay for it (adding a Skyframe edge on the package of the dependency in addition to its configured target) doesn't seem to be worth it.

The only non-hacky solution I can imagine is using error bubbling to transform the "cannot build in empty configuration" message to something more user-friendly.

[mai93]

We already have BaseTargetPrerequisitesSupplier that AspectFunction uses to get the dependencies of its base target without adding a dependency edge to them. This works because at the moment the base target needs to be fully configured before the aspect starts evaluating, so the aspect finds all target's dependencies ready in skyframe (here).

However, for this problem, the packages of the target's dependencies will not be ready. So is it possible to add a query function in skyframe that gets the value of the key (like the dependency package) without adding the dependency edge?

We also wanted to remove the constraint that the target is fully configured before initiating aspect propagation to its dependencies and I was thinking such function can be helpful for that. I've not looked into how possible this is, maybe someone already knows the answer.

[lberki]

By "value of the key" do you mean the SkyValue that results from evaluating a SkyKey? If so, I don't think it's impossible but the semantics would be quite hairy. Sadly, something like that seems to be necessary for starting the propagation of aspects earlier indeed (because otherwise how would you know which dependencies to propagate the aspect to and in what configuration?)