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 try block expressions #52602

Merged
merged 10 commits into from
Aug 23, 2018
Merged

Implement try block expressions #52602

merged 10 commits into from
Aug 23, 2018

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Jul 22, 2018

I noticed that try wasn't a keyword yet in Rust 2018, so...

Fix​es #52604 That was fixed by PR #53135
cc #31436 #50412

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2018
@scottmcm scottmcm force-pushed the tryblock-expr branch 2 times, most recently from 6fe5f79 to 26c185a Compare July 22, 2018 04:41
@rust-highfive

This comment has been minimized.

@scottmcm scottmcm changed the title WIP: Implement try block expressions Implement try block expressions Jul 22, 2018
@scottmcm
Copy link
Member Author

scottmcm commented Jul 22, 2018

Ok, I think this is in good enough for a review pass. I've never touched most of these parts of the compiler before, so I suspect there are opportunities for improvement in how I did things 🙃

@@ -413,23 +413,30 @@ declare_keywords! {
(49, Virtual, "virtual")
(50, Yield, "yield")

// Edition-specific keywords currently in use.
(51, Try, "try") // >= 2018 Edition Only
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Keywords are not normally moved into the "used" category until the feature is stable (e.g. macro/yield/async are all "reserved for future use").

@rust-highfive

This comment has been minimized.

@@ -6,22 +6,24 @@ The tracking issue for this feature is: [#31436]

------------------------

The `catch_expr` feature adds support for a `catch` expression. The `catch`
expression creates a new scope one can use the `?` operator in.
The `catch_expr` feature adds support for `try` blocks. A `try`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably rename this to try_expr?

@michaelwoerister
Copy link
Member

r? @nikomatsakis for re-assignment. I don't think I'm the right person to review this.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Very nice!

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 24, 2018

📌 Commit ec85915c5ccf9ec1ba2b4183add0fe7efbbaa00c has been approved by nikomatsakis

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2018
@nikomatsakis
Copy link
Contributor

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 24, 2018
@nikomatsakis
Copy link
Contributor

One question: do we want to provide a kind of smoother path for people using do catch today? e.g., a nice error message?

@nikomatsakis
Copy link
Contributor

Or do we just not care :)

@Centril
Copy link
Contributor

Centril commented Jul 24, 2018

@nikomatsakis Here's a sourcegraph query for it, https://sourcegraph.com/search?q=repogroup:crates+case:yes++%5Cbdo%5Cs%2Bcatch%5Cb+max:400

Most of these occurrences are irrelevant and so I think we should not care.

@Centril
Copy link
Contributor

Centril commented Jul 24, 2018

Also; having the feature called catch_expr for a try { .. } block will be confusing, so let's change that bit :)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 23, 2018
@kennytm
Copy link
Member

kennytm commented Aug 23, 2018

@bors retry

An error occurred while generating the build script.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2018
@bors
Copy link
Contributor

bors commented Aug 23, 2018

⌛ Testing commit 0095471 with merge 127d02057977122d7ca0707c27d3ce2a4cbd66db...

@bors
Copy link
Contributor

bors commented Aug 23, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 23, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-aux of your PR failed on Travis (raw log). 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.
[01:31:26] 
[01:31:26] testing https://github.com/servo/servo
[01:31:26] Initialized empty Git repository in /checkout/obj/build/ct/servo/.git/
[01:31:26] fatal: Could not parse object '17e97b9320fdb7cdb33bbc5f4d0fde0653bbf2e4'.
[01:31:46] fatal: unable to access 'https://github.com/servo/servo/': Could not resolve host: github.com
[01:31:46] thread 'main' panicked at 'assertion failed: status.success()', tools/cargotest/main.rs:128:13
[01:31:46] 
[01:31:46] 
[01:31:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/cargotest" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build/ct"
[01:31:46] expected success, got: exit code: 101
[01:31:46] expected success, got: exit code: 101
[01:31:46] 
[01:31:46] 
[01:31:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/test/pretty src/test/run-pass/pretty src/test/run-fail/pretty src/test/run-pass-valgrind/pretty src/test/run-pass-fulldeps/pretty src/test/run-fail-fulldeps/pretty src/tools/cargo src/tools/cargotest
[01:31:46] Build completed unsuccessfully in 0:35:04
[01:31:46] make: *** [check-aux] Error 1
[01:31:46] Makefile:60: recipe for target 'check-aux' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1aac925a
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:156fb9f1:start=1535005949727505251,finish=1535005949733912231,duration=6406980
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0255de5b
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:00dd4bfc
travis_time:start:00dd4bfc
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0252de30
$ dmesg | grep -i kill

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. (Feature Requests)

@scottmcm
Copy link
Member Author

I'm cursed today:

[01:31:46] fatal: unable to access 'https://github.com/servo/servo/': Could not resolve host: github.com

https://api.travis-ci.org/v3/job/419489535/log.txt

@varkor
Copy link
Member

varkor commented Aug 23, 2018

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2018
@bors
Copy link
Contributor

bors commented Aug 23, 2018

⌛ Testing commit 0095471 with merge 35bf1ae...

bors added a commit that referenced this pull request Aug 23, 2018
Implement try block expressions

I noticed that `try` wasn't a keyword yet in Rust 2018, so...

~~Fix​es #52604 That was fixed by PR #53135
cc #31436 #50412
@bors
Copy link
Contributor

bors commented Aug 23, 2018

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

@bors bors merged commit 0095471 into rust-lang:master Aug 23, 2018
@scottmcm scottmcm deleted the tryblock-expr branch August 23, 2018 18:37
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Aug 23, 2018
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 23, 2018
flip1995 added a commit to rust-lang/rust-clippy that referenced this pull request Aug 23, 2018
@alexcrichton
Copy link
Member

As a heads up, we've got a pretty serious issue which may be a bit of a roadblock for this. Currently the compiler doesn't actually warn about usages of the try identifier on the 2015 edition, but that's tracked by #53077 and fix for it is at #53685. Unfortunately though the primary use of the try keyword, the try! macro in the standard library, suffers from #53686.

For the edition we'll need to have at least some warning for users of the try! macro to rustfix into r#try or suggest ?, but currently no warnings are emitted which can make for a difficult edition transition experience. Some eyes and/or thoughts on #53686 would be greatly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.