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(expand): prevent infinity loop in macro containing only "///" #112345

Merged
merged 2 commits into from Jun 7, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jun 6, 2023

Fixes #112342

Issue #112342 was caused by an infinity loop in parse_tt_inner, and the state of it is as follows:

  • matcher: [Sequence, Token(Doc), SequenceKleeneOpNoSep(op: ZeroOrMore), Eof]

  • loop:

Iteration Action
0 enter Sequence
1 enter Token(Doc) and mp.idx += 1 had been executed
2 enter SequenceKleeneOpNoSep and reset mp.idx to 1
3 enter Token(Doc) again

To prevent the infinite loop, a check for whether it only contains DocComment in check_lhs_no_empty_seq had been added.

@rustbot
Copy link
Collaborator

rustbot commented Jun 6, 2023

r? @eholk

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 6, 2023
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me except two small nits, very good PR edit: see comment below

compiler/rustc_expand/src/mbe/macro_parser.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/mbe/macro_parser.rs Outdated Show resolved Hide resolved
@est31
Copy link
Member

est31 commented Jun 6, 2023

Hmm yeah so what is happening here is that the compiler attempts to try another repetition, infinitely many times. This would also happen for code like:

macro_rules! foo {
    ($()*) => {};
}
foo!();

Similarly:

macro_rules! foo {
    ($($(foo:tt)?)*) => {};
}

But thankfully the compiler catches that before it gets into the matching code and errors with "repetition matches empty token tree".

I think it would be best to instead edit check_lhs_no_empty_seqa to check if there is a sequence that has nothing but a doc comment. Make sure that it also errors for multiple doc comments :)

cc also #5067, #42755, #57597

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 6, 2023

it would be best to instead edit check_lhs_no_empty_seqa to check if there is a sequence that has nothing but a doc comment.

nice solution!

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 6, 2023

btw, what do you think emit warning such as "doc comment is Illegal syntax, it may throw error in future"?

@Nilstrieb
Copy link
Member

doing it like @est31 suggested is what I just wanted to suggest too, so that sounds great

emitting a future compatibility warning for doc comments in matcher position doesn't sound too bad to me but is a separate issue that should be discussed with the language team.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 6, 2023

Already revised, thank you both for your help, I learned a lot

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me if you add a test for /// together with another /// doc comment, as well as one test with /// and $(foo:tt)?.

compiler/rustc_expand/src/mbe/macro_rules.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/mbe/macro_rules.rs Outdated Show resolved Hide resolved
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some final nits. I promised approval before, but given you are learning I thought this would be useful to know.

compiler/rustc_expand/src/mbe/macro_rules.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/mbe/macro_rules.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/mbe/macro_rules.rs Outdated Show resolved Hide resolved
@bvanjoi bvanjoi force-pushed the fix-112342 branch 2 times, most recently from f181264 to f8175c9 Compare June 7, 2023 02:20
@est31
Copy link
Member

est31 commented Jun 7, 2023

@bors r=nilstrieb,est31

as per #112345 (comment)

@bors
Copy link
Contributor

bors commented Jun 7, 2023

📌 Commit 5eafab3 has been approved by nilstrieb,est31

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#112076 (Fall back to bidirectional normalizes-to if no subst-relate candidate in alias-relate goal)
 - rust-lang#112122 (Add `-Ztrait-solver=next-coherence`)
 - rust-lang#112251 (rustdoc: convert `if let Some()` that always matches to variable)
 - rust-lang#112345 (fix(expand): prevent infinity loop in macro containing only "///")
 - rust-lang#112359 (Respect `RUST_BACKTRACE` for delayed bugs)
 - rust-lang#112382 (download-rustc: Fix `x test core` on MacOS)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 42cf6da into rust-lang:master Jun 7, 2023
11 checks passed
@rustbot rustbot added this to the 1.72.0 milestone Jun 7, 2023
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. 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.

A simple macro with "///" hangs
7 participants