Skip to content
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

redundant_closure_call false positive in macros #1553

Closed
Tracked by #12046
tormol opened this issue Feb 18, 2017 · 7 comments
Closed
Tracked by #12046

redundant_closure_call false positive in macros #1553

tormol opened this issue Feb 18, 2017 · 7 comments
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have S-needs-discussion Status: Needs further discussion before merging or work can be started T-macros Type: Issues with macros and macro expansion

Comments

@tormol
Copy link

tormol commented Feb 18, 2017

Is there a better way to delegate the body of a method to a parameter?

@kornelski
Copy link
Contributor

I've ran into the same problem. I use a closure in a macro to work around macro hygiene and pass variables to an expression provided by the macro caller.

macro_rules! … {
…
    for (s,d) in src.iter().zip(dst.iter_mut()) {
        ($fix)(s,d);
    }
…
}

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2017

I think it would be fine to simply not trigger the lint inside macros.

Note that you can write this without closures, by passing in the names of s and d as $s:ident and $d:ident to the macro. This should result it the same amount of code written at the macro invocation.

@oli-obk oli-obk added S-needs-discussion Status: Needs further discussion before merging or work can be started good-first-issue These issues are a good way to get started with Clippy labels Mar 7, 2017
@phansch phansch added C-bug Category: Clippy is not doing the correct thing T-macros Type: Issues with macros and macro expansion labels Apr 21, 2019
@Kixunil
Copy link

Kixunil commented Nov 25, 2020

Hit this right now. Yes the macro could be rewritten to do it differently but I believe that passing a closure to macro is the most readable way to write it.

This problem can be bypassed by writing (($closure))(args) (use double parentheses) but making the lint saner should still be considered.

@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 18, 2020
@jpmckinney
Copy link

jpmckinney commented Mar 3, 2023

Note that you can write this without closures, by passing in the names of s and d as $s:ident and $d:ident to the macro. This should result it the same amount of code written at the macro invocation.

So would this be something like:

macro_rules! mymacro {
    ( $s:ident, $d:ident, $fix:expr ) => {for ($s, $d) in src.iter().zip(dst.iter_mut()) {
            $fix;
        }};
}

where $fix (which as a closure might have been |s, d| ...) just becomes the ... part?

I believe that passing a closure to macro is the most readable way to write it.

I agree, this:

mymacro!(..., (|s, d| s + d))

is easier to read than:

mymacro!(..., s, d, s + d)

(especially as the s + d expression becomes longer, and as the number of arguments increases).

This problem can be bypassed by writing (($closure))(args) (use double parentheses) but making the lint saner should still be considered.

This worked for me, except because cargo fmt removes the doubling, I add one set of parens in the macro definition and another set of parens in the macro call.

@kupiakos
Copy link

this:

mymacro!(..., (|s, d| s + d))

is easier to read than:

mymacro!(..., s, d, s + d)

(especially as the s + d expression becomes longer, and as the number of arguments increases).

@jpmckinney That's not the only way to refactor the macro. It could be rewritten to (scalably) take mymacro!(..., (s, d) => s + d) with ($($params:ident),*) => $fix:expr or even a closure-like syntax with |$($params:ident),*| $fix:expr.

@Kixunil
Copy link

Kixunil commented Jan 15, 2024

@kupiakos note that if you rewrite the macro to always take closure syntax then you can't pass in function pointers. Allowing any callable expression is best and the lint should be fixed.

bors added a commit that referenced this issue Jan 27, 2024
Fixed FP in `redundant_closure_call` when closures are passed to macros

There are cases where the closure call is needed in some macros, this in particular occurs when the closure has parameters. To handle this case, we allow the lint when there are no parameters in the closure, or the closure is outside a macro invocation.

fixes: #11274 #1553
changelog: FP: [`redundant_closure_call`] when closures with parameters are passed in macros.
h4nsu added a commit to Cardinal-Cryptography/PSP22 that referenced this issue Mar 26, 2024
@m-rph
Copy link
Contributor

m-rph commented Apr 27, 2024

Fixed through #12082

@m-rph m-rph closed this as completed Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have S-needs-discussion Status: Needs further discussion before merging or work can be started T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

No branches or pull requests

10 participants