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

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

Open
workingjubilee opened this issue Apr 19, 2024 · 4 comments
Open

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

workingjubilee opened this issue Apr 19, 2024 · 4 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Apr 19, 2024

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 #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 workingjubilee added A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Apr 19, 2024
@workingjubilee
Copy link
Member Author

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
Copy link
Member Author

workingjubilee commented Apr 19, 2024

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
Copy link
Member

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
Copy link
Member Author

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.

workingjubilee added a commit to pgcentralfoundation/pgrx that referenced this issue Apr 19, 2024
Apply a simple change to -sql-entity-graph and -macros:

Use `format_ident!` for every case we format... an identifier... and we
don't change the identifier, and reuse the existing span or worse, drop
the span on the floor by calling `Span::call_site`. This will, in
general, improve the error-reporting of spans so that errors refer back
to the original input that provoked the expansion.

It is possible we should by relocating synthesized spans, see
rust-lang/rust#124145 for more details. This
change is still preferred because it makes it easier to refactor these
again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants