Skip to content

[WIP] make LUB of higher-ranked things actually do EQ - #44211

Closed
nikomatsakis wants to merge 2 commits into
rust-lang:masterfrom
nikomatsakis:chalk-simplify-hr-lub-glb
Closed

[WIP] make LUB of higher-ranked things actually do EQ#44211
nikomatsakis wants to merge 2 commits into
rust-lang:masterfrom
nikomatsakis:chalk-simplify-hr-lub-glb

Conversation

@nikomatsakis

Copy link
Copy Markdown
Contributor

This is an experimental PR that I'm posting for the purposes of observing its effects via cargobomb (a crater run showed no regressions). The existing LUB/GLB code does some rather complex things around higher-ranked types (notably, grubbing about with region constraints) that I would like to move away from. I am not sure how often it is important in practice. This branch converts LUB/GLB of higher-ranked things into EQ.

r? @eddyb (this is just an experiment, don't really review =)

@nikomatsakis

Copy link
Copy Markdown
ContributorAuthor

@bors try

@bors

bors commented Aug 31, 2017

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 7226cda with merge ff1db21...

bors added a commit that referenced this pull request Aug 31, 2017
[DO NOT MERGE] make LUB of higher-ranked things actually do EQ
This is an experimental PR that I'm posting for the purposes of observing its effects via cargobomb (a crater run showed [no regressions](https://gist.github.com/nikomatsakis/5aaf3782b4332a3ba408315cc84f69bf)). The existing LUB/GLB code does some rather complex things around higher-ranked types (notably, grubbing about with region constraints) that I would like to move away from. I am not sure how often it is important in practice. This branch converts LUB/GLB of higher-ranked things into EQ.
r? @eddyb (this is just an experiment, don't really review =)
@bors

bors commented Aug 31, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-travis
State: approved= try=True

@eddyb

Copy link
Copy Markdown
Contributor

cc @rust-lang/infra Cargobomb run requested.

@aidanhsaidanhs added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Aug 31, 2017
@Mark-SimulacrumMark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2017
@Mark-Simulacrum

Copy link
Copy Markdown
Member

Marking as waiting on author since Travis failed. If these are intentional failures, please let us (infra team) know!

[01:07:43] running 23 tests
[01:07:43] ....FFFFFF.F.FFF.......
[01:07:43] failures:
[01:07:43]
[01:07:43] ---- test::glb_bound_free_infer stdout ----
[01:07:43] thread 'test::glb_bound_free_infer' panicked at 'unexpected error computing LUB: RegionsInsufficientlyPolymorphic(BrAnon(1), '_#1r)', /checkout/src/librustc_driver/test.rs:418:22
[01:07:43] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:07:43]
[01:07:43] ---- test::glb_bound_free stdout ----
[01:07:43] thread 'test::glb_bound_free' panicked at 'unexpected error computing LUB: RegionsInsufficientlyPolymorphic(BrAnon(1), ReFree(DefId { krate: CrateNum(0), node: DefIndex(0) => test_crate/8cd878b }, BrAnon(1)))', /checkout/src/l
ibrustc_driver/test.rs:418:22
[01:07:43]
[01:07:43] ---- test::glb_bound_static stdout ----
[01:07:43] thread 'test::glb_bound_static' panicked at 'unexpected error computing LUB: RegionsInsufficientlyPolymorphic(BrAnon(1), ReStatic)', /checkout/src/librustc_driver/test.rs:418:22
[01:07:43]
[01:07:43] ---- test::glb_free_free_with_common_scope stdout ----
[01:07:43] thread 'test::glb_free_free_with_common_scope' panicked at 'Unexpected error: mismatched types Expected: []', /checkout/src/librustc_driver/test.rs:78:12
[01:07:43]
[01:07:43] ---- test::lub_bound_bound_inverse_order stdout ----
[01:07:43] thread 'test::lub_bound_bound_inverse_order' panicked at 'unexpected error in LUB: expected bound lifetime parameterBrAnon(2), found concrete lifetime', /checkout/src/librustc_driver/test.rs:410:26
[01:07:43]
[01:07:43] ---- test::lub_bound_free stdout ----
[01:07:43] thread 'test::lub_bound_free' panicked at 'unexpected error in LUB: expected bound lifetime parameterBrAnon(1), found concrete lifetime', /checkout/src/librustc_driver/test.rs:410:26
[01:07:43]
[01:07:43] ---- test::lub_bound_static stdout ----
[01:07:43] thread 'test::lub_bound_static' panicked at 'unexpected error in LUB: expected bound lifetime parameterBrAnon(1), found concrete lifetime', /checkout/src/librustc_driver/test.rs:410:26
[01:07:43]
[01:07:43] ---- test::lub_free_bound_infer stdout ----
[01:07:43] thread 'test::lub_free_bound_infer' panicked at 'unexpected error in LUB: expected bound lifetime parameterBrAnon(1), found concrete lifetime', /checkout/src/librustc_driver/test.rs:410:26
[01:07:43]
[01:07:43] ---- test::lub_returning_scope stdout ----
[01:07:43] thread 'test::lub_returning_scope' panicked at 'Unexpected error: mismatched types Expected: []', /checkout/src/librustc_driver/test.rs:78:12
[01:07:43]
[01:07:43] ---- test::lub_free_free stdout ----
[01:07:43] thread 'test::lub_free_free' panicked at 'Unexpected error: mismatched types Expected: []', /checkout/src/librustc_driver/test.rs:78:12
[01:07:43]
[01:07:43]
[01:07:43] failures:
[01:07:43] test::glb_bound_free
[01:07:43] test::glb_bound_free_infer
[01:07:43] test::glb_bound_static
[01:07:43] test::glb_free_free_with_common_scope
[01:07:43] test::lub_bound_bound_inverse_order
[01:07:43] test::lub_bound_free
[01:07:43] test::lub_bound_static
[01:07:43] test::lub_free_bound_infer
[01:07:43] test::lub_free_free
[01:07:43] test::lub_returning_scope

@eddyb

eddyb commented Sep 1, 2017

Copy link
Copy Markdown
Contributor

@Mark-Simulacrum This is not supposed to be merged and those test failures probably make sense given the changes - we only need a cargobomb run result from it.

@Mark-SimulacrumMark-Simulacrum added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 2, 2017
@aidanhs

Copy link
Copy Markdown
Contributor

Cargobomb run started. Sorry for the delay, we didn't have spare boxes and then I was away for a few days.

@Mark-Simulacrum

Copy link
Copy Markdown
Member

Run is complete: http://cargobomb-reports.s3.amazonaws.com/pr-44211/index.html. 15 regressions straight off, but haven't triaged.

@eddyb

Copy link
Copy Markdown
Contributor

@nikomatsakis Looks like no actual regressions to me, only some flaky tests and timeouts.

@Mark-SimulacrumMark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 12, 2017
@arielb1

arielb1 commented Sep 18, 2017

Copy link
Copy Markdown
Contributor

Given that there were no regressions of crater, I think we can merge this.

However, I would like it if on an LUB failure, we could at least have a special error message noting the new compiler update.

@eddyb

Copy link
Copy Markdown
Contributor

@arielb1 from what @nikomatsakis was telling me, this branch is only an experiment to confirm that a separate reimplementation strategy could work without breaking existing code.

@nikomatsakis

Copy link
Copy Markdown
ContributorAuthor

While @eddyb is correct, now that the experiment was successful, I've been contemplating landing (some variant of) this branch, just to pave the way for the future work. It should be possible to make a variant of this that offers a specialized error message, at least.

@aidanhs

Copy link
Copy Markdown
Contributor

@nikomatsakis just pinging you for triage purposes to see how your contemplation of this PR mentioned in your last comment is going :)

@aidanhsaidanhs mentioned this pull request Sep 28, 2017
@aidanhs

Copy link
Copy Markdown
Contributor

@nikomatsakis ping again to see where this PR is up to! If this was just an experimental PR, maybe worth closing and reopening a fresh one with changes to be reviewed/merged?

@nikomatsakis

Copy link
Copy Markdown
ContributorAuthor

@aidanhs I think we've decided to turn into something a bit more than experimental. I'm still hoping to refresh the approach in the next day or two, so I'm inclined not to close just yet.

@nikomatsakis
nikomatsakisforce-pushed the chalk-simplify-hr-lub-glb branch from 7226cda to 083c433CompareOctober 9, 2017 20:37
@nikomatsakisnikomatsakis changed the title [DO NOT MERGE] make LUB of higher-ranked things actually do EQ[WIP] make LUB of higher-ranked things actually do EQOct 9, 2017
@carols10cents

Copy link
Copy Markdown
Member

I'm still hoping to refresh the approach in the next day or two, so I'm inclined not to close just yet.

7 days ago

Triage ping! How's it going?

@nikomatsakis

Copy link
Copy Markdown
ContributorAuthor

Some progress, got blocked by other things.

@shepmaster

Copy link
Copy Markdown
Member

This has been inactive for 18 days, so I'm going to go ahead and close this to keep things tidy. As always, please reopen once some updates happen! Thanks!

bors added a commit that referenced this pull request Nov 14, 2017
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
bors added a commit that referenced this pull request Nov 17, 2017
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
bors added a commit that referenced this pull request Nov 17, 2017
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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@nikomatsakis@bors@eddyb@Mark-Simulacrum@aidanhs@arielb1@carols10cents@shepmaster