rust-lang / rust

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

Lints stop firing if macro reports with `Span::call_site`? #124145

Open workingjubilee opened 4 months ago

workingjubilee commented 4 months ago

I was trying to apply this diff:

Code

         let variants = self.variants.iter().map(|variant| variant.ident.clone());
-        let sql_graph_entity_fn_name =
-            syn::Ident::new(&format!("__pgrx_internals_enum_{}", name), Span::call_site());
+        let sql_graph_entity_fn_name = quote::format_ident!("__pgrx_internals_enum_{}", name);

Current output

warning: function `__pgrx_internals_enum_SomeValue` should have a snake case name
  --> pgrx-examples/custom_types/src/generic_enum.rs:15:10
   |
15 | pub enum SomeValue {
   |          ^^^^^^^^^ help: convert the identifier to snake case: `__pgrx_internals_enum_some_value`
   |
   = note: `#[warn(non_snake_case)]` on by default

   Compiling schemas v0.0.0 (/home/jubilee/tcdi/pgrx/pgrx-examples/schemas)

Desired output

None?

Rationale and extra context

This takes in an enum's name, so of course it formats out to using __pgrx_internals_enum_SomeEnum or whatever. But format_ident is basically just constructing an Ident but with the Span of the original ident it is based on. Apparently the expanded code triggers lints only if it has that span, but Span::call_site does not! It seems odd/annoying for the lint to ignore irrational spans like this but punish me for trying to forward the correct span so that errors get reported more clearly from the macro.

This is slightly different from https://github.com/rust-lang/rust/issues/24580 because it's about the lint firing being inconsistent but I don't mind if it's closed for technically duplication I guess.

Other cases

No response

Rust Version

rustc 1.79.0-nightly (ccfcd950b 2024-04-15)
binary: rustc
commit-hash: ccfcd950b333fed046275dd8d54fe736ca498aa7
commit-date: 2024-04-15
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.3

Anything else?

No response

workingjubilee commented 4 months ago

honestly the main reason this issue was opened is because it seems like the only reason that this would vary so much is a serious bug in the linting framework.

workingjubilee commented 4 months ago

wait, am I supposed to be writing Span::call_site().located_at(source_span) to point the error at the source while making the span aware it is a "macro-tainted" span? doing that seems to cause the lints to stop firing...

jieyouxu commented 4 months ago

honestly the main reason this issue was opened is because it seems like the only reason that this would vary so much is a serious bug in the linting framework.

IIRC, many lints don't try to handle macro-generated code (or code with mixed soures) because it's a lot of complexity. The lints typically use Span::from_expansion to check if a span is macro-generated, and assumes that spans generated from proc macros properly account for hygiene. In this case, I think you need Span::mixed_site().located_at(user_span) in order for the span to be marked as "generated from macros" (from your proc macro) so lints can avoid firing on macro-generated spans.

workingjubilee commented 4 months ago

for note, just using Span::call_site, without redirecting the span to its source in some way, can result in the span just touching a single character in an obviously-illogical way, which can be quite vexing. I think this is a consequence mostly of nested proc macro expansions? -Zmacro-backtrace doesn't really illuminate this much, as far as I've noticed.