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.

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

Edition 2024: Make ! fall back to ! #123508

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

WaffleLapkin
Copy link
Member

This PR changes never type fallback to be ! (the never type itself) in the next, 2024, edition.

This makes the never type's behavior more intuitive (in 2024 edition) and is the first step of the path to stabilize it.

r? @compiler-errors

@WaffleLapkin WaffleLapkin added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. F-never_type `#![feature(never_type)]` labels Apr 5, 2024
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2024
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Apr 5, 2024

Nominating for T-lang discussion. This PR changes never type fallback in the 2024 edition. I'm asking for approval, given that this is part of the following plan:

  1. Prior to the stabilization of Rust 2024:
    • Change fallback to ! on the nightly 2024 edition (this PR).
    • Add a lint against fallback affecting a generic that is passed to an unsafe function.
      • Perhaps make this lint deny-by-default or a hard error in Rust 2024.
    • Add a future incompat lint for some/most of the code that will be broken by fallback to ! (i.e. check trait obligations on diverging type variables which are affected by fallback).
    • Add documentation/migration tools for people adopting 2024 edition.
  2. Stabilize Rust 2024.
  3. At a later date when 2024 edition adoption is widespread:
    • Make the breaking change to fall back to ! always everywhere.
    • Change Infallible = !.
    • Stabilize ! (!).

@WaffleLapkin WaffleLapkin added I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. A-edition-2024 Area: The 2024 edition labels Apr 5, 2024
@traviscross traviscross changed the title Edition 2024: Make ! fallback to ! Edition 2024: Make ! fall back to ! Apr 7, 2024
@compiler-errors compiler-errors added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2024
@crlf0710
Copy link
Member

Might worth add #66173 and #66757 as tests.

fn test66173() {
     let x = match true {
          true => Default::default(),
          false => panic!("..."),
     };
     // print `x` type
}

fn test66757() {
     struct E;

     impl From<!> for E {
          fn from(_: !) -> E {
               E
          }
     }

     #[allow(unreachable_code)]
     fn foo(never: !) {
          <E as From<!>>::from(never);  // Ok
          <E as From<_>>::from(never);  // Should the inference fail?
     }
}

@nikomatsakis
Copy link
Contributor

@rfcbot merge

This PR changes fallback to ! in Rust 2024. This has raised concerns about two particular kinds of problems, and we have plans to add two lints to catch common errors

  • Add a lint against fallback affecting a generic that is passed to an unsafe function.
  • Add a future incompat lint for some/most of the code that will be broken by fallback to ! (i.e. check trait obligations on diverging type variables which are affected by fallback).

These lints are valuable in all editions because they both address "suspicious" / error-prone code. They also catch the cases that could change behavior. Ideally we'll have them before we get to the new edition, if not, we'll want to revisit whether to proceed.

@rfcbot
Copy link

rfcbot commented Apr 10, 2024

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

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
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 Apr 10, 2024
@WaffleLapkin
Copy link
Member Author

@crlf0710 do those tests look good to you?

@tmandry
Copy link
Member

tmandry commented Apr 17, 2024

I'm happy with doing this, contingent on having mitigations to avoid unsafe code changing behavior.

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 17, 2024
@rfcbot
Copy link

rfcbot commented Apr 17, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@WaffleLapkin
Copy link
Member Author

@tmandry fyi the unsafe mitigation lint is being implemented in #123939. Needs a bit more polish, but it is generally working

c:

@traviscross traviscross removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Apr 24, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 27, 2024
@rfcbot
Copy link

rfcbot commented Apr 27, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 27, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2024
@traviscross traviscross removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Apr 30, 2024
@WaffleLapkin WaffleLapkin added the T-types Relevant to the types team, which will review and decide on the PR/issue. label May 9, 2024
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented May 9, 2024

I remembered that this should go through a T-types FCP. So here is a summary for the changes made/proposed here:

In the next edition (2024) unconstrained diverging variables will fallback to !, instead of the () which is currently used. While this does not directly unblock never type stabilization, this is a stepping stone on the way there. The plan is to change the fallback in 2024, allow active users to port their code and make sure it still works, make sure the lints work as they should, etc; And then, once we are satisfied with all of that, bring the same change to all editions.

This is what we agreed with the lang team the process should be, but I want to double check that T-types does not have objections.

As a reminder "diverging variable" is what we create when doing a never-to-any coercion (we insert them everywhere eagerly), for example:

{ return /* : ! */ } /* ?D, diverging variable */;

This does affect code semantics, because the fallback can very easily flow back into "live code":

match b {
    true => unsafe { core::mem::zeroed() }, // we will infer zeroed's type from the fallback
    false => return,
}

I'm working on lints to make sure users know that this change might happen / that they might have a foot gun. There is a lint against fallback affecting unsafe code (see #123748) and there will be a lint against obligations that can't be satisfied with a different fallback.

To reiterate: I'm proposing to change the never type fallback to ! (the never type itself) in the next edition, does that sound good?.

@BoxyUwU
Copy link
Member

BoxyUwU commented May 9, 2024

@rfcbot fcp merge

context:
Lang fcp information
Types fcp information

note: This doesn't stabilise naming the never type only the fallback of diverging type variables (though it can already be named in some circumstances on stable already) i.e. this does not allow writing impl Foo for !.

@compiler-errors
Copy link
Member

Can't do another fcp on top of a preexisting one I think

@compiler-errors compiler-errors added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 9, 2024
@compiler-errors
Copy link
Member

compiler-errors commented May 9, 2024

Let me see if I can cancel the last one, start a new one w/ both lang, and just check all of the preexisting lang boxes

@compiler-errors
Copy link
Member

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented May 9, 2024

@compiler-errors proposal cancelled.

@rfcbot rfcbot removed the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label May 9, 2024
@compiler-errors
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented May 9, 2024

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

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
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 May 9, 2024

// `feature(never_type_fallback)`: fallback to `!` or `()` trying to not break stuff
if tcx.features().never_type_fallback {
return FallbackToNiko;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a confusing enum variant name, I would like it to be more descriptive

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an idea what could be a better name?

The reason I named it like that, is that it is inherently confusing, in a way. + it's probably on the way to be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContextDependent

also, instead of naming the variants FallbackToX, I personally prefer using DivergingFallbackBehavior::ToNever and stop using a blob import for the variants

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DivergingFallbackBehavior::ToNever is a bit too long and annoying IMO, but sure. ContextDependent sounds good.

Should I rename things in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd like that, given that these are style nits I don't consider them to be blockin though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@lcnr
Copy link
Contributor

lcnr commented May 9, 2024

from a types team perspective, this change seems straightforward. While I am somewhat concerned that linting all affected cases may prove difficult, that's the responsibility of t-lang :p so I am just going to trust you with that.

Add a lint against fallback affecting a generic that is passed to an unsafe function.

I could imagine that there exist (hopefully internal) functions which trigger this pattern which are not marked as unsafe even though they can cause unsoundness when used incorrectly. I expect that lint to therefore not catch all places where this inference change causes unsoundness. I think that this is acceptable. I would like us to explicitly mention this possibility in the edition guide for this change.

@rfcbot reviewed

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 10, 2024 via email

@jackh726
Copy link
Member

I agree with @lcnr here that linting may prove difficult for at least some set of cases. It's a bit interesting because while it is T-lang's call on where we must lint, there may be some set of cases where we can lint but only with invasive type system changes (e.g. for some kind of tracking) and that would be T-types.

I overall am happy to have this land, T-lang should be mindful of the potential challenges to linting and that it may make sense to delay this until we have the lints written first.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 15, 2024
@rfcbot
Copy link

rfcbot commented May 15, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 25, 2024
@rfcbot
Copy link

rfcbot commented May 25, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition 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. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet