make .stash(..) work in macros without ICEing

[Centril]

Fixes #69396, which was injected by #64698, by unique-ifying the Span part of the key used when stashing the diagnostic such that the key's slot isn't overwritten in handler.stash_diagnostic(..).

This is similar to the usage of fresh_expansion in mark_span_with_reason (but that's not used for unique-ifying). Stashing is somewhat similar in the sense that it is sort of about compiler generated code as well.

An alternative approach would be to store Vec<Diagnostic> instead of Diagnostic, but the disadvantage of such a solution is that it might handle non-determinism poorly when stealing diagnostics (e.g. if stealing order would be different than stashing order). The approach in this PR feels more robust in comparison.

r? @petrochenkov

[petrochenkov]

😰

[Centril]

That bad, eh? 😂

[spastorino]

Suggested change

	/// AST fragements produced by parser recovery.
	/// AST fragments produced by parser recovery.

[petrochenkov]

I'd really like to avoid introducing new uses of fresh_expansion, especially for something that is not expansion/desugaring.

[petrochenkov]

The diagnostic stashing problem can be generalized as "Give me a unique ID at point A that can be retrieved later at point B".

The both attempts at stashing API tried to use some existing data as dual purpose - their primary meaning and the unique ID.

The first attempt was to use Span as the unique ID, obviously a bad idea, with proc macros you can have a whole wall of code having the same macro invocation span.

The second attempt is to create unique Spans using macro expansion machinery.
This gives better uniqueness, but cannot be used with e.g. identifier spans which need their original spans for name resolution. It's also a pretty big hack in the sense of using tools existing for entirely different purposes.

[petrochenkov]

"Proper" solution would be using a separate ID, DiagStashId or something.
These IDs would be added to nodes using diagnostic stashing, like ast::Ty or what else is uses it currently.
Is it an overkill? Maybe yes.

Another alternative is to hide these stash IDs in NodeIds instead of spans.

• If the node ID is already assigned, it can be used for stashing.
• If the node ID is not yet assigned it can be
  • Assigned immediately (not sure whether the necessary infrastructure is ready, plus some extra IDs eliminated during expansion can be created, plus some validators may complain about non-contiguous ID ranges or something.)
  • Temporary ID can be assigned replaced by the proper ID (fn visit_id in expand.rs).
    The proper node ID assignment can then update the "proper ID -> temporary ID" mapping, or update IDs in the diagnostic stash directly.

[petrochenkov]

I'd personally just nuke the whole diagnostic stashing and report everything in the traditional way :)
So far it didn't get any new uses since it was introduced.

[petrochenkov]

One more alternative: remove the assertion in fn stash_diagnostic.

If we hit this case, then we will report at least one error (assuming all stashed diagnostics are errors), so it's acceptable to omit some further errors.

This is perhaps the easiest way to fix #69396 without introducing more evil into the codebase. (Also the most backportable.)

[Centril]

Some thoughts:

I'd really like to avoid introducing new uses of fresh_expansion, especially for something that is not expansion/desugaring.

It might be a good idea to improve the docs on fresh_expansion to discourage new uses or something. (In general, the docs on Span & friends are pretty sparse.)

The second attempt is to create unique Spans using macro expansion machinery.
This gives better uniqueness, but cannot be used with e.g. identifier spans which need their original spans for name resolution. It's also a pretty big hack in the sense of using tools existing for entirely different purposes.

Could you elaborate a bit regarding name resolution? I don't think I understand the problem right now.

Is it an overkill? Maybe yes.

Yeah it does sound overkill -- the reason I used Span is because it was already used by almost everything, so I wouldn't need to make invasive AST structure changes.

Another alternative is to hide these stash IDs in NodeIds instead of spans.

This does sound like a promising approach since NodeIds are also a ubiquitous structure in the AST data types. I don't think the HIR validator should be a problem since expansion should already be done before we reach lowering. You might not even need to replace temporary IDs at all with proper ones if we simply start the "proper" ones at +1 the highest stash-NodeId?

I'd personally just nuke the whole diagnostic stashing and report everything in the traditional way :)
So far it didn't get any new uses since it was introduced.

For what it's worth, I do have other cases in mind where I would like to use the stashing API but haven't gotten around to it yet. (I've mostly discussed these with @estebank in private.)

I would be OK with nuking the API if the consensus is that it is too architecturally problematic, though the diagnostics regressions would be unfortunate and increase pressure for something like rust-lang/rfcs#2545. cc @rust-lang/wg-diagnostics

One more alternative: remove the assertion in fn stash_diagnostic.

Alright; I'll create another PR with this fix while we continue to discuss the long term strategy here.

[bors]

☔ The latest upstream changes (presumably #69635) made this pull request unmergeable. Please resolve the merge conflicts.

[Centril]

Closing this PR in favor of #70069 as this isn't a high priority item to work on for now.