Skip to content

feat: Drop Debug requirements and flip implementation defaults#756

Merged
Veykril merged 1 commit intosalsa-rs:masterfrom
Veykril:veykril/push-lwlrowwqoxml
Mar 15, 2025
Merged

feat: Drop Debug requirements and flip implementation defaults#756
Veykril merged 1 commit intosalsa-rs:masterfrom
Veykril:veykril/push-lwlrowwqoxml

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented Mar 15, 2025

The default caused a pretty severe hang for us in rust-analyzer due to a debug implementation formatting the entire dependency tree for each dependency recursively by accident so this proposes flipping the default.

This also drops a bunch of debug bounds internal which are unnecessary (without that you are required to somehow implement Debug for most salsa structs).

@netlify
Copy link

netlify bot commented Mar 15, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit a244890
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67d55f362107e50008f47eb9

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 15, 2025

CodSpeed Performance Report

Merging #756 will not alter performance

Comparing Veykril:veykril/push-lwlrowwqoxml (a244890) with master (1dc2438)

Summary

✅ 12 untouched benchmarks

@Veykril Veykril force-pushed the veykril/push-lwlrowwqoxml branch 2 times, most recently from c7c6d32 to dc2cefb Compare March 15, 2025 08:14
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me.

Would it be possible to inspect the structs derive attribute and only generate the Debug impl if someone derived Debug (and then remove it). Or should we add our own DebugSalsa derive macro?

@Veykril
Copy link
Member Author

Veykril commented Mar 15, 2025

Would it be possible to inspect the structs derive attribute and only generate the Debug impl if someone derived Debug (and then remove it). Or should we add our own DebugSalsa derive macro?

I believe that should be doable but:

  1. We can only do this syntactically, so it would fail if the user was to do something odd like qualifying the Debug name etc.
  2. More importantly, it would make it impossible to derive debug to get a debug impl that merely prints the ID.

I think a custom salsa debug derive might be the better option for this. It definitely sounds like the more proper way to do this (though I'd do that as a follow up).

@Veykril Veykril force-pushed the veykril/push-lwlrowwqoxml branch from dc2cefb to c18063c Compare March 15, 2025 10:05
@Veykril Veykril force-pushed the veykril/push-lwlrowwqoxml branch from c18063c to a244890 Compare March 15, 2025 11:06
@Veykril Veykril added this pull request to the merge queue Mar 15, 2025
Merged via the queue into salsa-rs:master with commit 1bbf4c2 Mar 15, 2025
11 checks passed
@Veykril Veykril deleted the veykril/push-lwlrowwqoxml branch March 15, 2025 15:24
@github-actions github-actions bot mentioned this pull request Mar 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants