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

Existential type ICE #52985

Closed
DutchGhost opened this Issue Aug 2, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@DutchGhost
Copy link

DutchGhost commented Aug 2, 2018

The ICE message is almost the same as #52701 , yet the code is verrry different:

#![feature(existential_type)]

existential type Foo: Copy;

// make compiler happy about using 'Foo'
fn bar(x: Foo) -> Foo { x }

fn main() {
    
    // make compiler happy about the types??
    let _: Foo = unsafe { std::mem::transmute(0u8) };
}
Backtrace:
Compiling playground v0.0.1 (file:///playground)
error: internal compiler error: librustc/traits/query/normalize.rs:124: infinite recursion generic_ty: Foo, substs: [], concrete_ty: Foo, ty: Foo

thread 'main' panicked at 'Box<Any>', librustc_errors/lib.rs:578:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: rustc::util::common::panic_hook
   5: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:479
   6: std::panicking::begin_panic
   7: rustc_errors::Handler::bug
   8: rustc::session::opt_span_bug_fmt::{{closure}}
   9: rustc::ty::context::tls::with_opt::{{closure}}
  10: rustc::ty::context::tls::with_context_opt
  11: rustc::ty::context::tls::with_opt
  12: rustc::session::opt_span_bug_fmt
  13: rustc::session::bug_fmt
  14: <rustc::traits::query::normalize::QueryNormalizer<'cx, 'gcx, 'tcx> as rustc::ty::fold::TypeFolder<'gcx, 'tcx>>::fold_ty
  15: rustc::traits::query::normalize::<impl rustc::infer::at::At<'cx, 'gcx, 'tcx>>::normalize
  16: rustc::ty::context::tls::with_related_context
  17: rustc::infer::InferCtxtBuilder::enter
  18: rustc_traits::normalize_erasing_regions::normalize_ty_after_erasing_regions
  19: rustc::ty::query::__query_compute::normalize_ty_after_erasing_regions
  20: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::normalize_ty_after_erasing_regions<'tcx>>::compute
  21: rustc::dep_graph::graph::DepGraph::with_task_impl
  22: rustc::ty::context::tls::with_related_context
  23: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  24: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  25: <rustc::ty::layout::LayoutCx<'tcx, rustc::ty::context::TyCtxt<'a, 'tcx, 'tcx>> as rustc_target::abi::LayoutOf>::layout_of
  26: rustc::ty::layout::SizeSkeleton::compute
  27: <rustc::middle::intrinsicck::ExprVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_expr
  28: rustc::hir::intravisit::walk_expr
  29: <rustc::middle::intrinsicck::ExprVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_expr
  30: <rustc::middle::intrinsicck::ExprVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_expr
  31: rustc::hir::intravisit::walk_block
  32: <rustc::middle::intrinsicck::ExprVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_expr
  33: <rustc::middle::intrinsicck::ItemVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_nested_body
  34: rustc::hir::Crate::visit_all_item_likes
  35: rustc::middle::intrinsicck::check_crate
  36: rustc::util::common::time
  37: rustc::ty::context::tls::enter_context
  38: <std::thread::local::LocalKey<T>>::with
  39: rustc::ty::context::TyCtxt::create_and_enter
  40: rustc_driver::driver::compile_input
  41: rustc_driver::run_compiler_with_pool
  42: <scoped_tls::ScopedKey<T>>::set
  43: <scoped_tls::ScopedKey<T>>::set
  44: syntax::with_globals
  45: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  46: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  47: rustc_driver::run
  48: rustc_driver::main
  49: std::rt::lang_start::{{closure}}
  50: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  51: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  52: std::rt::lang_start_internal
             at libstd/panicking.rs:289
             at libstd/panic.rs:392
             at libstd/rt.rs:58
  53: main
  54: __libc_start_main
  55: <unknown>
query stack during panic:
#0 [normalize_ty_after_erasing_regions] normalizing `ParamEnvAnd { param_env: ParamEnv { caller_bounds: [], reveal: All }, value: Foo }`
end of query stack
error: aborting due to previous error


note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.29.0-nightly (97085f9fb 2018-08-01) running on x86_64-unknown-linux-gnu

note: compiler flags: -C opt-level=3 -C codegen-units=1 --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

Unrelated data:

changing fn bar to the following compiles just fine:

fn bar() -> Foo { 0u8 } 

however, when removing the u8 here, we get a compile error:

#![feature(existential_type)]

existential type Foo: Copy;

fn bar() -> Foo { 0 } 

fn main() {
    let _ : Foo = unsafe { std::mem::transmute(0u8) };
}
Compiling playground v0.0.1 (file:///playground)
error[E0512]: transmute called with types of different sizes
 --> src/main.rs:8:28
  |
8 |     let _ : Foo = unsafe { std::mem::transmute(0u8) };
  |                            ^^^^^^^^^^^^^^^^^^^
  |
  = note: source type: u8 (8 bits)
  = note: target type: Foo (32 bits)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0512`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Aug 3, 2018

It should be enough to turn the panic into a delay_span_bug + return and wait for the later error about no definitions found.

So in https://github.com/rust-lang/rust/blob/master/src/librustc/traits/query/normalize.rs#L124 try the following things in the given order:

  1. replace the bug! call with self.error = true; return concrete_ty;
  2. replace the bug! call with self.tcx().sess.delay_span_bug(some_span, "original_bug_message"); return concrete_ty; Not sure where we'd get the span from, try extracting it out of the self.cause
  3. replace the bug! call with self.infcx.report_overflow_error(&obligation, true);

@oli-obk oli-obk added the E-mentor label Aug 3, 2018

@tristanburgess

This comment has been minimized.

Copy link
Contributor

tristanburgess commented Aug 7, 2018

I would like to take this if no one else is working on it.

Just to make sure I understand so far:

  • Foo is an existential type which implements the Copy trait.
  • bar() makes the compiler infer that Foo's concrete type is the return type of bar()
  • In main(), we need to match that concrete type when transmuting some data in order for it to be promoted to a Foo.
  • In the first (ICE-causing) snippet, bar() doesn't lock down the concrete type of Foo, so the compiler should throw an error that it can't match u8 and Foo, whose size is not known.
  • Instead, the code that determines the concrete type hits the infinite recursion case described because Foo can not be inferred concretely, and this code intentionally raises panic!(). We need to change this to raise a compile error instead.

Does that sound right? I'm going to spend some time as well to better understand this, because I have no idea yet what self.tcx().sess.delay_span_bug() or self.infcx.report_overflow_error() do yet, but I will run through the steps suggested in order to obtain a fix.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Aug 8, 2018

Ignore the other snippets, those are doing the right thing.

The issue is that main needs the existential type Foo, but only bar's return type defines it, but bars return type defines it in terms of Foo (the argument x). So we need to know the type of Foo to know the type of Foo. Which is not possible out of chicken/egg issues.

  • delay_span_bug tells the compiler to ICE if no real errors have been reported
  • report_overflow_error is what happens when you change the argument to &'static Foo and is probably the wrong thing to do, because it's not an overflow, but a cycle
    • Ideally we'd cause a cycle error. Maybe try calling tcx.type_of(def_id); before the panic. That might produce a better error than the first two methods
@tristanburgess

This comment has been minimized.

Copy link
Contributor

tristanburgess commented Aug 8, 2018

Using just option 2, we get the following, which appears to line up with transmute error implementation:

"this type's size can vary".to_string()

./dev/code/rust$ rustc +stage1 ./dev/tests/data/rust_52895_ice.rs 
error[E0512]: transmute called with types of different sizes
  -->  ./dev/tests/data/rust_52895_ice.rs:11:27
   |
11 |     let _: Foo = unsafe { std::mem::transmute(0u8) };
   |                           ^^^^^^^^^^^^^^^^^^^
   |
   = note: source type: u8 (8 bits)
   = note: target type: Foo (this type's size can vary)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0512`.

I think this is ok. Works as well if you go full bore with transmuting Foo to Foo without resolving Foo's concrete size (see below), and (to me) tells me that Foo's size can't be resolved to something concrete. I will begin creating regression test cases. Please let me know if this doesn't seem like the right error, or if there's anything else I should take into account.

#![feature(existential_type)]

existential type Foo: Copy;

// make compiler happy about using 'Foo'
fn bar(x: Foo) -> Foo { x }

fn main() {
    let x: Foo;
    let _: Foo = unsafe { std::mem::transmute(x) };
}

which results in the below.

./dev/code/rust$ rustc +stage1 ./dev/tests/data/rust_52895_confusing_mismatch.rs 
error[E0512]: transmute called with types of different sizes
  --> ./dev/tests/data/rust_52895_confusing_mismatch.rs:10:27
   |
10 |     let _: Foo = unsafe { std::mem::transmute(x) };
   |                           ^^^^^^^^^^^^^^^^^^^
   |
   = note: source type: Foo (this type's size can vary)
   = note: target type: Foo (this type's size can vary)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0512`.
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Aug 8, 2018

The error message is pretty good indeed. Although I fear out of the wrong reasons XD. I think this is what happens when one tries to transmute an unsized type.

One small thing that you could try is to return tcx.types.err instead of concrete_ty.

Ideally the error would point to the function at some point.

What happens in your current version if you minimize the test to

#![feature(existential_type)]

existential type Foo: Copy;

// make compiler happy about using 'Foo'
fn bar(x: Foo) -> Foo { x }

fn main() {
    let x = bar(unimplemented!());
}
@tristanburgess

This comment has been minimized.

Copy link
Contributor

tristanburgess commented Aug 8, 2018

Returning tcx.types.err instead gives the below output. I'll see if I can figure out how to get the error to point to bar() (assuming that's what you meant).

./dev/code/rust$ rustc +stage1 ./dev/tests/data/rust_52895_double_mismatch.rs 
error[E0512]: transmute called with types of different sizes
  --> ./dev/tests/data/rust_52895_ice.rs:11:27
   |
11 |     let _: Foo = unsafe { std::mem::transmute(0u8) };
   |                           ^^^^^^^^^^^^^^^^^^^
   |
   = note: source type: u8 (8 bits)
   = note: target type: Foo (size can vary because of [type error])

error: aborting due to previous error

The test reduction snippet suggested above produces the following:

./dev/code/rust$ rustc +stage1 ./dev/tests/data/rust_52895_minimal_ice.rs 
warning: unreachable expression
 --> ./dev/tests/data/rust_52895_minimal_ice.rs:9:13
  |
9 |     let x = bar(unimplemented!());
  |             ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unreachable_code)] on by default

warning: unused variable: `x`
 --> ./dev/tests/data/rust_52895_minimal_ice.rs:9:9
  |
9 |     let x = bar(unimplemented!());
  |         ^ help: consider using `_x` instead
  |
  = note: #[warn(unused_variables)] on by default
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Aug 8, 2018

Thanks for trying it out. the types.err way is definitely not the one we want to go. It's hiding the error.

So one further thing to try is the type_of loop from #52985 (comment) (or even just recursing on the normalize_ty_after_erasing_regions query).

To improve the span with the resulting query error, you can look for calls to normalize_ty_after_erasing_regions and whether there's any span around. if there is, call tcx.at(span).normalize_ty_after_erasing_regions(...) instead of calling it directly.

In case we are not actually seeing a good diagnostic, just going for a cycle error seems fine. We can then leave the issue open for further investigation into improving the span.

@tristanburgess

This comment has been minimized.

Copy link
Contributor

tristanburgess commented Aug 13, 2018

The only span around calls to normalize_ty_after_erasing_regions is a DUMMY_SP. I was able to get debug logging up to see the span that was at the line we wanted, but haven't yet figured out where it disappears to and why.

For now, I've gotten the cycle error in place and accompanied by the span corresponding to the existential type def just so that it's pointing the user at something relevant instead of nothing, captured in the PR detailed above.

bors added a commit that referenced this issue Aug 19, 2018

Auto merge of #53316 - tristanburgess:52895_existential_type_ICE, r=o…
…li-obk

52985: cause cycle err on inf trait normalization

Issue: #52985
 - If an existential type is defined, but no user code infers the
concrete type behind the existential type, normalization would
infinitely recurse on this existential type which is only defined in
terms of itself.
  - Instead of raising an inf recurse error, we cause a cycle error to
help highlight that the issue is that the type is only defined in terms
of itself.
  - Three known potential improvements:
    - If type folding itself was exposed as a query, used by
normalization and other mechanisms, cases that would cause infinite recursion would
automatically cause a cycle error.
    - The span for the cycle error should be improved to point to user
code that fails to allow inference of the concrete type of the existential type,
assuming that this error occurs because no user code can allow inference the
concrete type.
    - A mechanism to extend the cycle error with a helpful note would be nice. Currently,
the error is built and maintained by src/librustc/ty/query/plumbing,
with no known way to extend the information that the error gets built
with.

r? @oli-obk
@tristanburgess

This comment has been minimized.

Copy link
Contributor

tristanburgess commented Aug 20, 2018

Leaving open per discussion starting at #53316 (comment); need to investigate error handling at an earlier time than the transmute, since before transmute is even scanned we should be able to tell that the existential type does not have an underlying concrete type defined.

@tristanburgess

This comment has been minimized.

Copy link
Contributor

tristanburgess commented Aug 23, 2018

Forgot to put closes issue in the commit message, and I don't think I can close this myself. Per the above PR, this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment