salsa-rs / salsa

A generic framework for on-demand, incrementalized computation. Inspired by adapton, glimmer, and rustc's query system.
https://salsa-rs.netlify.app/
Apache License 2.0
2.09k stars 142 forks source link

Fix `DebugWithDb` in multi-crate Salsa projects #509

Closed camelid closed 1 month ago

camelid commented 2 months ago

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.

r? @nikomatsakis

netlify[bot] commented 2 months ago

Deploy Preview for salsa-rs canceled.

Name Link
Latest commit 75a6ca3ba1301d52bb1a26dc0a9a39b413cfb266
Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/667a35b1cbf8fc000896441d
camelid commented 2 months ago

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.

nikomatsakis commented 2 months ago

I'm up to merge this -- but any idea why CI is unhappy. Seems like a netlify issue I guess.

nikomatsakis commented 2 months ago

@camelid would you like to add some tests pre-merge?

camelid commented 2 months ago

I'm up to merge this -- but any idea why CI is unhappy. Seems like a netlify issue I guess.

Yeah, I think it's spurious.

@camelid would you like to add some tests pre-merge?

Sure 👍

camelid commented 2 months ago

@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!

error[E0277]: the trait bound `MyInputStruct: DebugWithDb<DB>` is not satisfied
  --> tests/derived-debugwithdb-is-generic.rs:34:26
   |
34 |     ensure_generic::<DB, MyInputStruct>();
   |                          ^^^^^^^^^^^^^ the trait `DebugWithDb<DB>` is not implemented for `MyInputStruct`
   |
note: required by a bound in `ensure_generic`
  --> tests/derived-debugwithdb-is-generic.rs:30:69
   |
30 | ..., S: DebugWithDb<DB>>() {}
   |         ^^^^^^^^^^^^^^^ required by this bound in `ensure_generic`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
33 | fn test_all<'db, DB: ?Sized + AsSalsaDatabase + DbWithJar<Jar>>() where MyInputStruct: DebugWithDb<DB> {
   |                                                                   ++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `MyTrackedStruct<'db>: DebugWithDb<DB>` is not satisfied
  --> tests/derived-debugwithdb-is-generic.rs:35:26
   |
35 |     ensure_generic::<DB, MyTrackedStruct<'db>>();
   |                          ^^^^^^^^^^^^^^^^^^^^ the trait `DebugWithDb<DB>` is not implemented for `MyTrackedStruct<'db>`
   |
note: required by a bound in `ensure_generic`
  --> tests/derived-debugwithdb-is-generic.rs:30:69
   |
30 | ..., S: DebugWithDb<DB>>() {}
   |         ^^^^^^^^^^^^^^^ required by this bound in `ensure_generic`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
33 | fn test_all<'db, DB: ?Sized + AsSalsaDatabase + DbWithJar<Jar>>() where MyTrackedStruct<'db>: DebugWithDb<DB> {
   |                                                                   +++++++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `MyInternedStruct<'db>: DebugWithDb<DB>` is not satisfied
  --> tests/derived-debugwithdb-is-generic.rs:36:26
   |
36 |     ensure_generic::<DB, MyInternedStruct<'db>>();
   |                          ^^^^^^^^^^^^^^^^^^^^^ the trait `DebugWithDb<DB>` is not implemented for `MyInternedStruct<'db>`
   |
note: required by a bound in `ensure_generic`
  --> tests/derived-debugwithdb-is-generic.rs:30:69
   |
30 | ..., S: DebugWithDb<DB>>() {}
   |         ^^^^^^^^^^^^^^^ required by this bound in `ensure_generic`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
33 | fn test_all<'db, DB: ?Sized + AsSalsaDatabase + DbWithJar<Jar>>() where MyInternedStruct<'db>: DebugWithDb<DB> {
   |                                                                   ++++++++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `MyDerivedStruct<'db>: DebugWithDb<DB>` is not satisfied
  --> tests/derived-debugwithdb-is-generic.rs:37:26
   |
37 |     ensure_generic::<DB, MyDerivedStruct<'db>>();
   |                          ^^^^^^^^^^^^^^^^^^^^ the trait `DebugWithDb<DB>` is not implemented for `MyDerivedStruct<'db>`
   |
note: required by a bound in `ensure_generic`
  --> tests/derived-debugwithdb-is-generic.rs:30:69
   |
30 | ..., S: DebugWithDb<DB>>() {}
   |         ^^^^^^^^^^^^^^^ required by this bound in `ensure_generic`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
33 | fn test_all<'db, DB: ?Sized + AsSalsaDatabase + DbWithJar<Jar>>() where MyDerivedStruct<'db>: DebugWithDb<DB> {
   |                                                                   +++++++++++++++++++++++++++++++++++++++++++

For more information about this error, try `rustc --explain E0277`.
error: could not compile `salsa` (test "derived-debugwithdb-is-generic") due to 4 previous errors
warning: build failed, waiting for other jobs to finish...
nikomatsakis commented 1 month ago

No longer relevant