Improve rustc_parse::Parser's debuggability
#124779
Conversation
This was added as pub in 2021 and remains only privately used in 2024!
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
workingjubilee
force-pushed
the
debug-formatting-my-beloved
branch
from
6f20e45 to
fe3ad37
Compare
| @@ -211,9 +211,10 @@ pub type ReplaceRange = (Range<u32>, Vec<(FlatToken, Spacing)>); | |||
| /// Controls how we capture tokens. Capturing can be expensive, | |||
| /// so we try to avoid performing capturing in cases where | |||
| /// we will never need an `AttrTokenStream`. | |||
| #[derive(Copy, Clone)] | |||
| #[derive(Copy, Clone, Default)] | |||
There was a problem hiding this comment.
Is this default used anywhere? If not, please let's keep this explicit. I think "this is the default mode" doesn't mean "this needs a Default impl", it just means "this is what's used in Parser::new").
| @@ -121,7 +121,7 @@ macro_rules! maybe_recover_from_interpolated_ty_qpath { | |||
| }; | |||
| } | |||
|
|
|||
| #[derive(Clone, Copy, Debug)] | |||
| #[derive(Clone, Copy, Debug, PartialEq)] | |||
There was a problem hiding this comment.
Why? I would prefer if people just write matches! here.
There was a problem hiding this comment.
tbh, no particularly strong reason, my bias is just to prefer to implement traits that seem applicable unless I can articulate my reservations about doing so, and I couldn't figure out that here.
| if let Some(subparser) = parser.subparser_name { | ||
| dbg_fmt.field("subparser_name", &subparser); | ||
| } | ||
| if parser.recovery == Recovery::Forbidden { |
There was a problem hiding this comment.
Make this an exhaustive match or a matches!()
It's annoying to debug the parser if you have to stop every five seconds to add a Debug impl.
I tried debugging a parser-related issue but found it annoying to not be able to easily peek into the Parser's token stream. Add a convenience fn that offers an opinionated view into the parser, but one that is useful for answering basic questions about parser state.
workingjubilee
force-pushed
the
debug-formatting-my-beloved
branch
from
fe3ad37 to
5e67a37
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (5ce96b1): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 676.869s -> 676.306s (-0.08%) |
The main event is the final commit where I add
Parser::debug_lookahead. Everything else was basically cleaning up things that bugged me (debugging, as it were) until I felt comfortable enough to actually work on it.The motivation is that it's annoying as hell to try to figure out how the debug infra works in rustc without having basic queries like
debug!(?parser);come up "empty". However, Parser has a lot of fields that are mostly irrelevant for most debugging, like the entire ParseSess. I thinkParser::debug_lookaheadwith a capped lookahead might be fine as a general-purpose Debug impl, but this adapter version was suggested to allow more choice, and admittedly, it's a refined version of what I was already handrolling just to get some insight going.