Skip to content

meta: expanded macros in suggestions break code #6851

@matthiaskrgr

Description

@matthiaskrgr
Member

There have been a couple of problems already with broken suggestions with macros.
The snippet function does not deal with macros very well which is why we have snippet_with_macro_callsite which uses the original macro unexpanded.

For example if we have some kind x.map(|_| vec![]) code, the redundant_closure lint would suggest to replace vec![] with $crate::vec::Vec::new which should just be Vec::new().
cargo clippy --fix fills in the suggestion and the crate no longer builds because what is x.map(|_| $crate::......

I'm wondering, what are valid cases where DO want to have an expanded macro inside a fix suggestion?
Can we fix this class of bugs once and for all by just making the snippet fn not expand macros by default?

Activity

added
C-bugCategory: Clippy is not doing the correct thing
S-needs-discussionStatus: Needs further discussion before merging or work can be started
L-suggestionLint: Improving, adding or fixing lint suggestions
T-macrosType: Issues with macros and macro expansion
I-suggestion-causes-errorIssue: The suggestions provided by this Lint cause an ICE/error when applied
on Mar 5, 2021
camsteffen

camsteffen commented on Mar 5, 2021

@camsteffen
Contributor

Can we fix this class of bugs once and for all by just making the snippet fn not expand macros by default?

What you're really suggesting is to always use span.source_callsite() instead of span by default. I think this would work for 98% of cases. The problem is when we are linting code like

macro_rules! fooey {
    (x:ident) => (x.map(|y| foo(y)));
}

fooey!(x);

Now, I think some of our lints are over-conservative and just drop out at fooey!(x). But there's no reason not to lint this code with redudant_closure. If you take span.source_callsite() from foo(y), I think you would get fooey!(x) which is not what you want. But I haven't confirmed this.

I don't like util methods like snippet_with_macro_callsite which is just a layer of obfuscation over span.source_callsite(), but that's a different issue.

camsteffen

camsteffen commented on Mar 5, 2021

@camsteffen
Contributor

But there's no reason not to lint this code with redudant_closure

Actually I take that back since foo(y) may include an implicit deref depending on what is passed to the macro. But maybe if x did not originate outside the macro? I don't know, macros are sticky. But I think my main point still holds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: Clippy is not doing the correct thingC-questionCategory: QuestionsI-suggestion-causes-errorIssue: The suggestions provided by this Lint cause an ICE/error when appliedL-suggestionLint: Improving, adding or fixing lint suggestionsS-needs-discussionStatus: Needs further discussion before merging or work can be startedT-macrosType: Issues with macros and macro expansion

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @matthiaskrgr@camsteffen

        Issue actions

          meta: expanded macros in suggestions break code · Issue #6851 · rust-lang/rust-clippy