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

Use PhantomData<fn() -> (T0, T1, ...)> everywhere #2115

Closed
addisoncrump opened this issue Apr 26, 2024 · 4 comments
Closed

Use PhantomData<fn() -> (T0, T1, ...)> everywhere #2115

addisoncrump opened this issue Apr 26, 2024 · 4 comments
Labels
cleanup Reducing our technical debt good first issue Good for newcomers

Comments

@addisoncrump
Copy link
Collaborator

addisoncrump commented Apr 26, 2024

This was a trick I saw here: astral-sh/ruff#10849 (comment)

This has a lot of nice qualities for derive that can help us with #2068:

  • Eq
  • Copy
  • Debug
  • Serialize/Deserialize (with #[serde(skip)])
  • Send/Sync (in case we ever do any async)

If a lifetime is stored in the PhantomData, then use PhantomData<fn() -> &'a &'b ... (T0, T1, ...)>.

@addisoncrump addisoncrump added good first issue Good for newcomers cleanup Reducing our technical debt labels Apr 26, 2024
@domenukk
Copy link
Member

Where exactly will this help us? Think I'm too stupid to understand

@addisoncrump
Copy link
Collaborator Author

No, this one's pretty arcane. I had no idea this trick existed until yesterday.

Basically, we currently require e.g. T: Debug in a lot of places because we hold PhantomData<T>, but this is actually an excessive restriction because T is never debug'd. Similarly with T: Copy, T: Eq, T: Serialize + Deserialize: we probably have lots of places where we can simplify the generic constraints because we never actually handle the type except in passing (e.g. in methods).

@fiveseven-lambda
Copy link

When deriving the Debug trait, the type parameter T is required to implement Debug as well. However, this requirement is imposed by the derive macro itself, not by the presence of PhantomData<T>. Therefore, changing PhantomData<T> to PhantomData<fn() -> T> will not alter this requirement. The derive macro will still enforce T: Debug regardless of which version of PhantomData is used.

@addisoncrump
Copy link
Collaborator Author

So it would seem. This is very annoying, I thought I checked that... 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Reducing our technical debt good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants