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

Add std types for Rocket (and uuid) #7

Merged
merged 3 commits into from
Jun 30, 2024

Conversation

the10thWiz
Copy link
Collaborator

Adds Static implementations for a number of std error types, and adds an optional dependency on uuid, with appropriate Static impls.

@the10thWiz the10thWiz force-pushed the additional_type_rocket branch from f1afdb7 to 9b03464 Compare June 29, 2024 23:56
src/transient.rs Outdated
}

unsafe impl<'a> Transient for &'a str {
type Static = &'static str;
type Transience = Co<'a>;
}
impl_refs!(&'a str ['a]);
impl Static for str {}
Copy link
Owner

Choose a reason for hiding this comment

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

The one problem here is that the unsafe impl<S: Static> Transient for S { ... } impl will not apply to this since it has an implicit S: Sized bound, and in fact the Transient trait has Sized as a super-type. The reason for this is kinda funky, and I was torn on whether to add it, so I am curious on your take.

Let's pretend that the Transient trait did not have the Sized requirement, and implement it for Vec<T: Transient>:

unsafe impl<T: Transient> Transient for Vec<T> {
    type Static = Vec<T::Static>;
    type Transience = T::Transience;
}

This will not work, since Vec requires its parameter to be Sized, which is implied for T, but not for T::Static. So you would need to add a where clause:

unsafe impl<T: Transient> Transient for Vec<T> 
where
    T::Static: Sized
{
    type Static = Vec<T::Static>;
    type Transience = T::Transience;
}

This wouldn't be too bad on its own, but then consider using it:

fn erase_vec<T: Transient>(x: &Vec<T>) -> &dyn Any<Co> {
    x
}

Again this won't work, since the compiler needs you to tell it again that T::Static: Sized with another where clause.

So to avoid this awkwardness in usage, I just decided to restrict Transient to Sized types so that the Transient::Static associated type also have a Sized bound. But this reminds me that I should probably do the same to the Static trait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although it would be nice to support unsized types, it looks like the std any doesn't support unsized types either.

I'm pretty sure the issue is that you can't upcast an unsized type into a trait object, since it already has metadata. E.g. casting &str to &dyn Any isn't possible, even though str: Any.

Copy link
Owner

@JRRudy1 JRRudy1 Jul 1, 2024

Choose a reason for hiding this comment

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

Oh you're totally right. I noticed that std::any::Any is indeed implemented for T: ?Sized and allows you to get a TypeId, but didn't look far enough to realize that all the useful dyn Any methods are implicitly T: Sized. So I'm happy to learn that we don't have a limitation beyond the stdlib version :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At least for now. Technically, something like std::boxed::ThinBox could eventually make it possible to upcast unsized types, but it's still nightly only for now.

I don't think it's worth even trying to make unsized types work until (and unless) std does.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, I am perfectly happy matching the stdlib's limitation 👍

@JRRudy1
Copy link
Owner

JRRudy1 commented Jun 30, 2024

Awesome, thanks for adding these! I think we should remove the impl for str as explained above unless you have any thoughts on getting around it, but otherwise looks good to merge.

…f `Static` to mirror the same requirement on the `Transient` trait.
@JRRudy1
Copy link
Owner

JRRudy1 commented Jun 30, 2024

I removed the Static impl for str, and added the Static: Sized requirement, as mentioned in my comment above. If we want to investigate removing this restriction we can do that in another Issue or PR.

@JRRudy1 JRRudy1 merged commit d952671 into JRRudy1:main Jun 30, 2024
4 checks passed
@JRRudy1 JRRudy1 mentioned this pull request Jun 30, 2024
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