Solution to trait alias bug #65673 #66813
Conversation
rust-highfive
added
the
S-waiting-on-review
alexreg
added
A-trait-system
This comment has been minimized.
This comment has been minimized.
|
@estebank Any idea what's going on with the diagnostics changes above? |
|
@eddyb I was hoping for a review of the actual functionality changes here or a suggestion as to why the diagnostics changed, but if you feel you’re not qualified to review, I can r? Niko or someone else. Just let me know, ta. |
|
I cannot easily see where the functional changes are when 90% of the non-test changes aren't that. Anyone else who doesn't point that out still wasted time searching for the functional changes. |
|
Please remove all nonfunctional changes. It requires a lot of time for reviewers to find the functional changes in your PR. Please do not write suggestive comments about a reviewer's ability to review your PRs. Not only did you choose them yourself, so you thought them qualified, but the desire not to review was rooted in the fact that it just requires a lot of time that both you and the reviewer could have better spent. r? @oli-obk |
|
As the PR author, I'd like Niko to review this (in his own time of course, no rush). r? @nikomatsakis |
alexreg
force-pushed
the
fix-trait-aliases-in-objects
branch
2 times, most recently
from
8c0fcca to
2f8a73f
Compare
This comment has been minimized.
This comment has been minimized.
|
Moderator note: please do not abuse the moderation features on github. If anyone isn't clear on what constitutes abuse, then please reach out to rust-mods@rust-lang.org. |
|
Moderator note: this is not the place to discuss moderation. As I said in my previous comment, please email the mods if you would like to discuss this further. Off topic comments in this thread will continue to be deleted. |
|
Looking into the diagnostics issue now, but pending that, I think this is the right solution. (In case no one has realised yet, non-functional changes that were accidentally left in following reverts to several files were removed shortly after the above discussion.) |
alexreg
force-pushed
the
fix-trait-aliases-in-objects
branch
from
2f8a73f to
34080fb
Compare
|
So, the effects on diagnostics for a few compile-fail tests aren't actually that bad, it seems (having discussed them briefly with @Centril). In fact, in one way (the inclusion of an extra diagnostic when you try to use two non-auto-traits), it seems to be better. Three ui tests are affected. So, @estebank, if you feel otherwise, do please let me know and we can try to improve them either here or in another PR. Otherwise, I'll leave this PR to Niko to review in his own time. |
There was a problem hiding this comment.
I think I understand what this PR is going for, and it mostly makes sense, but I don't quite think I grasp how we should be doing object safety checks on trait aliases. I left some comments, maybe @alexreg you have thoughts.
I'm largely sidestepping the question about cosmetic changes. I'd rather discuss it offline. However, I do think that the general team stance (per our prior discussions) that is that unnecessary cosmetic changes should be minimized. I did leave a few comments to verify that some sets of changes were indeed non-functional as they appeared to be; I can attest it took me some time comparing the code to check that this was the case.
| @@ -358,19 +358,21 @@ impl<'tcx> TraitAliasExpander<'tcx> { | |||
| debug!("expand_trait_aliases: trait_ref={:?}", trait_ref); | |||
|
|
|||
| // Don't recurse if this bound is not a trait alias. | |||
| let is_alias = tcx.is_trait_alias(trait_ref.def_id()); | |||
| if !is_alias { | |||
| if !tcx.is_trait_alias(trait_ref.def_id()) { | |||
There was a problem hiding this comment.
To be clear, I believe that there are no functional changes to this file? (Just checking)
There was a problem hiding this comment.
Yeah, I can remove this change if you like. I think I just forgot. Makes sense to inline this, but it's not important.
| @@ -1398,50 +1401,53 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { | |||
| err.emit(); | |||
| } | |||
|
|
|||
| let (mut auto_traits, regular_traits): (Vec<_>, Vec<_>) = | |||
There was a problem hiding this comment.
Hmm, I don't love the way these variables shadow the variables above. In general, this function seems to be awfully big, and these shadows are quite far apart. I had to expand the diff fully just to be sure that shadowing was even happening. I think we should extract a helper function for the logic above, perhaps, and then a separate helper for the logic below.
There was a problem hiding this comment.
Yep, this fn was already very big, and that’s just gotten worse now. I’ll see what I can do.
There was a problem hiding this comment.
Ah, complain_about_missing_associated_types has already been factored out now, which makes this fn size much more reasonable!
| traits::expand_trait_aliases(tcx, bounds.trait_bounds.iter().cloned()); | ||
| let (mut auto_traits, regular_traits): (Vec<_>, Vec<_>) = | ||
| traits::expand_trait_aliases(tcx, bounds.trait_bounds.iter().cloned()) | ||
| // Ensure that trait ref is to self type and not some type param. |
There was a problem hiding this comment.
This seems pretty subtle and not obviously correct. One thing I noticed is that the original ICE was based on something like trait Foo<Ix, OnSet> = where OnSet: Callback0, but I don't see any test that covers that particular scenario? It seems like the effect of this change is to going to be to wind up with an empty list here, which I guess means that you will get an error?
However, something like this would work ok:
trait Foo = where Self: Display;which I think would be equivalent to trait Foo = Display, right?
There was a problem hiding this comment.
I'm a bit worried here about an example like this:
#![feature(trait_alias)]
trait Foo<T> = where T: PartialEq<Self>;
fn main() {
let _: &dyn Foo<()> = &();
}When I test this, I currently get some wacky ICE =) But it seems like we would screen out the T: PartialEq<Self> from the list of regular_traits here. Then, later, when we check for object safety, we will never see that T: PartialEq<Self> is not object-safe.
My intuition is that trait Foo<T> = ... should work exactly as if you created a trait and implemented it, for the most part. And if we do that with this example we get object safety errors. I don't quite see where they are supposed to crop up here.
There was a problem hiding this comment.
The key thing is to avoid trying to create existential predicates for predicates where the self ty is not the dummy self ty, which is of course impossible. This filter might be blocking out too much (projections on Self too?), but all the tests seem to be working anyway...
The relevant test is issue-65673.rs, I believe (pre-existing, slightly modified).
There was a problem hiding this comment.
trait Foo = where Self: Display; should (and I think is) equivalent to trait Foo = Display; in most circumstances, except as a trait object. Since technically no principal trait is specified, the compiler will complain. Perhaps we should do that, but perhaps we should instead check that there's at least one predicate instead of trait?
There was a problem hiding this comment.
As for object safety, I guess we need to modify that code explicitly? Given such predicates on T definitely cannot be included in the trait object itself (they're simply not existential).
There was a problem hiding this comment.
I don't quite understand your response here, @alexreg.
What is the behavior on the example I gave, for example? I don't think that issue-65673.rs is equivalent. For one thing, it's run-pass, but I expect an error, but also the trait alias in issue-65673.rs is:
trait Alias<T> = Any where T: Trait; whereas the trait alias in my example was trait Foo<T> = where T: PartialEq<Self>;, which (notably) references Self (unlike issue-65673.rs).
Also, regarding the difference between trait Foo = Display and trait Foo where Self: Display, I guess I'd have to consult the RFC. It's a touch surprising to me since trait Foo: Display and trait Foo where Self: Display are equivalent, for better or worse.
There was a problem hiding this comment.
@nikomatsakis Okay, I've gotten your expected behaviour implemented. (And I managed to remove the Any from issue-65673.rs.) Now, the question is, do we want to allow just dyn or dyn 'static as a type?
There was a problem hiding this comment.
I don't know! I think I'm inclined to preserve the status quo, whatever it is, though if/when we generalize to dyn A + B I'd also be inclined to accept zero bounds.
There was a problem hiding this comment.
@nikomatsakis Okay, sounds fair. I'll push shortly and this PR will be ready for review.
| .map(|trait_ref| ty::ExistentialPredicate::Trait(*trait_ref.skip_binder())); | ||
| let auto_trait_predicates = auto_traits | ||
| .into_iter() | ||
| .map(|trait_ref| ty::ExistentialPredicate::AutoTrait(trait_ref.def_id())); |
There was a problem hiding this comment.
To confirm, also no functional changes here, correct? ( I didn't spot any )
There was a problem hiding this comment.
Correct, just made it more readable since I was editing this fn anyway.
|
ping from triage, @nikomatsakis any further review after the author's comment? |
|
☔ The latest upstream changes (presumably #67449) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: @alexreg can you please address the merge conflicts? |
|
@JohnCSimon Happy to rebase and fix conflicts, but it would be nice to get the node from Niko that this approach is the one we want to go with (per his last comments) before doing so. Ta. |
|
Triaged |
joelpalmer
added
S-inactive
|
Closing due to inactivity. Please reopen when changes are made. |
Supercedes PR #66392.
I believe the correct thing to do is to expand the trait aliases to check everything is in order (single non-auto trait, etc.) but then use the unexpanded list of trait to construct the existential predicates for the trait object.
r? @eddyb
CC @estebank @nikomatsakis @Centril