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

Implement ? in catch expressions #40229

Merged
merged 2 commits into from
Mar 21, 2017
Merged

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Mar 3, 2017

Builds on #39921. Final part of #39849.

r? @nikomatsakis

@cramertj cramertj changed the title Break to blocks Implement ? in catch expressions Mar 3, 2017
@bors
Copy link
Contributor

bors commented Mar 3, 2017

☔ The latest upstream changes (presumably #40133) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

at first read, this all looks good, I'll have to revisit it in more detail when it's rebased

@nikomatsakis
Copy link
Contributor

Ah, a thought. It'd be good to have some tests targeting the CFG etc. The idea would be to have initializations and moves that occur during a catch and check that we track it properly. For example:

let x;
let _v = do catch {
    x = 5;
    Ok(());
};
println!("{}", x); // OK
let x;
let _v = do catch {
    x = Ok(5)?;
    Ok(());
};
println!("{}", x); // ERROR

and similar tests for moves that may or may not occur (and perhaps a few for borrows that start in the catch and extend outside, etc).

@bors
Copy link
Contributor

bors commented Mar 12, 2017

☔ The latest upstream changes (presumably #40220) made this pull request unmergeable. Please resolve the merge conflicts.

// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(catch_expr)]
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 add a comment on this test explaining what the test aims to demonstrate (e.g., that a borrowed valid inside the do catch will propagate out?

I also think it'd be good to possibly break this test into distinct files.

let _: Result<(), ()> = do catch {
Err(())?;
cfg_res = 5;
Ok(())
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 modify the test to demonstrate that cfg_res is usable *within the do catch, if not outside?

Also, add a (run-pass) test like so, where cfg_res is initialized before the ?:

pub fn main() {
    let cfg_res;
    let _: Result<(), ()> = do catch {
        cfg_res = 5;
        Err(())?;
        Ok(())
    };
    use(cfg_res); // OK
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would also like some tests around lifetimes like these:

// Test that a borrow which only conditionally occurs still freezes `i` 

fn main() {
    let mut i = 222;
    let j;
    let x = do catch {
        Err(())?;
        Ok(&i)
    };
    i = 0; // ERROR
    println!("{}", i);
}
// Test that a borrow which only conditionally occurs still freezes `i` 

fn main() {
    let mut i = 222;
    let j;
    let x = do catch {
        Err(())?;
        j = &i;
        Ok(())
    };
    i = 0; // ERROR, I ... guess ... maybe not with NLL
    println!("{}", i);
}

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2017

📌 Commit bc07fe6 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 17, 2017

☔ The latest upstream changes (presumably #40598) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 20, 2017

📌 Commit 1f43731 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 21, 2017
…sakis

Implement `?` in catch expressions

Builds on rust-lang#39921. Final part of rust-lang#39849.

r? @nikomatsakis
bors added a commit that referenced this pull request Mar 21, 2017
Rollup of 10 pull requests

- Successful merges: #40229, #40312, #40332, #40502, #40556, #40576, #40667, #40671, #40681, #40685
- Failed merges:
@bors bors merged commit 1f43731 into rust-lang:master Mar 21, 2017
@bors
Copy link
Contributor

bors commented Mar 21, 2017

⌛ Testing commit 1f43731 with merge 58c701f...

@cramertj cramertj deleted the break-to-blocks branch March 22, 2017 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants