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

Add -Z span-debug to allow for easier debugging of proc macros #72799

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

Aaron1011
Copy link
Member

Currently, the Debug impl for proc_macro::Span just prints out
the byte range. This can make debugging proc macros (either as a crate
author or as a compiler developer) very frustrating, since neither the
actual filename nor the SyntaxContext is displayed.

This commit adds a perma-unstable flag -Z span-debug. When enabled,
the Debug impl for proc_macro::Span simply forwards directly to
rustc_span::Span. Once #72618 is merged, this will start displaying
actual line numbers.

While Debug impls are not subject to Rust's normal stability
guarnatees, we probably shouldn't expose any additional information on
stable until #![feature(proc_macro_span)] is stabilized. Otherwise,
we would be providing a 'backdoor' way to access information that's
supposed be behind unstable APIs.

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2020
@Mark-Simulacrum
Copy link
Member

I would personally lean towards avoiding the -Z flag and just implementing Debug for the proc_macro::Span type. If people are parsing the output of Debug impls then they're basically asking for trouble, I think it's fine if we end up breaking that in the future. (We can also add an explicit note to the documentation that such parsing is not permitted, and potentially even to the debug output itself).

@Aaron1011
Copy link
Member Author

Aaron1011 commented May 30, 2020

@Mark-Simulacrum: In the discussion about proc_macro_span, @matklad had raised some concerns about the interaction with incremental compilation: #54725 (comment)

If people are parsing the output of Debug impls then they're basically asking for trouble

I agree. However, I'd like to avoid even the possibility of breakage until some kind of consensus has been reached about what we do or do not want to expose to proc macros. Even if the majority of crates don't abuse the Debug impl, it only takes a few to complicate things.

@Aaron1011
Copy link
Member Author

Additionally, I don't think we'd ever want to forward to an internal Debug impl for a stable method. If we do end up changing the output on stable, we should probably decide on a desired format, and print exactly the fields we want.

src/librustc_expand/base.rs Outdated Show resolved Hide resolved
src/librustc_session/options.rs Outdated Show resolved Hide resolved
src/librustc_session/session.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

petrochenkov commented May 31, 2020

I'd personally keep this as a private patch that I apply only when actually debugging tokens streams (it's a one line change), but -Z flag is unstable and not a high cost, so it should be ok I guess.

@petrochenkov
Copy link
Contributor

To elaborate, you never know what you'll need during next debugging.
Today it's more detailed span info, tomorrow it's some additional information about types or logging in some random place, I already said this about debug! in #68941 (comment), same argument applies here.
So I remove all my debugging stuff before submitting PRs, for example.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2020
@varkor varkor removed their assignment May 31, 2020
@Aaron1011
Copy link
Member Author

To elaborate, you never know what you'll need during next debugging.
Today it's more detailed span info

My goal is to make the Span debugging experience more consistent - right now, you get a useful debug printout from compiler code, but a pretty useless one from a proc macro.

A -Z flag also has the advantage that it can easily be enabled when bisecting nightly builds.

@Aaron1011 Aaron1011 force-pushed the feature/span-debug branch from 46fbaa0 to f3f822e Compare May 31, 2020 20:21
@Aaron1011
Copy link
Member Author

@petrochenkov: Updated

@petrochenkov
Copy link
Contributor

Thanks!
r=me

(This will fail tidy and conflict with #72618 though.)

@rust-highfive

This comment has been minimized.

@Aaron1011 Aaron1011 force-pushed the feature/span-debug branch from f3f822e to b583f33 Compare June 4, 2020 19:03
@Aaron1011
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 4, 2020

📌 Commit b583f33a7db66ffce3394e4dce7d663ddce2820c has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2020
@petrochenkov
Copy link
Contributor

@Aaron1011
Could you address #72799 (comment)?

@Aaron1011
Copy link
Member Author

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2020
Currently, the `Debug` impl for `proc_macro::Span` just prints out
the byte range. This can make debugging proc macros (either as a crate
author or as a compiler developer) very frustrating, since neither the
actual filename nor the `SyntaxContext` is displayed.

This commit adds a perma-unstable flag `-Z span-debug`. When enabled,
the `Debug` impl for `proc_macro::Span` simply forwards directly to
`rustc_span::Span`. Once rust-lang#72618 is merged, this will start displaying
actual line numbers.

While `Debug` impls are not subject to Rust's normal stability
guarnatees, we probably shouldn't expose any additional information on
stable until `#![feature(proc_macro_span)]` is stabilized. Otherwise,
we would be providing a 'backdoor' way to access information that's
supposed be behind unstable APIs.
@Aaron1011 Aaron1011 force-pushed the feature/span-debug branch from b583f33 to b541d3d Compare June 4, 2020 19:39
@Aaron1011
Copy link
Member Author

@petrochenkov: Fixed

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2020

📌 Commit b541d3d has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
…rochenkov

Add `-Z span-debug` to allow for easier debugging of proc macros

Currently, the `Debug` impl for `proc_macro::Span` just prints out
the byte range. This can make debugging proc macros (either as a crate
author or as a compiler developer) very frustrating, since neither the
actual filename nor the `SyntaxContext` is displayed.

This commit adds a perma-unstable flag `-Z span-debug`. When enabled,
the `Debug` impl for `proc_macro::Span` simply forwards directly to
`rustc_span::Span`. Once rust-lang#72618 is merged, this will start displaying
actual line numbers.

While `Debug` impls are not subject to Rust's normal stability
guarnatees, we probably shouldn't expose any additional information on
stable until `#![feature(proc_macro_span)]` is stabilized. Otherwise,
we would be providing a 'backdoor' way to access information that's
supposed be behind unstable APIs.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
…rochenkov

Add `-Z span-debug` to allow for easier debugging of proc macros

Currently, the `Debug` impl for `proc_macro::Span` just prints out
the byte range. This can make debugging proc macros (either as a crate
author or as a compiler developer) very frustrating, since neither the
actual filename nor the `SyntaxContext` is displayed.

This commit adds a perma-unstable flag `-Z span-debug`. When enabled,
the `Debug` impl for `proc_macro::Span` simply forwards directly to
`rustc_span::Span`. Once rust-lang#72618 is merged, this will start displaying
actual line numbers.

While `Debug` impls are not subject to Rust's normal stability
guarnatees, we probably shouldn't expose any additional information on
stable until `#![feature(proc_macro_span)]` is stabilized. Otherwise,
we would be providing a 'backdoor' way to access information that's
supposed be behind unstable APIs.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#72026 (Update annotate-snippets-rs to 0.8.0)
 - rust-lang#72583 (impl AsRef<[T]> for vec::IntoIter<T>)
 - rust-lang#72615 (Fix documentation example for gcov profiling)
 - rust-lang#72761 (Added the documentation for the 'use' keyword)
 - rust-lang#72799 (Add `-Z span-debug` to allow for easier debugging of proc macros)
 - rust-lang#72811 (Liballoc impl)
 - rust-lang#72963 (Cstring `from_raw` and `into_raw` safety precisions)
 - rust-lang#73001 (Free `default()` forwarding to `Default::default()`)
 - rust-lang#73075 (Add comments to `Resolve::get_module`)
 - rust-lang#73092 (Clean up E0646)

Failed merges:

r? @ghost
@bors bors merged commit e135087 into rust-lang:master Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants