Remove adjacent all-const match arm hack. #49447
Conversation
An old fix for moves-in-guards had a hack for adjacent all-const match arms. The hack was explained in a comment, which you can see here: /~https://github.com/rust-lang/rust/pull/22580/files#diff-402a0fa4b3c6755c5650027c6d4cf1efR497 But hack was incomplete (and thus unsound), as pointed out here: rust-lang#47295 (comment) Plus, it is likely to be at least tricky to reimplement this hack in the new NLL borrowck. So rather than try to preserve the hack, we want to try to just remove it outright. (At least to see the results of a crater run.) [breaking-change] This is a breaking-change, but our hope is that no one is actually relying on such an extreme special case. (We hypothesize the hack was originally added to accommodate a file in our own test suite, not code in the wild.)
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @rust-lang/release please run crater on this. |
|
@rust-lang/release actually @nikomatsakis pointed out to me that the queue for crater runs is so long that the above request may not get the results soon enough to be of use. (@nikomatsakis has hypothesized to me that we may want to just land this and see if anyone complains ... 😷 ) |
|
I am opposed to landing this before we branch beta at least (so ~next week). I think we can queue this into crater so that it will finish by end of next week - will that work for this? I'm not quite sure what the urgency is... |
|
@Mark-Simulacrum its not urgent. But a month (the ETA niko gave me for a crater run) is still a long time, compared to just landing and seeing if any nightly users complain to us. (The idea is that we are trying to get a feeling for whether we need to actually support the "feature" that motivated this hack, because that impacts the plan for NLL.) |
|
@bors try |
|
⌛ Trying commit 347cf21 with merge 18d5da23af4abcac99d3b3cc48a52cd3da0a15d5... |
|
@Mark-Simulacrum I re-read your message and realized that you gave me an ETA that was much sooner than I had expected, and asked me if that was okay. So: Yes, please do a crater run, it can only help. :) |
|
Oh: I believe a crater run that just does (See also https://mozilla.logbot.info/rust-infra/20180329#c14535679 ) |
|
☀️ Test successful - status-travis |
|
Crater run started (priority bumped because it sounds like we want this soon). It's a check-only build, we'll see if it works (first attempt!). |
|
Hi @pnkfelix (crater requester), @nikomatsakis (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49447/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file. (interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious) |
|
Looks like no regressions found, so we should be able to land this. |
|
Nice! |
|
@bors r+ |
|
📌 Commit 347cf21 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=3 |
…komatsakis Remove adjacent all-const match arm hack. An old fix for moves-in-guards had a hack for adjacent all-const match arms. The hack was explained in a comment, which you can see here: /~https://github.com/rust-lang/rust/pull/22580/files#diff-402a0fa4b3c6755c5650027c6d4cf1efR497 But hack was incomplete (and thus unsound), as pointed out here: #47295 (comment) Plus, it is likely to be at least tricky to reimplement this hack in the new NLL borrowck. So rather than try to preserve the hack, we want to try to just remove it outright. (At least to see the results of a crater run.) [breaking-change] This is a breaking-change, but our hope is that no one is actually relying on such an extreme special case. (We hypothesize the hack was originally added to accommodate a file in our own test suite, not code in the wild.)
|
☀️ Test successful - status-appveyor, status-travis |
An old fix for moves-in-guards had a hack for adjacent all-const match arms.
The hack was explained in a comment, which you can see here:
/~https://github.com/rust-lang/rust/pull/22580/files#diff-402a0fa4b3c6755c5650027c6d4cf1efR497
But hack was incomplete (and thus unsound), as pointed out here:
#47295 (comment)
Plus, it is likely to be at least tricky to reimplement this hack in
the new NLL borrowck.
So rather than try to preserve the hack, we want to try to just remove
it outright. (At least to see the results of a crater run.)
[breaking-change]
This is a breaking-change, but our hope is that no one is actually
relying on such an extreme special case. (We hypothesize the hack was
originally added to accommodate a file in our own test suite, not code
in the wild.)