Add ok-wrapping to catch blocks, per RFC #49371

scottmcm · Mar 26, 2018

Updates the catch{} lowering to wrap the result in Try::from_ok.

r? @nikomatsakis

Fixes #41414
Fixes #43818

nikomatsakis · Apr 2, 2018

nikomatsakis reviewed Apr 2, 2018

View changes

nikomatsakis · Apr 2, 2018

src/librustc/hir/lowering.rs

+                            id: block.id, // FIXME!
+                            span: block.span, // FIXME?
+                            node: hir::ExprTup(hir_vec![]),
+                            attrs: ThinVec::new(), // FIXME?


this seems ok; this has to do with #[foo] attributes

nikomatsakis · Apr 2, 2018

src/librustc/hir/lowering.rs

+                    let mut block = this.lower_block(body, true).into_inner();
+                    let tail = block.expr.take().map_or_else(
+                        || hir::Expr {
+                            id: block.id, // FIXME!


I think you want to call this.next_id() here

nikomatsakis · Apr 2, 2018

src/librustc/hir/lowering.rs

+                    let tail = block.expr.take().map_or_else(
+                        || hir::Expr {
+                            id: block.id, // FIXME!
+                            span: block.span, // FIXME?


this would be where we report errors regarding this () expression; the block seems ok, but we might want to .. just use the closing brace }?

we probably also want to give it the "desugaring" span

nikomatsakis · Apr 2, 2018

src/librustc_mir/borrow_check/nll/mod.rs

-    let _: io::Result<()> = do catch {
+    // `do catch` changed to IIFE for bootstrapping; consider changing back
+    // when we get a new stage0 compiler
+    let _: io::Result<()> = (||{


How about we make a macro for this instead?

#[cfg(stage0)] macro_rules do_catch { ($t:expr) = { (|| $t)() } } #[cfg(not(stage0))] macro_rules do_catch { ($t:expr) = { do catch { $t } } }

And this way it can have ok-wrapping! 👍

nikomatsakis · Apr 2, 2018

src/librustc_mir/util/pretty.rs

-    let _: io::Result<()> = do catch {
+    // `do catch` changed to IIFE for bootstrapping; consider changing back
+    // when we get a new stage0 compiler
+    let _: io::Result<()> = (||{


nikomatsakis · Apr 2, 2018

src/librustc_mir/util/pretty.rs

-        let _: io::Result<()> = do catch {
+        // `do catch` changed to IIFE for bootstrapping; consider changing
+        // back when we get a new stage0 compiler
+        let _: io::Result<()> = (||{


nikomatsakis · Apr 2, 2018

first round of comments =) sorry for the delay. Cursed All Hands!

scottmcm · Apr 3, 2018

Ok, it looks like this is working now. Thanks for the pointers!

I added a UI test so you can see how the spans come out in type errors; if you would like them to be different please let me know how to do that -- this is my first time in this end of the compiler 🙂

TimNN · Apr 3, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:00:50] configure: rust.quiet-tests     := True
---
[00:40:47] .........................................................................i..........................
[00:40:53] ................i...................................................................................
---
[00:41:28] ............................................................................................i.......
[00:41:35] ................................................................i...................................
---
[00:42:30] .............................................i......................................................
---
[00:46:24] .............................i......................................................................
[00:46:39] ..............................................................i.....................................
[00:46:54] ...............................................i....................................................
[00:47:15] ....................................................................................................
[00:47:37] ....................................................................................................
[00:47:58] ....................................................................................................
[00:48:24] .i................................................................................................i.
[00:48:50] .................................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:49:00] ...................
[00:49:31] ....................................................................................................
[00:50:07] ...............................................................ii...................................
[00:50:53] ..........................i....................................................i.ii....test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:50:57] .............
[00:51:38] .......................................................................................iiiiiii......
---
[00:53:35] ....................................i...............................................................
[00:53:43] ....................................................................................................
[00:53:51] ..................i............................................................ii.iii...............
[00:53:58] ....................................................................................................
[00:54:06] ........i..............................i............................................................
[00:54:13] ....................................................................................................
[00:54:20] .....................i..............................................................................
[00:54:29] ....................................................................................................
[00:54:39] ....................................................................................................
[00:54:49] ....................................................................................................
[00:55:00] ....................................................................................................
[00:55:14] ....................................................................................................
[00:55:23] ..............i.....................................................................................
[00:55:33] .................i..ii..............................................................................
[00:55:42] ....................................................................................................
[00:55:53] ....................................................................................................
[00:56:03] ....................................................................................i...............
[00:56:13] ..............................i.....................................................................
---
[00:56:51] ............................i.......................................................................
[00:56:52] ....................................................................i...............................
[00:56:53] .................i......................................................
---
[00:57:08] ...........i........................
---
[00:57:38] i...i..ii....i.............ii........iii......i..i...i...ii..i..i..ii.....
---
[00:57:41] i.......i......................i......
---
[00:58:19] iiii.......i..i........i..i.i.............i..........iiii...........i...i..........ii.i.i.......ii..
[00:58:20] ....ii...
---
[01:07:39] ...i................................................................................................
---
[01:09:29] ......................................i.............................................................
[01:09:48] ....................................................................................................
[01:10:08] .............................................i......................................................
---
[01:11:40] ........................................................ii..........................................
---
[01:12:45] .............................................................i......................................
---
[01:17:34] ii..................................................................................................
[01:17:53] ....................................................................................................
[01:18:09] ...................iii......i......i...i......i.....................................................
[01:18:19] ....................................................................................................
[01:18:34] ........................................iiii........ii..............................................
[01:18:45] ....................................................................................................
[01:19:02] .......................................................................................i............
---
[01:28:26] command did not execute successfully: "/checkout/obj/build/bootstrap/debug/rustdoc" "--test" "/checkout/src/doc/unstable-book/src/language-features/catch-expr.md" "--test-args" ""
---
[01:28:26] ---- /checkout/src/doc/unstable-book/src/language-features/catch-expr.md - catch_expr (line 12) stdout ----
[01:28:26]  error[E0271]: type mismatch resolving `<std::result::Result<i32, std::num::ParseIntError> as std::ops::Try>::Ok == std::result::Result<i32, _>`
[01:28:26]   --> /checkout/src/doc/unstable-book/src/language-features/catch-expr.md:18:5
[01:28:26]    |
[01:28:26] 8  | /     Ok("1".parse::<i32>()?
[01:28:26] 9  | |         + "2".parse::<i32>()?
[01:28:26] 10 | |         + "3".parse::<i32>()?)
[01:28:26]    | |______________________________^ expected i32, found enum `std::result::Result`
[01:28:26]    |
[01:28:26]    = note: expected type `i32`
[01:28:26]               found type `std::result::Result<i32, _>`
[01:28:26]
[01:28:26] error[E0271]: type mismatch resolving `<std::result::Result<i32, std::num::ParseIntError> as std::ops::Try>::Ok == std::result::Result<i32, _>`
[01:28:26]   --> /checkout/src/doc/unstable-book/src/language-features/catch-expr.md:25:5
[01:28:26]    |
[01:28:26] 15 | /     Ok("1".parse::<i32>()?
[01:28:26] 16 | |         + "foo".parse::<i32>()?
[01:28:26] 17 | |         + "3".parse::<i32>()?)
[01:28:26]    | |______________________________^ expected i32, found enum `std::result::Result`
[01:28:26]    |
[01:28:26]    = note: expected type `i32`
[01:28:26]               found type `std::result::Result<i32, _>`
[01:28:26]
[01:28:26] thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:306:13
---
[01:28:26] make: *** [check] Error 1
[01:28:26] Makefile:58: recipe for target 'check' failed

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

nikomatsakis · Apr 4, 2018

nikomatsakis requested changes Apr 4, 2018

View changes

Left some thoughts about span

nikomatsakis · Apr 4, 2018

src/doc/unstable-book/src/language-features/catch-expr.md

@@ -15,16 +15,16 @@ expression creates a new scope one can use the `?` operator in.
 use std::num::ParseIntError;

 let result: Result<i32, ParseIntError> = do catch {
-    Ok("1".parse::<i32>()?
+    "1".parse::<i32>()?


nikomatsakis · Apr 4, 2018

src/test/run-pass/catch-expr.rs

    };
    assert_eq!(cfg_init_2, 6);

    let my_string = "test".to_string();
    let res: Result<&str, ()> = do catch {
-        Ok(&my_string)
+        // Unfortunately, deref doesn't fire here (#49356)
+        &my_string[..]


interesting.

Yeah, I filed #49356 about this. (Edit: Which I put in the comment; duh.) It feels to me like it ought to work, since the type needed here is completely determined by the context.

I can imagine why it fails, we might be able to improve it.

nikomatsakis · Apr 4, 2018

src/test/ui/catch-block-type-error.stderr

+LL | |         //~^ ERROR type mismatch
+LL | |         foo()?;
+LL | |     };
+   | |_____^ expected i32, found ()


Hmm, this is ungreat. Not sure what span would be best, maybe @estebank has thoughts. I think maybe just pointing to the end-brace would be good?:

error[E0271]: type mismatch resolving `<std::option::Option<i32> as std::ops::Try>::Ok == ()` --> $DIR/catch-block-type-error.rs:22:35 | LL | }; | ^ expected i32, found () | = note: expected type `i32` found type `()`

Also, the "type mismatch" line is kind of a mess, but that's separate I guess and maybe hard to fix -- we might be able to use the "extra info" from the span to help.

@scottmcm If you like how this looks I can give you a tip for how to achieve it =)

In general I think multi-line spans are to be avoided at basically any cost.

That seems plausible, though note that the multi-line span happens today for blocks:

error[E0308]: mismatched types --> src/main.rs:2:19 | 2 | let x: i32 = { | ___________________^ 3 | | println!("asdf"); 4 | | 3; 5 | | println!("asdf"); 6 | | }; | |_____^ expected i32, found () | = note: expected type `i32` found type `()`

I haven't looked into span futzing yet, so I don't know my unknowns. A few quick pointers would be great, or if you wanted to use me as a tester for a rustc book page...

note that the multi-line span happens today for blocks

yes, but that doesn't make it good =)

I haven't looked into span futzing yet, so I don't know my unknowns

I believe that the end_point function is what you want

Thanks! That did the trick. And huzzah for hosted compiler docs 🎉

nikomatsakis · Apr 10, 2018

nikomatsakis approved these changes Apr 10, 2018

View changes

very nice

nikomatsakis · Apr 10, 2018

@bors r+

bors · Apr 10, 2018

📌 Commit 9911de8 has been approved by nikomatsakis

bors · Apr 10, 2018

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

scottmcm · Apr 11, 2018

Rebased; please re-r+.

nikomatsakis · Apr 11, 2018

@bors r+

bors · Apr 11, 2018

📌 Commit 311ff5b has been approved by nikomatsakis

nikomatsakis · Apr 11, 2018

@bors delegate=scottmcm

(For future rebases)

bors · Apr 11, 2018

✌️ @scottmcm can now approve this pull request

bors · Apr 12, 2018

⌛️ Testing commit 311ff5b with merge 252a459...

bors · Apr 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 252a459 to master...

scottmcm assigned nikomatsakis Mar 26, 2018

pietroalbini added the S-waiting-on-review label Mar 26, 2018

kennytm added the E-needs-mentor label Mar 29, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from b9ea258 to a957c61 Apr 3, 2018

scottmcm changed the title ~~[Assistance Requested] Add ok-wrapping to catch blocks, per RFC~~ Add ok-wrapping to catch blocks, per RFC Apr 3, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from 7923aba to 316a16f Apr 3, 2018

rust-lang deleted a comment from TimNN Apr 3, 2018

scottmcm referenced this pull request Apr 4, 2018
Closed
Add size_hints for btree iterators #49550

scottmcm force-pushed the scottmcm:catch-wrapping branch from 0757abb to 9911de8 Apr 10, 2018

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 10, 2018

bors added S-waiting-on-author and removed S-waiting-on-bors labels Apr 10, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from 9911de8 to 311ff5b Apr 11, 2018

scottmcm added S-waiting-on-review and removed S-waiting-on-author labels Apr 11, 2018

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 11, 2018

bors merged commit 311ff5b into rust-lang:master Apr 12, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

scottmcm deleted the scottmcm:catch-wrapping branch Apr 12, 2018

jonhoo added a commit to mit-pdos/noria that referenced this pull request Apr 19, 2018

do catch now wraps in Ok automatically
See rust-lang/rust#49371

Unverified

The key that signed this is expired.

GPG key ID: D64AC9D67176DC71 Learn about signing commits

Loading status checks…

7ca16ec

joshtriplett referenced this pull request Apr 20, 2018
Merged
RFC: Reserve `try` for `try { .. }` block expressions #2388

rust-lang/rust

Join GitHub today

Add ok-wrapping to catch blocks, per RFC #49371

Conversation

scottmcm commented Mar 26, 2018 • edited

scottmcm assigned nikomatsakis Mar 26, 2018

pietroalbini added the S-waiting-on-review label Mar 26, 2018

kennytm added the E-needs-mentor label Mar 29, 2018

nikomatsakis reviewed Apr 2, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Apr 2, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from b9ea258 to a957c61 Apr 3, 2018

scottmcm changed the title [Assistance Requested] Add ok-wrapping to catch blocks, per RFC Add ok-wrapping to catch blocks, per RFC Apr 3, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from 7923aba to 316a16f Apr 3, 2018

This comment has been minimized.

scottmcm commented Apr 3, 2018 • edited

This comment has been minimized.

TimNN commented Apr 3, 2018

rust-lang deleted a comment from TimNN Apr 3, 2018

scottmcm referenced this pull request Apr 4, 2018

nikomatsakis requested changes Apr 4, 2018 View changes

nikomatsakis left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

scottmcm Apr 4, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

scottmcm force-pushed the scottmcm:catch-wrapping branch from 0757abb to 9911de8 Apr 10, 2018

nikomatsakis approved these changes Apr 10, 2018 View changes

nikomatsakis left a comment

This comment has been minimized.

nikomatsakis commented Apr 10, 2018

This comment has been minimized.

bors commented Apr 10, 2018

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 10, 2018

This comment has been minimized.

bors commented Apr 10, 2018

bors added S-waiting-on-author and removed S-waiting-on-bors labels Apr 10, 2018

scottmcm added some commits Mar 25, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from 9911de8 to 311ff5b Apr 11, 2018

This comment has been minimized.

scottmcm commented Apr 11, 2018

scottmcm added S-waiting-on-review and removed S-waiting-on-author labels Apr 11, 2018

This comment has been minimized.

nikomatsakis commented Apr 11, 2018

This comment has been minimized.

bors commented Apr 11, 2018

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 11, 2018

This comment has been minimized.

nikomatsakis commented Apr 11, 2018

This comment has been minimized.

bors commented Apr 11, 2018

This comment has been minimized.

bors commented Apr 12, 2018

bors added a commit that referenced this pull request Apr 12, 2018

This comment has been minimized.

bors commented Apr 12, 2018

Hide details View details bors merged commit 311ff5b into rust-lang:master Apr 12, 2018 2 checks passed

2 checks passed

scottmcm deleted the scottmcm:catch-wrapping branch Apr 12, 2018

jonhoo added a commit to mit-pdos/noria that referenced this pull request Apr 19, 2018

joshtriplett referenced this pull request Apr 20, 2018

scottmcm commented Mar 26, 2018 •

edited

nikomatsakis reviewed Apr 2, 2018

View changes

scottmcm force-pushed the scottmcm:catch-wrapping branch from `b9ea258` to `a957c61` Apr 3, 2018

scottmcm force-pushed the scottmcm:catch-wrapping branch from `7923aba` to `316a16f` Apr 3, 2018

scottmcm commented Apr 3, 2018 •

edited

nikomatsakis requested changes Apr 4, 2018

View changes

scottmcm Apr 4, 2018 •

edited

scottmcm force-pushed the scottmcm:catch-wrapping branch from `0757abb` to `9911de8` Apr 10, 2018

nikomatsakis approved these changes Apr 10, 2018

View changes

scottmcm force-pushed the scottmcm:catch-wrapping branch from `9911de8` to `311ff5b` Apr 11, 2018

bors merged commit `311ff5b` into rust-lang:master Apr 12, 2018
2 checks passed