-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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
base: master
Are you sure you want to change the base?
Conversation
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:
|
!
fallback to !
!
fall back to !
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?
}
} |
@rfcbot merge This PR changes fallback to
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. |
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. |
@crlf0710 do those tests look good to you? |
I'm happy with doing this, contingent on having mitigations to avoid unsafe code changing behavior. @rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
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. |
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 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 |
@rfcbot fcp merge context: 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 |
Can't do another fcp on top of a preexisting one I think |
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 |
@rfcbot fcp cancel |
@compiler-errors proposal cancelled. |
@rfcbot merge |
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. |
|
||
// `feature(never_type_fallback)`: fallback to `!` or `()` trying to not break stuff | ||
if tcx.features().never_type_fallback { | ||
return FallbackToNiko; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
I could imagine that there exist (hopefully internal) functions which trigger this pattern which are not marked as @rfcbot reviewed |
+1 to the release (and edition) notes talking about this possibility
…On Thu, May 9, 2024, at 7:32 PM, lcnr wrote:
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 triggers this pattern which ar enot marked as `unsafe` even though 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 <https://github.com/rfcbot> reviewed
—
Reply to this email directly, view it on GitHub <#123508 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZTZHCPARB3WIDDNS5LZBOXLZAVCNFSM6AAAAABFZR53G6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBTGEYDMNBXGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
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. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
779b684
to
e79aafc
Compare
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. |
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