Support format_args_capture

[tamasfe]

The format_args_capture feature seems to have been stabilized and will be enabled in 1.58.0 which is pretty soon. It would be nice to resolve the captured identifiers.

[JosephTLyons]

Auto-completion here would be fantastic!

[felipesere]

Is there some broad guidance if I wanted to work on this?

[Veykril]

The problem here is that we don't have free tokens that we can do analysis on, but jsut a string literal that may contain multiple references to things. This will require special cases in basically all places that allow interacting with local variables, so supporting this will most likely turn out rather messy.

So we either have to special case everything, which I really hope we don't have to but what I fear we will inevitable are forced to do. Or we'll have to think a bit about whether we can change some internal things in a more general way to allow this "mapping of partial spans of tokens" which we currently just can't do.

With that said I think the biggest support need here right now is probably renaming which I think we can just special case for the time being if the demand is there, with that in mind I believe the steps for that are the following:

• adjust https://github.com/rust-lang/rust-analyzer/blob/master/crates/hir-expand/src/builtin_fn_macro.rs#L234-L273 to properly extract the captures from within the format string and expand those like the rest in there. This change alone won't make any difference though, its just the simplest thing to tackle right now.

• adjust our usage searching code here https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-db/src/search.rs#L436-L540 to also look for ast::String literals containing the locals if we are in fact searching for a local and then extract the corresponding ast::NameRef from the expansion of step 1 with the text range being the format specifier reference in the string literal.

• adjust this function to also look for format strings (and maybe some follow up things) in the rename code to support renaming https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide/src/rename.rs#L121-L202

And finally put a big BIG reminder on those special cases that they should hopefully be temporary. With all said, this is probably a tricky one to fix, even if we only consider the special casing right now ...

[jplatte]

To clarify, it's just hovering over that part of the format string and getting info in a popover that would be very hard? If rename would be simple enough, does the same apply to making extract-to-function work on a snippet that references a given variable only through such a format string?

[Veykril]

Hover should be fairly easy to implement, just a check if we hover a string literal that is part a format_args invocation, extract the captured identifiers and check if the cursor hover pos is inside of the range of one of the identifiers. In general most are equally simple/difficult, its just that we need to special case this for basically every ide feature that interacts via tokens which is just horrible :) But necessary for the time being.

I only see us either special casing this everywhere or revamping our token map architecture which we need to rewrite anyways, though I believe if we make that text range based (which I think was the plan) we could fix this up in a better way.

[felipesere]

I've started a PR where I naively extract identifiers from the format_args.

I saw that crates/ide-db/src/syntax_helpers/format_string.rs
has most of a parser for that already built. Should be re-using that?
The fact that it was under ide-db made me doubt myself 🤷‍♂️

[jonas-schievink]

I'd really prefer to implement this on top of #9403 (which should allow mapping arbitrary spans instead of full tokens into a macro expansion) rather than special-casing implicit captures in every r-a feature

[Veykril]

So do I, I didn't mean to propose us to special case everything now. I was mainly proposing to special case renaming for now since that is the most important part (in my eyes at least) and special casing that should not be too much work.

I should've been clearer about that.

[felipesere]

I've been ill for the past few days so this has not progressed any further. When I'm back on my feet I expect to come back to trying to get renames to work across formatted strings. 😅

[Veykril]

I'd really prefer to implement this on top of #9403 (which should allow mapping arbitrary spans instead of full tokens into a macro expansion) rather than special-casing implicit captures in every r-a feature

Coming back to this actually, I just realized that we have no choice but to special case everything even if we fix our tokenmap. The reason is that strings are single tokens, so assuming our tokenmap was implemented with the ideal architecture, if we process a hover on a capture format identifier we will hover strictly speaking still the string token, so we still have to extract the hovered identifier etc. As such, there is no way for us not to special case these in every ide function that works on NameRefs ...

Macros keep making the language worse for IDEs it seems 😕

I've been ill for the past few days so this has not progressed any further. When I'm back on my feet I expect to come back to trying to get renames to work across formatted strings. 😅

No rush :)

[jonas-schievink]

