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.

Sign up for GitHub

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

Jump to bottom

Fix unreachable_code warnings for try{} block ok-wrapped expressions #64581

Merged
merged 6 commits into from
Oct 2, 2019
Merged

Fix unreachable_code warnings for try{} block ok-wrapped expressions #64581

merged 6 commits into from
Oct 2, 2019

Conversation

ztlpn
Copy link
Contributor

@ztlpn ztlpn commented Sep 18, 2019 •
edited by Centril
Loading

Fixes #54165 and fixes #63324.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
ztlpn
ztlpn commented Sep 18, 2019
View reviewed changes
src/librustc_typeck/check/expr.rs Outdated
// so we want avoid generating it.
// Ditto for the autogenerated `Try::from_ok(())` at the end of e.g. `try { return; }`.
let (is_try_block_ok_wrapped_expr, is_try_block_generated_expr) = match expr.node {
ExprKind::Call(_, ref args) if expr.span.is_desugaring(DesugaringKind::TryBlock) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is rather brittle. I wonder if it would be better to just ok-wrap the whole try-block rather than the final expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I tried that (desugaring try {...} to { Try::from_ok({...}) }) and the resulting code is indeed simpler (didn't push yet). The downside is that type errors now refer to the whole block and not to the tail expression (e.g. here). Thoughts?

Centril
Centril reviewed Sep 18, 2019
View reviewed changes
src/librustc/hir/lowering/expr.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/expr.rs Outdated Show resolved Hide resolved
src/test/ui/try-block/try-block-bad-type.stderr Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

r? @Centril

@Centril Centril 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 Sep 19, 2019
@ztlpn ztlpn requested a review from Centril September 19, 2019 11:26
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2019
@Centril
Copy link
Contributor

Centril commented Sep 19, 2019

Will schedule some time over the weekend to review this some more. :)

@bors

This comment has been minimized.

Sign in to view
@ztlpn ztlpn force-pushed the fix-ok-wrapping-unreachable-code branch from b7b1475 to 988ee46 Compare September 21, 2019 21:09
@rust-highfive

This comment has been minimized.

Sign in to view
@ztlpn ztlpn force-pushed the fix-ok-wrapping-unreachable-code branch from 988ee46 to 58eacaa Compare September 22, 2019 14:09
@ztlpn
Copy link
Contributor Author

ztlpn commented Sep 30, 2019

@Centril hope you'll find some time to look at this!

@Centril
Copy link
Contributor

Centril commented Oct 1, 2019

Sorry about the delay. Finally got around to this. It looks good, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2019

📌 Commit 58eacaa has been approved by Centril

@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 Oct 1, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
@Centril
Rollup merge of rust-lang#64581 - ztlpn:fix-ok-wrapping-unreachable-c…
e463a22 
…ode, r=Centril

Fix unreachable_code warnings for try{} block ok-wrapped expressions

Fixes rust-lang#54165 and fixes rust-lang#63324.
@Centril Centril mentioned this pull request Oct 1, 2019
Closed
@Centril
Copy link
Contributor

Centril commented Oct 1, 2019

Failed in #64951 (comment), @bors r-

(Will need to rebase to make .node into .kind.)

@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 Oct 1, 2019
@ztlpn ztlpn force-pushed the fix-ok-wrapping-unreachable-code branch from 58eacaa to 75fdb95 Compare October 1, 2019 17:10
@ztlpn ztlpn requested a review from Centril October 2, 2019 13:17
@Centril
Copy link
Contributor

Centril commented Oct 2, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2019

📌 Commit 75fdb95 has been approved by Centril

@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 Oct 2, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 2, 2019
@Centril
Rollup merge of rust-lang#64581 - ztlpn:fix-ok-wrapping-unreachable-c…
18d0c03 
…ode, r=Centril

Fix unreachable_code warnings for try{} block ok-wrapped expressions

Fixes rust-lang#54165 and fixes rust-lang#63324.
@Centril Centril mentioned this pull request Oct 2, 2019
Merged
bors added a commit that referenced this pull request Oct 2, 2019
@bors
Auto merge of #65009 - Centril:rollup-06g05xj, r=Centril
2daa404 
Rollup of 13 pull requests

Successful merges:

 - #64581 (Fix unreachable_code warnings for try{} block ok-wrapped expressions)
 - #64850 (Remove inlines from DepNode code)
 - #64914 (regression test for 64453 borrow check error.)
 - #64922 (Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions)
 - #64948 (Improve sidebar styling to make its integration easier)
 - #64961 (Make comment about dummy type a bit more clear)
 - #64967 (Don't mark borrows of zero-sized arrays as indirectly mutable)
 - #64973 (Fix typo while setting `compile-flags` in test)
 - #64980 (Enable support for `IndirectlyMutableLocals` in `rustc_peek` )
 - #64989 (Fix ICE #64964)
 - #64991 ([const-prop] Correctly handle locals that can't be propagated)
 - #64995 (Remove rustdoc warning)
 - #64997 (rustc book: nitpick SLP vectorization)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Oct 2, 2019

⌛ Testing commit 75fdb95 with merge 2daa404...

@bors bors merged commit 75fdb95 into rust-lang:master Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril approved these changes

Assignees

@Centril Centril

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
5 participants
@ztlpn @rust-highfive @petrochenkov @Centril @bors