Skip to content
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

Ignore expected type in diverging blocks #39485

Merged
merged 3 commits into from Feb 17, 2017

Conversation

Projects
None yet
8 participants
@canndrew
Copy link
Contributor

commented Feb 3, 2017

As per comment: #39297 (comment)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Feb 3, 2017

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

r? @nikomatsakis

Also, please use a more descriptive title (e.g. "Allow diverging blocks to have a type different than that of their trailing expression." but that's maybe too long).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

@canndrew instead of removing the affected tests, can you move at least one of them into a run-pass test, with a comment explaining why the code now type-checks?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

cc @eddyb

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

Oh, I see he already commented =)

@nikomatsakis nikomatsakis changed the title Small inference fix Allow diverging blocks to have arbitrary types Feb 3, 2017

@nikomatsakis nikomatsakis changed the title Allow diverging blocks to have arbitrary types Ignore expected type in diverging blocks Feb 3, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

It occurs to me that we perhaps ought to crater this too. That is, I guess that this can lead to types becoming constrained that were not otherwise (as we saw in #39009).

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2017

Is there a way to skip debuginfo tests when testing rustc? They always fail for me and it throws me off because sometimes I think the other tests have succeeded when I've gotten that far.

As for the failing run-pass tests. This now fails (unable to infer enough type information for E)

fn foo() -> Result<Foo, isize> {
    return Ok(Foo {
        x: Bar { x: 22 },
        a: return Err(32)
    });
}

Which is unfortunate. Shouldn't inference be able to infer that the Ok being returned has the type of the return type?

Also, this now fails (unable to infer enough type information for _):

pub fn index_colors<Pix>(image: &ImageBuffer<Pix, Vec<u8>>)
                         -> ImageBuffer<Luma<u8>, Vec<u8>>
where Pix: Pixel<Subpixel=u8> + 'static,
{
    let mut indices: ImageBuffer<_,Vec<_>> = loop { };
    for (pixel, idx) in image.pixels().zip(indices.pixels_mut()) {
        // failured occurred here ^^ because we were requiring that we
        // could project Pixel or Subpixel from `T_indices` (type of
        // `indices`), but the type is insufficiently constrained
        // until we reach the return below.
    }
    indices
}

Which makes sense, but is also unfortunate. (Because the function body is diverging, the type of indices as the final expression isn't unified with the return type).

canndrew added a commit to canndrew/rust that referenced this pull request Feb 4, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

@canndrew

Shouldn't inference be able to infer that the Ok being returned has the type of the return type?

Well, the whole change here is basically that it doesn't have to. (Since it is dead-code.)

cc @eddyb -- it makes sense that this would be causing some back-compat issues, but it's a drag. Definitely have to do a crater run I guess.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

Started a crater run.

@nikomatsakis

This comment has been minimized.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

So, according to that crater run there's 4 directly-broken packages: libimagestore and tera have already been fixed upstream, they just need to publish new versions or get people using those new versions. lion has been yanked from crates.io and twig can be fixed easily enough.

Of course if we did default to ! in these cases there wouldn't be any regressions. I'm not saying I prefer that solution just that it's a possibility.

What's everyone's take on all this?

Well, the whole change here is basically that it doesn't have to. (Since it is dead-code.)

Sure, I was just surprised that it didn't still force that particular bit of unification to happen.

@canndrew canndrew force-pushed the canndrew:inference-fix-39297 branch from d35e5b1 to 18be42c Feb 9, 2017

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

I've fixed the two failing tests.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2017

@canndrew ok, well, seems like we might as well go through with this. I feel like the current behavior is pretty inconsistent.

Of course if we did default to ! in these cases there wouldn't be any regressions. I'm not saying I prefer that solution just that it's a possibility.

I'm not sure I understand what you mean here by "default to !".

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

@canndrew ok, well, seems like we might as well go through with this. I feel like the current behavior is pretty inconsistent.

Sounds good to me.

I'm not sure I understand what you mean here by "default to !".

I just meant set the type variable which can't be inferred to !.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

@canndrew I see. Well, I'm not a big fan of that idea, for a couple of reasons. For one thing, it would re-create the problem we are trying to fix here. Consider e.g. something like this:

let mut v = vec![];
if condition {
    return;
    v.push(vec![]); // this type variable is created in dead code
}

Now the type of v becomes Vec<Vec<_0>> where _0 is "from dead code", and hence defaults to !. But we don't want that type to default to !.

In general, the rule of "dead code halts information flow up the tree (i.e., from expression to where expression is used)" but not "side-to-side" seems pretty good to me. Among other things, since the code is dead, there is (indeed) no data or information flow up the tree, and we can't know if you intended to add conversion and so forth.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

cc @Nercury -- twig-rs may be affected by this change. The testing of the PR got this error:

error[E0282]: unable to infer enough type information about `E`
   --> src/nodes/parser/expr.rs:379:5
    |
379 |     Ok(Expr::new_at(
    |     ^^ cannot infer type for `E`
    |
    = note: type annotations or generic parameter binding required

This PR affects information flow around dead-code. The problem seems to be that the type of this variable arg is "inferred" from an unimplemented! macro. The simplest fix is probably to remove this code but some type hints would help.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

I'm marking this for relnotes, since it does affect code. The relnotes could read:

"Fixed bugs around how type inference interacts with dead-code. The existing code generally ignores the type of dead-code unless a type-hint is provided; this can cause surprising inference interactions particularly around defaulting. The new code uniformly ignores the result type of dead-code."

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

📌 Commit 18be42c has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

⌛️ Testing commit 18be42c with merge 8725c90...

bors added a commit that referenced this pull request Feb 16, 2017

Auto merge of #39485 - canndrew:inference-fix-39297, r=nikomatsakis
Ignore expected type in diverging blocks

As per comment: #39297 (comment)
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

💔 Test failed - status-appveyor

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

⌛️ Testing commit 18be42c with merge 82756e4...

bors added a commit that referenced this pull request Feb 17, 2017

Auto merge of #39485 - canndrew:inference-fix-39297, r=nikomatsakis
Ignore expected type in diverging blocks

As per comment: #39297 (comment)
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

💔 Test failed - status-travis

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

@Nercury

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

@nikomatsakis twig was my early Rust project that is not finished and is paused now, so the breakage won't cause any issues

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

⌛️ Testing commit 18be42c with merge dc0bb3f...

bors added a commit that referenced this pull request Feb 17, 2017

Auto merge of #39485 - canndrew:inference-fix-39297, r=nikomatsakis
Ignore expected type in diverging blocks

As per comment: #39297 (comment)
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing dc0bb3f to master...

@bors bors merged commit 18be42c into rust-lang:master Feb 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@golddranks

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

There was some concerns about what is expected breakage and what is not in #39984 but I suspect they are more likely to be answered here.

There is this code that returns an Ok(()) in dead code: https://play.rust-lang.org/?gist=188e52dc2ba423a8f78c6e381f278cf1&version=nightly&backtrace=1 It's broken, because the inference can't infer the Err(_) variant anymore. The variant is directly inferrable from the signature of the function that contains the dead code.

So, is it expected breakage that the dead branch of code is 1) required to have fully known types but 2) is not type-inferred, unlike live branches, so these dead branches must have additional type annotations, which they wouldn't need if they were live branches?

nikomatsakis added a commit to nikomatsakis/rust that referenced this pull request Mar 18, 2017

Revert "Auto merge of rust-lang#39485 - canndrew:inference-fix-39297,…
… r=nikomatsakis"

This reverts commit dc0bb3f, reversing
changes made to e879aa4.

This is a temporary step intended to fix regressions. A more
comprehensive fix for type inference and dead-code is in the works.

nikomatsakis added a commit to nikomatsakis/rust that referenced this pull request Mar 22, 2017

Revert "Auto merge of rust-lang#39485 - canndrew:inference-fix-39297,…
… r=nikomatsakis"

This reverts commit dc0bb3f, reversing
changes made to e879aa4.

This is a temporary step intended to fix regressions. A more
comprehensive fix for type inference and dead-code is in the works.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 24, 2017

Rollup merge of rust-lang#40636 - nikomatsakis:revert-39485, r=eddyb
Revert rust-lang#39485, fixing type-inference regressions

This reverts PR rust-lang#39485, which should fix the immediate regressions. Eventually I'd like to land rust-lang#40224 -- or some variant of it -- which revisits the question fo dead-code and inference.

r? @eddyb
cc @canndrew

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 24, 2017

Rollup merge of rust-lang#40636 - nikomatsakis:revert-39485, r=eddyb
Revert rust-lang#39485, fixing type-inference regressions

This reverts PR rust-lang#39485, which should fix the immediate regressions. Eventually I'd like to land rust-lang#40224 -- or some variant of it -- which revisits the question fo dead-code and inference.

r? @eddyb
cc @canndrew

nikomatsakis added a commit to nikomatsakis/rust that referenced this pull request Mar 31, 2017

Revert "Auto merge of rust-lang#39485 - canndrew:inference-fix-39297,…
… r=nikomatsakis"

This reverts commit dc0bb3f, reversing
changes made to e879aa4.

This is a temporary step intended to fix regressions. A more
comprehensive fix for type inference and dead-code is in the works.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.