add Unsize trait implementation#427
Conversation
jackh726
left a comment
jackh726
left a comment
There was a problem hiding this comment.
I've looked through just a piece of this. Some comments/questions. Not sure how much you can answer @Areredify, they're more for @nikomatsakis.
| } | ||
| .cast(interner); | ||
|
|
||
| // FIXME(areredify) we need a `source_ty: 'lifetime` goal here |
There was a problem hiding this comment.
Let's file these FIXMEs in a followup issue so they don't potentially get lost. They're pretty important.
|
The other thing to maybe think about is how this might change based on trait upcasting (see rust-lang/rust#60900) |
|
Or (this one is unclear on whether it will be implemented), how this could interact with trait objects of multiple traits: rust-lang/rfcs#2035 |
|
Also, while going through these rules, I made this playground to help me thing about these rules. Might be helpful for others: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=cb638a1efd849d7dc64a8617eec5c1ab |
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
OK, I did a first pass,and my first thought is that it would be nice to break this PR up into three PRs.
- add
dyn Trait + 'xlifetime parameter - add
ObjectSafegoal + integration - add the
Unsizetrait
it would make it much easier to review.
That said, I read everything apart from the details of unsize.rs and it seems good.
|
So it safe to conclude that this PR is in "needs rebase" territory to reduce some duplication? |
|
(I'm still in favor of branching out the lifetime changes as well) |
|
Yes, this needs a rebase. And the tests I mentioned. I'm also in favor of splitting the lifetime changes. |
|
@nikomatsakis @jackh726 yes, I'll rebase it and split it and all tomorrow (ideally), sorry for the delay |
|
@Areredify no hurry =) let's land #393 first, it seems almost ready (just needs a few tweaks to the tests?) |
|
@nikomatsakis that's right, a slight tweak to the tests and a rebase |
|
@Areredify Any updates here? Would like to get this in, since it's pretty close. |
|
Sorry, I prioritized sem-syn lowering, but I plan to get this in before the next design meeting |
basil-cow
force-pushed
the
unsize
branch
4 times, most recently
from
4588fe6 to
1d16b9e
Compare
|
@nikomatsakis @jackh726 Ready for review, I think. I didn't add trait order tests because I feel like supporting it is out of scope of this pr. |
jackh726
left a comment
jackh726
left a comment
There was a problem hiding this comment.
I think the trait order is important. If it doesn't work correctly, then I would definitely like to see tests with FIXMEs. I'll do a more extensive review later
basil-cow
force-pushed
the
unsize
branch
2 times, most recently
from
793348f to
171998f
Compare
|
@jackh726 added |
jackh726
left a comment
jackh726
left a comment
There was a problem hiding this comment.
A couple small things, but this looks good to me otherwise. I would like to see some comments on the individual test goals about what they're for (similar to in the Fn PR). I think getting in this habit will be good :) There are a lot of tests here and it can be hard to keep track of what they are for/what they cover.
@Areredify once you've made these changes, feel free to merge unless @nikomatsakis has any comments
|
Oh, and please file followup issue(s) for the FIXMEs on merge :) |
|
done with the changes 🙂 |
jackh726
left a comment
jackh726
left a comment
There was a problem hiding this comment.
Test comment. But other than that, good to merge.
basil-cow
changed the title
DynTy and Unsize trait implementationUnsize trait implementation
| WhereClause, | ||
| }; | ||
|
|
||
| struct UnsizeParameterCollector<'a, I: Interner> { |
There was a problem hiding this comment.
Ooof but this file needs some comments. =) But I don't think it has to block landing necessarily. But we should file an issue or something to follow-up and explain what is going on.
4 participants
No description provided.