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

never_patterns: Count ! bindings as diverging #120104

Merged
merged 5 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions compiler/rustc_hir_typeck/src/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ use std::cell::RefCell;

use crate::coercion::CoerceMany;
use crate::gather_locals::GatherLocalsVisitor;
use crate::CoroutineTypes;
use crate::FnCtxt;
use crate::{CoroutineTypes, Diverges, FnCtxt};
use rustc_hir as hir;
use rustc_hir::def::DefKind;
use rustc_hir::intravisit::Visitor;
Expand Down Expand Up @@ -76,6 +75,12 @@ pub(super) fn check_fn<'a, 'tcx>(
let ty: Option<&hir::Ty<'_>> = try { inputs_hir?.get(idx)? };
let ty_span = ty.map(|ty| ty.span);
fcx.check_pat_top(param.pat, param_ty, ty_span, None, None);
if param.pat.is_never_pattern() {
fcx.function_diverges_because_of_empty_arguments.set(Diverges::Always {
span: param.pat.span,
custom_note: Some("any code following a never pattern is unreachable"),
});
}

// Check that argument is Sized.
if !params_can_be_unsized {
Expand Down Expand Up @@ -105,6 +110,7 @@ pub(super) fn check_fn<'a, 'tcx>(
hir::FnRetTy::Return(ty) => ty.span,
};
fcx.require_type_is_sized(declared_ret_ty, return_or_body_span, traits::SizedReturnType);
fcx.is_whole_body.set(true);
fcx.check_return_expr(body.value, false);

// Finalize the return check by taking the LUB of the return types
Expand Down
15 changes: 11 additions & 4 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// without the final expr (e.g. `try { return; }`). We don't want to generate an
// unreachable_code lint for it since warnings for autogenerated code are confusing.
let is_try_block_generated_unit_expr = match expr.kind {
ExprKind::Call(_, args) if expr.span.is_desugaring(DesugaringKind::TryBlock) => {
args.len() == 1 && args[0].span.is_desugaring(DesugaringKind::TryBlock)
ExprKind::Call(_, [arg]) => {
expr.span.is_desugaring(DesugaringKind::TryBlock)
&& arg.span.is_desugaring(DesugaringKind::TryBlock)
}

_ => false,
};

Expand All @@ -220,9 +220,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.warn_if_unreachable(expr.hir_id, expr.span, "expression");
}

// Hide the outer diverging and has_errors flags.
// Whether a past expression diverges doesn't affect typechecking of this expression, so we
Copy link
Member

Choose a reason for hiding this comment

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

Ok, so I understand what this is doing, but I'm not certain if I understand why it's doing this. Is this to make some test pass?

Copy link
Member Author

Choose a reason for hiding this comment

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

The old_diverges bit you mean? That's because if an expression diverges then it typechecks as !. So before we check a new expression we hide the previous state of diverges. Else in the following 1 would be given type !.

panic!();
let x = 1;

Copy link
Member

Choose a reason for hiding this comment

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

No, why you added this is_whole_body thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. Well the presence of ! in the arguments should affect the type of the function body, but not the type of any other expression in the function. So I only look at function_diverges_because_of_empty_arguments if expr is the function body

Copy link
Member Author

@Nadrieril Nadrieril Jan 18, 2024

Choose a reason for hiding this comment

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

The obvious thing to do would have been to set fcx.diverges directly while checking the function arguments, but that resulted in:

fn never_arg(!: Void) -> u32 {}
                             ^^ unreachable code

as well as a type error because the mechanism I explained above ensures that the type of the {} was not affected by previous diverges so the body is typechecked as () x)

Copy link
Member Author

@Nadrieril Nadrieril Jan 18, 2024

Choose a reason for hiding this comment

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

I guess I could use diverges directly instead of adding function_diverges_because_of_empty_arguments 🤔 . But I still need is_whole_body

Copy link
Member

Choose a reason for hiding this comment

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

I understand why this is implemented the way it is, though I'm still chewing on whether there's a better approach here. My main concern is the non-locality -- it relies on the fact that when type checking the top expression of the body, we'll always flow through check_expr_with_expectation_and_args first. While I believe typeck will remain in that way, it seems somewhat easy for a refactoring of control flow to make this no longer true always.

Ideally, we'd treat the initial check_expr call for the top expr of a body in a privileged way, and do this logic there, but currently all of that is tangled into the coercion code that we use to check the body as if it were an implicit return value. Sigh.

Copy link
Member

Choose a reason for hiding this comment

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

As an aside, could you please double check that this still behaves the right way for async fn?

async fn(!: Void) { body... }

implicitly desugars to:

fn(arg1: Void) -> ... {
  async { 
    let ! = arg1;
    body...
  }
}

so it should behave correctly afaict, but I think we should have tests for 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.

Yeah, I'm not a fan of the non-locality and the mutability of my implementation but I felt way out of my depth to refactor anything.

Good catch on the async fn, it in fact does not work. I'm surprised because I looked at all the call sites of check_pat_top and they all do something with never patterns except expression-lets

Copy link
Member Author

@Nadrieril Nadrieril Jan 18, 2024

Choose a reason for hiding this comment

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

Weird, it does desugar to what you said, but doesn't seem to detect the divergence

// reset `diverges` while checking `expr`.
let old_diverges = self.diverges.replace(Diverges::Maybe);

if self.is_whole_body.replace(false) {
// If this expression is the whole body and the function diverges because of its
// arguments, we check this here to ensure the body is considered to diverge.
self.diverges.set(self.function_diverges_because_of_empty_arguments.get())
};

let ty = ensure_sufficient_stack(|| match &expr.kind {
hir::ExprKind::Path(
qpath @ (hir::QPath::Resolved(..) | hir::QPath::TypeRelative(..)),
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1471,6 +1471,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Type check a `let` statement.
pub fn check_decl_local(&self, local: &'tcx hir::Local<'tcx>) {
self.check_decl(local.into());
if local.pat.is_never_pattern() {
self.diverges.set(Diverges::Always {
span: local.pat.span,
custom_note: Some("any code following a never pattern is unreachable"),
});
}
}

pub fn check_stmt(&self, stmt: &'tcx hir::Stmt<'tcx>) {
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,13 @@ pub struct FnCtxt<'a, 'tcx> {
/// the diverges flag is set to something other than `Maybe`.
pub(super) diverges: Cell<Diverges>,

/// If one of the function arguments is a never pattern, this counts as diverging code. This
/// affect typechecking of the function body.
pub(super) function_diverges_because_of_empty_arguments: Cell<Diverges>,

/// Whether the currently checked node is the whole body of the function.
pub(super) is_whole_body: Cell<bool>,

pub(super) enclosing_breakables: RefCell<EnclosingBreakables<'tcx>>,

pub(super) inh: &'a Inherited<'tcx>,
Expand All @@ -124,6 +131,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ret_coercion_span: Cell::new(None),
coroutine_types: None,
diverges: Cell::new(Diverges::Maybe),
function_diverges_because_of_empty_arguments: Cell::new(Diverges::Maybe),
is_whole_body: Cell::new(false),
enclosing_breakables: RefCell::new(EnclosingBreakables {
stack: Vec::new(),
by_id: Default::default(),
Expand Down
16 changes: 16 additions & 0 deletions tests/ui/rfcs/rfc-0000-never_patterns/120240-async-fn-never-arg.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// edition: 2018
// known-bug: #120240
#![feature(never_patterns)]
#![allow(incomplete_features)]

fn main() {}

enum Void {}

// Divergence is not detected.
async fn async_never(!: Void) -> ! {} // gives an error

// Divergence is detected
async fn async_let(x: Void) -> ! {
let ! = x;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0308]: mismatched types
--> $DIR/120240-async-fn-never-arg.rs:11:36
|
LL | async fn async_never(!: Void) -> ! {} // gives an error
| ^^ expected `!`, found `()`
|
= note: expected type `!`
found unit type `()`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0308`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#![feature(never_patterns)]
#![allow(incomplete_features)]
#![deny(unreachable_patterns)]
#![deny(unreachable_code)]

fn main() {}

enum Void {}

fn never_arg(!: Void) -> u32 {
println!();
//~^ ERROR unreachable statement
}

fn ref_never_arg(&!: &Void) -> u32 {
println!();
//~^ ERROR unreachable statement
}

fn never_let() -> u32 {
let ptr: *const Void = std::ptr::null();
unsafe {
let ! = *ptr;
}
println!();
//~^ ERROR unreachable statement
}

fn never_match() -> u32 {
let ptr: *const Void = std::ptr::null();
unsafe {
match *ptr { ! };
}
println!();
//~^ ERROR unreachable statement
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
error: unreachable statement
--> $DIR/diverge-causes-unreachable-code.rs:11:5
|
LL | fn never_arg(!: Void) -> u32 {
| - any code following a never pattern is unreachable
LL | println!();
| ^^^^^^^^^^ unreachable statement
|
note: the lint level is defined here
--> $DIR/diverge-causes-unreachable-code.rs:4:9
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
= note: this error originates in the macro `$crate::print` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

error: unreachable statement
--> $DIR/diverge-causes-unreachable-code.rs:16:5
|
LL | fn ref_never_arg(&!: &Void) -> u32 {
| -- any code following a never pattern is unreachable
LL | println!();
| ^^^^^^^^^^ unreachable statement
|
= note: this error originates in the macro `$crate::print` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

error: unreachable statement
--> $DIR/diverge-causes-unreachable-code.rs:25:5
|
LL | let ! = *ptr;
| - any code following a never pattern is unreachable
LL | }
LL | println!();
| ^^^^^^^^^^ unreachable statement
|
= note: this error originates in the macro `$crate::print` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

error: unreachable statement
--> $DIR/diverge-causes-unreachable-code.rs:34:5
|
LL | match *ptr { ! };
| ---------------- any code following this `match` expression is unreachable, as all arms diverge
LL | }
LL | println!();
| ^^^^^^^^^^ unreachable statement
|
= note: this error originates in the macro `$crate::print` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 4 previous errors

56 changes: 56 additions & 0 deletions tests/ui/rfcs/rfc-0000-never_patterns/diverges-not.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#![feature(never_patterns)]
#![feature(let_chains)]
#![allow(incomplete_features)]
#![deny(unreachable_patterns)]

fn main() {}

enum Void {}

// Contrast with `./diverges.rs`: merely having an empty type around isn't enough to diverge.

fn wild_void(_: Void) -> u32 {}
//~^ ERROR: mismatched types

fn wild_let() -> u32 {
let ptr: *const Void = std::ptr::null();
unsafe {
//~^ ERROR: mismatched types
let _ = *ptr;
}
}

fn wild_match() -> u32 {
let ptr: *const Void = std::ptr::null();
unsafe {
match *ptr {
_ => {} //~ ERROR: mismatched types
}
}
}

fn binding_void(_x: Void) -> u32 {}
//~^ ERROR: mismatched types

fn binding_let() -> u32 {
let ptr: *const Void = std::ptr::null();
unsafe {
//~^ ERROR: mismatched types
let _x = *ptr;
}
}

fn binding_match() -> u32 {
let ptr: *const Void = std::ptr::null();
unsafe {
match *ptr {
_x => {} //~ ERROR: mismatched types
}
}
}

// Don't confuse this with a `let !` statement.
fn let_chain(x: Void) -> u32 {
if let true = true && let ! = x {}
//~^ ERROR: mismatched types
}
55 changes: 55 additions & 0 deletions tests/ui/rfcs/rfc-0000-never_patterns/diverges-not.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
error[E0308]: mismatched types
--> $DIR/diverges-not.rs:12:26
|
LL | fn wild_void(_: Void) -> u32 {}
| --------- ^^^ expected `u32`, found `()`
| |
| implicitly returns `()` as its body has no tail or `return` expression

error[E0308]: mismatched types
--> $DIR/diverges-not.rs:17:5
|
LL | / unsafe {
LL | |
LL | | let _ = *ptr;
LL | | }
| |_____^ expected `u32`, found `()`

error[E0308]: mismatched types
--> $DIR/diverges-not.rs:27:18
|
LL | _ => {}
| ^^ expected `u32`, found `()`

error[E0308]: mismatched types
--> $DIR/diverges-not.rs:32:30
|
LL | fn binding_void(_x: Void) -> u32 {}
| ------------ ^^^ expected `u32`, found `()`
| |
| implicitly returns `()` as its body has no tail or `return` expression

error[E0308]: mismatched types
--> $DIR/diverges-not.rs:37:5
|
LL | / unsafe {
LL | |
LL | | let _x = *ptr;
LL | | }
| |_____^ expected `u32`, found `()`

error[E0308]: mismatched types
--> $DIR/diverges-not.rs:47:19
|
LL | _x => {}
| ^^ expected `u32`, found `()`

error[E0308]: mismatched types
--> $DIR/diverges-not.rs:54:37
|
LL | if let true = true && let ! = x {}
| ^^ expected `u32`, found `()`

error: aborting due to 7 previous errors

For more information about this error, try `rustc --explain E0308`.
38 changes: 38 additions & 0 deletions tests/ui/rfcs/rfc-0000-never_patterns/diverges.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// check-pass
// edition: 2018
#![feature(never_patterns)]
#![allow(incomplete_features)]
#![deny(unreachable_patterns)]

fn main() {}

enum Void {}

// A never pattern alone diverges.

fn never_arg(!: Void) -> ! {}

fn never_arg_returns_anything<T>(!: Void) -> T {}

fn ref_never_arg(&!: &Void) -> ! {}

fn never_let() -> ! {
let ptr: *const Void = std::ptr::null();
unsafe {
let ! = *ptr;
}
}

fn never_match() -> ! {
let ptr: *const Void = std::ptr::null();
unsafe {
match *ptr { ! };
}
// Ensures this typechecks because of divergence and not the type of the match expression.
println!();
}

// Note: divergence is not detected for async fns when the `!` is in the argument (#120240).
async fn async_let(x: Void) -> ! {
let ! = x;
}
Loading