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

Type ascription builtin is silently replaced by rustfmt(?) #6159

Open
Mark-Simulacrum opened this issue May 2, 2024 · 17 comments
Open

Type ascription builtin is silently replaced by rustfmt(?) #6159

Mark-Simulacrum opened this issue May 2, 2024 · 17 comments
Assignees

Comments

@Mark-Simulacrum
Copy link
Member

rustfmt complains about the type_ascribe macro asking for the builtin to be replaced with the ... : ... syntax:

Error: Diff in /checkout/library/core/src/macros/mod.rs at line 1712:
         reason = "placeholder syntax for type ascription"
     )]
     pub macro type_ascribe($expr:expr, $ty:ty) {
-        builtin # type_ascribe($expr, $ty)
+        $expr: $ty
     }

I've worked around this with #[rustfmt::skip] on the macro which seems to work, but filing an issue since it seems like somewhat odd behavior as well.

@Nilstrieb
Copy link
Member

looks like rustfmt wasn't updated to print type ascription correctly, this issue should be removed to the rustfmt repo probably

@Mark-Simulacrum Mark-Simulacrum transferred this issue from rust-lang/rust May 2, 2024
@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

@Mark-Simulacrum The diff is helpful, but can you also provide a complete code snippet that we can use to reproduce the issue. A few more questions. What version of rustfmt were you using when you ran into this issue, and are you using any rustfmt configurations?

@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

@Nilstrieb do you know how builtin # type_ascribe is represented in the AST?

for example, builtin # offset_of is represented by rustc_ast::ast::ExprKind::OffsetOf

@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

@Nilstrieb @Mark-Simulacrum Okay, I think the issue here is that builtin # type_ascribe gets parsed as rustc_ast::ast::ExprKind::Type.

From what I can tell Here's where this get's parsed in rustc.

In this case rustfmt is handling this as expected. It sees a ExprKind::Type in the ast and outputs one. I think this issue should be moved back to rust-lang/rust and the fix probably involves creating some special builtin ast node, though I don't know enough about later phases of the compiler to know how that might complicate things after parsing.

@Mark-Simulacrum Mark-Simulacrum transferred this issue from rust-lang/rustfmt May 2, 2024
@Nilstrieb
Copy link
Member

haha, looks like ast_pretty wasn't correctly updated then, fun. I don't think any AST changes are needed, it just needs to print the ExprKind::Type with the builtin syntax.

@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

@Nilstrieb Do you have thoughts on how rustfmt can tell the difference between builtin # type_ascribe and expr: ty if they both get parsed as ExprKind::Type?

@Nilstrieb
Copy link
Member

Nilstrieb commented May 2, 2024

the latter does not exist anymore

@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

If ExprKind::Type exclusively represents builtin # type_ascrib then I agree we don't need to make any ast changes. Do you know when the switch from expr: ty -> builtin # type_ascribe occurred? Maybe we should also update the doc comment on ExprKind::Type to explain that as well.

@Nilstrieb
Copy link
Member

It was implemented by Yukang and me, with the biggest change in rust-lang/rust#109128

@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

Thanks for the link!

@fmease
Copy link
Member

fmease commented May 2, 2024

And rust-lang/rust#122806 made the macro type_ascribe use builtin # type_ascribe.

@fmease
Copy link
Member

fmease commented May 2, 2024

rustc_ast_pretty does print builtin # offset_of and builtin # type_acribe correctly (well, it prints builtin # type_ascribe as type_ascribe! but I'm gonna fix that, too), only rustfmt doesn't. rustfmt actually crashes on builtin # offset_of.

Working on a patch for rust-lang/rustfmt, let's see if I can make it work.

@fmease fmease self-assigned this May 2, 2024
@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2024

rustc_ast_pretty does print builtin # offset_of and builtin # type_acribe correctly (well, it prints builtin # type_ascribe as type_ascribe! but I'm gonna fix that, too), only rustfmt doesn't. rustfmt actually crashes on builtin # offset_of.

Working on a patch for rust-lang/rustfmt, let's see if I can make it work.

Nice! I think you can just move the ExprKind::Type to the same match arm as ExprKind::OffsetOf in rust-lang/rustfmt

Though we haven't done a sync in a while so I'm not sure if we've pulled in some of the more recent changes in rustfmt yet.

fmease referenced this issue in fmease/rust May 3, 2024
…yn, r=compiler-errors

AST pretty: Use `builtin_syntax` for type ascription

Follow-up to rust-lang#122806.
CC #124619.
matthiaskrgr referenced this issue in matthiaskrgr/rust May 3, 2024
…yn, r=compiler-errors

AST pretty: Use `builtin_syntax` for type ascription

Follow-up to rust-lang#122806.
CC #124619.
rust-timer referenced this issue in rust-lang-ci/rust May 3, 2024
Rollup merge of rust-lang#124637 - fmease:ast-pretty-ty-asc-builtin-syn, r=compiler-errors

AST pretty: Use `builtin_syntax` for type ascription

Follow-up to rust-lang#122806.
CC #124619.
@fmease
Copy link
Member

fmease commented May 4, 2024

Moving it back to the rustfmt repo since this is strictly rustfmt-related (wrong output for type_ascribe, panic on offset_of) as AST pretty works perfectly fine.

@rustbot transfer rustfmt

@rustbot
Copy link
Collaborator

rustbot commented May 4, 2024

Error: The feature transfer is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Nilstrieb
Copy link
Member

someone needs to make a PR for that :D

@fmease
Copy link
Member

fmease commented May 5, 2024

@rustbot transfer rustfmt

@rustbot rustbot transferred this issue from rust-lang/rust May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants