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 FP in lint [needless_match] #8549

Merged
merged 4 commits into from
Apr 6, 2022
Merged

fix FP in lint [needless_match] #8549

merged 4 commits into from
Apr 6, 2022

Conversation

J-ZhengLi
Copy link
Member

@J-ZhengLi J-ZhengLi commented Mar 16, 2022

fixes: #8542
fixes: #8551
fixes: #8595
fixes: #8599


changelog: check for more complex custom type, and ignore type coercion in [needless_match]

@rust-highfive
Copy link

r? @giraffate

(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 Mar 16, 2022
@J-ZhengLi
Copy link
Member Author

r? @llogiq

@rust-highfive rust-highfive assigned llogiq and unassigned giraffate Mar 16, 2022
@J-ZhengLi J-ZhengLi marked this pull request as draft March 16, 2022 04:06
@J-ZhengLi J-ZhengLi marked this pull request as ready for review March 16, 2022 04:34
@J-ZhengLi
Copy link
Member Author

I remove the ref x => *x match arm check as it's logic seems problematic, I was already unsure about whether I should or not including it when I add the lint. Right now, removing it does seem to be a quick fix of what @teor2345 mentioned in #8542 (comment) , I just don't see the need to add it back, but maybe I'll do it when someone actually suggesting it. 🤦

@J-ZhengLi J-ZhengLi changed the title quick fix of issue#8542: FP in lint [needless_match] [WIP] quick fix of issue#8542: FP in lint [needless_match] Mar 17, 2022
@J-ZhengLi J-ZhengLi changed the title [WIP] quick fix of issue#8542: FP in lint [needless_match] quick fix of issue#8542: FP in lint [needless_match] Mar 17, 2022
Comment on lines 129 to 123
/// Check if the type of the match operand differs with the assigned local or function return type
///
/// Can also be used to check for the `let_expr` in `IfLet` as well.
fn is_type_differ(cx: &LateContext<'_>, match_expr: &Expr<'_>, expr: &Expr<'_>) -> bool {
if let Some(p_node) = get_parent_node(cx.tcx, expr.hir_id) {
Copy link
Member Author

@J-ZhengLi J-ZhengLi Mar 17, 2022

Choose a reason for hiding this comment

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

I'm not so sure about if this is the correct way to check for type coercion, I basically just check for the assigned variable/function return type to see if those are the same as match expression.

Although I saw methods like TypeckResult::is_coercion_cast or something, but I don't think those work in these case.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have clippy_utils::is_adjusted(cx, expr), which should do the trick more easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have clippy_utils::is_adjusted(cx, expr), which should do the trick more easily.

Well... I later realized that I might still need to keep it this way, because I do need to make sure that the type of match input and output are the same, not just coercion but also as what #8599 suggested. unless there is another util function that I can use to do the check.

@J-ZhengLi J-ZhengLi changed the title quick fix of issue#8542: FP in lint [needless_match] [WIP] quick fix of issue#8542: FP in lint [needless_match] Mar 17, 2022
@J-ZhengLi J-ZhengLi changed the title [WIP] quick fix of issue#8542: FP in lint [needless_match] fix FP in lint [needless_match] Mar 17, 2022
@J-ZhengLi
Copy link
Member Author

J-ZhengLi commented Mar 29, 2022

Also fixes #8595, #8599

It's kinda harsh seeing my mistakes being addressed multiple times tbh 😭 , so if anyone is not busy at the moment (sorry to bother if you are), please help me reviewing this, thanks~~~

edit: Oh... here are some 'ol little pings @giraffate @llogiq

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This looks good, I just have a very small suggestion. r=me when applied.

@@ -171,27 +191,30 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool {
false
}

fn has_identical_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegment<'_>]) -> bool {
fn same_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegment<'_>]) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have an over function for such things skmewhere in clippy_utils.

@llogiq
Copy link
Contributor

llogiq commented Apr 6, 2022

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 6, 2022

📌 Commit 85b081b has been approved by llogiq

@bors
Copy link
Contributor

bors commented Apr 6, 2022

⌛ Testing commit 85b081b with merge 81e004a...

@bors
Copy link
Contributor

bors commented Apr 6, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 81e004a to master...

@bors bors merged commit 81e004a into rust-lang:master Apr 6, 2022
@bors bors mentioned this pull request Apr 6, 2022
@J-ZhengLi J-ZhengLi deleted the issue8542 branch April 11, 2022 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
6 participants