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

Manual `Debug` impl for `salsa::Id` #508

Closed MichaReiser closed 1 month ago

MichaReiser commented 2 months ago

I've often been confused by the fact that the IDs printed by DebugWithDb don't match with the ids printed when dumping a salsa::Id with Debug.

This PR replaces the derived Debug implementation of salsa::Id with a manual Debug implementation that ensures that the IDs printed by Debug and DebugWithdb are consistent.

Open Questions

netlify[bot] commented 2 months ago

Deploy Preview for salsa-rs canceled.

Name Link
Latest commit 6975a47690a20ee188c05ea2ddd772bbd985378b
Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/669e8126dc6aa80008ff9f0f
nikomatsakis commented 2 months ago
* Should `Id` use `f.debug_tuple` to preserve the name in the `Debug` output?

I think yes :)

* Should `DebugWithDb` continue to use `as_u32` directly or should it just call the `Id`'s `Debug` implementation (It would change the output if we decide that `Debug` should output `Id(num)`.

I like that it uses the debug impl, keep that I think.

MichaReiser commented 2 months ago

I updated the Debug implementation to write Id({}).

MichaReiser commented 1 month ago

@nikomatsakis I addressed the feedback. Would you mind taking another look?

MichaReiser commented 1 month ago

Ha, I probably need to start over. @nikomatsakis I don't mind rebasing (re-doing) if this is something we want to merge.

nikomatsakis commented 1 month ago

I'm in favor of this but needs a rebase

MichaReiser commented 1 month ago

I'm in favor of this but needs a rebase

Done