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

Do not ICE when combining unsized locals and async #64527

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/librustc/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ impl<'tcx> ConstEvalErr<'tcx> {
) -> Result<DiagnosticBuilder<'tcx>, ErrorHandled> {
let must_error = match self.error {
err_inval!(Layout(LayoutError::Unknown(_))) |
err_inval!(Layout(LayoutError::Unsized(_))) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unsized type isnt generic, so returning TooGeneric doesnt make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's also why this entire approach looks odd... a generic type will eventually be monomorphized, so that's a program that cannot be executed now but can be executed later. But if the type is unsized now it will always be unsized, right, so why does it make any sense to delay the error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea that should just be reported via the machinery below, which should simplify the rest

err_inval!(TooGeneric) =>
return Err(ErrorHandled::TooGeneric),
err_inval!(TypeckError) =>
Expand Down
13 changes: 11 additions & 2 deletions src/librustc/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,16 @@ pub const FAT_PTR_EXTRA: usize = 1;
#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)]
pub enum LayoutError<'tcx> {
Unknown(Ty<'tcx>),
SizeOverflow(Ty<'tcx>)
SizeOverflow(Ty<'tcx>),
Unsized(Ty<'tcx>),
}

impl<'tcx> fmt::Display for LayoutError<'tcx> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match *self {
LayoutError::Unsized(ty) => {
write!(f, "the type `{:?}` has unknown size", ty)
}
LayoutError::Unknown(ty) => {
write!(f, "the type `{:?}` has an unknown layout", ty)
}
Expand Down Expand Up @@ -745,7 +749,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
let mut abi = Abi::Aggregate { sized: true };
let index = VariantIdx::new(0);
for field in &variants[index] {
assert!(!field.is_unsized());
if field.is_unsized() {
// Instead of asserting, return an error because this can happen with
// generators/async-await and unsized locals.
return Err(LayoutError::Unsized(field.ty));
}
align = align.max(field.align);

// If all non-ZST fields have the same ABI, forward this ABI
Expand Down Expand Up @@ -2492,6 +2500,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for LayoutError<'tcx> {
mem::discriminant(self).hash_stable(hcx, hasher);

match *self {
Unsized(t) |
Unknown(t) |
SizeOverflow(t) => t.hash_stable(hcx, hasher)
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_lint/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for VariantSizeDifferences {
let ty = cx.tcx.erase_regions(&t);
let layout = match cx.layout_of(ty) {
Ok(layout) => layout,
Err(ty::layout::LayoutError::Unsized(_)) |
Err(ty::layout::LayoutError::Unknown(_)) => return,
Err(err @ ty::layout::LayoutError::SizeOverflow(_)) => {
bug!("failed to get layout for `{}`: {}", t, err);
Expand Down
51 changes: 33 additions & 18 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,28 +669,43 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
let place_ty: Ty<'tcx> = place
.ty(&self.local_decls, self.tcx)
.ty;
if let Ok(place_layout) = self.tcx.layout_of(self.param_env.and(place_ty)) {
if let Some(value) = self.const_prop(rval, place_layout, statement.source_info) {
if let Place {
base: PlaceBase::Local(local),
projection: box [],
} = *place {
trace!("checking whether {:?} can be stored to {:?}", value, local);
if self.can_const_prop[local] {
trace!("storing {:?} to {:?}", value, local);
assert!(self.get_const(local).is_none());
self.set_const(local, value);

if self.should_const_prop() {
self.replace_with_const(
rval,
value,
statement.source_info,
);

match self.tcx.layout_of(self.param_env.and(place_ty)) {
Ok(place_layout) => {
if let Some(value) = self.const_prop(
rval,
place_layout,
statement.source_info,
) {
if let Place {
base: PlaceBase::Local(local),
projection: box [],
} = *place {
trace!("checking whether {:?} can be stored to {:?}", value, local);
if self.can_const_prop[local] {
trace!("storing {:?} to {:?}", value, local);
assert!(self.get_const(local).is_none());
self.set_const(local, value);

if self.should_const_prop() {
self.replace_with_const(
rval,
value,
statement.source_info,
);
}
}
}
}
Err(LayoutError::Unsized(_ty)) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add an if ty.is_generator() here, which would ensure the correct error is emitted, and just ICE in any other (in theory unreachable) case.

let sp = statement.source_info.span; // async fn block
self.tcx.sess.struct_span_err(
Copy link
Contributor

@Centril Centril Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm; const_prop is too my knowledge an optimization transform not necessary for correctness so why are we emitting an error here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I think it's totally fine to just keep doing what the old code did and ignore errors. They may be occuring because we're in a generic function and may be bogus for specific concrete types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we close this PR then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well; we cannot merge this PR as-is I think but maybe we can morph the PR into something else. You can always open a new one tho... up to you. :)

sp,
"unsized values can't be used in `async` functions",
).span_label(sp, "contains unsized values")
.emit();
}
Err(_) => {}
}
}
self.super_statement(statement, location);
Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/async-await/unsized-locals.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// edition:2018

#![feature(unsized_locals)]
#![feature(gen_future)]

use std::future::poll_with_tls_context;
use std::pin::Pin;
use std::fmt::Display;

async fn foo2() {}

async fn foo(x: Box<dyn Display>) { //~ ERROR unsized values can't be used in `async` functions
let x = *x;
foo2().await;
println!("hello {}", &x);
}

fn main() {
let mut a = foo(Box::new(5));
let b = unsafe {
Pin::new_unchecked(&mut a)
};
match poll_with_tls_context(b) {
_ => ()
};
}
13 changes: 13 additions & 0 deletions src/test/ui/async-await/unsized-locals.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: unsized values can't be used in `async` functions
--> $DIR/unsized-locals.rs:12:35
|
LL | async fn foo(x: Box<dyn Display>) {
| ___________________________________^
LL | | let x = *x;
LL | | foo2().await;
LL | | println!("hello {}", &x);
LL | | }
| |_^ contains unsized values

error: aborting due to previous error