-
Notifications
You must be signed in to change notification settings - Fork 183
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 trait upcasting #821
Conversation
I guess this is for r-a? But jack is working on switching it to use the new solver, and has made a lot of progress already. |
@lqd yes, this is for r-a. I started looking into it since it blocks stabilization of trait upcasting (rust-lang/rust#134367 (comment)). |
Ah I see, it seems worthwhile to land to unblock stabilization then. |
@WaffleLapkin: Can you please unset the style edition? This is both churn, and more importantly this crate needs to build on stable. The edition is not stable, so unless you want to wait until edition 2024 is stable in February to land this, then it should be reverted. |
The rest of the code uses A/B, so use them here too.
Co-authored-by: Ryo Yoshida <[email protected]>
Co-authored-by: Ryo Yoshida <[email protected]>
98cd031
to
92b4a48
Compare
@compiler-errors forgot for a moment that 2024 edition is not stable yet, oops /_= I undid formatting changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I have merge permissions on chalk—let alone if i'm capable of understanding it—but lgtm!
Let's maybe wait for a review from someone who does understand it then 😆 I'll give this a look when I'm back from the bakery |
heh, fair. (not that my approvals matters much anyways, i can't merge it!) |
I'll take another look at this over the weekend. I've read through #796 a while ago and nothing seemed surprising at the time (just didn't put enough thought into it to feel completely confident). One note is that I think auto releases may currently be broken, so we'll have to follow up with a manual release, I think, to fix things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for the most part, but I think we need one more test case.
} | ||
} | ||
|
||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like what's missing here is a test with the super trait being under a binder (e.g. trait Principal where for<'a> Self: Super<'a>
. I assume we can do this without modifying any of the test parsing code - if we can't, then I think it's okay to land this with just a FIXME added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does chalk represent dyn for<'a> Trait<'a>
? I assume that forall<'a> dyn Trait + 'a
is a distinct thing from dyn for<'a> Trait + 'a
. If it can represent that then it would probably also be good to test dyn for<'a> Trait<'a>
can be upcasted to dyn for<'a> Super<'a>
given some trait Trait<'a>: Super<'a>
in addition to the case of trait: for<'a> Super<'a>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be able to (there is certainly ir for it, but I'm not sure exactly how to specify it in the parser). If you can't figure it out, I'll look at it this evening.
That's a good test to add for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added: 5689335
@jackh726 are you going to release a new version? 👀 |
This is based on #796, but with a few more changes.
cc @Veykril @lowr @jackh726