rustc: target_features: allow for cfg-only stable target_features#155962
rustc: target_features: allow for cfg-only stable target_features#155962romancardenas wants to merge 2 commits intorust-lang:mainfrom
rustc: target_features: allow for cfg-only stable target_features#155962Conversation
|
rustbot has assigned @nikomatsakis. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
There was a problem hiding this comment.
Thanks! This is probably the easiest way to do it. It's not super clean since it cannot represent "feature is allowed in cfg but strictly forbidden otherwise", so the entire thing will need a refactor eventually... but I can't ask you to do that now so let's just go with this.
@rustbot author
| reason, | ||
| }); | ||
| } else if stability.requires_nightly().is_some() { | ||
| } else if stability.requires_nightly(true).is_some() { |
There was a problem hiding this comment.
| } else if stability.requires_nightly(true).is_some() { | |
| } else if stability.requires_nightly(/* in_cfg */ false).is_some() { |
This is where we emit warnings for unstable things in -Ctarget-feature. Yes it's pretty hacky to do that as a side-effect of constructing the cfg data... codegen backend initialization is a terrible spaghetti mess and someone decided this was the place to put that check. 🤷 (It's probably here to avoid emitting the warnings multiple times, or something like that.)
| reason, | ||
| }); | ||
| } else if let Some(nightly_feature) = stability.requires_nightly() | ||
| } else if let Some(nightly_feature) = stability.requires_nightly(false) |
There was a problem hiding this comment.
| } else if let Some(nightly_feature) = stability.requires_nightly(false) | |
| } else if let Some(nightly_feature) = stability.requires_nightly(/* in_cfg */ false) |
There was a problem hiding this comment.
If I'm not wrong, here is the only place where the new error message should be triggered right? I push an additional commit with the new error message, if needed.
There was a problem hiding this comment.
I don't understand the question. There are 3 places where we query feature stability, corresponding to -Ctarget-feature, #[target_feature], and cfg(target_feature), respectively. Only one of them should set in_cfg to true.
There was a problem hiding this comment.
In your review, you mentioned that:
It's not super clean since it cannot represent "feature is allowed in cfg but strictly forbidden otherwise", so the entire thing will need a refactor eventually...
If this PR is OK so far, I can try to add a new error message near lines 65 and 318, which corresponds to the check for #[target_feature] and -Ctarget-feature, respectively.
There was a problem hiding this comment.
I am still confused. I did mention that, yes. What does it have to do with the diff I suggested here?
To resolve that concern, we'd need a more fundamental refactor of the Stability type, into something more like
struct TargetFeatureInfo {
for_cfg: Stability,
for_toggle: Stability,
}That's a lot more work so I am not expecting / asking you to do it in this PR.
There was a problem hiding this comment.
Sorry for the confusion, it has nothing to do with the particular diff you suggested. It was just for me to know where error/warning messages were triggered. I will keep discussing this in a separate comment.
| if allow_unstable | ||
| || (gate.in_cfg() | ||
| && (sess.is_nightly_build() || gate.requires_nightly().is_none())) | ||
| && (sess.is_nightly_build() || gate.requires_nightly(true).is_none())) |
There was a problem hiding this comment.
| && (sess.is_nightly_build() || gate.requires_nightly(true).is_none())) | |
| && (sess.is_nightly_build() || gate.requires_nightly(/* in_cfg */ true).is_none())) |
|
Reminder, once the PR becomes ready for a review, use |
bb0826a to
109fc56
Compare
109fc56 to
df8d029
Compare
|
@rustbot ready |
…y stable features
|
I pushed a commit that includes a basic error handling for printing error and warning messages that explicitly mention a target feature that is Let me know if you don't like the approach and I will revert the latest commit. |
|
I like it. :) I'm a bit nervous about not having a test for this, but I am also not sure how a test could work. We'd need some sort of dummy feature that's guaranteed to always remain cfg_stable_toggle_unstable... but we don't actually want to have such a feature available on stable.^^ @bors r+ rollup |
…et-feature, r=RalfJung `rustc`: `target_features`: allow for `cfg`-only stable `target_features` This PR introduces a new stabilization level for `target_features`: `CfgOnlyStable`. The motivation is allowing the Rust compiler to expose `target_features` of targets so users can use `cfg(target_feature = "feature")` for conditional blocks depending on target features. However, `CfgOnlyStable` cannot be used for `#[target_feature(enable = "feature")]`, as this is still considered unstable. Accordingly, the compiler will still raise an error if these expressions are used on stable. This PR relates partially to rust-lang#150257. As discussed, for RISC-V targets, having the `"d"`, `"e"`, and `"f"` target features exposed will allow baremetal developers to adapt the code depending on the target's properties. r? @RalfJung
…uwer Rollup of 10 pull requests Successful merges: - #155848 ([doc]: Revert `core::io::ErrorKind` doc changes) - #155855 (Remove unnecessary `get_unchecked`) - #155543 (docs(unstable-book): Document const generics features) - #155962 (`rustc`: `target_features`: allow for `cfg`-only stable `target_features`) - #156043 (c-variadic: gate `va_arg` on `c_variadic_experimental_arch`) - #156082 (Move tests associated types) - #156092 (Clean up `TyCtxt::needs_crate_hash` usage and rename it to `needs_hir_hash`.) - #156104 (Relax `T: Sized` bound on `try_as_dyn` / `try_as_dyn_mut`) - #156128 (.mailmap: prefer matching just based on commit emails) - #156135 (Remove most uses of def_id_to_node_id)
This PR introduces a new stabilization level for
target_features:CfgOnlyStable. The motivation is allowing the Rust compiler to exposetarget_featuresof targets so users can usecfg(target_feature = "feature")for conditional blocks depending on target features. However,CfgOnlyStablecannot be used for#[target_feature(enable = "feature")], as this is still considered unstable. Accordingly, the compiler will still raise an error if these expressions are used on stable.This PR relates partially to #150257. As discussed, for RISC-V targets, having the
"d","e", and"f"target features exposed will allow baremetal developers to adapt the code depending on the target's properties.r? @RalfJung