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

Overlap checking with downstream impl is insufficient w.r.t. fundamental types; causes ICE #43355

Closed
qnighy opened this issue Jul 20, 2017 · 2 comments · Fixed by #46192
Closed
Labels
C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@qnighy
Copy link
Contributor

qnighy commented Jul 20, 2017

Overlap checker considers possible downstream impl, but it is insufficient if it may contain fundamental types. May cause ICE.

#![crate_type = "lib"]
#![crate_name = "foo"]

pub trait Trait1<X> {
    type Output;
}
pub trait Trait2<X> {}

impl<X, T> Trait1<X> for T where T: Trait2<X> {
    type Output = ();
}
impl<X> Trait1<Box<X>> for A {
    type Output = i32;
}

pub struct A;

fn f<X, T: Trait1<Box<X>>>() {
    println!("k: {}", ::std::mem::size_of::<<T as Trait1<Box<X>>>::Output>());
}

pub fn g<X, T: Trait2<Box<X>>>() {
    f::<X, T>();
}
extern crate foo;

use foo::{Trait2, A, g};

struct B;

impl Trait2<Box<B>> for A {}

fn main() {
    g::<B, A>();
}

In stable (1.18.0), ICE is triggered when compiling downstream main.

$ rustc --version
rustc 1.18.0 (03fc9d622 2017-06-06)
$ cargo build
   Compiling foo v0.1.0 (file:///C:/Users/qnighy/workdir/orphan-test/foo)
   Compiling orphan-test v0.1.0 (file:///C:/Users/qnighy/workdir/orphan-test)
error: internal compiler error: src\librustc\infer\mod.rs:715: Encountered errors `[FulfillmentError(Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { trait_ref: <foo::A as foo::Trait1<std::boxed::Box<B>>>, item_name: Output(445) }, _)),depth=1),Ambiguity)]` resolving bounds after type-checking

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

thread 'rustc' panicked at 'Box<Any>', src\librustc_errors\lib.rs:375
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: Could not compile `orphan-test`.

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

In nightly, ICE is triggered when compiling even upstream foo. Perhaps this is a different bug.

$ rustc +nightly --version
rustc 1.20.0-nightly (582af6e1a 2017-07-19)
$ cargo +nightly build
   Compiling foo v0.1.0 (file:///C:/Users/qnighy/workdir/orphan-test/foo)
error: internal compiler error: src\librustc\infer\mod.rs:573: Encountered errors `[FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<T as std::marker::Sized>)),depth=1),Unimplemented), FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<T as Trait2<std::boxed::Box<X>>>)),depth=1),Unimplemented)]` resolving bounds after type-checking

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.20.0-nightly (582af6e1a 2017-07-19) running on x86_64-pc-windows-msvc

thread 'rustc' panicked at 'Box<Any>', src\librustc_errors\lib.rs:437:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: Could not compile `foo`.

To learn more, run the command again with --verbose.
@kennytm
Copy link
Member

kennytm commented Jul 20, 2017

The nightly ICE should be a different bug, related to size_of of an associated type.

trait Trait1 {
    type Output;
}
fn f<T: Trait1>() {
    ::std::mem::size_of::<T::Output>();
}
fn main() {}

@Mark-Simulacrum Mark-Simulacrum added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ C-bug Category: This is a bug. labels Jul 26, 2017
@arielb1 arielb1 added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 22, 2017
@arielb1
Copy link
Contributor

arielb1 commented Nov 22, 2017

Yeah I always hated that check. So now we see it is insufficient. Let's hope we can fix this without nuking the entire ecosystem.

@nagisa nagisa added P-high High priority and removed I-nominated labels Nov 23, 2017
bors added a commit that referenced this issue Nov 23, 2017
coherence: fix is_knowable logic

A trait-ref that passes the orphan-check rules can still be implemented in a crate downstream from our crate (for example, `LocalType for LocalTrait<_>` might be matched by a `LocalType for LocalTrait<TypeFromDownstreamCrate>`), and this should be known by the `is_knowable`  logic.

Trait selection had a hackfix for this, but it's an hacky fix that does not handle all cases. This patch removes it.

cc #43355.

FIXME: make this a soft error. I suppose we'll crater first.

r? @nikomatsakis

Needs a crater run
bors added a commit that referenced this issue Dec 6, 2017
coherence: fix is_knowable logic

A trait-ref that passes the orphan-check rules can still be implemented in a crate downstream from our crate (for example, `LocalType for LocalTrait<_>` might be matched by a `LocalType for LocalTrait<TypeFromDownstreamCrate>`), and this should be known by the `is_knowable`  logic.

Trait selection had a hackfix for this, but it's an hacky fix that does not handle all cases. This patch removes it.

fixes #43355.

r? @nikomatsakis

Needs a crater run
bors added a commit that referenced this issue Dec 6, 2017
coherence: fix is_knowable logic

A trait-ref that passes the orphan-check rules can still be implemented in a crate downstream from our crate (for example, `LocalType for LocalTrait<_>` might be matched by a `LocalType for LocalTrait<TypeFromDownstreamCrate>`), and this should be known by the `is_knowable`  logic.

Trait selection had a hackfix for this, but it's an hacky fix that does not handle all cases. This patch removes it.

fixes #43355.

r? @nikomatsakis

Needs a crater run
bors added a commit that referenced this issue Feb 8, 2020
Improve performance of coherence checks

The biggest perf improvement in here is expected to come from the removal of the remaining #43355 warning code since the effectively runs the expensive parts of coherence *twice* (which can even be visualized by obtaining a flamegraph https://twitter.com/sheevink/status/1225963187511709696).
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 10, 2020
…arkor

Improve performance of coherence checks

The biggest perf improvement in here is expected to come from the removal of the remaining rust-lang#43355 warning code since that effectively runs the expensive parts of coherence *twice*, which can even be visualized by obtaining a flamegraph:

![image](https://user-images.githubusercontent.com/5047365/74091959-e1f41200-4a8b-11ea-969d-2849d3f86c63.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants