-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix ICE from #33364 by selectively skipping confirmation pass. #34573
Changes from all commits
d234bf9
1366cb7
e84b400
99a44e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1247,6 +1247,34 @@ fn confirm_callable_candidate<'cx, 'gcx, 'tcx>( | |
fn_sig, | ||
flag); | ||
|
||
if selcx.projection_mode().is_any() { | ||
// Issue #33364: `ProjectionMode::Any` means trans, during | ||
// which there is no (good) reason for confirmation to fail | ||
// (because type check has already admitted the source code). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not necessarily true if anything does probing (such as checking if a vtable needs to contain a specific method, based on where clauses of that method). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eddyb Wait so are you saying it is not actually sound for me to skip the confirms via the logic I have put here? Or ... are you "merely" saying that the comment as written here is misleading? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure in which situations this may backfire, but for example, vtable building in trans probes for obligations on methods to decide whether to include them. There are also secondary probes related to specialization, during selection. |
||
// | ||
// The only reason trans needs to run confirm is finding | ||
// solutions for outstanding unification variables. (For now, | ||
// that empirically appears to be necessary in some cases). | ||
// | ||
// So: if we can prove there is no reason to confirm, skip it. | ||
// | ||
// (Skipping confirmation sidesteps ICE in #33364. Even if | ||
// that were fixed by other means, skipping is still good | ||
// since redundant confirmations wastes compile-time.) | ||
|
||
let unsolved_tys = trait_ref.substs.types.iter().any(|t| t.is_ty_var()); | ||
if unsolved_tys || ret_type.region_depth() > 0 { | ||
debug!("confirm_callable_candidate need confirm for \ | ||
unsolved_tys: {} ret_type: {:?} region_depth {}", | ||
unsolved_tys, ret_type, ret_type.region_depth()); | ||
} else { | ||
debug!("confirm_callable_candidate skip confirm for \ | ||
trait_ref: {:?} ret_type: {:?}", | ||
trait_ref, ret_type); | ||
return Progress { ty: ret_type, obligations: vec![], cacheable: false, }; | ||
} | ||
} | ||
|
||
let predicate = ty::Binder(ty::ProjectionPredicate { // (1) recreate binder here | ||
projection_ty: ty::ProjectionTy { | ||
trait_ref: trait_ref, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// Issue #33364: This is a reduced version of the reported code that | ||
// caused an ICE. The ICE was due to a confirmation step during trans | ||
// attempting (and failing) to match up the actual argument type to | ||
// the closure (in this case, `&u32`) with the Fn argument type, which | ||
// is a projection of the form `<T as Foo<'b>>::Item`. | ||
|
||
use std::marker::PhantomData; | ||
|
||
trait Foo<'a> { | ||
type Item; | ||
fn consume<F>(self, f: F) where F: Fn(Self::Item); | ||
} | ||
struct Consume<A>(PhantomData<A>); | ||
|
||
impl<'a, A:'a> Foo<'a> for Consume<A> { | ||
type Item = &'a A; | ||
|
||
fn consume<F>(self, _f: F) where F: Fn(Self::Item) { | ||
if blackbox() { | ||
_f(any()); | ||
} | ||
} | ||
} | ||
|
||
#[derive(Clone)] | ||
struct Wrap<T> { foo: T } | ||
|
||
impl<T: for <'a> Foo<'a>> Wrap<T> { | ||
fn consume<F>(self, f: F) where F: for <'b> Fn(<T as Foo<'b>>::Item) { | ||
self.foo.consume(f); | ||
} | ||
} | ||
|
||
fn main() { | ||
// This works | ||
Consume(PhantomData::<u32>).consume(|item| { let _a = item; }); | ||
|
||
// This used to not work (but would only be noticed if you call closure). | ||
let _wrap = Wrap { foo: Consume(PhantomData::<u32>,) }; | ||
_wrap.consume(|item| { let _a = item; }); | ||
} | ||
|
||
pub static mut FLAG: bool = false; | ||
fn blackbox() -> bool { unsafe { FLAG } } | ||
fn any<T>() -> T { loop { } } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// Issue #33364: While working on the hack/fix to workaround an ICE, I | ||
// injected a new ICE that was not tested by our run-pass test suite | ||
// (but was exposed by attempting to bootstrap the compiler itself, as | ||
// well as our compile-fail tests). This test attempts to codify the | ||
// problem I encountered at that time, as a run-pass test. | ||
|
||
#![feature(unboxed_closures)] | ||
fn main() | ||
{ | ||
static X: u32 = 3; | ||
fn lifetime_in_ret_type_alone<'a>() -> &'a u32 { &X } | ||
fn apply_thunk<T: FnOnce<()>>(t: T, _x: T::Output) { t(); } | ||
|
||
apply_thunk(lifetime_in_ret_type_alone, &3); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This is somehow hackier than I had imagined. What I had hoped was that we would attach a (generic) filter function to the
FulfillmentContext
-- when not in trans, this filter function would accept all work, but within trans, it could be more selective.EDIT: Not that it wouldn't necessarily always be more selective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually told @pnkfelix about
ProjectionMode::Any
, he had some finer-grained flag initially.I'm not sure I understand what situations this hack can produce invalid results in.
Remember that trans doesn't just look for things that are guaranteed to succeed, but also has to be able to correctly tell if something would succeed (for various reasons pointed out in a comment above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb
Sometimes yes. It depends on context. This is why I was imagining a filter that the context can provide. =)
Certainly the majority of cases though trans is doing a lot more work than it ought to, just re-resolving things that it knows will pass (and which sometimes yield ICEs when they stumble over other bugs, as in this case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to try to make more progress on this, but I also want to make sure I understand the conversation here.
In particular, @nikomatsakis, when you wrote above that you expected a filter to be attached to the
FulfillmentContext
, did you mean that, or did you meanSelectionContext
?(I ask because I can at least see how a
SelectionContext
would connect up to the code as written, but attaching it to theFulfillmentContext
seems like ... its leaving out details about how that filter would actually prevent this particular confirmation while not messing things up elsewhere. I have no intuition right now as to where the filter fundamentally belongs)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for those following along this thread, niko left a comment below saying "I meant FulfillmentContext, but I may be incorrect.")