Add conf to disable disallowed_types in macros

[stepantubanov]

changelog: [disallowed_types]: add configuration to disable in external macros

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[J-ZhengLi]

in_external_macro is kinda... buggy? I assume, it doesn't detects macros with spanned quote for some reason.

So, if you have a macro defined like this:

#[proc_macro_attribute]
pub fn use_std_hash_map(_args: TokenStream, input: TokenStream) -> TokenStream {
    let mut item = syn::parse_macro_input!(input as syn::ItemFn);
    let span = item.block.brace_token.span;
    let block = item.block;
    item.block = syn::parse_quote_spanned! {
        span =>
        {
            let _ = std::collections::HashMap::<i32, i32>::default();
        }
    };
    quote! {#item}.into()
}

It still triggers this lint, I think you'll need a clippy_utils::is_from_proc_macro check additionally.

[stepantubanov]

Added (except for poly trait ref case - PolyTraitRef doesn't implement required trait there).

[y21]

Is this change motivated by some kind of "real" example? I think it would be reasonable that users might still want to be warned when macro-generated code uses a disallowed type? For example a program intended to be compiled to webassembly might want to disallow std::thread::Builder because webassembly has no threads, and they would also want to know about it even if a macro tries to do that.

[stepantubanov]

Is this change motivated by some kind of "real" example? I think it would be reasonable that users might still want to be warned when macro-generated code uses a disallowed type? For example a program intended to be compiled to webassembly might want to disallow std::thread::Builder because webassembly has no threads, and they would also want to know about it even if a macro tries to do that.

In my case I had this issue with OpenApi macro https://docs.rs/poem-openapi/latest/poem_openapi/attr.OpenApi.html:

struct InternalApi {
  ...
}

#[OpenApi(tag = "super::ApiTags::Tables")]
impl InternalApi {
  ...
}

The problem is usage of disallowed types generated by the macro. And the only way to ignore it is to set #![allow(clippy::disallowed_types)] at the module level, which is not great.

May be we could add an attribute allow_in_external_macros (alongside path and reason attributes) for each type and apply based on that?

[xFrednet]

May be we could add an attribute allow_in_external_macros (alongside path and reason attributes) for each type and apply based on that?

I'd suggest adding a configuration value instead. Something like lint-disallowed-in-external-macros. Here are our docs on how to add a new configuration value: adding configuration to a lint

[xFrednet]

Hey @stepantubanov, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[stepantubanov]

@xFrednet

May be we could add an attribute allow_in_external_macros (alongside path and reason attributes) for each type and apply based on that?

I'd suggest adding a configuration value instead. Something like lint-disallowed-in-external-macros. Here are our docs on how to add a new configuration value: adding configuration to a lint

You mean something like this?

# clippy.toml
lint-disallowed-types-in-external-macros = false # default is true

[stepantubanov]

@rustbot ready

[xFrednet]

Yep, I meant something like that. Another option would be to have a list of macros, that can use disallowed types. But thinking about it, this might be too complicated for almost all use cases, unless we add something like "*" to say that all external macros are allowed to use them 🤔.

I believe your current implementation is what I had originally in mind. Any preference or thoughts?

[stepantubanov]

Another option would be to have a list of macros, that can use disallowed types.

This could probably be useful in general, for other lints as well, so it'd be good to have a consistent way of configuring it.

I believe your current implementation is what I had originally in mind. Any preference or thoughts?

Seems good to me.

[xFrednet]

Another option would be to have a list of macros, that can use disallowed types.

This could probably be useful in general, for other lints as well, so it'd be good to have a consistent way of configuring it.

Do you mean the ability to allow lints in macros directly? While that would be cool, I don't think that most of our lints would currently support this. We would need to change all lints to take this new config into consideration.

I'd say it should be the responsibility of the macro author to make sure no lints are triggered, or they are allowed if needed. Even if this POV also has its downsides.

I believe your current implementation is what I had originally in mind. Any preference or thoughts?

Seems good to me.

With this, I assume you mean it seems good to you, to take the current implementation?

I'm now actually considering which option would be better. I'll create a Zulip thread for it, to ask the others. :)

Edit: Zulip Thread

[stepantubanov]

With this, I assume you mean it seems good to you, to take the current implementation?

Personally I would prefer a simple true/false option. If necessary it can later be extended to support list of macros in a backward-compatible way (by implementing custom deserialize that'll accept either a bool or a list of macros).

[xFrednet]

Hey @stepantubanov, the Zulip thread sadly didn't get many responses but it seems like two others agree with my suggestion to have a set if lints instead of a true/false value with "*" as a magic value. Would you mind changing your implementation to support this?

[stepantubanov]

@xFrednet
I started looking into it, but I'm not sure how to implement it. Is there an example of this logic somewhere in other lints? Specifically filtering based on which macro expanded the code.

Also, I'm wondering what would be desired behavior for this configuration when there's a chain of macro expansions (macro expansion contains other macros that also expand).

[xFrednet]

You can take a look at the disallowed_macros implementation. Apparently, it uses macro_backtrace to check the macro chain.

Also, I'm wondering what would be desired behavior for this configuration when there's a chain of macro expansions (macro expansion contains other macros that also expand).

That's a good question I didn't even consider at first. I'd say that the lint should be allowed if any macro in the chain is in the configuration.

[bors]

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.