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

Box::new(!) doesn't coerce to Box<Error> successfully #49593

Closed
alexcrichton opened this issue Apr 2, 2018 · 14 comments
Closed

Box::new(!) doesn't coerce to Box<Error> successfully #49593

alexcrichton opened this issue Apr 2, 2018 · 14 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Discovered in #49039 the recent stabilization of ! unfortunately has some hazards related to coercion on the libs side of things. We discussed this at the work week and decided that we can't land PRs like #49039 which would also mean that the libs team wouldn't be able to actually recommend using ! as an error type (bummer!)

This is intended to be a tracking issue for sorting this out. I'm tagging it as a regression to mainly keep track of it, but it's not strictly speaking a regression.

cc @rust-lang/libs
cc @rust-lang/lang

@alexcrichton alexcrichton added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 2, 2018
@alexcrichton alexcrichton added this to the 1.26 milestone Apr 2, 2018
@cramertj
Copy link
Member

cramertj commented Apr 2, 2018

@nikomatsakis, @dtolnay and I discussed this at the Rust all hands, and our preliminary opinion was that we should roll back the stabilization of ! and TryFrom until we get this sorted out. This is a super unfortunate result, but I think it's better than introducing ! and telling no one to use it as an error type.

As far as the error itself, this is happening because inference works backwards from the return type to assume that x in this expression must be of type Error:

use std::error::Error;
fn foo(x: !) -> Box<Error> {
    Box::new(x)
}

Error is !Sized, however, and so is not a valid argument to Box::new.

We discussed the possibility of disabling inference where it would produce unsized argument types, but this would interact poorly with future changes to remove the Sized bound on Box::new when unsized rvalues land. @nikomatsakis suggested we might be able to come up with something tailored to the specific case of Box::new and uninhabited types, but I'm not sure what he had in mind.

@SimonSapin
Copy link
Contributor

Is it possible to make the ! to anything coercion have "less priority" than the type implementing a trait to trait object unsizing coercion?

I really hope we can find a solution similar to this, in time for 1.26

@eddyb
Copy link
Member

eddyb commented Apr 2, 2018

I talked to @nikomatsakis as well and looking at the example again it's plausible we could try unsizing coercion before the ! -> _, however, Box::new can't take an unsized type anyway.

@nikomatsakis
Copy link
Contributor

I opened #49691 to track specifically the rollback part.

@nikomatsakis nikomatsakis removed the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Apr 5, 2018
@nikomatsakis
Copy link
Contributor

I'm removing the regression tag since this is not a regression, and we have #49691 to track that part. This issue should be added to the various spreadsheets tracking Rust 2018 progress though.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Apr 5, 2018

Starting at #35121 (comment) is some discussion of T: From<!> too, including how we can hack it before a real specialization solution is ready. I really think we should block stabilization on that bound too; ! as an error type is much less ergonomic without it.

@nagisa
Copy link
Member

nagisa commented Apr 8, 2018

Is keeping things as-is and introducing the coercion properly later on gonna cause some sort of pain or backcompat issues?

@alexcrichton
Copy link
Member Author

Removing from milestone now that ! is destabilized

@alexcrichton alexcrichton removed this from the 1.26 milestone Apr 24, 2018
@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jun 29, 2018
@droundy
Copy link
Contributor

droundy commented Jul 8, 2018

I am wondering if there is an explanation somewhere as to why functions that take ! values as input are even type checked and compiled? It seems like we could make the compiler aware that such a function can never be invoked and have it just skip them?

Similarly for any branches that involve creation or matching of never values, I suppose...

@SimonSapin
Copy link
Contributor

At the codegen (LLVM backend) level, the "presence" of an uninhabited value makes the code unreachable and I assume that no code is generated. But in my opinion it’s much less obvious that we should skip type-checking.

@varkor
Copy link
Member

varkor commented Jul 8, 2018

@droundy: you might like to gather a look at #47291, which does some of the things you're talking about.

@mxmerz
Copy link

mxmerz commented Nov 20, 2018

Has there been any progress on this issue in the last months?

arielb1 added a commit to arielb1/rust that referenced this issue Dec 16, 2018
This PR causes unsized coercions to not be disabled by `$0: Unsize<dyn
Object>` coercion obligations when we have an `$0: Sized` obligation
somewhere.

Note that `X: Unsize<dyn Object>` obligations can't fail *as
obligations* if `X: Sized` holds, so this still maintains some version
of monotonicity (I think that an unsized coercion can't be converted to
no coercion by unifying type variables).

Fixes rust-lang#49593 (unblocking never_type).
bors added a commit that referenced this issue Dec 19, 2018
trigger unsized coercions keyed on Sized bounds

This PR causes unsized coercions to not be disabled by `$0: Unsize<dyn
Object>` coercion obligations when we have an `$0: Sized` obligation somewhere.

This should be mostly backwards-compatible, because in these cases not doing the unsize coercion should have caused the `$0: Sized` obligation to fail.

Note that `X: Unsize<dyn Object>` obligations can't fail *as obligations* if `X: Sized` holds, so this still maintains some version of monotonicity (I think that an unsized coercion can't be converted to no coercion by unifying type variables).

Fixes #49593 (unblocking never_type).

r? @eddyb
cc @nikomatsakis
bors added a commit that referenced this issue Dec 20, 2018
trigger unsized coercions keyed on Sized bounds

This PR causes unsized coercions to not be disabled by `$0: Unsize<dyn
Object>` coercion obligations when we have an `$0: Sized` obligation somewhere.

This should be mostly backwards-compatible, because in these cases not doing the unsize coercion should have caused the `$0: Sized` obligation to fail.

Note that `X: Unsize<dyn Object>` obligations can't fail *as obligations* if `X: Sized` holds, so this still maintains some version of monotonicity (I think that an unsized coercion can't be converted to no coercion by unifying type variables).

Fixes #49593 (unblocking never_type).

r? @eddyb
cc @nikomatsakis
@jimmycuadra
Copy link
Contributor

Does this mean ! can be stabilized now?

@SimonSapin
Copy link
Contributor

Confirmed that the code below now compiles on master. (With a unreachable expression warning, but without error[E0277]: the size for values of type `dyn std::error::Error` cannot be known at compilation time like before.)

#![feature(never_type)]
pub fn foo(x: !) -> Box<std::error::Error> {
    Box::new(x)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests