Make declarative macro expansion a part of query system (cont. of #125356)

[futile]

This is a continuation of the effort to make declarative macro expansion a part of the query system from #125356 by @SparrowLii.

Description from that PR:

This is an attempt to make the macro expansion a part of incremental compilation.

Processing of procedural macros is difficult since they may involve interactions with the external environment. Declaring macros is a good start.

It is not yet possible to test the effect of this PR on incremental compilation since the new query is declared as eval_always.

Status of this PR:

• It is rebased against a much more recent master commit
• It contains the original commits from make declarative macro expansion a part of query system #125356 (in a rebased form)
• It adds the missing implementation for retrying macro matching that provides (better) error diagnostics
  • This was done by refactoring rustc_expand::mbe::diagnostics::failed_to_match_macro() to only require a ParseSess instead of an ExtCtxt. Otherwise, ExtCtxt would need to be in the interface of TcxMacroExpander, which is not possible, because ExtCtxt is part of rustc_expand, which depends on the crate that contains TcxMacroExpander, rustc_middle, and thus would introduce a circular dependency.
  • This refactoring moved the retrying down into the impl TcxMacroExpander for MacroRulesMacroExpander (this is just a change compared to the original PR, otherwise not important to know).
• This PR passes ./x test tests/ui, which produced errors before that were all due to the missing implementation of retry macro matching.
• This PR does not yet contain changes for the open discussions from make declarative macro expansion a part of query system #125356. I wanted to fix the tests first before tackling them, and I also need to figure out what the best solutions for them are. I'd welcome any help/support with this, e.g., opening up a corresponding discussion on this PR with a summary/the final decided fix if available :)

In general, I'm new to working on rustc, so would be thankful for a bit more background/explanations in discussions and comments :) Thanks! :)

(tangentially relevant: #99515)

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

⚠️ Warning ⚠️

• These commits modify submodules.

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[futile]

Did a rebase, to no longer contain changes to submodules (wow rust is moving fast 🙈).

[futile]

r? @petrochenkov

Since you reviewed and self-assigned the original PR, I thought this would be ok/maybe a better fit than auto-assignment.

Please feel free to re-/unassign if that's not the case!

[futile]

Oof, no idea where that error comes from, seems a dependency is broken/flaky? It's in stage0-checking, so can't be due to my changes yet I think? :O

[compiler-errors]

You need to revert the changes you've made to submodules in this PR.

[futile]

You need to revert the changes you've made to submodules in this PR.

Thank you very much! Was a bit finicky, but I think it's correct now, the changes don't appear in the commit that had them anymore.

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

• ebf019d

[petrochenkov]

I'm not sure what I should do with this PR.
Do you plan to work on merging this change instead of @SparrowLii, test whether it actually enables incremental reuse, and answer the questions from #125356? Then we can close that PR.

If some changes are refactorings orthogonal to #125356, then we can merge them in a separate PR before #125356 instead of on top of it (feel free to assign such a PR to me).

[petrochenkov]

Hmm, we can actually run benchmarks on this versions, since there are no CI failures anymore.
@bors try @rust-timer queue

[bors]

⌛ Trying commit 6134b01 with merge 3866917...

[futile]

Thanks for taking a look!

I'm not sure what I should do with this PR. Do you plan to work on merging this change instead of @SparrowLii, test whether it actually enables incremental reuse, and answer the questions from #125356? Then we can close that PR.

I do intend to finish this, and have already started testing whether this enables incremental reuse (see futile/rust@expand1-cont...futile:rust:cache-decl-macros), and am also up to (try to) answer the questions from the other PR. (I think incremental reuse will have to wait for #124141 though, more on that below. Actually might have found a way, see below.)

However, I want to make sure that I'm not taking away @SparrowLii's work here, but I haven't heard back from them yet, so not sure what to do about that.

If some changes are refactorings orthogonal to #125356, then we can merge them in a separate PR before #125356 instead of on top of it (feel free to assign me).

Maybe the changes in rustc_expand::mbe::diagnostics, see here?

They make the module depend less on ExtCtxt, and instead use ParseSess and DiagCtx directly. I can't judge how useful that refactoring is outside the scope of this PR, or if it's actually going in the wrong direction. But that would be one thing I see.

---

So, on the topic of incremental reuse. This will, somewhat obviously, require also making the macro arguments, a TokenStream, part of the key used for the query/caching. Otherwise changed arguments will not trigger recompilation, and instead reuse the old macro expansion result, which is wrong.

However. This will require us to somehow hash the TokenStream, which I thought was not (easily) possible, due to Nonterminals (which can contain parts of the AST). This was also discussed in the original PR: #125356 (comment).

In practice this triggered this panic when compiling core:

rust/compiler/rustc_ast/src/token.rs

Lines 1054 to 1061 in 60d1465

	impl<CTX> HashStable<CTX> for Nonterminal
	where
	CTX: crate::HashStableContext,
	{
	fn hash_stable(&self, _hcx: &mut CTX, _hasher: &mut StableHasher) {
	panic!("interpolated tokens should not be present in the HIR")
	}
	}

However! If I, instead, stable-hash the flattened TokenStream, using TokenStream::flattened(), it seems to actually JustWork™. Meaning, ./x test tests/incremental passes. Wow, just stumbled on that, awesome. I'll probably open a draft PR for the version with incremental reuse, to get full CI. Edit: PR is up here: #128747. Edit 2: CI is green on that PR as well!

That said, I'm not sure if it might make sense to wait for #124141 here anyway, since that should (trivially) enable stable-hashing the TokenStream. But I can't judge if using TokenStream::flattened() instead is wrong/slow/suboptimal for other reasons/etc.

Also ty for the perf run, hope it'll go well! :)

[bors]

☀️ Try build successful - checks-actions
Build commit: 3866917 (38669170c439fc753c04a96a366d3418552933ae)

[petrochenkov]

Maybe the changes in rustc_expand::mbe::diagnostics, see here?

Yes, from a brief look that's the part that can be split.

However! If I, instead, stable-hash the flattened TokenStream, using TokenStream::flattened(), it seems to actually JustWork™.

Yes, the flattening should work in the meantime.

[rust-timer]

Finished benchmarking commit (3866917): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

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)	1.2%	[0.2%, 5.3%]	60
Regressions ❌ (secondary)	0.7%	[0.1%, 1.7%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[0.2%, 5.3%]	60

Max RSS (memory usage)

Results (primary 5.6%, secondary 27.7%)

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)	5.6%	[0.8%, 22.8%]	114
Regressions ❌ (secondary)	27.7%	[1.3%, 93.4%]	32
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.6%	[0.8%, 22.8%]	114

Cycles

Results (primary 3.0%, secondary 3.9%)

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)	3.3%	[1.3%, 8.5%]	21
Regressions ❌ (secondary)	6.4%	[1.9%, 12.4%]	13
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-2.3%	[-2.7%, -2.1%]	5
All ❌✅ (primary)	3.0%	[-2.9%, 8.5%]	22

Binary size

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

Bootstrap: 760.99s -> 762.113s (0.15%)
Artifact size: 336.87 MiB -> 337.09 MiB (0.07%)

[futile]

Dang, but yeah, makes sense that it's a regression, since the path through the query system probably adds some overhead/instructions compared to the direct path. But ~1% on average is sadly a pretty hard hit 🙈

Let's wait what the perf results for #128747 will be, but having these results is nice to have a baseline for the caching cost.

But given these regressions it probably doesn't make sense to merge this without actual re-use, either through #128747 or another approach, which then also has to show an actual performance improvement.

[SparrowLii]

@futile Thank you very much for your work. I can't spend much time on continuing the incremental compilation work for the time being due to some other work. Feel free to close #125356 if your work can replace it. (Notify me or @petrochenkov )

(I was waiting for #124141 to be finished. According to oli's opinion, using TokenStream as the query input parameter may be a better way.)

[petrochenkov]

I'll close this in favor of #128747 to reduce the number of open PRs.
Landing any of this (*) is premature anyway until the whole system is ready and gives some perf benefits.

(*) Except for refactorings (6134b01, maybe something else).