fix: Make edition per-token, not per-file #18861
Conversation
rustbot
added
the
S-waiting-on-review
| struct#0:[email protected]#1# MyTraitMap2#0:[email protected]#0# {#0:[email protected]#1# | ||
| map#0:[email protected]#1#:#0:[email protected]#1# #0:[email protected]#1#::#0:[email protected]#1#std#0:[email protected]#1#::#0:[email protected]#1#collections#0:[email protected]#1#::#0:[email protected]#1#HashSet#0:[email protected]#1#<#0:[email protected]#1#(#0:[email protected]#1#)#0:[email protected]#1#>#0:[email protected]#1#,#0:[email protected]#1# | ||
| }#0:[email protected]#1# | ||
| struct#0:[email protected]#4# MyTraitMap2#0:[email protected]#2# {#0:[email protected]#4# |
There was a problem hiding this comment.
We really ought to do something about these tests, in new Salsa they change every time someone create a new interned type and that is awful.
There was a problem hiding this comment.
Yes, we talked about that on zulip iirc, for new salsa a bunch of tests need to change the rendering of what they expect. For this kind of stuff we likely need to have the test allocate a map and assign stable IDs that way
ChayimFriedman2
force-pushed
the
await-edition
branch
3 times, most recently
from
5103b4c to
0ccdf52
Compare
Yep, edition is part of hygiene (which is a per token, or well, per span thing) |
| struct#0:[email protected]#1# MyTraitMap2#0:[email protected]#0# {#0:[email protected]#1# | ||
| map#0:[email protected]#1#:#0:[email protected]#1# #0:[email protected]#1#::#0:[email protected]#1#std#0:[email protected]#1#::#0:[email protected]#1#collections#0:[email protected]#1#::#0:[email protected]#1#HashSet#0:[email protected]#1#<#0:[email protected]#1#(#0:[email protected]#1#)#0:[email protected]#1#>#0:[email protected]#1#,#0:[email protected]#1# | ||
| }#0:[email protected]#1# | ||
| struct#0:[email protected]#4# MyTraitMap2#0:[email protected]#2# {#0:[email protected]#4# |
There was a problem hiding this comment.
Yes, we talked about that on zulip iirc, for new salsa a bunch of tests need to change the rendering of what they expect. For this kind of stuff we likely need to have the test allocate a map and assign stable IDs that way
| @@ -58,8 +58,8 @@ impl BuiltinDeriveExpander { | |||
| tt: &tt::TopSubtree, | |||
| span: Span, | |||
| ) -> ExpandResult<tt::TopSubtree> { | |||
| let span = span_with_def_site_ctxt(db, span, id); | |||
| self.expander()(span, tt) | |||
| let span = span_with_def_site_ctxt(db, span, id, Edition::CURRENT); | |||
There was a problem hiding this comment.
Now I wonder, what is the edition rustc sets for these? Presumably its the edition of the standard library?
| @@ -369,7 +369,8 @@ pub fn expect_fragment<'t>( | |||
| ) -> ExpandResult<tt::TokenTreesView<'t, Span>> { | |||
| use ::parser; | |||
| let buffer = tt_iter.remaining(); | |||
| let parser_input = to_parser_input(edition, buffer); | |||
| // FIXME: Pass the correct edition per token. Due to the split between mbe and hir-expand it's complicated. | |||
| let parser_input = to_parser_input(buffer, &mut |_ctx| edition); | |||
There was a problem hiding this comment.
Why is that? syntax context lookup isn't defined on the ExpandDatabase is it?
There was a problem hiding this comment.
Ah drats, forgot we define the id in span, but not the content (we probably need to do use some extra trait setup to fix that post salsa swap to do dependency injection)
|
|
||
| pub fn is_root(self) -> bool { | ||
| self == Self::ROOT | ||
| self.into_u32() <= Edition::LATEST as u32 |
There was a problem hiding this comment.
So in new salsa we still do kind of the same thing see f0ab742
But instead of using 0 we now use the max ID - 1 (which is the actual maximum), now that we have multiple roots we would need to change the check to ignore the page bits but otherwise this change should not be a problem for the salsa upgrade
|
Neat, I knew we were missing this when I was implementing the hygiene setup but expected this to be a lot more annoying to get to work. Glad to have been proven wrong. |
More correctly, *also* per-token. Because as it turns out, while the top-level edition affects parsing (I think), the per-token edition affects escaping of identifiers/keywords.
ChayimFriedman2
force-pushed
the
await-edition
branch
from
0ccdf52 to
97afb7b
Compare
More correctly, also per-token. Because as it turns out, while the top-level edition affects parsing (I think), the per-token edition affects escaping of identifiers/keywords.
Fixes #18857.
CC @davidbarsky, I know the
SyntaxContextIdstuff is problematic for new Salsa and I don't know enough your solution, can you take a look this isn't problematic?