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

Make Typed, TypePath, GetTypeRegistration, etc etc supertraits of Reflect #12691

Open
SkiFire13 opened this issue Mar 24, 2024 · 4 comments
Open
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible

Comments

@SkiFire13
Copy link
Contributor

What problem does this solve or what need does it fill?

Often just a T: Reflect bound is not sufficient. #5772 has a good example of why this is an issue.

What solution would you like?

Have Reflect require all those traits, as it (almost?) never makes sense to implement Reflect without them. They are also all automatically derived when deriving Reflect (the only exception being FromReflect). #5772 claims this wasn't done because those traits are not object-safe, but this can be solved by just slapping some where Self: Sized on the offending methods.

What alternative(s) have you considered?

  • Keep the current status quo
  • Combine those traits under a shared trait, but different than Reflect
@SkiFire13 SkiFire13 added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 24, 2024
@MrGVSV
Copy link
Member

MrGVSV commented Mar 24, 2024

Reasons not to do this include separating concerns (i.e. GetTypeRegistration for registration, Typed for type info, etc.) and excluding dynamic types from certain impls (i.e. dynamic types do not implement GetTypeRegistration so as to prevent footguns).

The former is nice to have, but it does make working with generic type bounds a little cumbersome. Not to mention, manual implementors often forget to implement a trait that they actually need in order for something to work as expected. Merging these into Reflect would help in these areas.

The bigger problem is the latter. Certain types can't or shouldn't implement certain traits. For example, DynamicStruct can't implement FromReflect and it shouldn't implement GetTypeRegistration. We'd have to be careful to make sure we don't break these expectations if we went ahead with such a change.

It might also be worth waiting for Unique Reflect (#7207) to see if these issues regarding dynamic types can be mitigated by that PR (such as only merging traits into Reflect but not PartialReflect).

@MrGVSV MrGVSV added A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Mar 24, 2024
@SkiFire13
Copy link
Contributor Author

Tried experimenting with this, here's the results:

  • FromReflect: not all (user) types implement it, so it's excluded;
  • GetPath: can easily be required by Reflect;
  • GetTypeRegistration: the dynamic types (e.g. DynamicStruct) don't implement it, so Reflect cannot depend on it;
  • TypePath: there are some !Sized types that implement it (e.g. str, Path and [T]), and other implementations rely on that. It should be noted though that those types don't also implement most other reflect traits;
  • Typed: same as GetTypeRegistration.

Overall I agree that Unique Reflect seems to be addressing most of these issues (except maybe TypePath)

@soqb
Copy link
Contributor

soqb commented Apr 7, 2024

TypePath can still be a supertrait of Reflect. Reflect just cannot be a supertrait of TypePath!

@SkiFire13
Copy link
Contributor Author

TypePath can still be a supertrait of Reflect. Reflect just cannot be a supertrait of TypePath!

Yes that was the idea, but TypePath is currently not object safe, so making it a supertrait of Reflect would also make Reflect not object safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible
Projects
Status: Open
Development

No branches or pull requests

3 participants