RFC3239: Implement cfg(target) - Part 1

[Urgau]

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

cc @GuillaumeGomez
r? @petrochenkov

[GuillaumeGomez]

Awesome, thanks!

[CryZe]

Apparently my concern in the original RFC was never elaborated on, nor is the RFC in any way clear on this, but is this really intended to be a plain string comparison instead of a semantic comparison? That feels pretty wrong to me, but no one ever clarified it. (e.g. wasm32-wasi was called wasm32-unknown-wasi before)

[GuillaumeGomez]

Maybe the part 2 will answer your question? You can see it there: #96913

[CryZe]

Part 2 is the other part of the RFC, that is less controversial and properly defined. Unless that PR also somehow builds on top of this one and I missed something, but they seem pretty distinct from what I'm seeing.

[GuillaumeGomez]

Sorry, I thought you asked about having more control about the checks. You can have the full one (which is the same as returned by cargo iirc) or you can check each sub element directly (implemented in part 2).

[CryZe]

I would've assumed the one that this PR implements (yet again the RFC never clarified what this PR is supposed to do) to first parse the string into its parts and then compare it semantically, i.e. parse it and lower it into the representation of part 2.

[CryZe]

Okay, so since this wasn't clarified in the original RFC, I'd like to properly open up the discussion about this again. Can this be implemented as a triple parser instead and then be lowered to the other representation? Basically I'd like to see wasm32-unknown-wasi match against wasm32-wasi since those are semantically the same triples. If there's a technical reason as to why this can't be done, I'd like to hear it. Otherwise I think that would be a much better implementation than what I'm seeing in this PR.

[GuillaumeGomez]

Sounds acceptable to me. What do you think @joshtriplett ?

[Urgau]

Okay, so since this wasn't clarified in the original RFC, I'd like to properly open up the discussion about this again. Can this be implemented as a triple parser instead and then be lowered to the other representation? Basically I'd like to see wasm32-unknown-wasi match against wasm32-wasi since those are semantically the same triples. If there's a technical reason as to why this can't be done, I'd like to hear it. Otherwise I think that would be a much better implementation than what I'm seeing in this PR.

I don't think it would be possible or even something that we would want because:

1. As far as I know, neither rustc nor Rust has a clear definition of what a target-triple is. Clang as roughly a definition, but I don't think it would fit for Rust.
2. Target-triple are for human not machine, nothing prevent Rust from having a target-triple named "linux-pencil-version-2.1" or "ThisIs-ADummy-TargetTriple". How should we parse them ? Assuming we parsed them, how should we match on them ?
3. How to deal with current and future ambiguity ? Ex: "wasm32-wasi" would match "wasm32-unknown-wasi" but also "wasm32-unknown-wasi-gnu" (if it ever exists).
4. There is currently not enough information (to be fair, the information is there, but not in a way that would be usable for this) inside a Target to be able to do such a thing. (And that not even talking about custom target in json form). (True, but irrelevant, we would only use the target-triple, not the actual target).
5. Even if had a clear definition of a target-triple nothing would prevent users from using a different definition for there custom target (ie JSON) or simply by passing rustc --cfg target=888.
6. = in cfg currently means strict equality, making = in target mean "equivalence" would be unexpected, especially because the target would be between "". This would create confusion among users (I know I would be confused).
7. The principle of condition compilation in Rust is based behind the idea of a set of configuration options (ie key-value pairs that are either set or not set). This "equivalence" wouldn't fit in that rather simple and intuitive model.

Given all the aforementioned points, I don't think we should go on that direction.

[petrochenkov]

Given all the aforementioned points, I don't think we should go on that direction.

Given the @CryZe's comments I don't think we should go on this direction either.

I feel really skeptical about this whole thing.
Looks like a source of endless issues for the sake of a minor convenience.

This only works for built-in targets, custom targets parsed from json files don't have full names.
Want to use a custom target making tweaks to a built-in target? Some random libraries using cfg(target) stop working.
Even with built-in targets, add some new ABI flavor to an existing target? Some random libraries using cfg(target) stop working.

We need to stabilize target_abi / atomic cfgs / whatever else necessary instead.

[GuillaumeGomez]

Except that the RFC was accepted and this a needed feature. So what do we do at this point?

[Urgau]

Given all the aforementioned points, I don't think we should go on that direction.

Given the @CryZe's comments I don't think we should go on this direction either.

This might seems weird but I actually agree that this direction this not great either.
I only did this part of the RFC because I wanted to do the other part of the RFC and it seemed easy.

I feel really skeptical about this whole thing. Looks like a source of endless issues for the sake of a minor convenience.

Agree, especially with "over-fitting" instead of using the proper target_*.

This only works for built-in targets, custom targets parsed from json files don't have full names. Want to use a custom target making tweaks to a built-in target?

sess.opts.target_triple.triple() return the filename in the case of a custom target from a json file.

Some random libraries using cfg(target) stop working. Even with built-in targets, add some new ABI flavor to an existing target? Some random libraries using cfg(target) stop working.

I thought of that but because the RFC was accepted, I assumed that the risk was considered acceptable but I now realize that I was wrong. I don't know what would be a solution here.

Except that the RFC was accepted and this a needed feature. So what do we do at this point?

🔽 This:

We need to stabilize target_abi / atomic cfgs / whatever else necessary instead.

👍

[petrochenkov]

@GuillaumeGomez

Except that the RFC was accepted and this a needed feature. So what do we do at this point?

The harm from this feature was significantly underestimated compared to the benefits when this RFC was evaluated, IMO.
In theory, I could merge this and then try to block the feature from stabilization on the next step, but I'd prefer to do it sooner.

I think we need to start stabilizing the unstable target_* cfgs instead.
@Urgau also suggested finishing implementation of cfg_accessible on Zulip.

Every use of a target by its full name means that we are missing some finer-grained controls (or hitting some very rare case, then it's not that important to improve its ergonomics, especially if those improvements come with significant downsides).

[joshtriplett]

I don't have any objection to re-evaluating the use cases for the whole-target-string part of the RFC. I do think the target shorthand syntax may make that less necessary.

[Urgau]

In order to move this forward (close or merge), T-lang could discussion in this in a triage meeting or at least be pinged on this PR. @joshtriplett

[petrochenkov]

The cfg_accessible PR - #97391.

[joshtriplett]

Nominating for lang team discussion.

The specific lang team ask:

• It sounds like the use cases for cfg(target = "...") would be better served by either cfg_accessible or the cfg(target(x = ..., y = ...)) shorthand.
• Given that, and given concerns about the fragility of the cfg(target = "...") whole-target string matching, should we go back and un-approve the cfg(target = "...") syntax from the RFC and only approve the cfg(target(x = ..., y = ...)) shorthand?

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting. We agreed that we'd like to drop the cfg(target = "...") syntax, and just keep the cfg(target(x = "...", y = "...")) convenience shorthand.

Could someone please send a PR to the RFCs repository, adding a note to the top of this RFC that we decided to only accept the cfg(target(os = "...", arch = "...", ...)) shorthand, not the cfg(target = "...") whole-target-string matching? We'd be happy to merge that as documentation.

For this PR, we'd love to have a version that just implements the target shorthand.

[fbstj]

For this PR, we'd love to have a version that just implements the target shorthand.

I believe this was done in #96913, or am I missing something?

[petrochenkov]

Ok, then closing this PR.
(The shortcut syntax was implemented in #96913.)