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

Rewrite and refactor format_args!() builtin macro. #100996

Merged
merged 14 commits into from
Sep 28, 2022

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Aug 25, 2022

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:

  1. First, parse_args will parse the (literal, arg, arg, name=arg, name=arg) syntax, but doesn't parse the template (the literal) itself.
  2. 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.
  3. 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 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 m-ou-se self-assigned this Aug 25, 2022
@rust-highfive 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
Copy link
Member Author

m-ou-se commented Aug 25, 2022

@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
Copy link
Member

Alexendoo commented Aug 25, 2022

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

@rust-log-analyzer

This comment was marked as outdated.

@m-ou-se m-ou-se force-pushed the format-args-2 branch 2 times, most recently from 16441de to d813966 Compare August 25, 2022 14:52
@rust-log-analyzer

This comment was marked as outdated.

@m-ou-se m-ou-se force-pushed the format-args-2 branch 4 times, most recently from 746295f to 46505b5 Compare August 26, 2022 18:40
@m-ou-se
Copy link
Member Author

m-ou-se commented Aug 26, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

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

bors commented Aug 26, 2022

⌛ Trying commit 46505b5ee3d606a643d19a2322bff8cb52dce760 with merge f0b6903fbee6a3632b1b3059d24324e030e0430b...

@rust-log-analyzer

This comment was marked as outdated.

@bors
Copy link
Contributor

bors commented Aug 26, 2022

💔 Test failed - checks-actions

@bors 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
Copy link
Member Author

m-ou-se commented Aug 26, 2022

@bors try

@bors
Copy link
Contributor

bors commented Aug 26, 2022

⌛ Trying commit fe3eb3dcf6547820441af4c4f8fcb4bbc22b1b4e with merge 83ba636722d8a682cc06767b5b8577368ad42ed9...

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 27, 2022

@bors r=estebank

@bors
Copy link
Contributor

bors commented Sep 27, 2022

📌 Commit 20bb600 has been approved by estebank

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 Sep 27, 2022
@bors
Copy link
Contributor

bors commented Sep 27, 2022

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

@bors
Copy link
Contributor

bors commented Sep 27, 2022

💔 Test failed - checks-actions

@bors 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
@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 27, 2022

Weird, the relevant logs seem to be empty.

@bors retry

@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 Sep 27, 2022
@bors
Copy link
Contributor

bors commented Sep 28, 2022

⌛ Testing commit 20bb600 with merge d6734be...

@bors
Copy link
Contributor

bors commented Sep 28, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2022
@bors bors merged commit d6734be into rust-lang:master Sep 28, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 28, 2022
@rust-timer
Copy link
Collaborator

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.

mean1 range count2
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.

mean1 range count2
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.

mean1 range count2
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

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label Sep 28, 2022
@nnethercote
Copy link
Contributor

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

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 28, 2022
@m-ou-se m-ou-se deleted the format-args-2 branch September 28, 2022 10:41
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2022
…-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`
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…-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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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
Development

Successfully merging this pull request may close these issues.

9 participants