internal: Upgrade to the next Salsa #19661
Conversation
|
I can kick off a new salsa release if you want |
fe6e9f8 to
8ead327
Compare
|
It'll be better if you could also insert salsa-rs/salsa#803 into it :) |
|
Alright can you verify things work with latest salsa master? If yes I'll cut a release there |
|
Just checked - it works fine! 🚀 |
|
0.20.0 released |
| @@ -136,7 +136,8 @@ pub fn from_assoc_type_id(id: AssocTypeId) -> TypeAliasId { | |||
|
|
|||
| pub fn from_placeholder_idx(db: &dyn HirDatabase, idx: PlaceholderIndex) -> TypeOrConstParamId { | |||
| assert_eq!(idx.ui, chalk_ir::UniverseIndex::ROOT); | |||
| let interned_id = FromId::from_id(Id::from_u32(idx.idx.try_into().unwrap())); | |||
| // SAFETY: We cannot really encapsulate this unfortunately, so just hope this is sound. | |||
| let interned_id = FromId::from_id(unsafe { Id::from_u32(idx.idx.try_into().unwrap()) }); | |||
There was a problem hiding this comment.
This is really fishy in general I have to say, chalk even seems to allocate its own PlaceholderIndexes so we just gotta hope those don't ever leak out of it 😅
(this was kind of unsound already etiher way)
Hopefully whatever the new trait solver does allows us to get rid of this weird interning here.
There was a problem hiding this comment.
salsa even seems to allocate its own
PlaceholderIndexes
You mean Chalk?
There was a problem hiding this comment.
In general I'm not sure how unsafe using an incorrect Id is; it should not cause UB because you can safely create an Id then use it on a different database.
| /// # Invariant | ||
| /// | ||
| /// This is either a valid `salsa::Id` or a root `SyntaxContext`. | ||
| u32, |
There was a problem hiding this comment.
Why do we need this change? Ideally in the future we will be able to use the salsa macros to define the type here and then this change is moot.
There was a problem hiding this comment.
I did it in order to encapsulate the unsafety of creating an Id well; previously it was already an Id and you could just hope nobody will ever use it incorrectly if it's a root SyntaxContext (that isn't really an Id), now the conversion to Id checks if it is a root.
If/when we convert this to Salsa macros this can go.
8ead327 to
071aae2
Compare
This impacts our manual `salsa::Id` wrappers. I refactored them a bit to improve safety.
This one does need fixpoint.
…t `&dyn Database`
071aae2 to
6e4abf1
Compare
|
Okay. Rebased and changed to use Salsa from crates.io. |
This depends on Salsa from GitHub; should maybe wait until there is a release.
Fixes #19455.