Fix attrs.list(attrs.one_of(attrs.source(), xxx)))

[cormacrelf]

In #520, I was wondering why you couldn't pass a label with multiple default_outputs to cxx_library. This is why:

https://github.com/facebook/buck2/blob/c7637e656536d9e5a4cdeb2f6c801e71c40c0df6/prelude/decls/cxx_common.bzl#L17

The srcs attr was exercising this code here, and hitting the error "Expected a single artifact".

Note that this will not help with attrs.tuple(attrs.source(), xxx). That still has weird behaviour. For the example I posted, if you use srcs = [(":id", ["-Wall"])] instead, you get this:

Value <source scratch/two.cpp> of type artifact does not match the type annotation list[resolved_macro] for argument flags

You would have to add some third variant to the one_of with attrs.tuple(attrs.list(attrs.source), ...) to handle that case.

[cormacrelf]

I guess I can fix the tuple weirdness too. Tuples should run resolve_single on their child attrs, otherwise you end up with the wrong number of things in the tuple and a misalignment of types.

[cormacrelf]

I have a second diff that adds context to the errors, if that's appropriate. I found it a huge improvement, but it might make normal errors a bit verbose? Let me know if you want this.

For:
    somerule(
        gumbo = {
            "files": ":ls",
        }
    )
Before:
    Expected a single artifact from root//scratch:ls (platforms//:host#9f5b0576f0e10a13), but it returned 2 artifacts
After:
    0: Resolving attribute `gumbo`
    1: In attrs.dict() value for key "files"
    2: Resolving attrs.source() with label root//scratch:ls (platforms//:host#9f5b0576f0e10a13)
    3: Expected a single artifact from root//scratch:ls (platforms//:host#9f5b0576f0e10a13), but it returned 2 artifacts

For:
    cxx_library(
        name = "lib",
        srcs = [ ("one.cpp", ["$(hello)"]) ]
    )
Before:
    0: Error resolving `$(hello)`.
    1: There was no mapping for hello.
After:
    0: Resolving attribute `srcs`
    1: In attrs.list() [0]
    2: In attrs.tuple() [1]
    3: In attrs.list() [0]
    4: In attrs.arg()
    5: Error resolving `$(hello)`.
    6: There was no mapping for hello.

[facebook-github-bot]

@ndmitchell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ndmitchell]

Thanks, the one of fixes look amazing. Super subtle! The tuple is probably just a bug from whoever copied the code for list.

For the context, my concern is that a lot of the end users of build systems won't know what attrs.list or anything are, so it would confuse them. Maybe the right amount is:

After:
    0: Resolving attribute `srcs`
    4: In attrs.arg()
    5: Error resolving `$(hello)`.
    6: There was no mapping for hello

Which attribute you are working on seems super helpful. And the attribute you hope for. But the nesting might be confusing, and shows a lot of stuff people don't know about. Does that sound plausible?

[cormacrelf]

These errors use anyhow::Context, which is blessed by being very simple, but I think we could count up recursion depth as the stack unwinds through the ?s, and add context conditionally. Certainly possible.

Does buck2_error have anything useful for that? I see it in commits all the time but I haven't used it. It seems to have a Context trait like anyhow.

[cjhopman]

I think it'd be good for us to figure out with buck2_error how to propagate up information necessary for producing good messages without needing that information itself to be a line in the error message.

Like, we should be able to make that message be:

After:
    0: Failed to coerce attribute `srcs`.
           value `$(hello)` cannot be coerced to attr.arg() (in `srcs[0][1][0]`)
           because there is no mapping for template `hello` (available user-defined mappings are `foo`, `bar`) 
           See https://buck2.build/docs/api/build/globals/#templateplaceholderinfo for information about string templates

the main difference there just that 1-5 are all collapsed into the second line