[let_chains, 1/6] Remove hir::ExprKind::If by Centril · Pull Request #59288 · rust-lang/rust

Centril · 2019-03-19T06:19:09Z

petrochenkov · 2019-03-19T08:02:28Z

IIRC, I got some lifetime issues after trying to desugar allifs into matches (rather than those containing is), with RefCells being held for too long (or too little?) and panicking etc.
Don't remember the details right now, perhaps it's fixable.

petrochenkov · 2019-03-19T08:14:13Z

This seems like a nice simplification even regardless of if-let chains, we only need to make sure that it doesn't regress 1) codegen 2) compile times, and 3) diagnostics.

Similarly, desugaring while into loop is also required for implementing pattern matching expressions, but is also a nice simplification otherwise.

(I'll try to review the code later today.)

petrochenkov · 2019-03-19T08:23:18Z

On the other hand, the desugaring for pattern matching expressions at the AST/HIR border turned out incredibly hacky, when I tried it.
It also made impossible to support those expressions in pattern guards.

match thing {PAT(y)ifletSome(x) = y{
print(x);// Nope}}

So it's a pretty real possibility that it's better done somewhere at HIR->MIR time.

Centril · 2019-03-19T08:29:38Z

This seems like a nice simplification even regardless of if-let chains, we only need to make sure that it doesn't regress 1) codegen 2) compile times, and 3) diagnostics.

I think I fixed all diagnostics issues that I had... some even got improved. :) As for codegen there are issues in #59288 (comment) wrt. mir-opt tests (thus the WIP) that I don't really understand yet.

Similarly, desugaring while into loop is also required for implementing pattern matching expressions, but is also a nice simplification otherwise.

Yup, I'd like to do that as step 4 in the process after having done the if part of the RFC fully. Maybe even remove continue from HIR after step 6...

(I'll try to review the code later today.)

:)

On the other hand, the desugaring for pattern matching expressions at the AST/HIR border turned out incredibly hacky, when I tried it.

When I tried it out with LBV + a fold in lowering it didn't feel so bad. Haven't thought about the implications of if-let-guards + if-let-chains tho wrt. lowering. :)

Centril · 2019-03-19T10:46:23Z

(I believe @oli-obk is looking into the mir-opt stuff)

@Centril

Run branch cleanup after copy prop This is preliminary work for rust-lang#59288 (comment) which gets rid of `if` in the HIR. cc @rust-lang/wg-mir-opt @Centril

oli-obk · 2019-03-19T14:21:54Z

@bors try

bors · 2019-03-19T14:22:09Z

⌛ Trying commit db4452fab28c9374dd6676b702dc36b43e2a45b5 with merge bf82f43c83ada4fffaf93a633dfce5c7f55d57fe...

bors · 2019-03-19T16:36:46Z

☀️ Try build successful - checks-travis
Build commit: bf82f43c83ada4fffaf93a633dfce5c7f55d57fe

oli-obk · 2019-03-19T17:21:09Z

@rust-timer build

oli-obk · 2019-03-19T17:23:00Z

@rust-timer build bf82f43c83ada4fffaf93a633dfce5c7f55d57fe

rust-timer · 2019-03-19T17:23:03Z

Success: Queued bf82f43c83ada4fffaf93a633dfce5c7f55d57fe with parent ef4d1c4, comparison URL.

rust-timer · 2019-03-19T20:05:22Z

Finished benchmarking try commit bf82f43c83ada4fffaf93a633dfce5c7f55d57fe

Centril · 2019-05-10T18:01:41Z

Filed FIXME issue, rebased, and did a minor cleanup after the .await PR.

@oli-obk Please take a look :)

oli-obk · 2019-05-10T19:15:52Z

@bors r+

bors · 2019-05-10T19:15:53Z

📌 Commit f9cc5a6 has been approved by oli-obk

bors · 2019-05-10T22:58:58Z

⌛ Testing commit f9cc5a6 with merge acc7e65...

@oli-obk

