feat: Implement attribute input token mapping, fix attribute item token mapping #9970
Conversation
| let token_id = if let Some(item) = item { | ||
| let call_id = match self.expanded.file_id.0 { | ||
| HirFileIdRepr::FileId(_) => return None, | ||
| HirFileIdRepr::MacroFile(macro_file) => macro_file.macro_call_id, | ||
| }; | ||
| let loc = db.lookup_intern_macro(call_id); | ||
|
|
||
| let token_range = token.value.text_range(); | ||
| match &loc.kind { | ||
| MacroCallKind::Attr { attr_args, invoc_attr_index, .. } => { | ||
| let attr = item.attrs().nth(*invoc_attr_index as usize)?; | ||
| match attr.token_tree() { | ||
| Some(token_tree) | ||
| if token_tree.syntax().text_range().contains_range(token_range) => | ||
| { | ||
| let attr_input_start = | ||
| token_tree.left_delimiter_token()?.text_range().start(); | ||
| let range = token.value.text_range().checked_sub(attr_input_start)?; | ||
| let token_id = | ||
| self.macro_arg_shift.shift(attr_args.1.token_by_range(range)?); | ||
| Some(token_id) | ||
| } | ||
| _ => None, | ||
| } | ||
| } | ||
| _ => None, | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let token_id = match token_id { | ||
| Some(token_id) => token_id, | ||
| None => { | ||
| let range = | ||
| token.value.text_range().checked_sub(self.arg.value.text_range().start())?; | ||
| let token_id = self.macro_arg.1.token_by_range(range)?; | ||
| self.macro_def.map_id_down(token_id) | ||
| } | ||
| }; |
There was a problem hiding this comment.
The actual mapping is unfortunately now split a bit(that is between attributes and macrodef/macrorules mapping which happens in macro_def.map_id_down) and I don't think we can easily fix this up as attributes need a lot more info here. Maybe someone else sees a way to refactor this better.
Currently this means speculative expansion needs to reimplement this mapping as well.
Veykril
changed the title
| MacroCallKind::Attr { attr_args, .. } => Some(attr_args), | ||
| MacroCallKind::Attr { attr_args, .. } => { | ||
| let mut attr_args = attr_args.0.clone(); | ||
| mbe::Shift::new(¯o_arg.0).shift_all(&mut attr_args); |
There was a problem hiding this comment.
This creates a new shift instance on every expansion, is this fine?
If not we can either have the macro_arg query return a Shift instance which would be only relevant to attribute macros.
Or swap things around, shift the attribute item input IDs by the normal attribute input IDs and store the Shift in the MacroCallKind::Attr. Though in this case we would have to clonse the entire item TokenTree.
I'm unsure which approach is best of the three(the current one and the two proposed ones) but I believe turning the shifting around and storing in MacroCallKind::Attr might be best?
Veykril
changed the title
Veykril
changed the title
|
Haven't read this through yet, but my gut feeling is that we should be able to use existing infra without modifications. I don't immediately see the place where we produce two inputs for proc macro, but can we modify that place to use disjoint ids? In some sense, can we say that the input to proc macro is always a single subtree with none parens and two children (the attribute and the item)? Again, don't yet understand the code, might be talking nonsense here! |
|
Ah, I see now! Here we get two arguments from separate code paths. The item is |
Ah, so the thing is, becaues we don't know if attr's arg is actually the macro input. Things like |
| Literal(SmolStr), | ||
| /// `#[attr(subtree)]` | ||
| TokenTree(Subtree), | ||
| TokenTree(tt::Subtree, mbe::TokenMap), |
There was a problem hiding this comment.
Yeah, it seems like we need to add TokenMap here, this is a fundamentally missing info.
|
Can we add a test here? |
|
We don't really have a testing infra for mapping yet do we? Guess I'll look into that first then. |
|
After looking around a bit I'm actually not really sure how we would test the token mappings for proc-macros 🤔 |
|
It’s ok to merge with s without a test then! Though having some completion test with proc macros might be nice! We don’t need to test token maps directly, we want to test hat ide features work |
|
👍 Having completion tests with proc-macros involved would be nice, though I do wonder how we can realize that. bors r+ |
2 participants
The token mapping for items with attributes got overwritten partially by the attributes non-item input, since attributes have two different inputs, the item and the direct input both.
This PR gives attributes a second TokenMap for its direct input. We now shift all normal input IDs by the item input maximum(we maybe wanna swap this see below) similar to what we do for macro-rules/def. For mapping down we then have to figure out whether we are inside the direct attribute input or its item input to pick the appropriate mapping which can be done with some token range comparisons.
Fixes #9867