fix: avoid proc-macro re-expansion on trivia-only edits

[Albab-Hasan]

expand_proc_macro depended on macro_arg, a salsa query that re-runs on every edit because token spans embed byte offsets. typing a comment inside an attributed item shifts those offsets so the macro argument subtree changed and the proc-macro was re-expanded even though trivia is stripped from the token tree before the macro sees it.

add MacroArgKey, a newtype over MacroArgResult whose PartialEq ignores span byte ranges. add proc_macro_raw_output, a tracked query that calls the subprocess and is backdated on trivia only edits via MacroArgKey. expand_proc_macro is now a plain function that reads the potentially backdated raw output and remaps stale span byte ranges to current positions using the fresh macro_arg, so parse_macro_expansion always produces a fresh ExpansionSpanMap on trivia edits without reinvoking the subprocess. upmapping and descend_into_macros remain correct. input positions are stored as Box<[TextRange]> (8 bytes/token) and only one TT is cached per proc-macro call.

closes #17213

[ChayimFriedman2]

This will terribly break (panics etc.) thing that use spans returned by macros to lookup the AST etc.. I don't think we can do this.

Also you made the new query transparent which means it won't have the desired effect at all.

[ChayimFriedman2]

Also you made the new query transparent which means it won't have the desired effect at all.

Apologies, I see you didn't. My other point still stands. Plus this means it'll have memory impact, so we need to measure it's not too large.

[Albab-Hasan]

the break i can see is descend_into_macros silently failing for tokens after the edit point stale ExpansionSpanMap ranges dont match the fresh source spans. is that the lookup the AST you meant or is there a more direct panic path you had in mind.

[ChayimFriedman2]

Consider e.g. trying to upmap a token from macro expansion. The span will be off, causing us to look for the token in the wrong place, meaning we'll either find the wrong token or panic. There are many more failure modes like that.

[Albab-Hasan]

fixed the upmapping issue. proc_macro_raw_output now caches the subprocess (backdated on trivia via MacroArgKey), expand_proc_macro is a plain function that remaps stale byte-ranges using the fresh macro_arg so ExpansionSpanMap is always current. memory: one TT per call instead of two. input positions stored as Box<[TextRange]> (8 vs 20 bytes/token).

[ChayimFriedman2]

You just made retrieving macro expansion results way more expensive. This can be fixed by making expand_proc_macro() a query, but that will mean using even more memory.

You also made the system more complicated and fragile for dubious benefit.

memory: one TT per call instead of two. input positions stored as Box<[TextRange]> (8 vs 20 bytes/token).

You cannot analyze it like that. The only way to have a reliable interpretation of the memory impact is to run analysis-stats on some large projects. And I believe it will be large. You can remediate it somewhat by making macro_arg() returning an Arc.

It's also not true that storing text ranges alone will be more efficient than tts storing full ranges because we compress tts. Of course we can do it here too, but that will mean even more complexity.

Frankly I think this is just not worth it. Rerunning macros when their input changes is fine almost always, the number of reruns is O(1). A side-table that applies to all macros (not just procedural macros) like @Veykril proposed in #17323 might be worth it, but even then I'm not sure.

[Albab-Hasan]

parse_macro_expansion is #[salsa_macros::tracked(lru=512)] so expand_proc_macro runs once per revision per macro, not on every retrieval. callers after the first just read the cached result. on trivia edits the cost is collect_spans + in memory remap instead of a subprocess call. on structural edits span_remap is always empty so remap_tt_spans returns the tree without cloning. same cost as before. i could run analysis-stats on a large project for the memory numbers, but a struct with 4 derives fires 4 subprocess calls per keypress inside a doc comment today which is felt.

[ChayimFriedman2]

i could run analysis-stats on a large project for the memory numbers

Please do. If, for example, this adds a 200mb to a typical project, then the answer is pretty much "no" no matter how big is the speed improvement.

a struct with 4 derives fires 4 subprocess calls per keypress inside a doc comment today which is felt.

How is this "felt"? The most latency sensitive thing we have is completion, but completion is not relevant if you're typing trivia. We can also expand derives in parallel (which we don't do currently). Can you really "feel" the latency from the expansion when typing trivia? What IDE operation are you waiting on? If you can bring hard data that shows how big the improvement is, that will be much more convincing.

Even if this is pretty cheap (I admit I didn't reviewed the code more than a cursory look), it still adds overhead to all proc macro expansions, even those not triggered from trivia changes. And, as I said, this also adds code complexity.

[ChayimFriedman2]

As for projects to measure the memory impact on: of course rust-analyzer itself, but this is not enough as rust-analyzer barely uses proc macros, contrary to other popular projects. We also tend to check on https://github.com/oxidecomputer/omicron and https://github.com/facebook/buck2/.

[Veykril]

Yea as Chayim said, we have to update the spans from the output somehow, if they desync, all the IDE features will break.

For declarative macros, all of this is obviously unnecessary, they are fast enough here. The main compelling reason for us to do this here is that we want to avoid shelling out to proc-macros in these cases because some of them can be non-trivial. I recall talking to dioxus folks whose macros are a bit on the heavy side with things they do and they found the macros re-expanding on trivia edits to be odd.

I think the ideal here would be to be able to know within the proc-macro query whether we are re-computing due to a span only change (trivia edit) and then trying to modify the previous expansion result (that is accessing salsa's previous memo) and updating the spans there iff we know the proc-macro did not inspect the spans. That is likely the best we an do here, though I dont' think salsa allows us to do this today.

[ChayimFriedman2]

For declarative macros, all of this is obviously unnecessary, they are fast enough here. The main compelling reason for us to do this here is that we want to avoid shelling out to proc-macros in these cases because some of them can be non-trivial. I recall talking to dioxus folks whose macros are a bit on the heavy side with things they do and they found the macros re-expanding on trivia edits to be odd.

Declarative macros can also be quite expensive.

I'm interested to hear, do the Dioxus folks find the macros reexpanding on trivia edits to be problematic? To what feature? Could they implement their own caching, maybe more granular?

[Veykril]

I'm interested to hear, do the Dioxus folks find the macros reexpanding on trivia edits to be problematic?

I don't recall tbh, its been 3 years or since 😅

Either way I would find it useful for us to be able to avoid calling out to the proc-macro server unless required, though it depends on how complex the solution to that would be.

We could definitely do the same for declarative macros as well fwiw.

[ChayimFriedman2]

If it is not too complex and doesn't have too much speed and memory costs, sure, but if it does (which will likely be the case), then we need to weigh them, and given that the current situation is pretty much fine, I don't believe that will be worth it.