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

Unreachable code is not borrow-checked #91377

Closed
SNCPlay42 opened this issue Nov 29, 2021 · 6 comments
Closed

Unreachable code is not borrow-checked #91377

SNCPlay42 opened this issue Nov 29, 2021 · 6 comments
Labels
A-borrow-checker Area: The borrow checker C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@SNCPlay42
Copy link
Contributor

I tried this code:

fn main() {
    let a = 1;
    
    loop {}
        
    let z = &a;
    let x = &mut a;
    let y = &mut a;
    
    *x = 2;
    *y = 3;
    
    println!("{}", *z);
}

I expected this to fail to compile, since I create one immutable and two mutable borrows of an immutable variable that all have to be alive at once.

It's not unsound to accept this, since none of it is even codegened, but unreachable code is otherwise held to the same standards as reachable code (must type-check, etc.).

Instead, this code is accepted with only warnings:

warning: unreachable statement
 --> src/lib.rs:6:5
  |
4 |     loop {}
  |     ------- any code following this expression is unreachable
5 |         
6 |     let z = &a;
  |     ^^^^^^^^^^^ unreachable statement
  |
  = note: `#[warn(unreachable_code)]` on by default

warning: unused variable: `a`
 --> src/lib.rs:2:9
  |
2 |     let a = 1;
  |         ^ help: if this is intentional, prefix it with an underscore: `_a`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: `playground` (lib) generated 2 warnings

On Rust 1.35 and before, this code was rejected in the 2015 edition (which used the old lexical borrowck).

@rustbot label regression-from-stable-to-stable

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 29, 2021
@camelid
Copy link
Member

camelid commented Nov 30, 2021

I'm not sure if this is really a regression since I'm pretty sure it's expected behavior. Borrow-checking is now a flow-based analysis, so unreachable code can't be borrow-checked I think.

You can also use uninitialized variables in unreachable code, since initialization-checking is also a flow-based analysis (playground):

pub fn foo() -> i32 {
    loop {}

    let x;
    x
}

cc #79735, a similar issue but about the unsafety-checker

@camelid camelid added A-borrow-checker Area: The borrow checker T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 30, 2021
@SNCPlay42
Copy link
Contributor Author

You can also use uninitialized variables in unreachable code, since initialization-checking is also a flow-based analysis

Is this closely related to the borrowck code? I notice that whether this is accepted also depends on whether the edition+rustc version uses NLL borrowck or not.

@inquisitivecrystal
Copy link
Contributor

I think this is expected behavior, per nikomatsakis. I'm sorry I don't have a more canonical source. It does seem to be implied by the NLL RFC, but that requires a fair bit of reading between the lines.

@SNCPlay42
Copy link
Contributor Author

Hmm. Actually, in terms of initialization-checking the code in my original post, I notice that it is either

  1. invalid since there is no control flow path from where a is initialized to where the borrows use it, or
  2. valid since those borrows that use a are never reached

And 2 does seem like the less surprising option.

@apiraino
Copy link
Contributor

apiraino commented Dec 2, 2021

Adding T-lang nomination to get this issue in their agenda.

Removing prioritization and regression labels as per WG-prioritization discussion on Zulip

@rustbot label -I-prioritize +I-lang-nominated -regression-from-stable-to-stable

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. and removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 2, 2021
@apiraino apiraino added the C-discussion Category: Discussion or questions that doesn't represent real issues. label Dec 2, 2021
@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 14, 2021
@nikomatsakis
Copy link
Contributor

Hi all! When we were implementing NLL, we couldn't figure out what to do about errors in unreachable code, since it's not clear what things are initialized, which borrows ought to be in scope, etc, and so we decided to accept programs like this (for better or worse). Going to close this as "working as designed" -- if we were going to make changes here, I expect what we could do is talk about some kind of lint, to avoid breaking working code, but it doesn't seem like a high priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants