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

Conversation

Nadrieril
Copy link
Member

A binding that is a never pattern is not reachable, hence counts as diverging code. This allows in particular fn foo(!: Void) -> SomeType {} to typecheck.

r? @compiler-errors

@Nadrieril Nadrieril added the F-never_patterns `#![feature(never_patterns)]` label Jan 18, 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 Jan 18, 2024
@rust-log-analyzer

This comment has been minimized.

@Nadrieril Nadrieril force-pushed the never-pat-diverges branch 2 times, most recently from 083eedc to 8f76b9c Compare January 18, 2024 19:05
@@ -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

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after fixing up the async test commit

tests/ui/rfcs/rfc-0000-never_patterns/diverges.rs Outdated Show resolved Hide resolved
@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 Jan 21, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 22, 2024

📌 Commit 3ff1024 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2024
@Nadrieril
Copy link
Member Author

Ty!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…mpiler-errors

never_patterns: Count `!` bindings as diverging

A binding that is a never pattern is not reachable, hence counts as diverging code. This allows in particular `fn foo(!: Void) -> SomeType {}` to typecheck.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119664 (Fix tty detection for msys2's `/dev/ptmx`)
 - rust-lang#120104 (never_patterns: Count `!` bindings as diverging)
 - rust-lang#120109 (Move cmath into `sys`)
 - rust-lang#120143 (Consolidate logic around resolving built-in coroutine trait impls)
 - rust-lang#120159 (Track `verbose` and `verbose_internals`)
 - rust-lang#120188 (compiler: update freebsd and netbsd base specs.)
 - rust-lang#120216 (Fix a `trimmed_def_paths` assertion failure.)
 - rust-lang#120220 (Document `Token{Stream,Tree}::Display` more thoroughly.)
 - rust-lang#120233 (Revert stabilization of trait_upcasting feature)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119664 (Fix tty detection for msys2's `/dev/ptmx`)
 - rust-lang#120104 (never_patterns: Count `!` bindings as diverging)
 - rust-lang#120109 (Move cmath into `sys`)
 - rust-lang#120143 (Consolidate logic around resolving built-in coroutine trait impls)
 - rust-lang#120159 (Track `verbose` and `verbose_internals`)
 - rust-lang#120216 (Fix a `trimmed_def_paths` assertion failure.)
 - rust-lang#120220 (Document `Token{Stream,Tree}::Display` more thoroughly.)
 - rust-lang#120233 (Revert stabilization of trait_upcasting feature)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 042cc72 into rust-lang:master Jan 23, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2024
Rollup merge of rust-lang#120104 - Nadrieril:never-pat-diverges, r=compiler-errors

never_patterns: Count `!` bindings as diverging

A binding that is a never pattern is not reachable, hence counts as diverging code. This allows in particular `fn foo(!: Void) -> SomeType {}` to typecheck.

r? ``@compiler-errors``
@Nadrieril Nadrieril deleted the never-pat-diverges branch January 23, 2024 00:29
DianQK pushed a commit to DianQK/rust that referenced this pull request Jan 23, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119664 (Fix tty detection for msys2's `/dev/ptmx`)
 - rust-lang#120104 (never_patterns: Count `!` bindings as diverging)
 - rust-lang#120109 (Move cmath into `sys`)
 - rust-lang#120143 (Consolidate logic around resolving built-in coroutine trait impls)
 - rust-lang#120159 (Track `verbose` and `verbose_internals`)
 - rust-lang#120216 (Fix a `trimmed_def_paths` assertion failure.)
 - rust-lang#120220 (Document `Token{Stream,Tree}::Display` more thoroughly.)
 - rust-lang#120233 (Revert stabilization of trait_upcasting feature)

r? `@ghost`
`@rustbot` modify labels: rollup
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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