Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cfg(wasm) unifying target_arch="wasm32" and target_arch="wasm64" #83879

Closed
nagisa opened this issue Apr 5, 2021 · 14 comments
Closed

cfg(wasm) unifying target_arch="wasm32" and target_arch="wasm64" #83879

nagisa opened this issue Apr 5, 2021 · 14 comments
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Apr 5, 2021

In #80525 we added to nightly a cfg(wasm) which is set when either wasm32 or wasm64 targets are used. We don't have top-level cfg values for other architectures in a similar situation, only for windows vs unix which is set based on the OS.

This ended up being an insta-stable addition due to oversight of mine.

This issue is in order to gather consensus from T-lang members that we do indeed want this cfg. I will make myself a note to either remove this cfg or put it behind a feature gate before the next nightly->beta rollover that happens on May 6th if such consensus is not gathered.

Zulip discussion
PR adding the cfg

@nagisa nagisa added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Apr 5, 2021
@nagisa
Copy link
Member Author

nagisa commented Apr 5, 2021

@rfcbot fcp merge

@nagisa nagisa added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Apr 5, 2021
@nagisa nagisa changed the title cfg(wasm) unifying target_arch="wasm32" and target_arch="wasm64" cfg(wasm) unifying target_arch="wasm32" and target_arch="wasm64" Apr 5, 2021
@Amanieu
Copy link
Member

Amanieu commented Apr 5, 2021

Related: rust-lang/stdarch#974

@joshtriplett
Copy link
Member

Based on discussions on Zulip, it sounds like we don't want to have this be insta-stable. However, I do think we want to have cfg(wasm), it just needs a feature-gate for a little while. (And we may want to stabilize it sooner rather than later, if there are no issues, so that people can start using it rather than writing wasm32-specific code.)

Gathering consensus on adding a feature-gated cfg(wasm) that's true on WebAssembly targets:

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 5, 2021

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 5, 2021
@ehuss
Copy link
Contributor

ehuss commented Apr 5, 2021

I'd like to note that having bare aliases like this can be confusing for some users. I occasionally run into users who get tripped up by unix/windows, and don't understand why other things like macos don't work, or just in general don't understand what "families" are. Having these bare aliases masks what the underlying fields are (target_family, target_os, target_arch), which I think contributes to that confusion. I'm concerned that adding more aliases like this will contribute to more confusion, as this would add a special case that is different from the family aliases. Seeing this, users may try other arch names without 32/64 and get confused, or not understand the difference between a family and an arch.

@joshtriplett
Copy link
Member

@ehuss I would be in favor of adding aliases for other common cases, to make them simpler to write and simpler to read.

@ehuss
Copy link
Contributor

ehuss commented Apr 5, 2021

I agree that they look easier to read (and are more aesthetically pleasing), but I am concerned that they are harder to understand, especially when they are "common" exceptions. Users sometimes learn by looking at existing code to build their mental model of how things work (instead of reading or remembering documentation), and when they try to apply their mental model to new situations (like writing cfg(arm)) it can be frustrating and confusing when it doesn't work. This is particularly insidious with cfg expressions because they don't necessarily fail at compile time, they are just ignored (baring RFC 3013, which I don't think will necessarily help here, and may actually be made worse by this change). Cargo in particular is unable to verify these things.

I don't want to oversell my objection here. It is very difficult to predict how well users will understand a concept like this. I can just say that there are some users who struggle with the current system caused by "unix" and "windows", and I am concerned this may contribute to more confusion. Adding more aliases may make it even harder to understand when the thing you want to match on isn't part of that blessed set of aliases.

@devsnek
Copy link
Contributor

devsnek commented Apr 6, 2021

One thing to note specifically in the case of wasm is that wasm32 and wasm64 are not actually separate architectures, it is just a weird abstraction in llvm to represent wasm memory64 types. I would like to provide a very ergonomic way for the rust ecosystem to quickly coalesce on wasm-supporting crates targeting the wasm architecture without having to write out cfg(any(target_arch = "wasm32", target_arch = "wasm64")). I don't think it's reasonable for everyone who wants to target wasm architecture to know the specifics of how memory64 works and the choices that llvm made and that they should be making sure they're including all the wasm targets.

All that being said, I'm fine with other solutions besides the specific cfg(wasm) incantation, as long as we figure out something. For example, cfg(target_arch ~= "wasm") could also work (though I don't like that as much as cfg(wasm)).

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 8, 2021
…-Simulacrum

Remove the insta-stable `cfg(wasm)`

The addition of `cfg(wasm)` was an oversight on my end that turns out to have a number
of downsides:

* It was introduced as an insta-stable addition, forgoing the usual
  staging mechanism we use for potentially far-reaching changes;
* It is a breaking change for people who are using `--cfg wasm` either
  directly or via cargo for other purposes;
* It is not entirely clear if a bare `wasm` cfg is a right option or
  whether `wasm` family of targets are special enough to warrant
  special-casing these targets specifically.

As for the last point, there appears to be a fair amount of support for
reducing the boilerplate in specifying architectures from the same
family, while ignoring their pointer width. The suggested way forward
would be to propose such a change as a separate RFC as it is potentially
a quite contentious addition.

cc rust-lang#83879 `@devsnek`
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

Speaking personally, I am so happy I can write 'unix' vs 'windows', and I would be very happy to be able to type 'wasm'.

@nagisa
Copy link
Member Author

nagisa commented Apr 9, 2021

An update on the current state:

We have removed the cfg(wasm) for reasons outlined in the PR above (#83981). We'll be looking into repurposing target_family for this purpose instead. Slightly more typing, but no backward compatibility hazards.

@nikomatsakis
Copy link
Contributor

@nagisa should we cancel the FCP and close?

@nagisa
Copy link
Member Author

nagisa commented Apr 13, 2021 via email

@nikomatsakis
Copy link
Contributor

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Apr 13, 2021

@nikomatsakis proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants