Skip to content
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

Implement Debug trait for most public structs #468

Merged
merged 4 commits into from
Nov 18, 2020

Conversation

NeoLegends
Copy link
Contributor

@NeoLegends NeoLegends commented Sep 13, 2020

This adds a debug implementation for every struct within rusty_v8. This will make consuming it from other libraries a lot easier. The manual debug implemetations should be just like the autogenerated ones except they don't Debug the field that's not Debug. If you prefer we can also derive those impls (and ignore the offenders) using https://github.com/mcarton/rust-derivative but I didn't want to add a dependency here.

I would suggest to require every new struct or enum in the future also to be annotated with Debug.

@bnoordhuis
Copy link
Contributor

I believe the decision to omit Debug is intentional. I remember a discussion but I can't find it anymore.

The rationale as I remember it:

  1. All those auto-generated Debug impls bloat the binary, and

  2. Most rusty_v8 types don't have a meaningful debug representation because deep down they're all indirect pointers. Two Locals that refer to the same value don't necessarily point to the same location.

(My treacherous memory may have backfilled 2. but I'm sure of 1.)

@NeoLegends
Copy link
Contributor Author

That would be quite unfortunate.

  • Unused debug impls will be stripped by the optimizer, so I don't necessarily agree on 1) (it does increase compile times a bit, though). It does however, pose a pain point for consumers of the library because they'll be forced to manually roll their Debug implementations for all their types that contain a v8 type. std has a Debug implementation for every type of theirs for the same reason, as far as I know.
  • Point 2) seems more like a semantic issue rather than an issue with the Debug impls themselves. With a bit of work the respective implementations could very well provide meaningful representations. And on the other hand, if it's impossible to provide meaningful data I personally think having an "empty" Debug implementation that does not actually provide any details is still a usability win just because #[derive(Debug)] works in downstream crates. "Empty" implementations also help with the binary bloat.

@solson
Copy link

solson commented Sep 15, 2020

std has a Debug implementation for every type of theirs for the same reason, as far as I know.

Yep, a few years ago they made a concerted effort to add Debug impls everywhere, see rust-lang/rust#31869. You can also find an argument for it in the official API guidelines.

@bartlomieju
Copy link
Member

  • It does however, pose a pain point for consumers of the library because they'll be forced to manually roll their Debug implementations for all their types that contain a v8 type.

I strongly agree on this point. I was bitten several times by it working on deno_core. @piscisaureus @bnoordhuis if Debug impls are stripped for release builds I think it's worth to land this PR.

@ry
Copy link
Member

ry commented Nov 16, 2020

Generally I think we should try to be as minimal as possible, but if lack of Debug implementations is creating real hardship for users, we ought to just add them. I can't imagine this is going to effect build time and/or runtime very much at all.

@piscisaureus
Copy link
Member

+1 from me too

@bnoordhuis
Copy link
Contributor

For the record, I'm okay with this too. I'll review but @NeoLegends, can you do a git rebase on top of master first? Thanks.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @NeoLegends!

@bartlomieju bartlomieju changed the title #[derive(Debug)] all the things Implement Debug trait for most public structs Nov 18, 2020
@bartlomieju bartlomieju merged commit efe0e76 into denoland:master Nov 18, 2020
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.

6 participants