internal: Upgrade to the next Salsa#19661
Conversation
rustbot
added
the
S-waiting-on-review
|
I can kick off a new salsa release if you want |
ChayimFriedman2
force-pushed
the
salsa-v20
branch
from
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 |
| 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.
ChayimFriedman2
force-pushed
the
salsa-v20
branch
from
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`
ChayimFriedman2
force-pushed
the
salsa-v20
branch
from
071aae2 to
6e4abf1
Compare
|
Okay. Rebased and changed to use Salsa from crates.io. |
3 participants
This depends on Salsa from GitHub; should maybe wait until there is a release.
Fixes #19455.