Call `compute_locs` once per rule by nnethercote · Pull Request #95669 · rust-la...

Copy link

Contributor

nnethercote commented 12 days ago

rust-timer commented 12 days ago

Awaiting bors try build completion.

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

bors commented 12 days ago

Trying commit a68238b with merge 6db6a2d...

nnethercote commented 12 days ago

Local instruction count measurements on check full builds. Those marked with * are from rustc-perf.

Benchmark Profile Scenario % Change Significance Factor?

num-derive-0.3.3 check full -1.99% 9.93x

*html5ever-0.26.0 check full -1.93% 9.63x

scroll_derive-0.11.0 check full -1.88% 9.40x

pest_generator-2.1.3 check full -1.86% 9.32x

ctor-0.1.21 check full -1.78% 8.92x

mockall_derive-0.11.0 check full -1.62% 8.12x

yansi-0.5.0 check full -1.60% 8.01x

futures-macro-0.3.19 check full -1.55% 7.77x

serde_derive-1.0.136 check full -1.32% 6.59x

async-std-1.10.0 check full -1.15% 5.77x

quote-stress check full -1.09% 5.44x

inotify-0.10.0 check full -0.73% 3.63x

time-macros-0.2.3 check full -0.51% 2.54x

*hyper-0.14.18 check full -0.48% 2.38x

*wg-grammar check full -0.37% 1.85x

petrochenkov commented 12 days ago

But the next commit will be moving compute_locs
outwards to a place that isn't suitable for the missing fragment
identifier checking

Why isn't it suitable?
compile_declarative_macro is actually a much better place for reporting these errors, because they are definition-site errors rather than use-site errors.
In compile_declarative_macro those errors can be reported right away, without stashing them to ParseSess::missing_fragment_specifiers, which then can be removed.

petrochenkov commented 12 days ago

I guess the next logical step is lowering macros to MatcherLocs on encountering their definitions and also encoding them to metadata so other crates can load macros in already pre-compiled form, like other items.

bors commented 12 days ago

Try build successful - checks-actions
Build commit: 6db6a2d (6db6a2df45e20133e0d7c5f7353baf3c5e94e711)

rust-timer commented 12 days ago

rust-timer commented 12 days ago

Finished benchmarking commit (6db6a2d): comparison url.

Summary:

• Primary benchmarks: relevant improvements found
• Secondary benchmarks: relevant improvements found

Regressions
(primary) Regressions
(secondary) Improvements
(primary) Improvements
(secondary) All
(primary)

count1 0 0 22 11 22

mean2 N/A N/A -1.7% -1.2% -1.7%

max N/A N/A -4.2% -3.3% -4.2%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

1. number of relevant changes

2. the arithmetic mean of the percent change

nnethercote commented 12 days ago

Why isn't it suitable? compile_declarative_macro is actually a much better place for reporting these errors, because they are definition-site errors rather than use-site errors. In compile_declarative_macro those errors can be reported right away, without stashing them to ParseSess::missing_fragment_specifiers, which then can be removed.

I wish this was true, because I would like to remove missing_fragment_specifiers. But the errors given depend on whether or not the macro with the missing fragment specifier is used. Consider this program:

#[allow(unused_macros)]
macro_rules! m_used { ( $id ) => {} }
macro_rules! m_unused { ( $id ) => {} }
fn main() { m_used!(x); }

Here's the current compiler output:

error: missing fragment specifier
 --> v.rs:2:25
  |
2 | macro_rules! m_used { ( $id ) => {} }
  |                         ^^^

error: missing fragment specifier
 --> v.rs:3:27
  |
3 | macro_rules! m_unused { ( $id ) => {} }
  |                           ^^^
  |
  = note: `#[deny(missing_fragment_specifier)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

Note that the two errors are different. Even worse, if you add #![allow(missing_fragment_specifier)], then the second error disappears.

Based on this, if we want to keep the current behaviour, we need to keep the checking during macro expansion. If you think it's reasonable to change the behaviour here, I'd prefer to do it in a follow-up, to keep this PR just about performance improvements. Therefore, I have kept the checking in the macro expansion phase for now.

petrochenkov commented 11 days ago

@nnethercote
The intent is to make

macro_rules! m_unused { ( $id ) => {} }

an error.

Previously it wasn't possible to catch missing specifiers at definition site without a significant rewrite, so the old implementation caught them at call site on a best effort basis.
It's ok to move it to the definition site in a separate PR.

petrochenkov commented 11 days ago

@bors r+

bors commented 11 days ago

Commit 238d907 has been approved by petrochenkov

bors commented 11 days ago

Testing commit 238d907 with merge c2afaba...

bors commented 11 days ago

Test successful - checks-actions
Approved by: petrochenkov
Pushing c2afaba to master...

nnethercote commented 11 days ago

It's ok to move it to the definition site in a separate PR.

Thanks.

I just looked over the history here, these issues/PRs are relevant: #40107, #63239, #75516, #76605, #80296. Seems like this has been tried in the past and had to be reverted due to widespread breakage?

What steps are required for this? Would the #[deny(missing_fragment_specifier)] be removed? Would an MCP be needed? Would a crater run be needed? Can this only be done in a new edition?

petrochenkov commented 11 days ago

Would the #[deny(missing_fragment_specifier)] be removed?

No, no, I suggest to keep it as a lint, as it is now, just report in at definition site (and even if the macro is unused).

rust-timer commented 11 days ago

Finished benchmarking commit (c2afaba): comparison url.

Summary:

• Primary benchmarks: relevant improvements found
• Secondary benchmarks: relevant improvements found

Regressions
(primary) Regressions
(secondary) Improvements
(primary) Improvements
(secondary) All
(primary)

count1 0 0 23 10 23

mean2 N/A N/A -1.4% -1.3% -1.4%

max N/A N/A -3.5% -3.4% -3.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes

2. the arithmetic mean of the percent change