Call compute_locs once per rule

[nnethercote]

This fixes the small regressions on wg-grammar and hyper-0.14.18 seen in #95555.

r? @petrochenkov

[nnethercote]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit a68238b68f9fd4adbe9ea196d1473c297577fb32 with merge 6db6a2df45e20133e0d7c5f7353baf3c5e94e711...

[nnethercote]

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]

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]

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]

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

[rust-timer]

Queued 6db6a2df45e20133e0d7c5f7353baf3c5e94e711 with parent 949b98c, future comparison URL.

[rust-timer]

Finished benchmarking commit (6db6a2df45e20133e0d7c5f7353baf3c5e94e711): 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]

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]

@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]

@bors r+

[bors]

📌 Commit 238d907 has been approved by petrochenkov

[bors]

⌛ Testing commit 238d907 with merge c2afaba...

[bors]

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

[nnethercote]

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]

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]

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 ↩