Skip to content

fix ignored_unit_patterns: include refs in the suggestion #15578

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix ignored_unit_patterns: include refs in the suggestion #15578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions clippy_lints/src/ignored_unit_patterns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ use hir::{Node, PatKind};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::mir::Mutability;
use rustc_middle::ty::{self, Ty};
use rustc_session::declare_lint_pass;

declare_clippy_lint! {
Expand Down Expand Up @@ -39,7 +41,9 @@ impl<'tcx> LateLintPass<'tcx> for IgnoredUnitPatterns {
fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx hir::Pat<'tcx>) {
if matches!(pat.kind, PatKind::Wild)
&& !pat.span.from_expansion()
&& cx.typeck_results().pat_ty(pat).peel_refs().is_unit()
&& let pat_ty = cx.typeck_results().pat_ty(pat)
&& let (pat_ty, refs) = peel_refs_with_mutabilities(pat_ty)
&& pat_ty.is_unit()
{
match cx.tcx.parent_hir_node(pat.hir_id) {
Node::Param(param) if matches!(cx.tcx.parent_hir_node(param.hir_id), Node::Item(_)) => {
Expand All @@ -52,15 +56,29 @@ impl<'tcx> LateLintPass<'tcx> for IgnoredUnitPatterns {
},
_ => {},
}

let sugg = refs.into_iter().map(Mutability::ref_prefix_str).chain(["()"]).collect();
span_lint_and_sugg(
cx,
IGNORED_UNIT_PATTERNS,
pat.span,
"matching over `()` is more explicit",
"use `()` instead of `_`",
String::from("()"),
sugg,
Applicability::MachineApplicable,
);
}
}
}

/// Like [`rustc_middle::ty::Ty::peel_refs`], but returns the mutability of each peeled ref
///
/// `&&mut &mut &mut T` returns `(T, [Not, Mut, Mut, Mut])`
fn peel_refs_with_mutabilities(ty: Ty<'_>) -> (Ty<'_>, Vec<Mutability>) {
let (mut ty, mut refs) = (ty, vec![]);
while let ty::Ref(_, dest_ty, mutbl) = ty.kind() {
ty = *dest_ty;
refs.push(*mutbl);
}
(ty, refs)
}
19 changes: 18 additions & 1 deletion tests/ui/ignored_unit_patterns.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub fn moo(_: ()) {
fn test_unit_ref_1() {
let x: (usize, &&&&&()) = (1, &&&&&&());
match x {
(1, ()) => unimplemented!(),
(1, &&&&&()) => unimplemented!(),
Copy link
Member

Choose a reason for hiding this comment

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

Hm, seems like matching () is also valid and the &s are not required. Does that also work (without compiler error) in closures?

Copy link
Contributor Author

@ada4a ada4a Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well it depends. You're right that the refs are not required in match arms, and they are not required in closures most of the time. But as #15187 shows, it stops working when you try casting the closure to impl Fn(&()), for example.

The solution I tried first was to eliminate all the cases where we know the refs aren't needed (e.g. all match arm patterns), but it now looks like the only case where they are needed are the situations where a closure accepting &() later gets coerced to fn(&())/impl Fn*(&()). Therefore, we could theoretically try catching only those cases, but that will probably require writing some kind of a HIR visitor, and those things are kind of intimidating to me tbh...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo the first solution (don't add refs when suggesting for match arms, but do add them for all closure params) is a good balance of simplicity and correctness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's not even that!

  1. casting/coercing the closure to both an fn and a Fn* trait object works actually just fine

  2. changing anything about the issue's reproducer, like:

    • using the type directly instead of the LifetimeBound type alias
    • replacing dyn with impl in the type alias

    either stops the thing from compiling, or makes it so that () starts getting accepted as well

I guess I'm not well-versed enough in the type system shenanigans to pinpoint the combination of factors that makes this exact expression require &() instead of (). If you have any ideas, let me know (and let me know if you know a way I could test for said factors) -- otherwise, we might want to just close the issue as WONTFIX, given how specific it is?..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flip1995 friendly ping

//~^ ERROR: matching over `()` is more explicit
_ => unimplemented!(),
};
Expand All @@ -59,3 +59,20 @@ fn test_unit_ref_2(v: &[(usize, ())]) {
let _ = x;
}
}

fn issue15187() {
let func: fn(&()) = |&()| {};
//~^ ERROR: matching over `()` is more explicit
let func: fn(&mut ()) = |&mut ()| {};
//~^ ERROR: matching over `()` is more explicit
let func: fn(&&mut ()) = |&&mut ()| {};
//~^ ERROR: matching over `()` is more explicit
let func: fn(&&mut &()) = |&&mut &()| {};
//~^ ERROR: matching over `()` is more explicit

#[allow(clippy::match_single_binding)]
match &() {
&() => todo!(),
//~^ ERROR: matching over `()` is more explicit
}
}
17 changes: 17 additions & 0 deletions tests/ui/ignored_unit_patterns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,20 @@ fn test_unit_ref_2(v: &[(usize, ())]) {
let _ = x;
}
}

fn issue15187() {
let func: fn(&()) = |_| {};
//~^ ERROR: matching over `()` is more explicit
let func: fn(&mut ()) = |_| {};
//~^ ERROR: matching over `()` is more explicit
let func: fn(&&mut ()) = |_| {};
//~^ ERROR: matching over `()` is more explicit
let func: fn(&&mut &()) = |_| {};
//~^ ERROR: matching over `()` is more explicit

#[allow(clippy::match_single_binding)]
match &() {
_ => todo!(),
//~^ ERROR: matching over `()` is more explicit
}
}
34 changes: 32 additions & 2 deletions tests/ui/ignored_unit_patterns.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,43 @@ error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:50:13
|
LL | (1, _) => unimplemented!(),
| ^ help: use `()` instead of `_`: `()`
| ^ help: use `()` instead of `_`: `&&&&&()`

error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:57:13
|
LL | for (x, _) in v {
| ^ help: use `()` instead of `_`: `()`

error: aborting due to 9 previous errors
error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:64:26
|
LL | let func: fn(&()) = |_| {};
| ^ help: use `()` instead of `_`: `&()`

error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:66:30
|
LL | let func: fn(&mut ()) = |_| {};
| ^ help: use `()` instead of `_`: `&mut ()`

error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:68:31
|
LL | let func: fn(&&mut ()) = |_| {};
| ^ help: use `()` instead of `_`: `&&mut ()`

error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:70:32
|
LL | let func: fn(&&mut &()) = |_| {};
| ^ help: use `()` instead of `_`: `&&mut &()`

error: matching over `()` is more explicit
--> tests/ui/ignored_unit_patterns.rs:75:9
|
LL | _ => todo!(),
| ^ help: use `()` instead of `_`: `&()`

error: aborting due to 14 previous errors