Skip to content

Conversation

Alexendoo
Copy link
Member

Since #100996 format_args capture for macro expanded strings aren't prevented when the span of the expansion points to a string literal, e.g.

// 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

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 30, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2022
@Alexendoo Alexendoo force-pushed the format-args-macro-str branch from 912aa69 to 41b8f7e Compare September 30, 2022 16:39
@Alexendoo Alexendoo force-pushed the format-args-macro-str branch from 41b8f7e to 71db0dd Compare September 30, 2022 16:40
@m-ou-se
Copy link
Member

m-ou-se commented Sep 30, 2022

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 30, 2022

📌 Commit 71db0dd has been approved by m-ou-se

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 30, 2022
@bors
Copy link
Collaborator

bors commented Oct 1, 2022

⌛ Testing commit 71db0dd with merge edadc7c...

@bors
Copy link
Collaborator

bors commented Oct 1, 2022

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing edadc7c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2022
@bors bors merged commit edadc7c into rust-lang:master Oct 1, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 1, 2022
@Alexendoo Alexendoo deleted the format-args-macro-str branch October 1, 2022 18:05
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (edadc7c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)
8.3% [5.4%, 13.9%] 3
Regressions ❌
(secondary)
2.5% [1.7%, 2.9%] 3
Improvements ✅
(primary)
-8.6% [-19.8%, -2.4%] 22
Improvements ✅
(secondary)
-3.3% [-3.5%, -3.1%] 2
All ❌✅ (primary) -6.6% [-19.8%, 13.9%] 25

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)
- - 0
Improvements ✅
(primary)
-6.2% [-29.5%, -1.3%] 189
Improvements ✅
(secondary)
-4.1% [-14.3%, -1.3%] 98
All ❌✅ (primary) -6.2% [-29.5%, -1.3%] 189

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@matthiaskrgr
Copy link
Member

Hm, this slowed down rustc compiler bootstrap by almost a minute (8%) ?? on perf.rlo
https://perf.rust-lang.org/compare.html?start=744e397d8855f7da87d70aa8d0bd9e0f5f0b51a1&end=edadc7ccdda644ef8149869d2f24018a1dac202a&stat=instructions:u

@Alexendoo
Copy link
Member Author

Seems pretty unlikely as this just changes a snippet span back to what it was a few days ago

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
merged-by-bors This PR was explicitly merged by bors. 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.

7 participants