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

Type inference regression in recent nightly #39984

Closed
Yamakaky opened this issue Feb 20, 2017 · 17 comments
Closed

Type inference regression in recent nightly #39984

Yamakaky opened this issue Feb 20, 2017 · 17 comments
Assignees
Labels
A-typesystem Area: The type system P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Yamakaky
Copy link
Contributor

Yamakaky commented Feb 20, 2017

https://play.rust-lang.org/?gist=188e52dc2ba423a8f78c6e381f278cf1&version=nightly&backtrace=1

source (copied by @pnkfelix from gist linked above):

fn main() {}

fn t() -> Result<(), String> {
    return Err("".into());
    Ok(())
}

Maybe it's because Ok(()) is unreachable?

See rust-lang-deprecated/error-chain#132 for the original report.

The breakage happens between nightlies 668864d 2017-02-16 and 536a900 2017-02-17.

@golddranks
Copy link
Contributor

golddranks commented Feb 20, 2017

Seems similar to #39808, in the sense that there is some unreachable code that apparently was used to help inference before?

@TimNN
Copy link
Contributor

TimNN commented Feb 20, 2017

Diff between the nightly-2017-02-17 and nightly-2016-02-18.

I expect this was caused by #39485 (and thus is expected breakage?).

@TimNN TimNN added A-typesystem Area: The type system regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 20, 2017
@Yamakaky
Copy link
Contributor Author

Seems like it.

@golddranks
Copy link
Contributor

golddranks commented Feb 22, 2017

Is it written somewhere what inference breaking was within range of "expected" and what is not? It started to bother me: the problem doesn't seem to be that some type info isn't available because it would be inferred from dead code. After all, the function signature here is not dead code, and everything's inferrable from there. The problem seems to be that the type of the dead expression isn't fully inferred from the non-dead code. But should the type of E matter since the code path is dead? And if it does, shouldn't it be fully inferrable from the function signature?

@Yamakaky
Copy link
Contributor Author

Exactly. BTW, it's not convenient if dead code doesn't typecheck. It happens to me a lot when I'm writing new code, along with unused variables for example.

@dhardy
Copy link
Contributor

dhardy commented Feb 26, 2017

As @nikomatsakis points out, existing code could be broken by doing proper type-checking in dead code.
However, this code would already have carried a dead_code warning (same as this case).

Ironic that disabling type checking of dead-code is actually causing type errors (this issue).

I stand by my reasoning that dead code should be type checked (even if it breaks some existing code), however it would also make sense if the dead code warning was reported before errors in the dead code.

@golddranks
Copy link
Contributor

So let me check: the recent changes disabled type-checking of dead code? And nevertheless it complains that the type of E in the dead code isn't known?

@dhardy
Copy link
Contributor

dhardy commented Feb 27, 2017

I'm not very familiar with how the compiler works, but I suspect the demand_coerce function also causes some type inference to happen, and without that some other part of the code complains that a type parameter never got deduced.
https://github.com/rust-lang/rust/pull/39485/files#diff-1d1b0d29a2e8da97c6bfb6e364d920c7L4165

I'd rather someone who actually understands the compiler answer that question though...

@nikomatsakis
Copy link
Contributor

Hmm. I think what we're doing in #39485 isn't quite right. I'm not sure the best answer here. Before #39485, we mostly but not completely ignored the types that arose from dead code. I'm nominating this for discussion in compiler team meeting; I think we have to address this regression in some way.

@nikomatsakis
Copy link
Contributor

triage: P-high

@tbu-
Copy link
Contributor

tbu- commented Mar 11, 2017

One of my crates is also experiencing this, with generated code similar to this (example heavily reduced):

pub fn encode() -> Result<(), ()> {
    try!(unimplemented!());
    Ok(())
}

Previously, this resulted in a dead code warning,

warning: unreachable expression, #[warn(unreachable_code)] on by default
 --> a.rs:3:5
  |
3 |     Ok(())
  |     ^^^^^^

but now, it errors out:

error[E0282]: type annotations needed
 --> a.rs:3:5
  |
3 |     Ok(())
  |     ^^ cannot infer type for `E`

error: aborting due to previous error

This is although the type for E is entirely clear, it should be () in this case, because the function has a result of Result<(), ()>.

@tbu-
Copy link
Contributor

tbu- commented Mar 11, 2017

I think this might be the same issue as #39808, although that one is tagged regression-from-stable-to-stable.

@arielb1
Copy link
Contributor

arielb1 commented Mar 16, 2017

1.17 is now beta.

@arielb1 arielb1 added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 16, 2017
@Yamakaky
Copy link
Contributor Author

Yamakaky commented Mar 16, 2017

So, is this issue considered a regression or a wontfix?

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 17, 2017
@brson
Copy link
Contributor

brson commented Mar 23, 2017

Fix is enqueued.

@sunjay
Copy link
Member

sunjay commented Mar 23, 2017

@brson Does this also fix my issue here? #40662

If so, you can close that issue as a duplicate.

@nikomatsakis
Copy link
Contributor

@sunjay done

@bors bors closed this as completed in 0f1eb8a Mar 25, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 29, 2017
change the strategy for diverging types

The new strategy is as follows. First, the `!` type is assigned
in two cases:

- a block with a diverging statement and no tail expression (e.g.,
  `{return;}`);
- any expression with the type `!` is considered diverging.

Second, we track when we are in a diverging state, and we permit a value
of any type to be coerced **into** `!` if the expression that produced
it is diverging. This means that `fn foo() -> ! { panic!(); 22 }`
type-checks, even though the block has a type of `usize`.

Finally, coercions **from** the `!` type to any other are always
permitted.

Fixes rust-lang#39808.
Fixes rust-lang#39984.
bors added a commit that referenced this issue Mar 30, 2017
change the strategy for diverging types

The new strategy is as follows. First, the `!` type is assigned
in two cases:

- a block with a diverging statement and no tail expression (e.g.,
  `{return;}`);
- any expression with the type `!` is considered diverging.

Second, we track when we are in a diverging state, and we permit a value
of any type to be coerced **into** `!` if the expression that produced
it is diverging. This means that `fn foo() -> ! { panic!(); 22 }`
type-checks, even though the block has a type of `usize`.

Finally, coercions **from** the `!` type to any other are always
permitted.

Fixes #39808.
Fixes #39984.
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants