Stop emitting spans from proc macro compile time in quote expansion

[bjorn3]

Before this commit if the proc_macro::quote!{} macro was used, the span of each token as written in the source of the proc macro itself would be saved in the crate metadata of the proc macro and then recovered at proc macro runtime to forward this to the macro expansion of the proc macro. This commit stops doing this and instead generates def-site spans for each token. This removes the only case where spans from the proc macro source have a semantic effect on the compilation of crates that use the proc macro. This makes it easier to stop requiring all dependencies of proc macros to be present when using the proc macro. And will make it easier to stop requiring a proc macro to be present when using a crate that used this proc macro internally but doesn't expose it as part of it's public api. The latter is necessary to be able to cross-compile tools that link against rustc internals without requiring to be built as part of rustc with the -Zdual-proc-macro hack. It may also enable using proc macros inside the standard library or it's dependencies without breaking cross-compilation.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[bjorn3]

While Span::def_site() is unstable, so is quote!{}. In addition Span::def_site() is the closest to the behavior before this PR.

[petrochenkov]

cc #84278 @Aaron1011

[rustbot]

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

[bjorn3]

Forgot to mention two extra reasons for removing this:

• Rust-analyzer can't support it as it can't load spans from the crate metadata, nor does it have a span format compatible with rustc.
• The current quote! implementation is likely buggy. I think if quote! is used inside a dependency of a proc macro, the span will not be saved or would be saved only inside the table for the dependency, but then at runtime it would be looked up in the table for the proc macro itself, which would either crash or return the wrong span depending on how many spans the proc macro has saved.

[Aaron1011]

This feels like a really unfortunate regression, and I'd like to avoid if at all possible.

This makes it easier to stop requiring all dependencies of proc macros to be present when using the proc macro. And will make it easier to stop requiring a proc macro to be present when using a crate that used this proc macro internally but doesn't expose it as part of it's public api.

Can you elaborate on this? Setting aside hygiene, could we make this 'best-effort' (similar to how we try to load the span source file from disk), where we won't show a span location + backtrace in error messages if the needed proc-macro dependency is missing? I would expect this to be unobserable to proc-macros (expect for the unstable span inspection apis, which I assume would have a similar problem with 'normal' cross-crate spans).

Rust-analyzer can't support it as it can't load spans from the crate metadata, nor does it have a span format compatible with rustc.

I'm not familiar with how proc-macros and spans are handled in rust-analyzer, but couldn't it keep falling back to Span::def_site?

The current quote! implementation is likely buggy. I think if quote! is used inside a dependency of a proc macro, the span will not be saved or would be saved only inside the table for the dependency, but then at runtime it would be looked up in the table for the proc macro itself, which would either crash or return the wrong span depending on how many spans the proc macro has saved.

I think you're right - however, I think it could be easily fixed by also encoding a crate number / identifier (and gracefully falling back to Span::def_site() if that crate isn't loaded at runtime).

[Veykril]

I'm not familiar with how proc-macros and spans are handled in rust-analyzer, but couldn't it keep falling back to Span::def_site?

It could (or well it actually does, though incorrectly, currently we incorrectly return call site hygiene). Though it would be really nice if we could actually have the proper spans here accessible (how ours differ doesn't really matter, the point is that ours will always differ as rustc's spans are generally unusable to us). r-a side issue tracking this rust-lang/rust-analyzer#16464

[petrochenkov]

The current quote! implementation is likely buggy. I think if quote! is used inside a dependency of a proc macro, the span will not be saved or would be saved only inside the table for the dependency, but then at runtime it would be looked up in the table for the proc macro itself, which would either crash or return the wrong span depending on how many spans the proc macro has saved.

That's something that I hoped to fix eventually.
Right now dependencies of proc macros are (partially?) pruned from the dependency tree, or at least spans from them degrade to something less precise. Ideally the full dependency tree would be correctly preserved.

Of course, alternatively, proc macro crates could be redesigned to give a guarantee of never being needed after doing their work and producing some code. E.g. if we have a compiled rlib with some of source code originally generated by proc macros, that rlib won't contain any traces of the proc macro crates used for generation.
Not sure if it's possible (maybe it is), and which exactly abilities proc macros will have to lose to achieve that, but in any case it's a pretty big design decision.

[bjorn3]

Can you elaborate on this? Setting aside hygiene, could we make this 'best-effort' (similar to how we try to load the span source file from disk), where we won't show a span location + backtrace in error messages if the needed proc-macro dependency is missing? I would expect this to be unobserable to proc-macros (expect for the unstable span inspection apis, which I assume would have a similar problem with 'normal' cross-crate spans).

