RFC3239: Implement cfg(target) - Part 2

[Urgau]

This pull-request implements the compact cfg(target(..)) part of RFC 3239.

I recommend reviewing this PR on a per commit basics, because of some moving parts.

cc @GuillaumeGomez
r? @petrochenkov

[petrochenkov]

The implementation looks significantly overcomplicated due to all the multi-spans and supporting #96913 (comment).

I suggest to get rid of all of that, lower target(a, b = "x", c(y), "d") to all(target_a, target_b = "x", target_c(y), "d") immediately, and then continue processing it as any other predicate.

[petrochenkov]

This part is not outright harmful like #96909, but I'd still be interested to see it dogfood-ed on the rust-lang/rust codebase to see how much useful it is, because I'm not sure whether it actually pulls its weight.

[Urgau]

The implementation looks significantly overcomplicated due to all the multi-spans and supporting #96913 (comment).

As requested I have removed #96913 (comment) but I kept the multi-spans because without them the lint from the check cfg are confusing as they would just show the last part.

I suggest to get rid of all of that, lower target(a, b = "x", c(y), "d") to all(target_a, target_b = "x", target_c(y), "d") immediately, and then continue processing it as any other predicate.

Isn't this what is already done ? Or you mean doing this in the parser or somewhere else ?

This part is not outright harmful like #96909, but I'd still be interested to see it dogfood-ed on the rust-lang/rust codebase to see how much useful it is, because I'm not sure whether it actually pulls its weight.

I suppose this would be for another PR, not this one, right ?

@rustbot ready

[petrochenkov]

I suppose this would be for another PR, not this one, right ?

Yes, once this change reaches bootstrap compiler.

[petrochenkov]

Or you mean doing this in the parser or somewhere else ?

No, no, during cfg evaluation, roughly in the same place as now.

I see that right now errors like "cfg predicate key must be an identifier" are reported in two places.
I'd expect them to be reported once on the desugared all(target_a, target_b = "x", target_c(y), "d") representation.

I kept the multi-spans because without them the lint from the check cfg are confusing as they would just show the last part.

Could you perhaps leave this purely diagnostic change to a separate PR?
Right now the multi-span support changes represent a significant part of the PR while not being necessary for functional correctness.
(FWIW, highlighting only the last part would be fine by me.)

[Urgau]

I kept the multi-spans because without them the lint from the check cfg are confusing as they would just show the last part.

Could you perhaps leave this purely diagnostic change to a separate PR?

Sure, done.

I'd expect them to be reported once on the desugared all(target_a, target_b = "x", target_c(y), "d") representation.

Done, but the way it's done feels like a hack to me which is why I've put it in the last commit. This approach would also completely remove the possibility of having multi-spans or simply better diagnostics. Anyway If you prefer this approach I will squash the commits.

@rustbot ready

[petrochenkov]

Looks much better now.

[Urgau]

The implementation is now strangely clean and simple. 😄

@rustbot ready

[petrochenkov]

@bors r+

[bors]

📌 Commit b9ae3db has been approved by petrochenkov