-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Don't show no_effect warning on unit structs implementing fn_once #7898
Conversation
r? @camsteffen (rust-highfive has picked a reviewer for you, use r? to override) |
clippy_lints/src/no_effect.rs
Outdated
Res::Def(DefKind::Struct | DefKind::Variant | DefKind::Ctor(..), ..) | ||
cx.qpath_res(qpath, callee.hir_id), | ||
Res::Def( | ||
DefKind::Struct | DefKind::Variant | DefKind::Ctor(_, CtorKind::Fn | CtorKind::Fictive), |
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 this is the right fix. Perhaps try inspecting tcx.type_dependent_def(callee.hir_id)
.
Add a test with
struct GreetStruct(u32);
impl FnOnce<(&str,)> for GreetStruct {
type Output = ();
extern "rust-call" fn call_once(self, (who,): (&str,)) -> Self::Output {
println!("hello {}", who);
}
}
fn main() {
GreetStruct(1)("world");
}
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.
@camsteffen Thanks for suggestion, I am not that familiar with the rustc API.
I looked into it briefly but it gives none
as result, I think that it requires MethodCall and not Call as ExprKind.
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.
Hmm yeah I haven't seen it work with Call
either, it was just a guess...
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.
Ah did you try expr.hir_id
?
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.
@camsteffen I've tried it with expr.hir_id and it works. So one option would be to add something like this:
fn is_fn_once(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> bool {
if_chain! {
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
if match_def_path(cx, def_id, &paths::FN_ONCE_CALL_ONCE);
then {
return true;
}
}
false
}
Hardcoded path is used as it seems FnOnce
trait has no corresponding Symbol. But I've noticed we also have false positive for FnMut
and probably Fn
case, so those should also be added.
Other way that we could go is more similar to first solution I had, if we use:
Res::Def(DefKind::Variant | DefKind::Ctor(_, CtorKind::Fn | CtorKind::Fictive), ..)
in match instead of Res::Def(DefKind::Struct | DefKind::Variant | DefKind::Ctor(..), ..)
we avoid these cases. I think original intention was to cover construction of touples (as far as I can see from tests) but implementation seems to be too broad.
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 think you can just do this
ExprKind::Call(callee, args) => {
if cx.typeck_results().type_dependent_def(expr.hir_id).is_some() {
// type-dependent function call like `impl FnOnce for X`
return false;
}
..
Thanks! @bors r+ |
📌 Commit e9bf5ec has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #7792
changelog: Don't show [
no_effect
] or [unecessary_operation
] warning for unit struct implementing FnOnce