Skip to content

never_patterns: lower a never arm to an unreachable terminator#120758

Closed
Nadrieril wants to merge 6 commits intorust-lang:masterfrom
Nadrieril:lower-never-pats2
Closed

never_patterns: lower a never arm to an unreachable terminator#120758
Nadrieril wants to merge 6 commits intorust-lang:masterfrom
Nadrieril:lower-never-pats2

Conversation

@Nadrieril
Copy link
Member

This introduces ExprKind::Unreachable (in hir and thir), which is lowered to an unreachable terminator in mir. For the non-trivial cases (mir lowering and type checking), I modeled it after the case of a match expression without arms, and the result looks sensible.

I then use this to lower never arms. This doesn't change the final optimized mir because the UninhabitedEnumBranching pass would detect the empty variant anyway, but this way is cleaner.

r? @compiler-errors

@Nadrieril Nadrieril added the F-never_patterns `#![feature(never_patterns)]` label Feb 7, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2024

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

@rust-log-analyzer

This comment has been minimized.

@Nadrieril
Copy link
Member Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2024

enum Void {}

// EMIT_MIR never_patterns.opt1.UninhabitedEnumBranching.before.mir
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a MIR building test, outputting the just-built mir ?

Copy link
Member Author

@Nadrieril Nadrieril Feb 8, 2024

Choose a reason for hiding this comment

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

Yes! How may I do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it just EMIT_MIR never_patterns.opt1.built.after.mir?

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@Nadrieril
Copy link
Member Author

I'm a bit disappointed at the match x { ! } diagnostic change but I don't know how to avoid it

Struct,
Gen,
Await,
Unreachable,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just give ExprKind::Unreachable a dummy precedence? Like, just give it the same precedence as Lit or something.

}

// We add a fake `loop {}` arm body so that it typecks to `!`.
// FIXME(never_patterns): Desugar into a call to `unreachable_unchecked`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Your PR description doesn't explain the challenges of just lowering a call to, for example, the unreachable intrinsic. I feel like that's far less invasive than adding a new HIR expr kind.

Copy link
Member Author

@Nadrieril Nadrieril Feb 8, 2024

Choose a reason for hiding this comment

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

Ah yes, we discussed this on Zulip. Intrinsics don't exist at the hir level, they have to be constructed during mir lowering/codegen, so that's essentially what I'm doing.

Alternatives would be to make unreachable_unchecked a lang item, or keep the loop and rely on the UninhabitedEnumBranching mir opt to elide it. I went with Unreachable because that's what everyone seemed to prefer; I don't have a strong opinion.

@Nadrieril
Copy link
Member Author

Wait I just realized something: when lowering Ok(x) | Err(!), I'll have to manage the never pattern so that it leads to an Unreachable terminator. So I can just do that for all never patterns, which means I can leave my dummy loop in hir... Closing this, I might not need it after all 😅

@Nadrieril Nadrieril closed this Feb 8, 2024
@Nadrieril Nadrieril deleted the lower-never-pats2 branch July 20, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-never_patterns `#![feature(never_patterns)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants