Rewrite and refactor format_args!() builtin macro. by m-ou-se · Pull Request #100996 · rust-lang/rust

m-ou-se · 2022-08-25T10:57:29Z

This is a near complete rewrite of compiler/rustc_builtin_macros/src/format.rs.

This gets rid of the massive unmaintanable Context struct, and splits the macro expansion into three parts:

First, parse_args will parse the (literal, arg, arg, name=arg, name=arg) syntax, but doesn't parse the template (the literal) itself.
Second, make_format_args will parse the template, the format options, resolve argument references, produce diagnostics, and turn the whole thing into a FormatArgs structure.
Finally, expand_parsed_format_args will turn that FormatArgs structure into the expression that the macro expands to.

In other words, the format_args builtin macro used to be a hard-to-maintain 'single pass compiler', which I've split into a three phase compiler with a parser/tokenizer (step 1), semantic analysis (step 2), and backend (step 3). (It's compilers all the way down. ^^)

This can serve as a great starting point for #99012, which will only need to change the implementation of 3, while leaving step 1 and 2 unchanged.

It also makes rust-lang/compiler-team#541 easier, which could then upgrade the new FormatArgs struct to an ast node and remove step 3, moving that step to later in the compilation process.

It also fixes a few diagnostics bugs.

This also significantly reduces the amount of generated code for cases with arguments in non-default order without formatting options, like "{1} {0}" or "{a} {}", etc.

m-ou-se · 2022-08-25T12:47:00Z

@Alexendoo I saw you've sent a few PRs touching this code recently. In case you were planning to send more of those, be aware I'm refactoring the entire thing. ^^

Alexendoo · 2022-08-25T13:02:06Z

Thanks for the heads up! The main one I was planning to do was #101000 so that is handy 😄

m-ou-se · 2022-08-26T19:44:20Z

@bors try @rust-timer queue

rust-timer · 2022-08-26T19:44:22Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-08-26T19:44:29Z

⌛ Trying commit 46505b5ee3d606a643d19a2322bff8cb52dce760 with merge f0b6903fbee6a3632b1b3059d24324e030e0430b...

bors · 2022-08-26T20:55:26Z

💔 Test failed - checks-actions

m-ou-se · 2022-08-26T20:56:52Z

@bors try

bors · 2022-08-26T20:57:00Z

⌛ Trying commit fe3eb3dcf6547820441af4c4f8fcb4bbc22b1b4e with merge 83ba636722d8a682cc06767b5b8577368ad42ed9...

With efficient lookup through a hash map.

m-ou-se · 2022-09-27T11:46:45Z

@bors r=estebank

bors · 2022-09-27T11:46:47Z

📌 Commit 20bb600 has been approved by estebank

It is now in the queue for this repository.

bors · 2022-09-27T13:27:20Z

⌛ Testing commit 20bb600 with merge 87338df73726f7517d94a980dd56de09bf6cc975...

bors · 2022-09-27T14:49:54Z

💔 Test failed - checks-actions

m-ou-se · 2022-09-27T15:05:42Z

Weird, the relevant logs seem to be empty.

@bors retry

bors · 2022-09-28T01:36:32Z

⌛ Testing commit 20bb600 with merge d6734be...

bors · 2022-09-28T04:18:13Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing d6734be to master...

rust-timer · 2022-09-28T06:02:27Z

Finished benchmarking commit (d6734be): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.5%	[1.1%, 2.0%]	6
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.2%]	7
Improvements ✅ (secondary)	-1.2%	[-1.5%, -0.6%]	7
All ❌✅ (primary)	-0.3%	[-0.7%, -0.2%]	7

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[1.8%, 5.7%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-3.9%, -1.0%]	8
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[2.8%, 4.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.9%, -2.8%]	2
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change↩↩² ↩³
number of relevant changes↩↩² ↩³

nnethercote · 2022-09-28T06:16:52Z

keccak is noisy at the moment. The other changes are likely real, but the wins and losses balance out.

@rustbot label: +perf-regression-triaged

…-ou-se Fix `format_args` capture for macro expanded format strings Since rust-lang#100996 `format_args` capture for macro expanded strings aren't prevented when the span of the expansion points to a string literal, e.g. ```rust // not a terribly realistic example, but also happens for proc_macros that set // the span of the output to an input str literal, such as indoc macro_rules! x { ($e:expr) => { $e } } fn main() { let a = 1; println!(x!("{a}")); } ``` The tests didn't catch it as the span of `concat!()` points to the macro invocation r? `@m-ou-se`

m-ou-se added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 25, 2022

m-ou-se self-assigned this Aug 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2022

m-ou-se mentioned this pull request Aug 25, 2022
Separate CountIsStar from CountIsParam in rustc_parse_format. #101000
Merged

m-ou-se force-pushed the format-args-2 branch 2 times, most recently from c8de89a to 9a62d8dCompare August 25, 2022 13:00

m-ou-se force-pushed the format-args-2 branch from 9a62d8d to d5ea0efCompare August 25, 2022 13:31

This comment was marked as outdated.
Sign in to view

m-ou-se force-pushed the format-args-2 branch 2 times, most recently from 16441de to d813966Compare August 25, 2022 14:52

This comment was marked as outdated.
Sign in to view

m-ou-se force-pushed the format-args-2 branch 4 times, most recently from 746295f to 46505b5Compare August 26, 2022 18:40

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 26, 2022

This comment was marked as outdated.
Sign in to view

bors 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 Aug 26, 2022

m-ou-se added 3 commits September 27, 2022 13:31

Turn format arguments Vec into its own struct.
cf53fef
With efficient lookup through a hash map.

Update doc comments.
ba7bf1d

Remove confusing drop.
20bb600

m-ou-se force-pushed the format-args-2 branch from 13d187a to 20bb600Compare September 27, 2022 11:46

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 Sep 27, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 27, 2022

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 Sep 27, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2022

bors merged commit d6734be into rust-lang:masterSep 28, 2022

rustbot added this to the 1.66.0 milestone Sep 28, 2022

bors mentioned this pull request Sep 28, 2022
builtin_macros: Migrate diagnostic #101935
Closed

rustbot added the perf-regression Performance regression. label Sep 28, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 28, 2022

m-ou-se deleted the format-args-2 branch September 28, 2022 10:41

Alexendoo mentioned this pull request Sep 30, 2022
Fix format_args capture for macro expanded format strings #102519
Merged

Uh oh!

Uh oh!

Conversation

m-ou-se commented Aug 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-ou-se commented Aug 25, 2022

Uh oh!

Alexendoo commented Aug 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

m-ou-se commented Aug 26, 2022

Uh oh!

rust-timer commented Aug 26, 2022

Uh oh!

bors commented Aug 26, 2022

Uh oh!

This comment was marked as outdated.

bors commented Aug 26, 2022

Uh oh!

m-ou-se commented Aug 26, 2022

Uh oh!

bors commented Aug 26, 2022

Uh oh!

m-ou-se commented Sep 27, 2022

Uh oh!

bors commented Sep 27, 2022

Uh oh!

bors commented Sep 27, 2022

Uh oh!

bors commented Sep 27, 2022

Uh oh!

m-ou-se commented Sep 27, 2022

Uh oh!

bors commented Sep 28, 2022

Uh oh!

bors commented Sep 28, 2022

Uh oh!

rust-timer commented Sep 28, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

nnethercote commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

m-ou-se commented Aug 25, 2022•
edited
Loading

Alexendoo commented Aug 25, 2022•
edited
Loading