Skip to content

unwrap_used, expect_used: accept macro result as receiver #14575

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

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

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Apr 9, 2025

changelog: [unwrap_used, expect_used]: lint even when the receiver is a macro expansion result

This also paves the way for expanding more method call lints to expanded receivers or arguments.

Fixes #13455

@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 9, 2025
@samueltardieu
Copy link
Contributor Author

Lintcheck shows many new hits, as expected (mostly with write!() and friends), which is ok since this those are "restriction" lints, and the hits are legitimate.

Comment on lines +4682 to +4690
pub fn method_call_expansion_ok<'tcx>(
recv: &'tcx Expr<'tcx>,
) -> Option<(&'tcx str, &'tcx Expr<'tcx>, &'tcx [Expr<'tcx>], Span, Span)> {
if let ExprKind::MethodCall(path, receiver, args, call_span) = recv.kind {
let name = path.ident.name.as_str();
return Some((name, receiver, args, path.ident.span, call_span));
}
None
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really add anything over just matching inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to make it easy for some lints use the pattern

if let Some((_, outer_receiver, _, _)) = method_call(expr) {
  if let Some((_, inner_receiver, _)) = method_call_expansion_ok(outer_receiver) {}
}

since in some cases they wouldn't care if the inner_receiver was a macro call.

For example, we could easily (I just made the check) warn on

macro_rules! iter { ($e:expr) => { $e.iter() }; }
let _ = iter!([String::new()]).cloned().count();

with

error: unneeded cloning of iterator items
 --> /tmp/t.rs:6:13
  |
6 |     let _ = iter!([String::new()]).cloned().count();
  |             ^^^^^^^^^^^^^^^^^^^^^^---------^^^^^^^^
  |                                   |
  |                                   help: remove

Just by replacing a call to method_call() in methods/mod.rs with a call to method_call_expansion_ok(), and using the right span when printing the error message. There are really no good reasons not to lint when the receiver of .cloned() comes from expansion here, and this function would facilitate that.

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
Development

Successfully merging this pull request may close these issues.

False negative - unwrap_used doesn't trigger on macros that return Result
3 participants