-
Couldn't load subscription status.
- Fork 13.9k
Simplify higher-ranked LUB/GLB #45853
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
Simplify higher-ranked LUB/GLB #45853
Conversation
0948c82 to
77ca1b7
Compare
c924176 to
e782f33
Compare
Coherence is based only on type equality, while this PR only affects subtyping - I don't believe the LUB code can be run from coherence. |
|
if that wasn't clear, r=me unless there's some other complication about coherence. |
|
@bors r+ |
|
📌 Commit e782f33 has been approved by |
|
⌛ Testing commit e782f33137258d27566d2217f6e068c2ff24d3c0 with merge 827619703a023bb5e57d6450979a91040aca1f61... |
|
💔 Test failed - status-appveyor |
|
|
|
I think we should wait for his PR to land and then rebase this one on top of it |
e782f33 to
0bbbfaf
Compare
|
@bors r=arielb1 |
|
📌 Commit 0bbbfaf has been approved by |
|
⌛ Testing commit 0bbbfaf with merge 55324a9a5b7b2a7eaa7bae4c8ffb4c1967f16de1... |
|
💔 Test failed - status-travis |
|
Edit: |
|
@kennytm thanks, I'll take a look more closely then. Thought we had squelched that. |
|
@nikomatsakis Looks like Travis passed this time. |
7985ac6 to
bb0e756
Compare
|
Huh. Interesting. |
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.
duplicate call to check_and_note_conflicting_crates?
|
@arielb1 argh |
bd9167e to
5e9469c
Compare
|
Fixed, added more comments. |
|
@bors r=arielb1 |
|
📌 Commit 5e9469c has been approved by |
|
@bors r- |
|
I think it's a good idea to have #45825 land first |
|
Agreed. Waiting for #45825 to land. |
|
☔ The latest upstream changes (presumably #45825) made this pull request unmergeable. Please resolve the merge conflicts. |
Good riddance.
The ordering can affect error msg, and this map is not a high performance pathway.
5e9469c to
9877fa0
Compare
|
@bors r=arielb1 |
|
📌 Commit 9877fa0 has been approved by |
Simplify higher-ranked LUB/GLB This is a better version of #44211. It still makes higher-ranked LUB/GLB into a hard equality test, however, it does try to identify that something changed and issue a notice to the user. I wroteup #45852 as a tracking issue for this change. Currently, this moves straight to a hard-error, on the basis that the crater run in #44211 saw no impact. It might be good to retest -- or perhaps to try for a warning period. Trying to do the latter in a precise way would be somewhat painful, but an imprecise way might suffice -- that is, we could issue warning *whenever* a LUB/GLB operation succeeds that will later fail, even if it doesn't ultimately impact the type check. I could experiment with this. ~~I am *mildly* wary about landing this independently of other code that moves to a universe-based system. In particular, I was nervous that this change would make coherence accepts new pairs of impls that will later be errors. I have the code for the universe-based approach available, I hope to open an PR and run some tests on its impact very shortly.~~ @arielb1 points out that I was being silly. r? @arielb1
|
💔 Test failed - status-appveyor |
|
@bors retry
Time breakdown
Everything, even LLVM build went slower than usual. I think it's just AppVeyor's problem. |
Simplify higher-ranked LUB/GLB This is a better version of #44211. It still makes higher-ranked LUB/GLB into a hard equality test, however, it does try to identify that something changed and issue a notice to the user. I wroteup #45852 as a tracking issue for this change. Currently, this moves straight to a hard-error, on the basis that the crater run in #44211 saw no impact. It might be good to retest -- or perhaps to try for a warning period. Trying to do the latter in a precise way would be somewhat painful, but an imprecise way might suffice -- that is, we could issue warning *whenever* a LUB/GLB operation succeeds that will later fail, even if it doesn't ultimately impact the type check. I could experiment with this. ~~I am *mildly* wary about landing this independently of other code that moves to a universe-based system. In particular, I was nervous that this change would make coherence accepts new pairs of impls that will later be errors. I have the code for the universe-based approach available, I hope to open an PR and run some tests on its impact very shortly.~~ @arielb1 points out that I was being silly. r? @arielb1
|
☀️ Test successful - status-appveyor, status-travis |
This is a better version of #44211. It still makes higher-ranked LUB/GLB into a hard equality test, however, it does try to identify that something changed and issue a notice to the user. I wroteup #45852 as a tracking issue for this change.
Currently, this moves straight to a hard-error, on the basis that the crater run in #44211 saw no impact. It might be good to retest -- or perhaps to try for a warning period. Trying to do the latter in a precise way would be somewhat painful, but an imprecise way might suffice -- that is, we could issue warning whenever a LUB/GLB operation succeeds that will later fail, even if it doesn't ultimately impact the type check. I could experiment with this.
I am mildly wary about landing this independently of other code that moves to a universe-based system. In particular, I was nervous that this change would make coherence accepts new pairs of impls that will later be errors. I have the code for the universe-based approach available, I hope to open an PR and run some tests on its impact very shortly.@arielb1 points out that I was being silly.r? @arielb1