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 <low.ryoshida@gmail.com>
Co-authored-by: Ryo Yoshida <low.ryoshida@gmail.com>
WaffleLapkin
force-pushed
the
trait_upcast
branch
from
98cd031 to
92b4a48
Compare
|
@compiler-errors forgot for a moment that 2024 edition is not stable yet, oops /_= I undid formatting changes. |
davidbarsky
left a comment
davidbarsky
left a comment
There was a problem hiding this comment.
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. |
jackh726
left a comment
jackh726
left a comment
There was a problem hiding this comment.
Looks good for the most part, but I think we need one more test case.
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
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.
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.
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.
@jackh726 are you going to release a new version? 👀 |
6 participants
This is based on #796, but with a few more changes.
cc @Veykril @lowr @jackh726