I was thinking that hover would see that the cursor is hovering part of a macro input, and map down the cursor position, not the whole token it's on. Basically, only ever map down ranges and positions rather than tokens, for all macros and all IDE functionality.

Of course, this requires the base infra for macros to be changed, but that was the plan with the new TokenMap architecture already. We might also run into LSP limitation where only whole tokens are supported for some operations.

[Veykril]

Oh, now I see. Yes that would get rid of the problem mostly. Some of our apis actually already work with offsets like Semantics::find_nodes_at_offset_with_descend, so I guess whwat felipesere can do here is hide the current special casing behind the semantics like that already. Then in the end if we swap the token map architecture it shouldn't require a lot of changes in the ide layer.

[felipesere]

So I'm recovered and back on this :)
Is Zulip or here the best place to ask questions?

[Veykril]

zulip would be better, but either works.

[CAD97]

The proc_macro API currently doesn't have any even unstable way to split Spans, so format_args! is the only current way that a macro will emit subspanned tokens.

Now that #[attributes(…)] have been able to take arbitrary token streams for a significant period of time, the percentage of macros taking code as strings is going down, but some still do.

The "perfect" solution would probably be if proc-macro-srv could be used to process format_args! and gave back the properly subset spans. The tokenmap would still need to handle this case, but probably more of the logic could be better encapsulated that way. (I'm just guessing, though.)

For the purpose of hover etc., doesn't VSCode support language embedding? AIUI, the typical way template strings are handled in the IDE is language embedding — telling the IDE that it should treat the string as the language "Rust template string", and then that language having the grammar and highlighting rules (and semantic handling) for the template string grammar, potentially recursively embedding the parent language again with more featureful template strings like Javascript's.

I don't know exactly how it functions, nor even if it's exposed via the semantic highlight / LSP interface or just the tmGrammar interface; I just know that intellisence inside template string embedding has worked well for me both with Typescript in VSCode and Kotlin on the IntelliJ Platform, so it's at least a problem which has been successfully solved multiple times before. (Though it may have been a pile of hacks all of those times as well... though given Kotlin was designed alongside its IDE integration, it's somewhat difficult to believe JetBrains would've made a decision specifically difficult to support in their own IDE.) However, both of those cases are also built-in language features identifiable by the token, instead of extra interpolation semantics added to normal nontemplate strings by how they're used, so.... IIRC, more contextual language embedding (e.g. SQL template strings) only work when "sufficiently" close to where they're given to the magic recognized function, which might be a more relevant comparison.

[Veykril]

The "perfect" solution would probably be if proc-macro-srv could be used to process format_args! and gave back the properly subset spans. The tokenmap would still need to handle this case, but probably more of the logic could be better encapsulated that way. (I'm just guessing, though.)

That wouldn't really simplify things. The main problem we have is the fact that the tokenmap doesn't work with text ranges at all atm. So we just cannot support this span splitting until we rewrite that. I am looking into rewriting the tokenmap atm though, I just unfortunately do not have a lot of time for rust-analyzer right now.

I am not sure how a language embedding would help, since you'd still need to resolve the identifiers, and I don't think you can span rust-analyzer twice to embed itself like that. Overall that also seems way too complicated (in fact, how would the embedded server kn ow about the outside part of the format call for resolution).

Typescript and kotlin have the benefit that template strings are first class, unlike rust which makes this way harder for rust-analyzer unfortunately, as we have to first drill down the macro expansion instead of just looking at the literal immediately.

[nyurik]

rust-lang/rust#106745 has just merged, which moves format_args into AST. Would this help with rust-analyzer processing of this issue?

[Veykril]

No, rust-analyzer doesn't reuse any of that rustc's machinery (except for the lexer and target layout calculation). I hope I'll be able to get back to rewriting our token map soon though, since thats what this issue is mainly blocked on.

[nyurik]

Thanks!

@Veykril could you give some feedback on rust-lang/rust-clippy#10087 (comment) , maybe to coordinate it better with the users? Thanks! :)

[HKalbasi]

I think implementing the expansion even without token map rewrite slightly helps here, since it will make things like renaming inapplicable instead of working wrong, and makes things like find all references working (and doesn't help at all in things like auto complete).