-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Make Semantics<'db, DB>
support Semantics<'db, dyn HirDatabase>
, take two
#19930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Semantics<'db, DB>
support Semantics<'db, dyn HirDatabase>
, take two
#19930
Conversation
I think I prefer to have a new constructor |
But CC @rust-lang/rust-analyzer what is your opinion? |
Whatever enabled type-erased |
Just taking |
536faf5
to
0890be4
Compare
Oh, you're right! I've made the corresponding changes. |
0890be4
to
1a3feee
Compare
@ChayimFriedman2 any chance to get this into the upcoming release? |
I'm waiting for opinions from other team members. CC @rust-lang/rust-analyzer again. |
I think seems reasonable to me! |
It had already missed the release, but here you go. |
Much appreciated, @lnicola! |
(This is a follow-up for #19850)
@ChayimFriedman2 it looks like I was a bit too quick trusting the suggested fix of just replacing
DB
withDB: ?Sized
: it doesn't work, theSemantics<'_, dyn HirDatabase>
isn't actually constructible.I only did a quick type-level check like this on the previous PR, following your change request, which compiled just fine:
But replacing
todo!()
withSemantics::new(db)
would have quickly shown that a couple ofimpl
s would requireDB: ?Sized
as well (iirc I had those initially, but clippy complained about them being unnecessary, so I removed them).That said: even with those fixed, the "just change
DB
toDB: ?Sized
doesn't work, due to this:So I'd like to re-propose either:
DynSemantics<'db>
in addition to the existing sema types (as I initially proposed), orSemanticsImpl<'db>
withDynSemantics<'db>
(or rather refactoring it into that shape).I've pushed the missing
DB: ?Sized
changes as a commit to this PR to illustrate the issue.I'll replace it with whatever approach gets picked.