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

rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg #120218

Merged
merged 3 commits into from Jan 22, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jan 22, 2024

r? @ytmimi and/or @calebcartwright
cc @fmease

I'm putting this on r-l/rust since it should fix the nightly rustfmt version. If you don't care about having this regression until the next rustfmt->rust sync, then I can move that PR over to r-l/rustfmt.


Any idea why the formatting would have changed [from #119099]?

Copied over explanation:

This has to do with the weirdness of the way that parse_macro_arg works. Unlike parsing nonterminal args in a macro-by-example, it eagerly tries, for example, to parse a type without checking that the beginning token may begin a type:

https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/parse/macros/mod.rs#L54

Contrast this to the nonterminal parsing code, which first checks that the nonterminal may begin with a given token:

NonterminalKind::Ty => token.can_begin_type(),

In #119099, @fmease implemented a change so that const Tr would be parsed as dyn const Tr (a trait object to a const trait) in edition 2015.

This is okay for the purposes of macros, because he explicitly made sure that const did not get added to the list of tokens that may begin a :ty nonterminal kind: #119099 (comment)

However, since rustfmt is not so careful about eagerly parsing macro args before checking that they're legal in macro position, this changed the way that the string of tokens is being parsed into macro args.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 22, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-log-analyzer

This comment has been minimized.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 22, 2024

Could we also add a test case for rust-lang/rustfmt#6035

@compiler-errors
Copy link
Member Author

I did have a test but I lost it during rebasing 💀

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

I'm personally happy to apply the fix here in r-l/rust given that this is a very rare circumstance and it'll address the nightly issue sooner. @calebcartwright any objections?

Might be better to hold off on merging until Caleb has had a chance to look this over.

@rust-log-analyzer

This comment has been minimized.

@calebcartwright
Copy link
Member

Probably the worst part about making the change here in-tree is having to do a bit more manual formatting 😄

Thanks for the quick fix, I'm fine for this to be made in tree

r=me once the minor formatting issue is fixed

@calebcartwright
Copy link
Member

calebcartwright commented Jan 22, 2024

and more generally, I think it's fair to say that we've got a decent chunk of code in rustfmt that's likely duplicative of something within rustc and doesn't necessarily need to exist within rustfmt.

I suspect this is mostly a relic from the days when rustfmt consumed the internals from the auto publish crates and the rustfmt team felt the rustc APIs were immutable, but if there's ever any opportunity to drop more code from rustfmt and defer something to a rustc module, I am 100% in favor

@compiler-errors
Copy link
Member Author

@bors r=calebcartwright,ytmimi

@bors
Copy link
Contributor

bors commented Jan 22, 2024

📌 Commit 5297433 has been approved by calebcartwright,ytmimi

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 Jan 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…=calebcartwright,ytmimi

rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg

r? `@ytmimi` and/or `@calebcartwright`
cc `@fmease`

I'm putting this on r-l/rust since it should fix the nightly rustfmt version. If you don't care about having this regression until the next rustfmt->rust sync, then I can move that PR over to r-l/rustfmt.

---

> Any idea why the formatting would have changed [from rust-lang#119099]?

**Copied over explanation:**

This has to do with the weirdness of the way that `parse_macro_arg` works. Unlike parsing nonterminal args in a macro-by-example, it eagerly tries, for example, to parse a type without checking that the beginning token may begin a type:

https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/parse/macros/mod.rs#L54

Contrast this to the nonterminal parsing code, which first checks that the nonterminal may begin with a given token:

https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L47

In rust-lang#119099, `@fmease` implemented a change so that `const Tr` would be parsed as `dyn const Tr` (a trait object to a const trait) in edition 2015.

This is okay for the purposes of macros, because he explicitly made sure that `const` did not get added to the list of tokens that may begin a `:ty` nonterminal kind: rust-lang#119099 (comment)

However, since rustfmt is not so careful about eagerly parsing macro args before checking that they're legal in macro position, this changed the way that the string of tokens is being parsed into macro args.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#117910 (Refactor uses of `objc_msgSend` to no longer have clashing definitions)
 - rust-lang#118639 (Undeprecate lint `unstable_features` and make use of it in the compiler)
 - rust-lang#119801 (Fix deallocation with wrong allocator in (A)Rc::from_box_in)
 - rust-lang#120058 (bootstrap: improvements for compiler builds)
 - rust-lang#120059 (Make generic const type mismatches not hide trait impls from the trait solver)
 - rust-lang#120097 (Report unreachable subpatterns consistently)
 - rust-lang#120137 (Validate AggregateKind types in MIR)
 - rust-lang#120164 (`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`)
 - rust-lang#120181 (Allow any `const` expression blocks in `thread_local!`)
 - rust-lang#120204 (Builtin macros effectively have implicit #[collapse_debuginfo(yes)])
 - rust-lang#120218 (rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117910 (Refactor uses of `objc_msgSend` to no longer have clashing definitions)
 - rust-lang#118639 (Undeprecate lint `unstable_features` and make use of it in the compiler)
 - rust-lang#119801 (Fix deallocation with wrong allocator in (A)Rc::from_box_in)
 - rust-lang#120058 (bootstrap: improvements for compiler builds)
 - rust-lang#120059 (Make generic const type mismatches not hide trait impls from the trait solver)
 - rust-lang#120097 (Report unreachable subpatterns consistently)
 - rust-lang#120137 (Validate AggregateKind types in MIR)
 - rust-lang#120164 (`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`)
 - rust-lang#120181 (Allow any `const` expression blocks in `thread_local!`)
 - rust-lang#120218 (rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0b0f0be into rust-lang:master Jan 22, 2024
11 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
Rollup merge of rust-lang#120218 - compiler-errors:parse_macro_arg, r=calebcartwright,ytmimi

rustfmt: Check that a token can begin a nonterminal kind before parsing it as a macro arg

r? ``@ytmimi`` and/or ``@calebcartwright``
cc ``@fmease``

I'm putting this on r-l/rust since it should fix the nightly rustfmt version. If you don't care about having this regression until the next rustfmt->rust sync, then I can move that PR over to r-l/rustfmt.

---

> Any idea why the formatting would have changed [from rust-lang#119099]?

**Copied over explanation:**

This has to do with the weirdness of the way that `parse_macro_arg` works. Unlike parsing nonterminal args in a macro-by-example, it eagerly tries, for example, to parse a type without checking that the beginning token may begin a type:

https://github.com/rust-lang/rustfmt/blob/bf967319e258acb9b1648a952bba52665eceaf52/src/parse/macros/mod.rs#L54

Contrast this to the nonterminal parsing code, which first checks that the nonterminal may begin with a given token:

https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L47

In rust-lang#119099, ``@fmease`` implemented a change so that `const Tr` would be parsed as `dyn const Tr` (a trait object to a const trait) in edition 2015.

This is okay for the purposes of macros, because he explicitly made sure that `const` did not get added to the list of tokens that may begin a `:ty` nonterminal kind: rust-lang#119099 (comment)

However, since rustfmt is not so careful about eagerly parsing macro args before checking that they're legal in macro position, this changed the way that the string of tokens is being parsed into macro args.
@rustbot rustbot added this to the 1.77.0 milestone Jan 22, 2024
@compiler-errors compiler-errors deleted the parse_macro_arg branch January 24, 2024 16:37
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.

None yet

6 participants