The expansion context (including hygiene) is the exact thing I'm worried about. If you want to preserve just source location we could encode the start and end BytePos of the span instead. That would still get most of the benefits of this PR I think.

[bjorn3]

That's something that I hoped to fix eventually.
Right now dependencies of proc macros are (partially?) pruned from the dependency tree, or at least spans from them degrade to something less precise. Ideally the full dependency tree would be correctly preserved.

encode_crate_deps contains a empty_proc_macro!(self); call, ensuring that it is skipped for proc macros.

Of course, alternatively, proc macro crates could be redesigned to give a guarantee of never being needed after doing their work and producing some code. E.g. if we have a compiled rlib with some of source code originally generated by proc macros, that rlib won't contain any traces of the proc macro crates used for generation.
Not sure if it's possible (maybe it is), and which exactly abilities proc macros will have to lose to achieve that, but in any case it's a pretty big design decision.

I want to see this implemented and making this easier to implement is one of the goals of this PR. I think with this PR it is doable if the proc macro is marked as private dependency. Proc macros already skip most of their crate metadata generation anyway, so it is hard to accidentally depend on the crate metadata of a proc macro after expansion, other than through spans and expansion contexts.

[petrochenkov]

Some history:

• rustc_metadata: Load metadata for indirect macro-only dependencies #69432 by me removed an incorrect optimization skipping some indirect macro dependencies during crate loading.
• Treat dependencies of proc-macro crates like normal dependencies #69976 attempted to remove more of those incorrect optimizations but wasn't merged for performance reasons.
• Serialize all foreign SourceFiles into proc-macro crate metadata #73706 was merged instead, it implemented a way to "inline" spans from dependencies of proc-macro crates into those proc-macro crates, it allowed to drop dependencies of proc-macro crates from the dependency graph.
• metadata: Span data and SourceFiles for macro-only dependencies can be inlined #74731 is a tracking issue for "inlining" more spans from proc-macro crates themselves into libraries using them.

So things were more nuanced than I recalled in #125721 (comment).

[petrochenkov]

From those threads it's clear that proc-macro crates cannot always be eliminated from the graph.

• If a library reexports something from a proc-macro crate, the proc-macro crate needs to be kept.
• If a library has a public macro M generated by a proc-macro crate PM, then proc-macro crate almost certainly needs to be kept because def-site contexts from M may point to PM (assuming macros 2.0 exist). I'm pretty sure we already have some similar incorrect behavior in macro-generated proc-macros (or at least it is different from other macro-generated macros).
  • Upd: Although we could postulate that every proc macro has a unique empty def site environment, to enable optimizations.
    It will break proc macros referring to their neighbor proc macros with crate::my_neighbor_proc_macro, that mostly affects macros using unstable Span::def_site, but in theory may affect stable macros using Span::mixed_site in combination with $crate (which is probably very rare).
• Are there any other cases where it needs to be kept (besides just showing code locations in diagnostics)?
  I'm honestly not sure. Hygiene data, for example, may be used by some method call or associated item disambiguation machinery. What if an associated item is defined by a macro? Can the macro's def site be significant for such disambiguation? We really need to add a bunch of asserts to the compiler catching interesting cases like this.

[petrochenkov]

@bjorn3
Do you plan to work on this topic further in some near future?
I wouldn't want to end up in the situation where some useful functionality like quote diagnostics is removed, but proc-macros are still always required to be kept around.

[bjorn3]

How about removing the expansion context but keeping the source locations for the serialized spans? Foreign source locations are already inlined in the local proc-macro metadata since #73706 if I understand correctly so keeping them avoids regressing diagnostics without being unable to skip loading dependencies of proc macros, but expansion contexts refer to DefId that can't be just inlined as multiple proc-macros could share the same dependency, in which case the DefId has to be the same.

From those threads it's clear that proc-macro crates cannot always be eliminated from the graph.

Can macros 2.0 refer to private dependencies? If not all of these cases are disallowed if the proc macro dependency is marked as private, right?

[petrochenkov]

How about removing the expansion context but keeping the source locations for the serialized spans?

Yeah, something similar to #73706 would be the best solution for things shown in diagnostics, I think.

Can macros 2.0 refer to private dependencies?

Private in which sense?
That --extern priv:mycrate cargo feature (link) or something else? Macros 2.0 are not aware of that.

[bjorn3]

That --extern priv:mycrate cargo feature

Yes

Macros 2.0 are not aware of that.

👍

[bors]

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

[JohnCSimon]

@bjorn3 ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

[bjorn3]

There is some discussion ongoing in #54722 about how exactly we want quote to behave with respect to spans.