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

Verify that the AST does not have DUMMY_SP #35148

Open
arielb1 opened this issue Jul 31, 2016 · 5 comments
Open

Verify that the AST does not have DUMMY_SP #35148

arielb1 opened this issue Jul 31, 2016 · 5 comments
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-proc-macros Area: Procedural macros C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Jul 31, 2016

Currently, syntax extensions can generate code with DUMMY_SP in its AST. All compiler diagnostics from that code, and all stack backtraces generated by that code, are basically worthless.

That should not be possible. The lowered HIR should not have any DUMMY_SP.

cc @rust-lang/compiler

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 31, 2016

I think that if a syntax extension generates code with DUMMY_SP, we should use the span of its caller instead.

@arielb1 arielb1 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-syntaxext Area: Syntax extensions I-nominated labels Jul 31, 2016
@cgswords
Copy link
Contributor

cgswords commented Aug 4, 2016

While this is a good idea, it seems a better idea is to have the expander walk the plugin's output and respan anything with a DUMMY_SP to have the span of the macro invocation site. Doing this as part of expansion has a few advantages:

  1. We don't have to produce errors here or worry about those checks during HIR lowering
  2. This takes a lot of responsibility off of the plugin writer (most importantly, they won't have to pass span information around to recursive calls, subsequent (and generated) invocations, etc.)
  3. It also future-proofs the plugin system (eg when contexts aren't provided in Macros 2.0).

To elaborate on the third, when contexts go away, the user is not guaranteed to even receive anything with a span on it (in the case that the macro's input is empty). To fix this, either the macro will need to take a span to use, take the whole invocation form as input (okay, but still, a macro writer have to get and thread spans around), or need some other way to get to these spans (TLS, I guess). If the expander takes care of this, however, none of this will ever become a concern.

@Mark-Simulacrum
Copy link
Member

As far as I know nothing has been done related to this. @nrc @jseyfried This seems relevant to the macros 2.0 work, perhaps that will solve or help some of this?

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@jyn514
Copy link
Member

jyn514 commented Jul 14, 2021

I'm not sure if this is still relevant - can proc macros still generate code with DUMMY_SP? How could I test it?

@jyn514 jyn514 added A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros A-proc-macros Area: Procedural macros and removed A-syntaxext Area: Syntax extensions A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros labels Jul 14, 2021
@petrochenkov
Copy link
Contributor

It would be interesting to insert a check to AST -> HIR lowering to figure out what sources are dummy spans coming from in practice.

I think at least one legitimate source of dummy spans right now is prelude injection (and maybe proc macro harness and test harness injection).

If #84373 is merged, then we'll have a single point to place such check in - fn lower_span, but a commit introducing lower_span can be landed independently from whole #84373 if necessary.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 29, 2021
ast_lowering: Introduce `lower_span` for catching all spans entering HIR

This PR cherry-picks the `fn lower_span` change from rust-lang#84373.
I also introduced `fn lower_ident` for lowering spans in identifiers, and audited places where HIR structures with spans or identifiers are constructed and added a few missing `lower_span`s/`lower_ident`s.

Having a hook for spans entering HIR can be useful for things other than rust-lang#84373, e.g. rust-lang#35148.
I also want to check whether this change causes perf regressions due to some accidental inlining issues.

r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-proc-macros Area: Procedural macros C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

6 participants