Completion in macros #3513
Completion in macros #3513
Conversation
|
It works in the most important macros, at least 😄 You can see I have to type one extra letter for dot completions to work in |
| let mut fake_ident_token = fake_ident_token; | ||
|
|
||
| // Are we inside a macro call? | ||
| while let (Some(actual_macro_call), Some(macro_call_with_fake_ident)) = ( |
There was a problem hiding this comment.
Originally I've thought that recusively expanding the macros is exactly how we should do IDE features. However, @edwin0cheng had an idea which seems like it kinda can be better? In syntax highlighting, we descend to the bottom of macro expansion in one step:
I think ideally this is what we want to do for completion as well. Although I am not sure if expand_hypothetical can be made to work this way, especially if we switch from "resolve" to "record" strategy for SourceAnalyzer
There was a problem hiding this comment.
Some vague thoughs for negative durability in salsa for such hypothetical cases...
There was a problem hiding this comment.
Support in salsa for throwaway 'alternative universes' would be cool...
There was a problem hiding this comment.
But about the recursive expanding, I wanted to avoid a situation where the 'hypothetical' expansion is too different; I think we need to 'follow along' both to keep track. There are probably a lot of ways this can currently break if the inserted token changes the expansion too much.
| match self.token.kind() { | ||
| // workaroud when completion is triggered by trigger characters. | ||
| IDENT => self.token.text_range(), | ||
| IDENT => self.original_token.text_range(), |
There was a problem hiding this comment.
Hm, this seems like a preexisting bug? Perhaps we need to flip the naming? original_x => x, x => fake_x?
There was a problem hiding this comment.
This is a newly introduced distinction between the token in the original source file and the token in the macro expansion, not between real/fake 😅
There was a problem hiding this comment.
... and in most situations we do want the macroexpanded token.
Two uses only needed the crate; one was wrong and should use the module from the scope instead.
crates/ra_hir/src/semantics.rs
Outdated
| pub fn expand_hypothetical( | ||
| &self, | ||
| actual_macro_call: &ast::MacroCall, | ||
| hypothetical_call: &ast::MacroCall, | ||
| token_to_map: SyntaxToken, | ||
| ) -> Option<(SyntaxNode, SyntaxToken)> { | ||
| let macro_call = | ||
| self.find_file(actual_macro_call.syntax().clone()).with_value(actual_macro_call); | ||
| let sa = self.analyze2(macro_call.map(|it| it.syntax()), None); | ||
| let macro_call_id = macro_call | ||
| .as_call_id(self.db, |path| sa.resolver.resolve_path_as_macro(self.db, &path))?; | ||
| let macro_file = macro_call_id.as_file().macro_file().unwrap(); | ||
| let (tt, tmap_1) = | ||
| hir_expand::syntax_node_to_token_tree(hypothetical_call.token_tree().unwrap().syntax()) | ||
| .unwrap(); | ||
| let range = token_to_map | ||
| .text_range() | ||
| .checked_sub(hypothetical_call.token_tree().unwrap().syntax().text_range().start())?; | ||
| let token_id = tmap_1.token_by_range(range)?; | ||
| let macro_def = hir_expand::db::expander(self.db, macro_call_id)?; | ||
| let (node, tmap_2) = hir_expand::db::parse_macro_with_arg( | ||
| self.db, | ||
| macro_file, | ||
| Some(std::sync::Arc::new((tt, tmap_1))), | ||
| )?; | ||
| let token_id = macro_def.0.map_id_down(token_id); | ||
| let range = tmap_2.range_by_token(token_id)?.by_kind(token_to_map.kind())?; | ||
| let token = algo::find_covering_element(&node.syntax_node(), range).into_token()?; | ||
| Some((node.syntax_node(), token)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Neat!
So what this function does is, replace the original macro call with the hypothetical_call arguments. And I see the limitation here: we don't want to pollute the salsa database such that we can't convert the hypothetical_call to an actual MacroCallId. So we have to expand it manually.
One small nit: we might change the arguments to reflect it only replace the arguments :
pub fn expand_hypothetical(
&self,
actual_macro_call: &ast::MacroCall,
hypotheticall_args: &ast::TokenTree,
token_to_map: SyntaxToken,
)And I can imagine we could move some of the logic here to hir_expand but overall it is awesome !
There was a problem hiding this comment.
Good points, I'll move most of it to hir_expand, then I think it can be pretty contained there.
flodiebold
force-pushed
the
completion-in-macros
branch
2 times, most recently
from
cb06162 to
59da22f
Compare
flodiebold
force-pushed
the
completion-in-macros
branch
from
59da22f to
afdf08e
Compare
|
Here's something cool:
Or this:
😄 |
|
LGTM ! |
|
bors r+ |
Build succeeded |
I experimented a bit with completion in macros. It's kind of working, but there are a lot of rough edges.
println!("", x<|>)the expansions will look the same and everything is fine; but in that case we could maybe have achieved the same result in a simpler way. If we have something likem!(<|>)wherem!()doesn't even expand or expands to something very different, we don't really know what to do anyway.m!(x.<|>)the original token tree doesn't parse as an expression; if we havem!(match x { <|> })the hypothetical token tree doesn't parse. It would be nice if we could have better error recovery in these cases.