-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix unit_arg
suggests wrongly for Default::default
#14881
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
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @samueltardieu. Use |
unit_arg
suggests wrongly for Default::default
unit_arg
suggests wrongly for Default::default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it behaves as expected in the following cases:
macro_rules! mac {
(def) => { Default::default() };
(nondef $e:expr) => { $e };
(func $f:expr) => { $f() };
}
fn_take_unit(mac!(def));
fn_take_unit(mac!(nondef Default::default()));
fn_take_unit(mac!(func Default::default));
Turns out this lint cannot correctly handle macros at all: fn some_other_fn(_: &i32) {}
macro_rules! another_mac {
() => {
some_other_fn(&Default::default());
};
($e:expr) => {
some_other_fn(&$e);
};
}
fn_take_unit(another_mac!());
fn_take_unit(another_mac!(1)); |
Thank you! Now both the |
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right fix: we should not specialize it for Default::default()
. Any expression whose type is not fully determined without being coerced to ()
is susceptible to trigger this.
For example, the following
fn def<T: Default>() -> T {
Default::default()
}
fn take_unit(_: ()) {}
fn main() {
take_unit(def());
}
exhibits the same problem as the original issue, without using Default::default()
.
clippy_utils::ty::expr_type_is_certain()
might be useful here. If it returns false
, let _: () = …;
might be a good suggestion. And if the expression is default()
, it can be totally removed from the suggestion, unless it comes from a macro (I don't think we should look inside macros).
Closes #14857
changelog: [
unit_arg
] fix wrong suggestion forDefault::default