Ignore auto traits order #531
Conversation
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
There was a problem hiding this comment.
So, this is purely a chalk-integration fix? I'm not sure it I would prefer a fix that guarantees the order from chalk-solve. Did you take a look at how rustc does it? Does it sort traits during lowering or during trait solving? The difference is crucial.
Either way, I don't think it really makes sense to differentiate between auto and regular traits. Just sort them all. (We do have the condition some that there can only be one regular trait right? That's also crucial.)
tests/test/builtin_impls/unsize.rs
Outdated
| // Different order of traits in target and source | ||
| // FIXME: this doesn't work because trait object unification | ||
| // respects where clause order, which it shouldn't | ||
| // Different order of traits in target and sourcex |
There was a problem hiding this comment.
Sorry, I didn't mention it, but I think there is maybe a similar comment elsewhere in the file, the other test you edited...?
There was a problem hiding this comment.
it just says, "See above", which I think is still relevant, no ?
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
zaharidichev
force-pushed
the
zd/ignore-auto-traits-order
branch
from
3e229c1 to
ea8f2fa
Compare
There was a problem hiding this comment.
If rustc does this during ast lowering, then this is fine. Remove the printlns and this LGTM. @zaharidichev feel free to merge when those changes are made
|
I think we should maybe add some tests for trait object unification? |
|
@Areredify what do you mean? Did we miss tests for |
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
|
@Areredify is this in tests/unify.rs ? |
|
@jackh726 We didn't, but I would say it's a roundabout way to test unification |
|
Ah, I see. Hmm, I think this is fine. It might be worth adding specific tests for unification of all types. But let's do that separately. (Maybe file an issue?) |
|
@Areredify I can take the unification tests issue if you provide a pinch of guidance. |
|
Let's do that in a separate PR :) |
This PR aims to fix #510 by sorting the auto traits defs durgin lowering