rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.23k stars 12.7k forks source link

function "in_external_macro” judgment is incorrect #131993

Open yu532758082 opened 2 weeks ago

yu532758082 commented 2 weeks ago

I use the derive macro provided by the clap crate in my code, and then call the in_external_macro function in the context of a custom lint diagnostic code. The function determines that the code derived from the derive macro is local code.

I tried this code:

fn main() {
    let _a = 1;
}

use clap::Args;

#[derive(Args)]
pub struct ToFrom {
    #[arg(long = "workspace")]
    a: i32,
}

dependency is clap = { version = "=4.2.0", features = ["derive"] }

My lint code is:

    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
           let res = in_external_macro(cx.sess(), block.span);
        info!("check block in_external_macro res {:?}", res);
        if res == false {
            info!("check block {:?}", block);
        }
}

I expected to see this happen: I think all code generated by derived macros should belong to external macros derived from external crates.

Instead, this happened: Some of the code generated by the derived macro is considered to be local code

The error macro code is expanded as follows. The blocks inside the .arg function are considered native code.

                .arg({
                    #[allow(deprecated)]
                    let arg = clap::Arg::new("a")
                        .value_name("A")
                        .required(true && clap::ArgAction::Set.takes_values())
                        .value_parser({
                            use ::clap_builder::builder::via_prelude::*;
                            let auto = ::clap_builder::builder::_AutoValueParser::<
                                i32,
                            >::new();
                            (&&&&&&auto).value_parser()
                        })
                        .action(clap::ArgAction::Set);
                    let arg = arg.long("workspace");
                    let arg = arg.required(false);
                    arg
                });

rustc --version --verbose:

+nightly-2024-03-07
jieyouxu commented 2 weeks ago

Can you please provide a complete reproducer, including how your lint is setup? It doesn't need to be minimal, I just need something that's more complete instead of having to figure out how to reproduce the issue you are observing.

Is this in the context of clippy lints, or rustc lints, or some other custom lint framework?

yu532758082 commented 2 weeks ago

Can you please provide a complete reproducer, including how your lint is setup? It doesn't need to be minimal, I just need something that's more complete instead of having to figure out how to reproduce the issue you are observing.

Is this in the context of clippy lints, or rustc lints, or some other custom lint framework?

My lint context is custom, but the logic is similar to clippy, which should have no effect on the function return value.

jieyouxu commented 2 weeks ago

My lint context is custom, but the logic is similar to clippy, which should have no effect on the function return value.

I can't test your changes because I don't have a complete example, but, if you point your code to use a local clap checkout clap = { path = "my/local/checkout", features = ["derive"] }, then modify that clap checkout's derive proc-macro https://github.com/clap-rs/clap/blob/61f5ee514f8f60ed8f04c6494bdf36c19e7a8126/clap_derive/src/derives/args.rs

such that all the code with the code pattern matching quote_spaned! { $SPAN => $PARTIAL }

quote_spanned! { ty.span()=>
                #arg_matches.#get_many(#id)
                    .map(|v| v.collect::<Vec<_>>())
                    .unwrap_or_else(Vec::new)
            }

becomes quote! { $PARTIAL }, i.e. stop forwarding the user's span without attaching a layer of syntax context

quote! { #arg_matches.#get_many(#id)
                    .map(|v| v.collect::<Vec<_>>())
                    .unwrap_or_else(Vec::new)
}

or maybe

quote_spanned! { Span::call_site().located_at(ty.span()) =>
                #arg_matches.#get_many(#id)
                    .map(|v| v.collect::<Vec<_>>())
                    .unwrap_or_else(Vec::new)
            }

Does making any of the two changes cause in_external_macro in your custom lint to no longer report false?

yu532758082 commented 2 weeks ago

My lint context is custom, but the logic is similar to clippy, which should have no effect on the function return value.

I can't test your changes because I don't have a complete example, but, if you point your code to use a local clap checkout clap = { path = "my/local/checkout", features = ["derive"] }, then modify that clap checkout's derive proc-macro https://github.com/clap-rs/clap/blob/61f5ee514f8f60ed8f04c6494bdf36c19e7a8126/clap_derive/src/derives/args.rs

such that all the code with the code pattern matching quote_spaned! { $SPAN => $PARTIAL }

quote_spanned! { ty.span()=>

arg_matches.#get_many(#id)

                .map(|v| v.collect::<Vec<_>>())
                .unwrap_or_else(Vec::new)
        }

becomes quote! { $PARTIAL }, i.e. stop forwarding the user's span without attaching a layer of syntax context

quote! { #arg_matches.#getmany(#id) .map(|v| v.collect::<Vec<>>()) .unwrap_or_else(Vec::new) } or maybe

quote_spanned! { Span::call_site().located_at(ty.span()) =>

arg_matches.#get_many(#id)

                .map(|v| v.collect::<Vec<_>>())
                .unwrap_or_else(Vec::new)
        }

Does making any of the two changes cause in_external_macro in your custom lint to no longer report false?

I change quote_spam! to quote but not change the result

yu532758082 commented 15 hours ago

I will add a way to reproduce it

1、Download clippy source code 2、 In clippy_lints/src/absulutepath.rs Add inside ` impl LateLintPass<'> for AbsolutePaths {} `:

       use rustc_hir::{Block, HirId, ItemKind, Node, Path};
       use rustc_lint::{LateContext, LateLintPass, LintContext};
        use rustc_middle::lint::in_external_macro;
           fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) {
        let res = in_external_macro(cx.sess(), block.span);
        println!("check block in_external_macro res {:?}", res);
        if res == false {
            println!("check block {:?}", block);
        }
    }

3、 Add "clap" in dev-dependency in Outermost layer Cargi.toml [dev-dependencies]
clap = { version = "=4.2.0", features = ["derive"] } 4、Add dependencies in compile-test add

     extern crate clap;

/// All crates used in UI tests are listed here
static TEST_DEPENDENCIES: &[&str] = &[
    "clippy_config",
    "clippy_lints",
    "clippy_utils",
    "futures",
    "if_chain",
    "itertools",
    "parking_lot",
    "quote",
    "regex",
    "serde_derive",
    "serde",
    "syn",
    "tokio",
    "clap",   // new added
];

5、Add a ui-test in tests/ui create bug.rs

fn main() {
    let _a = 1;
}

use clap::Args;

#[derive(Args)]
pub struct ToFrom {
    #[arg(long = "workspace")]
    a: i32,
}

6、 run cargo test --test compile-test bug.rs -- --bless in rust-clippy direcetory

7、 we can see

check block in_external_macro res false
check block Block { stmts: [Stmt { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).50), kind: Local(Local { pat: Pat { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).105), kind: Binding(BindingAnnotation(No, Not), HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).105), arg#0, None), span: tests/ui/bug.rs:10:5: 11:11 (#0), default_binding_modes: true }, ty: None, init: Some(Expr { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).51), kind: MethodCall(PathSegment { ident: action#0, hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).52), res: Err, args: None, infer_args: true }, Expr { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).53), kind: MethodCall(PathSegment { ident: value_parser#0, hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).54), res: Err, args: None, infer_args: true }, Expr { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).55), kind: MethodCall(PathSegment { ident: required#0, hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).56), res: Err, args: None, infer_args: true }, Expr { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).57), kind: MethodCall(PathSegment { ident: value_name#0, hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).58), res: Err, args: None, infer_args: true }, Expr { hir_id: HirId(DefId(0:15 ~ bug[2a17]::{impl#1}::augment_args).59), kind: Call(E

This corresponds to the macro expansion part Image

I think this block should return true for the in_external_macro function.