Uh oh!
There was an error while loading. Please reload this page.
Issue 42545 type inference regression - #42659
Conversation
eddyb
commented
Jun 14, 2017
@bors r+ |
bors
commented
Jun 14, 2017
📌 Commit ef26ac3 has been approved by |
aidanhs
commented
Jun 15, 2017
@bors r- Lots of travis failures, e.g. |
nikomatsakis
commented
Jun 15, 2017
Bah. I hadn't considered all the duplicate errors. I'll see what I can do about suppressing those. |
nikomatsakis
commented
Jun 15, 2017
@bors r=eddyb |
bors
commented
Jun 15, 2017
📌 Commit 125968b has been approved by |
Won't 125968b have the dropck ICE again on code with type errors? Maybe skip calling regionck from wfcheck if there are type errors in https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/wfcheck.rs#L60 - or have smarter span management so that the spans stay the same for both errors, which would get the error deduplicated. @bors r- |
nikomatsakis
commented
Jun 16, 2017
I don't think so, because that was one of those "delayed bug" things, where it only ICEs if an error is not reported.
Do we detect and remove duplicate errors based on span etc already? I had thought about tinkering more, but decided it was better to fix the ICE in a narrow way. Still, I could imagine looking at the obligations individually and assigning the span of the enclosing impl if they do not involve anything "local" to the function. |
Sure.
Then ok. Feel free to r+, but if you can do something simple with the spans that would be best. |
nikomatsakis
commented
Jun 16, 2017
The problem is that these errors are reported at different times, I think, and hence I'm not sure that logic will apply. That is, the one error is occurring (I believe) when we check the WF of the impl, and the other when we are checking the individual methods. |
nikomatsakis
commented
Jun 16, 2017
I verified that they are in different inference contexts, so -- barring significant restructuring -- just tinkering with spans wouldn't cut it. |
nikomatsakis
commented
Jun 16, 2017
@bors r=eddyb |
bors
commented
Jun 16, 2017
📌 Commit 125968b has been approved by |
nikomatsakis
commented
Jun 17, 2017
@bors r- Had an idea of how to solve the remaining duplicate error problem. |
0ab9594 to
9fec409Comparenikomatsakis
commented
Jun 17, 2017
@bors r=eddyb |
bors
commented
Jun 17, 2017
📌 Commit 9fec409 has been approved by |
bors
commented
Jun 17, 2017
…sion, r=eddyb Issue 42545 type inference regression Fix an ICE that results from type inference obligations being dropped on the floor. Specifically, when computing the implied bounds, we would sometimes do normalizations that get stored in the cache, but we would *not* try to solve the resulting obligations. This can sometimes leave type variables uninferred. Occurs only rarely because implied bounds are processed in regionck which happens very late, so usually the cache is populated already from somewhere else. I think that the *proper* fix here is probably lazy normalization. This fix is intentionally very narrow both because this code is on the chopping block and because this needs a beta backport. r? @eddyb cc @arielb1
bors
commented
Jun 17, 2017
☀️ Test successful - status-appveyor, status-travis |
[beta] backports - #42785 - #42740 - #42735 - #42728 - #42695 - #42659 - #42634 - #42566 I just unilaterally accepted all the non-accepted nominations. Everything picked cleanly. Still testing locally. cc @rust-lang/compiler r? @alexcrichton
[beta] backports - #42785 - #42740 - #42735 - #42728 - #42695 - #42659 - #42634 - #42566 I just unilaterally accepted all the non-accepted nominations. Everything picked cleanly. Still testing locally. cc @rust-lang/compiler r? @alexcrichton
Fix an ICE that results from type inference obligations being dropped on the floor. Specifically, when computing the implied bounds, we would sometimes do normalizations that get stored in the cache, but we would not try to solve the resulting obligations. This can sometimes leave type variables uninferred. Occurs only rarely because implied bounds are processed in regionck which happens very late, so usually the cache is populated already from somewhere else.
I think that the proper fix here is probably lazy normalization. This fix is intentionally very narrow both because this code is on the chopping block and because this needs a beta backport.
r? @eddyb
cc @arielb1