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

Fix macro matching for NoDelim delimited sequences. #96307

Closed
wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

PR #95159 unintentionally changed the behaviour of declarative macro
matching for NoDelim delimited sequences.

The fix is simple: remove the explicit NoDelim delimiters in the
MatcherLoc representation. This gives back the original behaviour.

As for why this took so long to be found: it seems that NoDelim
sequences are unusual. It took a macro that generates another macro
(from the yarte_lexer crate, found via a crater run) to uncover this.

Fixes #96305.

r? @petrochenkov

PR rust-lang#95159 unintentionally changed the behaviour of declarative macro
matching for `NoDelim` delimited sequences.
- Before rust-lang#95159, delimiters were implicit in `mbe::Delimited`. When
  doing macro matching, normal delimiters were generated out of thin air
  as necessary, but `NoDelim` delimiters were not. This was done within
  `TokenTree::{get_tt,len}`.
- After rust-lang#95159, the delimiters were explicit. There was an unintentional
  change whereby `NoDelim` delimiters were represented explicitly just
  like normal delimeters.
- rust-lang#95555 introduced a new matcher representation (`MatcherLoc`) and the
  `NoDelim` delimiters were made explicit within it, just like
  `mbe::Delimited`.
- rust-lang#95797 then changed `mbe::Delimited` back to having implicit
  delimiters, but because matching is now being done on `MatcherLoc`,
  the behavioural change persisted.

The fix is simple: remove the explicit `NoDelim` delimiters in the
`MatcherLoc` representation. This gives back the original behaviour.

As for why this took so long to be found: it seems that `NoDelim`
sequences are unusual. It took a macro that generates another macro
(from the `yarte_lexer` crate, found via a crater run) to uncover this.

Fixes rust-lang#96305.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 22, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2022
@nnethercote
Copy link
Contributor Author

The only problem here is there is no test case added, because the only test case I have spans two crates, and I don't know how to incorporate that into rustc's testing structure.

@Aaron1011
Copy link
Member

You can use an // aux-build comment to make compile test build another crate.

@petrochenkov
Copy link
Contributor

This is unfortunate case, I'd say that #95159 fixed a bug.
If #96305 is the sole regression found by crater (or there are few similar regressions), then it would be better to keep things as is.
NoDelims are regular brackets like () or {}, just invisible, so they are supposed to match or not match in the same way.

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 22, 2022

In general, nonterminal tokens from macro_rules are not supposed to be usable in left-hand sides of nested macro_rules (with exception of single-token nonterminals).
#49326 introduced this restriction (for slightly different reasons), but not in a sufficiently reliable way, unfortunately - after being converted to token streams for metadata encoding-decoding they become usable again.

It would be better to report this case early, at macro definition site, when parsing the LHS of the macro definition.

@petrochenkov
Copy link
Contributor

An alternative fix for the regression from #96305 is to stop wrapping single-token literal matchers into groups, like it was done in #92472 with ident matchers.

@petrochenkov
Copy link
Contributor

Given that the macro in #96305 correctly reports an error if used locally, I's say that

It would be better to report this case early, at macro definition site, when parsing the LHS of the macro definition.

is the right course of action here.

See Nonterminal/NoDelim when parsing a macro LHS => report an error (or a deny-by-default deprecation lint if there are too many regressions).

@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 Apr 22, 2022
@nnethercote
Copy link
Contributor Author

Given that the macro in #96305 correctly reports an error if used locally, I's say that

It would be better to report this case early, at macro definition site, when parsing the LHS of the macro definition.

is the right course of action here.

See Nonterminal/NoDelim when parsing a macro LHS => report an error (or a deny-by-default deprecation lint if there are too many regressions).

Fair enough. I've added this information to #96305, and I will close this PR.

@nnethercote nnethercote closed this May 5, 2022
@nnethercote nnethercote deleted the fix-96305 branch May 5, 2022 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yarte_lexer 0.0.1 compilation regression
5 participants