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

Fix clippy::collapsible_match with let expressions #88163

Merged
merged 1 commit into from
Aug 22, 2021

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Aug 19, 2021

This fixes rust-lang/rust-clippy#7575 which is a regression from #80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap.

The fix is basically to re-generalize match and if let for the lint implementation (they were split because if let no longer desugars to match in the HIR).

Also fixes rust-lang/rust-clippy#7586 and fixes rust-lang/rust-clippy#7591
cc @rust-lang/clippy
@xFrednet do you want to review this?

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Aug 19, 2021
@Mark-Simulacrum
Copy link
Member

r? @Manishearth

@xFrednet
Copy link
Member

Sure, I can take a look. 👍

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Only a small NIT when it comes to the lint message. I would be okay with merging this and fixing it after the sync if you want this to land ASAP. However, I would prefer to get this fixed before the sync (Also because I'm not sure if we'll have a sync before next week).


I needed some time to get used to the new syntax and structure with the let expression, and now I like it. Great work!

Another sidenote: The commit is not linked to your GH account (The email probably isn't set). This is not a dealbreaker, just FYI in case that is not intended 🙃

src/tools/clippy/clippy_lints/src/collapsible_match.rs Outdated Show resolved Hide resolved
@camsteffen camsteffen force-pushed the collapsible-match-fix branch 2 times, most recently from 03114cc to de608cf Compare August 19, 2021 18:31
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

LGTM

Just one last question

Comment on lines -24 to -26
let _ = str_to_int("1");
let _ = str_to_int_ok("2");
let _ = strange_some_no_else("3");
Copy link
Member

Choose a reason for hiding this comment

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

Rust allows unused-lints in normal compiler UI tests. Are they now also disabled in Clippy's UI tests? (When running them locally via cargo test or inside the Clippy CI)

Copy link
Member

Choose a reason for hiding this comment

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

Well, looks like @rust-log-analyzer answered the question for us ^^. We could add something like that in the future to Clippy as well ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rust allows unused-lints in normal compiler UI tests.

Didn't know that. Sounds good for Clippy.

@rust-log-analyzer

This comment has been minimized.

/// must have "wild-like" branches that can be combined.
fn is_wild_like(cx: &LateContext<'_>, pat_kind: &PatKind<'_>, arm_guard: &Option<Guard<'_>>) -> bool {
if arm_guard.is_some() {
enum IfLetOrMatch<'hir> {
Copy link
Member

Choose a reason for hiding this comment

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

should be in utils IMO, but we can do that refactor once this is synced back

@Manishearth
Copy link
Member

@bors delegate=camsteffen

@bors
Copy link
Contributor

bors commented Aug 19, 2021

✌️ @camsteffen can now approve this pull request

@Manishearth
Copy link
Member

r=me when tests pass

@camsteffen
Copy link
Contributor Author

@bors r=Manishearth

@bors
Copy link
Contributor

bors commented Aug 19, 2021

📌 Commit ada9282 has been approved by Manishearth

@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 19, 2021
camsteffen added a commit to camsteffen/rust that referenced this pull request Aug 19, 2021
…=Manishearth

Fix clippy::collapsible_match with let expressions

This fixes rust-lang/rust-clippy#7575 which is a regression from rust-lang#80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap.

The fix is basically to re-generalize `match` and `if let` for the lint implementation (they were split because `if let` no longer desugars to `match` in the HIR).

Also fixes rust-lang/rust-clippy#7586
cc `@rust-lang/clippy`
`@xFrednet` do you want to review this?
@bors
Copy link
Contributor

bors commented Aug 22, 2021

⌛ Testing commit ada9282 with merge 7481e6d...

@bors
Copy link
Contributor

bors commented Aug 22, 2021

☀️ Test successful - checks-actions
Approved by: Manishearth
Pushing 7481e6d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2021
@bors bors merged commit 7481e6d into rust-lang:master Aug 22, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 22, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 3, 2021
…anishearth

Fix clippy::collapsible_match with let expressions

This fixes rust-lang/rust-clippy#7575 which is a regression from rust-lang#80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap.

The fix is basically to re-generalize `match` and `if let` for the lint implementation (they were split because `if let` no longer desugars to `match` in the HIR).

Also fixes rust-lang/rust-clippy#7586 and fixes rust-lang/rust-clippy#7591
cc `@rust-lang/clippy`
`@xFrednet` do you want to review this?
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2021
…earth

Add expansion to while desugar spans

In the same vein as rust-lang#88163, this reverts a change in Clippy behavior as a result of rust-lang#80357 (and reverts some `#[allow]`s): This changes `clippy::blocks_in_if_conditions` to not fire on `while` loops. Though we might actually want Clippy to lint those cases, we should introduce the change purposefully, with tests, and possibly under a different lint name.

The actual change here is to add a desugaring expansion to the spans when lowering a `while` loop.

r? `@Manishearth`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 7, 2021
…earth

Add expansion to while desugar spans

In the same vein as rust-lang#88163, this reverts a change in Clippy behavior as a result of rust-lang#80357 (and reverts some `#[allow]`s): This changes `clippy::blocks_in_if_conditions` to not fire on `while` loops. Though we might actually want Clippy to lint those cases, we should introduce the change purposefully, with tests, and possibly under a different lint name.

The actual change here is to add a desugaring expansion to the spans when lowering a `while` loop.

r? `@Manishearth`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
8 participants