Do not suggest #![feature(...)] if we are in beta or stable channel.
#23974
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
|
(i'm still working on a local build to double-check that this work, and does not break any tests that might be over-zealously checking the help output, but I figured I'd get this up in the meantime.) |
|
looks good here. maybe a test? |
| @@ -409,6 +409,12 @@ impl<'a> Context<'a> { | |||
|
|
|||
| pub fn emit_feature_err(diag: &SpanHandler, feature: &str, span: Span, explain: &str) { | |||
| diag.span_err(span, explain); | |||
|
|
|||
| // #23973: do not suggest `#![feature(...)]` if we are in beta/stable | |||
There was a problem hiding this comment.
do we leave these kind of comments?
There was a problem hiding this comment.
(In particular, the detail of why we are early returning requires one keep reading beneath the match statement ... I figure a one-line comment with issue number is worthwhile in that context. If I had done this by moving the fileline_help invocations into the _ => { ... } arm instead, then I would not have included the comment, and would have treated the code as self-explanatory in that case. But as soon as I fiddle with return or break or other abnormal control-transfers, I try to err on the side of leaving more comments.)
There was a problem hiding this comment.
interesting, I guess it's a different workflow. I tend to reach for blame, which then points to this PR. shrugs
There was a problem hiding this comment.
(Oh, don't get me wrong; I'm a heavy user of blame too, via M-x vc-annotate. But that can require tracing through a chain of history as code gets moved around or inconsequentially revised by intermediate authors.)
|
I do not yet know how to write tests that are only run on beta/stable or likewise only nightly/development... but I'll look into it. :) |
|
It doesn’t look like |
|
@SimonSapin hmm let me go look at how that is controlled in that case. |
|
I think |
|
@SimonSapin but I cannot access that from |
|
CC @brson, winner of the |
|
@SimonSapin However, I will be happy to cut and paste the first line from let disable_unstable_features = option_env!("CFG_DISABLE_UNSTABLE_FEATURES").is_some();I.e. this is probably the right environment variable to use, rather than inspecting the value of the |
|
Hum, yes, that seems right. The bootstrap key thing is probably not relevant since by this time we’ve already decided to emit an error. |
|
and its a net win for the code clarity! :) |
|
@brson I don't see an obvious way for me to write a clear regression test for this. (I could make a |
|
Could the Other than that though r=me, I agree that adding a test for this is probably too difficult. |
|
@alexcrichton its probably higher priority for me to figure out how to fix the tests that this breaks ... |
|
@brson @alexcrichton I just ripped out the checks for the help in our test suite ... but I don't know whether this is then a net win overall. In other words: What is the better of two bad options: failing to check in our test suite that the feature names continue to match expectations (i.e. the current state of this PR), or providing a misleading help message to beta users who attempt to use a gated feature (i.e., the current state of the master branch, without this PR, as is)? |
|
@pnkfelix I don't think it's so very important to test that, personally. |
|
Agree that losing those tests is minor. |
|
I am not going to attempt to move |
|
@bors: p=1 |
Do not suggest `#![feature(...)]` if we are in beta or stable channel. Fix #23973
Do not suggest
#![feature(...)]if we are in beta or stable channel.Fix #23973