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

feat: Implement builtin#format_args, using rustc's format_args parser #15559

Merged
merged 7 commits into from
Sep 6, 2023

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Sep 5, 2023

format_args! now expands to builtin#format_args(template, args...), the actual expansion now instead happens in lowering where we desugar this expression by using lang paths.

As a bonus, we no longer need to evaluate format_args as an eager macro which means less macro expansions overall -> less cache thrashing!

Fixes #15082
cc rust-lang/rust#110680

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2023
@Veykril Veykril force-pushed the builtin-format-args branch from a491b92 to 815b7dc Compare September 5, 2023 17:16
@Veykril Veykril force-pushed the builtin-format-args branch from 815b7dc to abe8f1e Compare September 5, 2023 17:19
@Veykril
Copy link
Member Author

Veykril commented Sep 6, 2023

Note this currently copies rustc's format_args parser crate, as it depends on rustc_datastructures and I am waiting for my upstream PR to get merged at which point we can depend on a published copy of it instead, hence why the PR is even more massive than it should be

@Veykril Veykril force-pushed the builtin-format-args branch from 84dd5e0 to 6f55b8c Compare September 6, 2023 16:04
@Veykril Veykril force-pushed the builtin-format-args branch from 6f55b8c to c0e4026 Compare September 6, 2023 16:08
@Veykril Veykril force-pushed the builtin-format-args branch from e7c11a7 to 3fa0bf0 Compare September 6, 2023 16:31
@Veykril
Copy link
Member Author

Veykril commented Sep 6, 2023

exprs: 662115, ??ty: 30 (0%), ?ty: 116 (0%), !ty: 3
pats: 155657, ??ty: 4 (0%), ?ty: 5 (0%), !ty: 0

This does increase analysis-stats mir memory consumption by ~80 mb though, definitely something we wanna take a look at again (as in to optimize if possible). That makes me wonder if there is some easy to write MIR opt that can optimize for MIR size?

@@ -247,160 +243,22 @@ fn format_args_expand_general(
_db: &dyn ExpandDatabase,
_id: MacroCallId,
tt: &tt::Subtree,
end_string: &str,
// FIXME: Make use of this so that mir interpretation works properly
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, (I'm probably not fixing this in this PR) format_args_nl! will behave the same as format_args! that is no newline will be appended. This is fine for most hings but mir eval.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unsure how to solve this, we could give the construct an extra "parameter" or something that tells it to emit an additional newline, make a builtin#format_args_nl construct or try to fiddle with the template string which I really do not want to do (and I don't think we can reasonably do that).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm leaning towards adding a second builtin# variant, it seems simplest

@Veykril Veykril force-pushed the builtin-format-args branch from 078c5d5 to 5046889 Compare September 6, 2023 17:18
@Veykril Veykril marked this pull request as ready for review September 6, 2023 17:44
@Veykril
Copy link
Member Author

Veykril commented Sep 6, 2023

Let's see how this goes
@bors r+

@bors
Copy link
Contributor

bors commented Sep 6, 2023

📌 Commit 96f1923 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 6, 2023

⌛ Testing commit 96f1923 with merge f29867b...

@bors
Copy link
Contributor

bors commented Sep 6, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f29867b to master...

@bors bors merged commit f29867b into rust-lang:master Sep 6, 2023
@Veykril Veykril deleted the builtin-format-args branch September 6, 2023 18:04
@HKalbasi
Copy link
Member

HKalbasi commented Sep 6, 2023

That makes me wonder if there is some easy to write MIR opt that can optimize for MIR size

Yes, there are some easy optimization passes that we can implement. Generating a better MIR in first place is also something we can try, which may achieve somethings that easy optimizations can't.

Looks like debug render on hover is still broken though, so something fails in mir eval still

Yes, also failed mirs on self count is 2488, while I hoped it to be dropped to near zero after this PR. Will look at it.

@Veykril Veykril changed the title Implement builtin#format_args, using rustc's format_args parser feat: Implement builtin#format_args, using rustc's format_args parser Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

format_args! is broken
4 participants