-
Notifications
You must be signed in to change notification settings - Fork 152
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
Fix DebugWithDb
in multi-crate Salsa projects
#509
Conversation
Previously, the auto-generated impls hard-coded the database's type to be `&dyn Db`, with `Db` being the local crate's database. Due to limitations of trait upcasting, this meant that Salsa structs containing Salsa structs from multiple different crates would not be printed properly. They would fallback to the plain `std::fmt::Debug` impl, which just prints an ID -- not very useful. Now, the impls are generic over the database type. Any database that implements the local database trait will be accepted. For more background, see this Zulip conversation [1]. [1]: https://salsa.zulipchat.com/#narrow/stream/145099-general/topic/recursive.20DebugWithDb.3F
Unfortunately, this regresses some unrelated error messages.
✅ Deploy Preview for salsa-rs canceled.
|
Ideally, we'd have some tests to make sure this works cross-crate, but I thought I'd let you see what you think of the impl first :) It's also unfortunate that this regresses some unrelated error messages, although at present I don't see a good way around it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks right to me.
I'm up to merge this -- but any idea why CI is unhappy. Seems like a netlify issue I guess. |
@camelid would you like to add some tests pre-merge? |
Yeah, I think it's spurious.
Sure 👍 |
The new auto-impl for `DebugWithDb` caused this error message to regress because it expects the `__salsa_crate_Db` to be in the same location as the `Jar`. This fixes it by using an absolute path. Note that using custom `Jar` locations wasn't supported properly before either.
@nikomatsakis I just pushed up a test that ensures the derived impls are generic. This seemed easier than making a multi-crate test and also tests the actual change rather than its effect. I also ran the same test on master and confirmed that it errors out, thus it is working properly. So I think this PR is ready to be merged!
|
No longer relevant |
Previously, the auto-generated impls hard-coded the database's type to
be
&dyn Db
, withDb
being the local crate's database. Due tolimitations of trait upcasting, this meant that Salsa structs containing
Salsa structs from multiple different crates would not be printed
properly. They would fallback to the plain
std::fmt::Debug
impl, whichjust prints an ID -- not very useful.
Now, the impls are generic over the database type. Any database that
implements the local database trait will be accepted.
For more background, see this Zulip conversation.
r? @nikomatsakis