Stabilize ! in Rust 1.41.0 by Centril · Pull Request #65355 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation83 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
petertodd, kornelski, oxalica, 95th, david-sawatzke, fogti, Elrendio, dnrusakov, slerpyyy, and schneiderfelipe reacted with thumbs up emoji mark-i-m, dnrusakov, 12101111, taiki-e, JohnTitor, Kerollmops, tomaka, matthewjasper, DutchGhost, runiq, and 56 more reacted with hooray emoji stepancheg reacted with confused emoji crlf0710, thomaseizinger, dnrusakov, taiki-e, tomaka, faern, crepererum, runiq, jplatte, repnop, and 20 more reacted with heart emoji hvariant, Lokathor, dnrusakov, hellow554, CAD97, RalfJung, hadronized, BatmanAoD, 95th, YaLTeR, and 2 more reacted with rocket emoji
Relevant to the language team, which will review and decide on the PR/issue.
Marks issues that should be documented in the release notes of the next release.
`#![feature(never_type)]`
labels
Dear language team and the community at large.
Hereby, I propose (which is the fourth time around for us) that we do stabilize never_type.
@rfcbot merge
petertodd, Ericson2314, CAD97, and justinrlle reacted with thumbs up emoji Ixrec, Ericson2314, CAD97, kornelski, chpio, 95th, hgzimmerman, YaLTeR, runiq, tusharsnx, and purplesyringa reacted with laugh emoji hellow554, Ericson2314, daboross, and 95th reacted with heart emoji
This comment has been minimized.
Centril. In #57012 (comment) you cancelled an existing FCP for this exact same proposal and replaced it with this one, with no explanation. I have trouble thinking of a legitimate reason to do this. The only effective differences are teams and concerns.
You chose to only include T-lang in this FCP, whereas the previous FCP had both T-lang and T-libs. (This is despite having a “_Redefine core::convert::Infallible as !_” commit in this PR. The libs team has previously discussed this very change as desirable but potentially causing breakage.)
You get a blank slate in terms of registered blocking concerns, which the previous FCP had two remaining: #57012 (comment)
- indirect-coherence-breakage seems to be fixed by reserve impl From<!> for T #62661
- You do not even attempt to address or even discuss my fallback concern. You only mention unspecific “previous language team consensus” in passing. As far as I know, such consensus happened before the concern was formally raised or the underlying issue was known.
Because I happen to be in T-libs and not T-lang, I currently cannot formally register this concern again. I cannot imagine this is a coincidence rather than blatant abuse of the FCP process to force through your preferred outcome.
To reiterate the concern:
The proposed fallback change introduces Undefined Behavior in some previously-valid stable unsafe code. The affected code pattern was found to be common among users of a popular crate (https://crates.io/crates/objc).
This change was previously in Nightly, and got reverted (#50121) after we found miscompilations: #48950 (comment), #48950 (comment). Nothing on the Rust side has changed since on this front.
Given that stability is one of the main goals of the Rust project, I feel it is not acceptable to silently change language semantics from under the feet of crate maintainers and introduce UB without having something to help them find out if they’re affected.
Finally, the fallback change is not necessary to stabilize the never type. (It’s the other way around.) The two do not have to be bundled together.
kentfredric, steveklabnik, ashleygwilliams, phaylon, lovesegfault, Osspial, Aaron1011, gnzlbg, JesseWright, dlight, and 6 more reacted with thumbs up emoji jhpratt, CAD97, Karrq, and wattiau-rg reacted with confused emoji hdhoang reacted with heart emoji
@rfcbot concern fallback
Thanks for raising this issue Simon. Because of your comment, I've taken a closer look at the fallback issue; previously I had been sitting out of this and letting other contributors handle it. I had not understood the extent of the breakage it introduced until now. This seems like a very egregiously breaking change to me:
- This breakage causes undefined behavior in user code, not just a compilation error.
- This breakage has known examples which crater could not catch because they were in platform specific code.
- It would be unreasonable, in my opinion, to claim that the current fallback behavior was unspecified in a way that unsafe code could not rely on.
- This breakage is not strictly necessary to unblock the most important component of this change (stabilizing
!).
Previously, not following closely, I had understood the fallback issue to only be a matter of introducing type errors. But this is a much more serious breakage than that, and I don't think there has been enough work in proposing a plan to communicate this breakage and help the community prevent introducing UB as a result of this change.
I'm personally still in favor of merging the proposal in #58184 as soon as possible. It does not seem appropriate to me to block #58184 while doing nothing to resolve the very valid concerns that have been blocking a more extensive stabilization.
Whether or not T-libs should be part of this decision, I still see no reason to cancel that FCP and start a new one except trying to sweep a registered concern under the rug without addressing it.
Thank you for taking the time to find links to relevant comments. There was indeed a lot of discussion, but I don’t see in it the team reaching any consensus.
In fact, much of the discussion revolved around a lint that was meant to catch some regressions known at the time to be caused by the fallback change. But it became apparent later that miscompilations in users of the objc crate were an entirely different category of regression, not caught by that lint.
To reiterate my position:
- I am not arguing that
!is not a better fallback than()for new code - I am not arguing that we should not make this change at all, or that that RFC 1122 does not allow making it at all
- I am requesting the fallback change be blocked on:
- Some tooling (whether a rustc lint or something else) to help maintainers figure out whether they or their dependencies are affected by a soundness regression in the same category as some uses of the
objccrate
* (Note that a warning is not enough since Cargo silences them in dependencies by default.) - Communication from the Rust project about this tooling and change
- Some transition period after having the tool available and announced and before making the change in Nightly. (When this was first in Nightly I spent a couple days in a debugger before figuring out what was going on.)
I feel that this request is supported by RFC 1122:Changes that alter dynamic semantics versus typing rules
- Some tooling (whether a rustc lint or something else) to help maintainers figure out whether they or their dependencies are affected by a soundness regression in the same category as some uses of the
In some cases, fixing a bug may not cause crates to stop compiling, but rather will cause them to silently start doing something different than they were doing before. In cases like these, the same principle of using mitigation measures to lessen the impact (and ease the transition) applies, but the precise strategy to be used will have to be worked out on a more case-by-case basis. This is particularly relevant to the underspecified areas of the language described in the next section.
- I am noting that the never type does not require the fallback change, and suggesting (Stabilize (only) the never type #58184) that it be stabilized separately without blocking unnecessarily.
- Some tooling (whether a rustc lint or something else) to help maintainers figure out whether they or their dependencies are affected by a soundness regression in the same category as some uses of the
objccrate
- (Note that a warning is not enough since Cargo silences them in dependencies by default.)
- Communication from the Rust project about this tooling and change
- Some transition period after having the tool available and announced and before making the change in Nightly. (When this was first in Nightly I spent a couple days in a debugger before figuring out what was going on.)
I'm fine with all of these points but I'm not personally going to invest my own time in the non-trivial effort that this will require (especially doing all of them).
- I am noting that the never type does not require the fallback change, and suggesting (Stabilize (only) the never type #58184) that it be stabilized separately without blocking unnecessarily.
Me and @cramertj have previously noted that we disagree with this because we think the inference fallback change will simply then not happen (if for no other reason than the substantial effort required due to the requirements you've set up). I also believe that one of the core motivations for stabilizing ! is language consistency and the benefits that the fallback change brings. I don't think it's sufficiently motivated to stabilize the type without the fallback change.
Meanwhile I will split out some of the categorization work done in this PR to another one so it doesn't bit-rot.
I do realize that these mitigations would require significant effort, and I’m also not volunteering. I still feel that this is the bar to meet when making unsound a category of programs that are currently valid and sound on the Stable channel.
As to the risk of the change not happening because no-one is willing to spend that effort, wouldn’t that be a sign that no-one feels that it’s important enough?
wesleywiser, mark-i-m, phaylon, withoutboats, kornelski, Aaron1011, Kixunil, felix91gr, theduke, and liigo reacted with thumbs up emoji jhpratt, apoelstra, and selvmaya reacted with confused emoji
The breakage to objc as a result of the fallback change is concerning, and I appreciate that you're working to hold us to our backwards compatibility commitments, as well as to ensure that real-world users don't suffer as a result of this change to what is ultimately a rather niche corner of the language. You're requesting that we do something better than "just change it", and I agree that having the kinds of migration tooling you suggest in #65355 (comment) would allow us to make this transition more confidently.
However, it is not currently a high enough priority for me to personally invest the time to implement the tooling. It isn't a blocker to any of my work, and I don't see it becoming one. I don't feel like the issues caused by this fallback change are more widespread nor more interesting than other kinds of inference changes that we've successfully made in the past. I do believe, however, that fallback to ! will ultimately result in a better language and better user experience, and I believe the cost that we're paying now with this transition is the correct tradeoff to deliver a better language.
I feel like your position on this issue is valid and reasonable, and I feel like you've already had to go to quite the lengths to keep it in our minds (how many issues have we had for stabilizing never_type now? it must be a half dozen at least). I haven't seen any new arguments being made across these issues, so I think the right step now is for the lang team to meet and make a decision on this issue. It sounds like from @withoutboats's comment above that they agree with your concerns, and I think that everyone on the language team understands the issues you've raised. I feel like a decision made amongst the lang team is going to be one that takes all perspectives into account, and that continuing to debate this topic across various GH threads is unproductive at this point. Do you agree, or are there other steps you'd like to see us take to make sure that we've understood your perspective?
I would like to give my position:
First of all, we had discussed in several lang team meetings whether this question of fallback was ultimately a lang team or libs team call. Ultimately, I believe it is pretty squarely a lang team decision, though one where I think libs team feedback is important and welcome.
We had talked some time ago about whether it would make sense to cancel the FCP and re-label the issue with T-lang, so as to clarify which team was responsible for deciding this particular aspect, though I don't believe we ever had a firm place to do so. From a purely technical POV, I think such a move is fine. However, I wouldn't have wanted it to come as a surprise, and I would've preferred to approach the problem via discussion first. In my role as lang team lead, I apologize for that, @SimonSapin.
As for the merits of the concern itself, I am glad that boats recreated the concern, as I didn't feel like we had personally concluded this. I could imagine that in lang team meetings, though, I didn't make that fully clear; I'll admit I was somewhat relying on Simon's concern to "hold the place" for my own misgivings. I still think it's not clear we can "get away" with this change, for the reasons previously enumerated. (Recently, I've been wondering if we ought to consider it for an edition boundary.)
Pauan, wesleywiser, matklad, izik1, CAD97, johnthagen, 95th, matthieu-m, shirshak55, benesch, and slanterns reacted with heart emoji
As for the merits of the concern itself, I am glad that boats recreated the concern, as I didn't feel like we had personally concluded this
To clarify this: What I had in mind here is mostly to prepare a summary document, which includes the pros and cons, and discuss that in a meeting. The same as any other concern, ultimately. I do think we have to reach some kind of final decision here and we can't hold off forever on doing so (as we've been doing).
One more clarification: I don't really think it's unreasonable of @Centril to feel that there was a consensus amongst the lang team. I think the situation was somewhat ambiguous myself, which is an interesting procedural point. (I'd like it if we made it more clear when "resloving" concerns in a final way.)
I do believe, however, that fallback to ! will ultimately result in a better language and better user experience, and I believe the cost that we're paying now with this transition is the correct tradeoff to deliver a better language
Just as a casual observer, it seems that the difficulty of the decision stems from the extremes here: it would be really good to make the fallback work, as that would unlock a lot of great optimizations and APIs, but it would be really bad to cause UB in stable programs, which would greatly undermine rust's safety guarantee (not just its stability guarantees).
Recently, I've been wondering if we ought to consider it for an edition boundary.
NLL seems to set a precedence for this.
which would greatly undermine rust's safety guarantee (not just its stability guarantees).
To clarify, the only undefined behavior resulting from this change is in unsafe code that was making assumptions about what type variables would fall back to. AFAIK this is not something we previously documented before, but was admittedly clearly observable.
NLL seems to set a precedence for this.
NLL is going to be the only option in the 2015 edition as well as the 2018 edition, and the old borrowchecker is being removed. The edition provided a convenient mechanism for testing out NLL and MIR borrowchecking on new or actively-maintained code, but it was never the plan to keep around the old borrowchecker permanently.
I don't feel like the issues caused by this fallback change are more widespread nor more interesting than other kinds of inference changes that we've successfully made in the past.
Did those other changes silently introduce UB in previously-sound programs? I feel this is an entirely different category of change, compared to making some programs fail to compile until they add a type annotation.
To clarify, the only undefined behavior resulting from this change is in
unsafecode that was making assumptions about what type variables would fall back to.
For what it’s worth this assumption was never made knowingly. The unsafe code is in a library (objc), and only some (mis)uses of that library are affected. The library even documents unambiguous use. (let () = send_msg!(…); instead of send_msg!(…);)
I've been wondering if we ought to consider it for an edition boundary.
I proposed that. #57012 (comment) claims it would be impractical.
First of all, we had discussed in several lang team meetings whether this question of fallback was ultimately a lang team or libs team call. Ultimately, I believe it is pretty squarely a lang team decision, though one where I think libs team feedback is important and welcome.
This sounds totally reasonable. And maybe the outcome will be that the lang team collectively decides to make the change without mitigation.
What I find unacceptable is one team member unilaterally resetting a decision-making tool, pretending that a concern discussed at length does not exist, and claiming team consensus that as far as I can tell doesn’t exist.
claiming team consensus that as far as I can tell doesn’t exist.
FWIW, my perception (prior to reading @withoutboats's comment above) was also that the lang team was unified on this issue. I agree with your other points, though-- the fallback concern is certainly important to address. I don't want to view this new FCP as an opportunity to forget about the fallback issue.
Can I ask what this perception was based on? A few team members arguing one way in a comment thread while others do not comment on that particular topic is not team consensus IMO.
If a decision was made in a synchronous meeting, there should be some communication about it.
As I wrote earlier, I think that lang team consensus was in an "ambiguous" state. We definitely had meetings where we expressed a desire to make this change. Clearly some of us (e.g., @withoutboats) had not deeply engaged on the issue. I know that I personally harbored some doubts though I don't think I fully expressed them, in part because (as I wrote) I was relying on the existing concern to carry them for the moment. The end result is that I could easily see where one would feel that consensus was reached.
I have been wanting to move us to a procedure where whenever contentious questions arise (whether they be from a lang team member or not), we prepare a kind of "document" where we summarize the pros/cons and so forth. This document can also record the resulting consensus, and include a spot for "dissents" -- i.e., a space for lang team members (or other major stakeholders) to document their reasons for disagreeing with the consensus (note that disagreeing with the consensus doesn't mean that one blocks the final decision).
One advantage of this process is that it records the pros/cons for posterity, and provides a document for people to point to later to avoid repeated debate. But I see now it might also have the side-effect of helping us to clarify when a decision has been reached, and what the considerations were at the time.
In any case, ignoring the procedural hiccups here, I think that preparing such a document is the right way forward in this particular case as well. I would be happy to help in preparing such a document.
Aaron1011, jebrosen, LukasKalbertodt, runiq, cramertj, teiesti, malbarbo, Kerollmops, samanpa, OptimisticPeach, and 4 more reacted with hooray emoji jplatte, hellow554, samanpa, and RalfJung reacted with rocket emoji
I was starting to think that this would ! happen 😄
LukasKalbertodt, acfoltzer, lnicola, runiq, jleedev, lo48576, ssokolow, teiesti, hellow554, malbarbo, and 26 more reacted with laugh emoji crlf0710, acfoltzer, runiq, leo60228, hayd, hellow554, RustyYato, DianaNites, qnighy, keysmashes, and 3 more reacted with hooray emoji
flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request
flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request
bors added a commit to rust-lang/rust-clippy that referenced this pull request
bors added a commit to rust-lang/rust-clippy that referenced this pull request
bors added a commit to rust-lang/rust-clippy that referenced this pull request
I think this may be causing a regression perhaps? #66757
I’m only a bystander, but the fallback change seems to me like an “obvious” candidate to be edition-gated. It’s a small language change that doesn’t affect most code, but causes rather nasty breakage in the code it does affect. In that respect, it’s comparable to adding a new keyword, or to other 2018 edition changes. And the compiler is apparently already capable of selecting the old or new behavior on a crate-by-crate basis.
One downside of using an edition is that the next edition isn’t planned to happen for years, thanks to a desire for stability. But it seems like that may be motivating people to just go ahead and make the same breaking changes without an edition – which would be a perverse outcome.
This was referenced
Dec 11, 2019
Marks issues that should be documented in the release notes of the next release.
label
#67224 is reverting the stabilization (again).
So I've de-milestoned the PR.
Centril added a commit to Centril/rust that referenced this pull request
…f-never-type, r=centril
Revert stabilization of never type
Fixes rust-lang#66757
I decided to keep the separate never-type-fallback feature gate, but tried to otherwise revert rust-lang#65355. Seemed pretty clean.
( cc @Centril, author of rust-lang#65355, you may want to check this over briefly )
bors added a commit that referenced this pull request
…e, r=
Revert stabilization of never type
Fixes #66757
I decided to keep the separate never-type-fallback feature gate, but tried to otherwise revert #65355. Seemed pretty clean.
( cc @Centril, author of #65355, you may want to check this over briefly )
bors added a commit that referenced this pull request
…e, r=centril
Revert stabilization of never type
Fixes #66757
I decided to keep the separate never-type-fallback feature gate, but tried to otherwise revert #65355. Seemed pretty clean.
( cc @Centril, author of #65355, you may want to check this over briefly )
Dessix added a commit to microsoft/snocat that referenced this pull request
