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 Self bounds from methods and ?Sized relaxation (if possible) to impl header #54

Merged
merged 3 commits into from
Jul 21, 2019

Conversation

LukasKalbertodt
Copy link
Member

These are two fairly independent features, so I'm not quite sure why I'm trying to merge both in one PR 🤔 At least there are two commits!

Commit 1 solves #52 as it adds a ?Sized relaxation to the proxy type on the impl header whenever it's possible.

Commit 2 solves #11 in that it searches for all Self bounds on methods and adds them to the proxy parameter. This is unfortunate, but the only sane way to handle that.

@KodrAus If you don't have the time, just tell me, then we'll merge this unreviewed. I would probably publish this as 0.4.0 after merging, if you are fine with that?

It's only impossible when there is `Self` by value in in parameter or
return position in any method. This commit does not yet check for
`Self` in non-receiver parameters as this crate cannot deal with that
yet anyway.

Furthermore, a `where Self: Sized` bound on methods does not help in
that we still cannot add the `?Sized` relaxation. This is related to
issue #11.
@KodrAus
Copy link
Member

KodrAus commented Jul 17, 2019

This looks good to me! It might be worth adding a compile test that has an explicit Self: Sized bound on the trait itself, even though the trait would otherwise be object-safe.

Currently, the `impl` block we generate for those traits is a bit
stupid as it contains `T: Trait + ?Sized` (where `Trait: Sized`), but
it compiles fine. So I don't see a reason to make our code more
complicated just to avoid this strangeness.
@LukasKalbertodt
Copy link
Member Author

@KodrAus Good point! I forgot about that. Luckily the current implementation seems to work fine with these traits.

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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