Skip to content

Don't give APITs names with macro expansion placeholder fragments in it #142393

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Don't give APITs names with macro expansion placeholder fragments in it #142393

wants to merge 1 commit into from
+77 −18

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jun 12, 2025 •
edited
Loading

The DefCollector previously called pprust::ty_to_string to construct a name for APITs (arg-position impl traits). The ast::Ty that was being formatted however has already had its macro calls replaced with "placeholder fragments", which end up rendering like !() (or ICEing, in the case of #140333, since it led to a placeholder struct field with no name).

Instead, collect the name of the APIT before we visit its macros and replace them with placeholders in the macro expander. This makes the implementation a bit more involved, but AFAICT there's no better way to do this since we can't do a reverse mapping from placeholder fragment -> original macro call AST.

Fixes #140333

@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2025

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

compiler-errors
compiler-errors commented Jun 12, 2025
View reviewed changes
tests/ui/impl-trait/name-mentioning-macro.stderr
LL | let () = x;
| ^^ - this expression has type `impl Foo<bar!()>`
| |
| expected type parameter `impl Foo<bar!()>`, found `()`
Copy link
Member Author

@compiler-errors compiler-errors Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For contrast, this used to render like impl Foo<!()>, where !() is the placeholder fragment.

@petrochenkov petrochenkov self-assigned this Jun 12, 2025
@davidtwco
Copy link
Member

r=me if @petrochenkov is happy since he's also assigned

@compiler-errors compiler-errors mentioned this pull request Jun 12, 2025
Closed
petrochenkov
petrochenkov reviewed Jun 17, 2025
View reviewed changes
@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 Jun 17, 2025
@compiler-errors
Copy link
Member Author

flipping state back b/c of a pending question

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2025
@bors

This comment was marked as resolved.

Sign in to view
@petrochenkov
Copy link
Contributor

#142690 should help with #142393 (comment)
(Also node id of ast::Ty itself will need to be used as a key, to avoid the additional node id assignment reordering.)
@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2025
@compiler-errors
Copy link
Member Author

(Also node id of ast::Ty itself will need to be used as a key, to avoid the additional node id assignment reordering.)

Yeah, I don't think it matters which node id was being used as a key. I think that PR + using the ty's node id is sufficient.

@bors

This comment was marked as resolved.

Sign in to view
@compiler-errors
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 25, 2025
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 25, 2025

📌 Commit 8cd3fa0 has been approved by petrochenkov

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 25, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2025
@matthiaskrgr
Rollup merge of rust-lang#142393 - compiler-errors:nofield, r=petroch…
ceeb84c 
…enkov

Don't  give APITs names with macro expansion placeholder fragments in it

The `DefCollector` previously called `pprust::ty_to_string` to construct a name for APITs (arg-position impl traits). The `ast::Ty` that was being formatted however has already had its macro calls replaced with "placeholder fragments", which end up rendering like `!()` (or ICEing, in the case of rust-lang#140333, since it led to a placeholder struct field with no name).

Instead, collect the name of the APIT *before* we visit its macros and replace them with placeholders in the macro expander. This makes the implementation a bit more involved, but AFAICT there's no better way to do this since we can't do a reverse mapping from placeholder fragment -> original macro call AST.

Fixes rust-lang#140333
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov

@davidtwco davidtwco

Assignees

@petrochenkov petrochenkov

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ICE: None in pretty printing self.print_ident(field.ident.unwrap());
5 participants
@compiler-errors @rustbot @davidtwco @bors @petrochenkov