[let_chains, 1/6] Remove hir::ExprKind::If Per #53667 (comment). r? @oli-obk

bors · 2019-05-11T01:41:29Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing acc7e65 to master...

rust-highfive · 2019-05-11T01:42:54Z

📣 Toolstate changed by #59288!

Tested on commit acc7e65.
Direct link to PR: #59288

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @Xanewok, @rust-lang/infra).

Manishearth · 2019-05-11T01:47:37Z

Have you looked into how hard it will be to make clippy migrate to these? We have a lot of lints around ifs, and the lowering may lose some context necessary to make these work. Removing lints because they're no longer possible isn't an option.

IMO for such changes where there's a good chance clippy lints will be unfixably broken we should be notified of this before it merges :/

Manishearth · 2019-05-11T01:48:25Z

I appreciate it being split up, though, it's easier to make this evaluation if one thing breaks at a time :)

Centril · 2019-05-11T01:51:04Z

We have a lot of lints around ifs, and the lowering may lose some context necessary to make these work.

The lowering preserves information about the source; you can check MatchSource::IfDesugar to see whether it originated from an if.

@oli-obk thought it would work for clippy and as you know, he also works on clippy...

Manishearth · 2019-05-11T01:53:52Z

Ah okay, thanks. I'll see how hard this is to fix. For the later steps can you work with us to ensure stuff isn't broken before landing? Mostly just pinging me when the PR is ready to merge so I can work on a patch

…

On Fri, May 10, 2019, 6:51 PM Mazdak Farrokhzad ***@***.***> wrote: We have a lot of lints around ifs, and the lowering may lose some context necessary to make these work. The lowering preserves information about the source; you can check MatchSource::IfDesugar to see whether it originated from an if. @oli-obk <https://github.com/oli-obk> thought it would work for clippy and as you know, he also works on clippy... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#59288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMK6SFNF7Z4ALSPPWZJFZ3PUYRDVANCNFSM4G7NO2EQ> .

Centril · 2019-05-11T01:58:37Z

For the later steps can you work with us to ensure stuff isn't broken before landing? Mostly just pinging me when the PR is ready to merge so I can work on a patch

Sure. Fwiw, I expected Oliver to be the "work with us" party... ;)

The next steps will be to introduce ast::ExprKind::Let and then to use 'label: { ... break 'label value }. That will probably be less straightforward but I think we can introduce similar desugaring info as needed. I could introduce ast::ExprKind::Let in a separate intermediate step however without connecting the parser (already done in a PR but I could probably split things).

Manishearth · 2019-05-11T02:21:11Z

That's fair, though it's better if more clippy folks know. I'm working on a fix now.

…

