Skip to content

[unconditional_recursion]: compare by Tys instead of DefIds #12177

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

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Jan 20, 2024

Fixes #12154
Fixes #12181 (this was later edited in, so the rest of the description refers to the first linked issue)

Before this change, the lint would work with DefIds and use those to compare types. This PR changes it to compare types directly. It fixes the linked issue, but also other false positives I found in a lintcheck run. For example, one of the issues is that some types don't have DefIds (primitives, references, etc., leading to possible FNs), and the helper function used to extract a DefId didn't handle type parameters.

Another issue was that the lint would use .peel_refs() in a few places where that could lead to false positives (one such FP was in the http crate). See the doc comment on one of the added functions and also the test case for what I mean.

The code in the linked issue was linted because the receiver type is T (a ty::Param), which was not handled in get_ty_def_id and returned None, so this wouldn't actually get to comparing self_arg != ty_id here, and skip the early-return:

if let Some(ty) = cx.typeck_results().expr_ty_opt(receiver)
&& let Some(ty_id) = get_ty_def_id(ty)
&& self_arg != ty_id
{
// Since this called on a different type, the lint should not be
// triggered here.
return;
}

This alone could be fixed by doing something like && get_ty_def_id(ty).map_or(true, |ty_id)| self_arg != ty_id), but we don't really need to work with DefIds in the first place, I don't think.

changelog: [unconditional_recursion]: avoid linting when the other comparison type is a type parameter

@rustbot
Copy link
Collaborator

rustbot commented Jan 20, 2024

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 20, 2024
@bors
Copy link
Contributor

bors commented Jan 25, 2024

☔ The latest upstream changes (presumably #12200) made this pull request unmergeable. Please resolve the merge conflicts.

xtexx
xtexx approved these changes Jan 28, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Feb 7, 2024

DefIds are actually unsuited to the task. They treat Foo<X> and Foo<Y> as the same thing.

Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Feb 7, 2024

📌 Commit 87a6300 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 7, 2024

⌛ Testing commit 87a6300 with merge 62dcbd6...

@bors
Copy link
Contributor

bors commented Feb 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 62dcbd6 to master...

@bors bors merged commit 62dcbd6 into rust-lang:master Feb 7, 2024
@kpreid
Copy link
Contributor

kpreid commented Mar 13, 2024

A user observed the unfixed issue in rustc 1.77.0-beta.7 (339fb6965 2024-03-06), so I think this deserves consideration for backport.

@rustbot label +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 13, 2024
@flip1995 flip1995 added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Mar 14, 2024
@flip1995
Copy link
Member

rust-lang/rust#122510

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2024
…ulacrum

[beta] Clippy backports

Backports included in this PR:

- rust-lang/rust-clippy#12276 Fixing the lint on some platforms before hitting stable
- rust-lang/rust-clippy#12405 Respect MSRV before hitting stable
- rust-lang/rust-clippy#12266 Fixing an (unlikely) ICE
- rust-lang/rust-clippy#12177 Fixing FPs before hitting stable

Each backport on its own might not warrant a backport, but the collection of those are nice QoL fixes for Clippy users, before those bugs hit stable.

All of those commits are already on `master`.

r? `@Mark-Simulacrum`
@xFrednet xFrednet removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 18, 2024
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
8 participants