[let_chains, 1/6] Remove hir::ExprKind::If

[Centril]

Per #53667 (comment).

r? @oli-obk

[petrochenkov]

IIRC, I got some lifetime issues after trying to desugar all ifs 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]

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]

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) if let Some(x) = y {
        print(x); // Nope
    }
}

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

[Centril]

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]

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

[oli-obk]

@bors try

[bors]

⌛ Trying commit db4452fab28c9374dd6676b702dc36b43e2a45b5 with merge bf82f43c83ada4fffaf93a633dfce5c7f55d57fe...

[bors]

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

[oli-obk]

@rust-timer build

[oli-obk]

@rust-timer build bf82f43c83ada4fffaf93a633dfce5c7f55d57fe

[rust-timer]

Success: Queued bf82f43c83ada4fffaf93a633dfce5c7f55d57fe with parent ef4d1c4, comparison URL.

[rust-timer]

Finished benchmarking try commit bf82f43c83ada4fffaf93a633dfce5c7f55d57fe

[Centril]

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

@oli-obk Please take a look :)

[oli-obk]

@bors r+

[bors]

📌 Commit f9cc5a6 has been approved by oli-obk

[bors]

⌛ Testing commit f9cc5a6 with merge acc7e65...

[bors]

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

[rust-highfive]

📣 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]

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]

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

[Centril]

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]

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]

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]

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]

This ended up being a non-trivial perf regression.

[Centril]

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