Preparatory changes for macro parsing BFS->DFS#158974
Conversation
None of the existing tests captured a case where non-terminal binds appeared differently ordered than their definition in their rule, so I wrote a test for it. The existing macro parsing code is quite resilient to this kind of mis-ordering; it took quite a convoluted macro to trigger a case where the ordering is incorrect today. I've documented the parse tree (based on my own mental model) that the convoluted macro causes.
The order of `bb_mps` and `next_mps` depends on arbitrary implementation choices, e.g. the order in which `$(a)? b` causes `a b` and `b` to be explored. To stop depending on these implementation details, this commit sorts `bb_mps` and `next_mps` by `mp.idx` (corresponding to the position in the rule) before presenting error messages. This makes it easier to refactor the macro parsing implementation.
In the next commit, the work done in `Tracker::after_arm()` upon a `Failure` will be moved into `Tracker::build_failure()`. For this to work, the `WhichMatcher` parameter to `after_arm()` needs to be available to `build_failure()`. Rather than threading it through `TtParser`, this commit stores it in `CollectTrackerAndEmitter` and keeps it updated using a new `Tracker::prepare()` method. `tests/ui/macros` pass.
Rather than preparing a failure using `Tracker::build_failure()` and consuming it from `Tracker::after_arm()`, the parser now calls `Tracker::failure()` the moment a failure is detected, and this will eagerly perform all the necessary processing. This makes the contents of `Failure` redundant, so they can be removed in the next commit.
Now that everything is processed in `Tracker::failure()`, this data does not need to be propagated. This also removes some generics.
The appropriate token (span) and the diagnostic message are now computed in `diagnostics.rs`. Following this commit, _all_ diagnostic messages have been moved from `macro_parser.rs` to `diagnostics.rs`. Yay!
It is being used by `Parser::nonterminal_may_begin_with()`.
This commit deliberately causes panics. See the next commit for more details and a fix.
Until now, `:tt`, `:item`, and `:stmt` metavars could be attempted against `token::Eof`. *HOWEVER*, when a `token::Eof` is processed, `parse_tt_inner()` ignores `bb_mps`, not even checking for ambiguity. It's as if those `bb_mps` never existed; as if `nonterminal_may_begin_with()` returned `false` for `Eof`. This commit makes that behavior more explicit. This does not change user visible behavior. `tests/ui/macros` pass.
The previous commit fixed a case where some meta-variables were being treated as if they *could* match `token::Eof` and then silently ignored. This commit adds a test to ensure meta-variables continue to have a consistent response to an `Eof` (in theory, they could vary between fatal non-terminal parsing errors and non-fatal matching failures).
This makes it easier to change the matching logic and to use the same logic from multiple places.
|
The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @nnethercote (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Could you tell more, what is the BFS->DFS change and its motivation and goals? |
|
Hey @petrochenkov! I initially got interested in the macro parser when I learned about degenerate cases causing exponential-time (and exponential-space) complexity here. I wrote a blog post about it with more details: https://bal-e.org/blog/2026/oops-cubic-macro/. In short, the macro parser explores ambiguity using a BFS, but it only cares about one result (if there are other results, it only cares about their existence, not the data within; at least on the fast path). A DFS feels simpler here:
I'm quite confident the macro parser can be changed to DFS without affecting user behavior (#158894 added a user-visible change, but only for a minor detail in error messages). In any case, I'll write about it more when I get to the main PR. I think these preparatory PRs are worth merging independently of the BFS->DFS change. Happy to answer in more detail too :) |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 3041d62 with merge 71875d2… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/29032045657 |
Preparatory changes for macro parsing BFS->DFS
6 participants
This PR contains some miscellaneous commits I accumulated while working on the BFS->DFS change. Their goal is to simplify the code and clarifying existing behavior. It is a conceptual follow-up to #158577. High-level overview:
Adds context about the current match arm to
Trackerthrough the newTracker::prepare(), so that theWhichMatcherparameter is available implicitly. In a later PR, this will be used to referenceMatcherLocs by index.Reformulates matching failures to work more like ambiguity errors wrt.
Tracker;Tracker::build_failure()(which would build a failure consumed byTracker::after_arm()) becomesTracker::failure()which eagerly processes the error. This removes the need forParseResult::Failureto store any data at all. This relies on the match arm context provided byTracker::prepare().There is a subtle edge case involving
token::Eofand non-terminal parsing;Parser::nonterminal_may_begin_with()would sometimes returntruefortoken::Eof, even though the non-terminal parse would never be attempted.TtParserdid not checkbb_mpswhen handlingtoken::Eof, so non-terminal parses at EOF were being silently dropped. I changedParser::nonterminal_may_begin_with()to always returnfalsefortoken::Eof, making this behavior much more visible. I added a test case to make sure meta-variables return the same error uniformly when parsed against EOF.Contains #158894, which should be merged soon (after which I'll rebase onto
main).Best reviewed commit-by-commit.
r? @nnethercote