On Fri, May 10, 2019, 6:59 PM Mazdak Farrokhzad ***@***.***> wrote: For the later steps can you work with us to ensure stuff isn't broken before landing? Mostly just pinging me when the PR is ready to merge so I can work on a patch Sure. Fwiw, I expected Oliver to be the "work with us" party... ;) The next steps will be to introduce ast::ExprKind::Let and then to use 'label: { ... break 'label value }. That will probably be less straightforward but I think we can introduce similar desugaring info as needed. I could introduce ast::ExprKind::Let in a separate intermediate step however without connecting the parser (already done in a PR but I could probably split things). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#59288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMK6SEZ33ZJV22OE6W6LZ3PUYSAFANCNFSM4G7NO2EQ> .

nnethercote · 2019-06-03T09:56:51Z

This ended up being a non-trivial perf regression.

Centril · 2019-06-03T11:51:44Z

Yep; this was expected. #60730 will hopefully reduce some of those regressions.

rust-highfive assigned oli-obk Mar 19, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2019

petrochenkov self-assigned this Mar 19, 2019

This comment has been minimized.
Sign in to view

Centril force-pushed the hir-if-to-match branch from 35ac9ca to b5613e3Compare March 19, 2019 08:37

This comment has been minimized.
Sign in to view

oli-obk mentioned this pull request Mar 19, 2019
Run branch cleanup after copy prop #59290
Merged

oli-obk reviewed Mar 19, 2019
View reviewed changes

Comment threadsrc/librustc/hir/mod.rs Outdated
Comment threadsrc/librustc_typeck/check/_match.rs Outdated
Comment threadsrc/librustc_typeck/check/demand.rs Outdated
Comment threadsrc/librustc_typeck/check/mod.rs Outdated

This comment has been minimized.
Sign in to view

Centril force-pushed the hir-if-to-match branch 2 times, most recently from 772ab34 to 9a0d37dCompare March 21, 2019 10:11

This comment has been minimized.
Sign in to view

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 Mar 22, 2019

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2019

Centril mentioned this pull request May 10, 2019
Cleanup hack to expect scrutinee: bool in if-to-match desugaring in match-expr typeck #60707
Closed

check_match: add FIXME for removing of hack.
f9cc5a6

Centril force-pushed the hir-if-to-match branch from c6eac6a to f9cc5a6Compare May 10, 2019 17:56

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 May 10, 2019

bors added a commit that referenced this pull request May 10, 2019
Auto merge of#59288- Centril:hir-if-to-match, r=oli-obk
acc7e65
[let_chains, 1/6] Remove hir::ExprKind::If Per #53667 (comment). r? @oli-obk

bors added the merged-by-bors This PR was explicitly merged by bors. label May 11, 2019

bors merged commit f9cc5a6 into rust-lang:masterMay 11, 2019

rust-highfive mentioned this pull request May 11, 2019
rls no longer builds after rust-lang/rust#59288 #60722
Closed

bors mentioned this pull request May 11, 2019
Implementation of RFC 2289 (associated_type_bounds) #57428
Merged

Manishearth mentioned this pull request May 11, 2019
Rustup to rustc 1.36.0-nightly (acc7e652f 2019-05-10) rust-lang/rust-clippy#4080
Merged
5 tasks

This was referenced May 15, 2019
[let_chains, 2/6] Introduce Let(..) in AST, remove IfLet + WhileLet and parse let chains #60861
Merged
Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" #53667
Closed

Centril mentioned this pull request Jun 20, 2019
[let_chains, 3/6] And then there was only Loop #61988
Merged

c410-f3r mentioned this pull request Nov 23, 2020
Reintroduce hir::ExprKind::If #79328
Merged
6 tasks

c410-f3r mentioned this pull request Mar 14, 2022
Stabilize let_chains in Rust 1.64 #94927
Merged

Uh oh!

Uh oh!

Conversation

Centril commented Mar 19, 2019

Uh oh!

This comment has been minimized.

petrochenkov commented Mar 19, 2019

Uh oh!

petrochenkov commented Mar 19, 2019

Uh oh!

petrochenkov commented Mar 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Centril commented Mar 19, 2019

Uh oh!

This comment has been minimized.

Centril commented Mar 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oli-obk commented Mar 19, 2019

Uh oh!

bors commented Mar 19, 2019

Uh oh!

This comment has been minimized.

bors commented Mar 19, 2019

Uh oh!

oli-obk commented Mar 19, 2019

Uh oh!

oli-obk commented Mar 19, 2019

Uh oh!

rust-timer commented Mar 19, 2019

Uh oh!

rust-timer commented Mar 19, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Centril commented May 10, 2019

Uh oh!

oli-obk commented May 10, 2019

Uh oh!

bors commented May 10, 2019

Uh oh!

bors commented May 10, 2019

Uh oh!

bors commented May 11, 2019

Uh oh!

rust-highfive commented May 11, 2019

Uh oh!

Manishearth commented May 11, 2019

Uh oh!

Manishearth commented May 11, 2019

Uh oh!

Centril commented May 11, 2019

Uh oh!

Manishearth commented May 11, 2019 via email

Uh oh!

Centril commented May 11, 2019

Uh oh!

Manishearth commented May 11, 2019 via email

Uh oh!

nnethercote commented Jun 3, 2019

Uh oh!

Centril commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

petrochenkov commented Mar 19, 2019•
edited
Loading