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 if-let match guards #79051

Merged
merged 7 commits into from
Dec 17, 2020
Merged

Conversation

LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Nov 14, 2020

Implements rust-lang/rfcs#2294 (tracking issue: #51114).

I probably should do a few more things before this can be merged:

  • Add tests (added basic tests, more advanced tests could be done in the future?)
  • Add lint for exhaustive if-let guard (comparable to normal if-let statements)
  • Fix clippy

However since this is a nightly feature maybe it's fine to land this and do those steps in follow-up PRs.

Thanks a lot @matthewjasper ❤️ for helping me with lowering to MIR! Would you be interested in reviewing this?
r? @ghost for now

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@LeSeulArtichaut LeSeulArtichaut force-pushed the if-let-guard branch 2 times, most recently from fb65462 to 3333608 Compare November 16, 2020 20:57
@LeSeulArtichaut LeSeulArtichaut marked this pull request as ready for review November 16, 2020 21:17
@LeSeulArtichaut
Copy link
Contributor Author

@matthewjasper This should be ready for review now!

@LeSeulArtichaut LeSeulArtichaut added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2020
@LeSeulArtichaut LeSeulArtichaut changed the title [WIP] Implement if-let match guards Implement if-let match guards Nov 17, 2020
@bors

This comment has been minimized.

@LeSeulArtichaut LeSeulArtichaut force-pushed the if-let-guard branch 2 times, most recently from e19b725 to 0cb8491 Compare November 28, 2020 16:27
@LeSeulArtichaut
Copy link
Contributor Author

Thanks a lot @Nadrieril for the pointers!
@matthewjasper This is ready for review again, sorry for the time I took to rebase this PR.

@LeSeulArtichaut LeSeulArtichaut force-pushed the if-let-guard branch 5 times, most recently from 967aeab to 95d536e Compare November 29, 2020 16:01
@LeSeulArtichaut
Copy link
Contributor Author

All right, CI passes again now 😅

@bors

This comment has been minimized.

@LeSeulArtichaut LeSeulArtichaut 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 Dec 5, 2020
@bors
Copy link
Contributor

bors commented Dec 16, 2020

⌛ Testing commit 0917260 with merge 02d3146f99803253859dae64af1f8c9508ddc37f...

@bors
Copy link
Contributor

bors commented Dec 17, 2020

💔 Test failed - checks-actions

@rust-log-analyzer
Copy link
Collaborator

Your PR failed (pretty log, 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.

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 @rust-lang/infra. (Feature Requests)

@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 Dec 17, 2020
@LeSeulArtichaut
Copy link
Contributor Author

Failure looks spurious?

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

Looks like aarch64-gnu was cancelled on Github's end? There's no logs.

@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 Dec 17, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#79051 (Implement if-let match guards)
 - rust-lang#79877 (Allow `since="TBD"` for rustc_deprecated)
 - rust-lang#79882 (Fix issue rust-lang#78496)
 - rust-lang#80026 (expand-yaml-anchors: Make the output directory separator-insensitive)
 - rust-lang#80039 (Remove unused `TyEncoder::tcx` required method)
 - rust-lang#80069 (Test that `core::assert!` is valid)
 - rust-lang#80072 (Fixed conflict with drop elaboration and coverage)
 - rust-lang#80073 (Add support for target aliases)
 - rust-lang#80082 (Revert rust-lang#78790 - rust-src vendoring)
 - rust-lang#80097 (Add `popcount` and `popcnt` as doc aliases for `count_ones` methods.)
 - rust-lang#80103 (Remove docs for non-existent parameters in `rustc_expand`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1e1ba7c into rust-lang:master Dec 17, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 17, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the if-let-guard branch December 17, 2020 06:03
@LeSeulArtichaut LeSeulArtichaut added the F-if_let_guard `#![feature(if_let_guard)]` label Dec 17, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 18, 2020
…, r=davidtwco

Change the message for `if_let_guard` feature gate

`if-let` guards are now implemented by rust-lang#79051 🎉
Thanks `@camelid` for pointing this out 🙂
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 18, 2020
…, r=davidtwco

Change the message for `if_let_guard` feature gate

`if-let` guards are now implemented by rust-lang#79051 🎉
Thanks ``@camelid`` for pointing this out 🙂
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 20, 2020
…thewjasper

Implement if-let match guards

Implements rust-lang/rfcs#2294 (tracking issue: rust-lang#51114).

I probably should do a few more things before this can be merged:
- [x] Add tests (added basic tests, more advanced tests could be done in the future?)
- [x] Add lint for exhaustive if-let guard (comparable to normal if-let statements)
- [x] Fix clippy

However since this is a nightly feature maybe it's fine to land this and do those steps in follow-up PRs.

Thanks a lot `@matthewjasper` ❤️ for helping me with lowering to MIR! Would you be interested in reviewing this?
r? `@ghost` for now
@Mark-Simulacrum
Copy link
Member

This was a regression of 3% on the match-stress-enum benchmark, but since it's directly adding more code to it that seems relatively expected. https://perf.rust-lang.org/compare.html?start=a6491be5be9344a325b7e49b0114f3cf67ef199e&end=9b84d36a0b9ea3bf305f36f08d50aa42c26f96c2&stat=instructions:u

let tpat = self.lower_pattern(&mut cx, pat, &mut false).0;
check_if_let_guard(&mut cx, &tpat, pat.hir_id);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this loop could be merged with the one above, that would probably reduce the perf impact

Copy link
Member

@camelid camelid Dec 25, 2020

Choose a reason for hiding this comment

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

I'll try opening a PR. (EDIT: #80367)

camelid added a commit to camelid/rust that referenced this pull request Jul 23, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 27, 2021
…adrieril

Combine two loops in `check_match`

Suggested by Nadrieril in
rust-lang#79051 (comment).

Opening to get a perf run. Hopefully this code doesn't require everything in the
first loop to be done before running the second! (It shouldn't though.)

cc `@Nadrieril`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-if_let_guard `#![feature(if_let_guard)]` 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.

9 participants