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.

Sign up for GitHub

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

Jump to bottom

Stabilize ! in Rust 1.41.0 #65355

Merged
merged 5 commits into from
Nov 21, 2019
Merged

Stabilize ! in Rust 1.41.0 #65355

merged 5 commits into from
Nov 21, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 13, 2019 •
edited
Loading

This PR stabilizes the never_type (written !). The type represents computations that we know diverge in the type system and therefore has no values / inhabitants / elements / members.

The current nightly version is 1.40.0 which will become stable on 2019-12-19.

Tracking issue: #35121.
Closes #57012.
Closes #58184.
Original stabilization report: #57012 (comment)

Additional notes:

r? @nikomatsakis

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. F-never_type `#![feature(never_type)]` labels Oct 13, 2019
@Centril Centril added this to the 1.40 milestone Oct 13, 2019
@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2019 •
edited
Loading

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

@rfcbot
Copy link

rfcbot commented Oct 13, 2019 •
edited
Loading

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

Concerns:

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 Oct 13, 2019
@Centril Centril mentioned this pull request Oct 13, 2019
Closed
@rust-highfive

This comment has been minimized.

Sign in to view
@SimonSapin
Copy link
Contributor

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)

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.

@withoutboats
Copy link
Contributor

@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.

@Centril
Copy link
Contributor Author

Centril commented Oct 14, 2019 •
edited
Loading

(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.)

I believe the libs team has already decided that Infallible should be redefined as ! but if you insist that enum Infallible {} should remain the definition, then I will remove that commit.

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.

It's definitely not a coincidence not including T-libs but it's definitely also not an abuse of the FCP process not to include a team that isn't supposed to make decisions about how the type system behaves. You can always make your case as a community member and as you've seen parts of the language team seems to have agreed with you.

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.

I think RFC 1122 clearly suggests otherwise.

The following comments include the discussion re. the fallback change:

@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 14, 2019 •
edited
Loading

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 objc crate
      • (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

    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.

@Centril
Copy link
Contributor Author

Centril commented Oct 14, 2019

@SimonSapin

  • 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 objc crate

    • (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).

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.

@SimonSapin
Copy link
Contributor

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?

@cramertj
Copy link
Member

@SimonSapin

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?

@cramertj cramertj mentioned this pull request Oct 14, 2019
Closed
@nikomatsakis
Copy link
Contributor

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.)

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 14, 2019 •
edited
Loading

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.)

@mark-i-m
Copy link
Member

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.

@cramertj
Copy link
Member

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.

@SimonSapin
Copy link
Contributor

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 unsafe code 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.

@SimonSapin
Copy link
Contributor

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.

@cramertj
Copy link
Member

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.

@SimonSapin
Copy link
Contributor

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.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 14, 2019 •
edited
Loading

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.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 21, 2019
@bors
Copy link
Contributor

bors commented Nov 21, 2019

⌛ Testing commit 238d03b with merge 35ef33a...

@bors bors merged commit 238d03b into rust-lang:master Nov 21, 2019
@Centril Centril deleted the almost-is-never-enough branch November 21, 2019 17:53
@Aaron1011
Copy link
Member

I was starting to think that this would ! happen 😄

flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request Nov 22, 2019
@flip1995
Remove never_type feature
4ad77bd 
Stablized in rust-lang/rust#65355
flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request Nov 22, 2019
@flip1995
Remove never_type feature
3c308b8 
Stablized in rust-lang/rust#65355
@Manishearth Manishearth mentioned this pull request Nov 22, 2019
Merged
@pnkfelix pnkfelix mentioned this pull request Nov 22, 2019
Open
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
@bors
Auto merge of #4825 - Manishearth:rustup, r=Manishearth
19769b5 
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
@bors
Auto merge of #4825 - Manishearth:rustup, r=Manishearth
3870050 
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
@kngwyu kngwyu mentioned this pull request Nov 23, 2019
Closed
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
@bors
Auto merge of #4825 - Manishearth:rustup, r=flip1995
60e8413 
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
@ehuss ehuss mentioned this pull request Nov 24, 2019
Closed
@alexcrichton
Copy link
Member

I think this may be causing a regression perhaps? #66757

@alexcrichton alexcrichton mentioned this pull request Nov 25, 2019
Closed
@comex
Copy link
Contributor

comex commented Nov 28, 2019

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.

@SimonSapin
Copy link
Contributor

I proposed this in #57012 (comment), see also the following comment.

This was referenced Dec 11, 2019
Merged
Open
Closed
@Centril Centril removed the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 11, 2019
@Centril Centril removed this from the 1.41 milestone Dec 11, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 11, 2019

#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 Dec 12, 2019
@Centril
Rollup merge of rust-lang#67224 - nikomatsakis:revert-stabilization-o…
922e5dc 
…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 Dec 14, 2019
@bors
Auto merge of #67224 - nikomatsakis:revert-stabilization-of-never-typ…
1d0fe77 
…e, r=<try>

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 Dec 14, 2019
@bors
Auto merge of #67224 - nikomatsakis:revert-stabilization-of-never-typ…
6f82984 
…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 )
@MattesWhite MattesWhite mentioned this pull request Dec 15, 2019
Closed
Dessix added a commit to microsoft/snocat that referenced this pull request Feb 24, 2022
@Dessix
Removed #![feature(never_type)] as it is stable in 1.41.0 and we're M…
e9fedd3 
…SRV 1.58.1

See rust-lang/rust#65355 for stabilization PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-never_type `#![feature(never_type)]` finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stabilize (only) the never type Stabilize never_type *again*