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 some suggestions for redundant_pattern_matching #4352

Merged
merged 3 commits into from Aug 21, 2019

Conversation

@phansch
Copy link
Member

commented Aug 7, 2019

.. and change the Applicability to MaybeIncorrect.

Fixes the problem displayed in #4344 (comment).

We now append {} to the suggestion so that the conditional has the
correct syntax again.

(If we were to remove the if instead, it would trigger the
unused_must_use warning for #[must_use] types.)

changelog: Fix some suggestions for redundant_pattern_matching

@flip1995

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I have 2 concerns here:

  1. What if the if-block isn't empty? I don't see a test for that nvm there is a test and the lint doesn't trigger there.**
  2. The suggestion is semantically not the same:

While this expression:

match foo {
    Ok(_) => true,
    Err(_) => false,
}

returns a bool, the suggested expression:

if foo.is_ok() {}

returns the unit type (). I'm not sure if this lint also triggers for constructs like let _ = match ...*, but if this suggestion would get applied, weird follow up type errors of rustc would be the result. (I don't think a test exists for that either)


* Since stray match;/if let; expressions returning something other than () are pretty rare, this lint should trigger also on those, otherwise it would be pretty useless (except the case of a match/if let (without ;) that returns from a function)

** That would be a good first issue IMO, since you just need to get the expression inside the if block and use

/// Like `snippet_block`, but add braces if the expr is not an `ExprKind::Block`.
/// Also takes an `Option<String>` which can be put inside the braces.
pub fn expr_block<'a, T: LintContext>(cx: &T, expr: &Expr, option: Option<String>, default: &'a str) -> Cow<'a, str> {

to create the new block

@flip1995

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

cc #4344 (comment)

Oh you already mentioned my first point. Without fixing this, we should just turn the suggestion into MaybeIncorrect

@phansch

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

What if the if-block isn't empty? I don't see a test for that

Yeah, that was also brought up as a problem in #4344 and this PR doesn't address that.
One solution could be to skip the suggestion if the block contains statements.

The suggestion is semantically not the same

It seems like there are a lot of test cases missing. Maybe these suggestions should be marked as MaybeIncorrect for now? I'll have a look and see if I can fix the problem with the return type.

@phansch phansch changed the title Fix suggestions for redundant_pattern_matching Fix some suggestions for redundant_pattern_matching Aug 8, 2019

@flip1995

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Just edited my first comment 😉

@phansch phansch force-pushed the phansch:fix_redundant_pattern_matching branch from 85a736c to 400015f Aug 8, 2019

@phansch

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@flip1995 Was this what you had in mind? I opted to stop suggesting the if ... {} conditional because, as you said, it's not the same thing as the original code. I also added more test cases but removed the // run-rustfix for now.

@phansch phansch force-pushed the phansch:fix_redundant_pattern_matching branch 2 times, most recently from f1d32e8 to d41a5d6 Aug 8, 2019

@flip1995
Copy link
Member

left a comment

Yeah that was somehow what I had in mind, but there are different cases, that we need to take into account before we can turn this suggestion to MachineApplicable.

New test cases look good now. One test case should be added for the auto applicability:

let x = if let Some(_) = opt { true } else { false };
takes_bool(x);
let y = if let Some(_) = opt {};
takes_unit(y);

to make sure we pass the type checker with the suggestions. I would just add them now, so we don't forget about them once someone makes these suggestions auto applicable.

tests/ui/redundant_pattern_matching.stderr Outdated Show resolved Hide resolved
phansch added 3 commits Aug 7, 2019
Fix suggestions for redundant_pattern_matching
Fixes the problem displayed in #4344 (comment).

We now append `{}` to the suggestion so that the conditional has the
correct syntax again.

(If we were to _remove_ the `if` instead, it would trigger the
`unused_must_use` warning for `#[must_use]` types.
Add more testcases for redundant_pattern_matching
These should make sure that, when the suggestions are fixed, they are
fixed for all these cases.

@phansch phansch force-pushed the phansch:fix_redundant_pattern_matching branch from d41a5d6 to 436d429 Aug 21, 2019

@flip1995

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

📌 Commit 436d429 has been approved by flip1995

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

⌛️ Testing commit 436d429 with merge 57c67a2...

bors added a commit that referenced this pull request Aug 21, 2019
Auto merge of #4352 - phansch:fix_redundant_pattern_matching, r=flip1995
Fix some suggestions for redundant_pattern_matching

.. and change the Applicability to `MaybeIncorrect`.

Fixes the problem displayed in #4344 (comment).

We now append `{}` to the suggestion so that the conditional has the
correct syntax again.

(If we were to _remove_ the `if` instead, it would trigger the
`unused_must_use` warning for `#[must_use]` types.)

changelog: Fix some suggestions for `redundant_pattern_matching`
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 57c67a2 to master...

@bors bors merged commit 436d429 into rust-lang:master Aug 21, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.