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

Fix Rustdoc ICE when checking blanket impls #55258

Merged
merged 1 commit into from
Oct 26, 2018
Merged
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
10 changes: 9 additions & 1 deletion src/librustc/infer/combine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ impl<'infcx, 'gcx, 'tcx> CombineFields<'infcx, 'gcx, 'tcx> {
dir: RelationDir)
-> RelateResult<'tcx, Generalization<'tcx>>
{
debug!("generalize(ty={:?}, for_vid={:?}, dir={:?}", ty, for_vid, dir);
// Determine the ambient variance within which `ty` appears.
// The surrounding equation is:
//
Expand All @@ -273,8 +274,15 @@ impl<'infcx, 'gcx, 'tcx> CombineFields<'infcx, 'gcx, 'tcx> {
root_ty: ty,
};

let ty = generalize.relate(&ty, &ty)?;
let ty = match generalize.relate(&ty, &ty) {
Ok(ty) => ty,
Err(e) => {
debug!("generalize: failure {:?}", e);
return Err(e);
}
};
let needs_wf = generalize.needs_wf;
debug!("generalize: success {{ {:?}, {:?} }}", ty, needs_wf);
Ok(Generalization { ty, needs_wf })
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/infer/equate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ impl<'combine, 'infcx, 'gcx, 'tcx> TypeRelation<'infcx, 'gcx, 'tcx>
let infcx = self.fields.infcx;
let a = infcx.type_variables.borrow_mut().replace_if_possible(a);
let b = infcx.type_variables.borrow_mut().replace_if_possible(b);

debug!("{}.tys: replacements ({:?}, {:?})", self.tag(), a, b);

match (&a.sty, &b.sty) {
(&ty::Infer(TyVar(a_id)), &ty::Infer(TyVar(b_id))) => {
infcx.type_variables.borrow_mut().equate(a_id, b_id);
Expand Down
35 changes: 35 additions & 0 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1522,6 +1522,33 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
.map(|v| v.get(tcx))
}

/// Determines whether can we safely cache the result
/// of selecting an obligation. This is almost always 'true',
/// except when dealing with certain ParamCandidates.
///
/// Ordinarily, a ParamCandidate will contain no inference variables,
/// since it was usually produced directly from a DefId. However,
/// certain cases (currently only librustdoc's blanket impl finder),
/// a ParamEnv may be explicitly constructed with inference types.
/// When this is the case, we do *not* want to cache the resulting selection
/// candidate. This is due to the fact that it might not always be possible
/// to equate the obligation's trait ref and the candidate's trait ref,
/// if more constraints end up getting added to an inference variable.
///
/// Because of this, we always want to re-run the full selection
/// process for our obligation the next time we see it, since
/// we might end up picking a different SelectionCandidate (or none at all)
fn can_cache_candidate(&self,
result: &SelectionResult<'tcx, SelectionCandidate<'tcx>>
) -> bool {
match result {
Ok(Some(SelectionCandidate::ParamCandidate(trait_ref))) => {
!trait_ref.skip_binder().input_types().any(|t| t.walk().any(|t_| t_.is_ty_infer()))
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to trait_ref.needs_infer(), but potentially much slower (since methods like needs_infer rely on precomputed flags), I wonder how much of a perf regression this is.

Both TraitRef::input_types and Ty::walk are warning flags, we might want to audit their uses (and make Ty::walk more annoying to use, probably).

cc @nikomatsakis @pnkfelix @matthewjasper

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, @eddyb is correct, this does seem slower (I hadn't noticed this PR at the time, though I agree also with @arielb1's comment that it's a bit surprising to see inference variables in the param env, though I suppose there may be some path by which it can occur -- and eventually it'll be true)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I don't think I agree that input_types is a warning flag, though -- walk maybe is..?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, should we open an issue to try converting this to use the needs_infer call?

Copy link
Member

Choose a reason for hiding this comment

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

It's included in #69294 (comment), I haven't had the chance to split that into its own PR for perf testing (presumably we can just land it with rollup=never and see the results there. it can't be slower than walk-ing, anyway).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I agree that input_types is a warning flag

Isn't it a leftover from the days of "output" modes on parameters? Like before associated types fully became a thing?

Also it's going to get really bad with const generics, so @varkor or @yodaldevoid would probably have to remove input_types just to make its callsites correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a leftover from the days of "output" modes on parameters? Like before associated types fully became a thing?

It seems like it just screens out lifetime parameters. I guess you're right that there is probably generally little reason to do this.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like it just screens out lifetime parameters

And const parameters 😨 (presumably), hence my comments on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. And I suppose it's probably just wrong to ignore needs-infer in lifetime parameters anyway. OK, I agree it's probably a 'red flag' of some kind.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it seems like you're going to fix this in #69294, so that's good.

},
_ => true
}
}

fn insert_candidate_cache(
&mut self,
param_env: ty::ParamEnv<'tcx>,
Expand All @@ -1531,6 +1558,14 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
) {
let tcx = self.tcx();
let trait_ref = cache_fresh_trait_pred.skip_binder().trait_ref;

if !self.can_cache_candidate(&candidate) {
debug!("insert_candidate_cache(trait_ref={:?}, candidate={:?} -\
candidate is not cacheable", trait_ref, candidate);
return;

}

if self.can_use_global_caches(param_env) {
if let Err(Overflow) = candidate {
// Don't cache overflow globally; we only produce this
Expand Down
11 changes: 9 additions & 2 deletions src/librustdoc/clean/blanket_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ impl<'a, 'tcx, 'rcx, 'cstore> BlanketImplFinder <'a, 'tcx, 'rcx, 'cstore> {
name: Option<String>,
) -> Vec<Item>
where F: Fn(DefId) -> Def {
debug!("get_blanket_impls(def_id={:?}, ...)", def_id);
let mut impls = Vec::new();
if self.cx
.tcx
Expand Down Expand Up @@ -78,6 +79,8 @@ impl<'a, 'tcx, 'rcx, 'cstore> BlanketImplFinder <'a, 'tcx, 'rcx, 'cstore> {
}
self.cx.tcx.for_each_relevant_impl(trait_def_id, ty, |impl_def_id| {
self.cx.tcx.infer_ctxt().enter(|infcx| {
debug!("get_blanet_impls: Considering impl for trait '{:?}' {:?}",
trait_def_id, impl_def_id);
let t_generics = infcx.tcx.generics_of(impl_def_id);
let trait_ref = infcx.tcx.impl_trait_ref(impl_def_id)
.expect("Cannot get impl trait");
Expand All @@ -104,8 +107,8 @@ impl<'a, 'tcx, 'rcx, 'cstore> BlanketImplFinder <'a, 'tcx, 'rcx, 'cstore> {
drop(obligations);

debug!(
"invoking predicate_may_hold: {:?}",
trait_ref,
"invoking predicate_may_hold: param_env={:?}, trait_ref={:?}, ty={:?}",
param_env, trait_ref, ty
);
let may_apply = match infcx.evaluate_obligation(
&traits::Obligation::new(
Expand All @@ -117,6 +120,10 @@ impl<'a, 'tcx, 'rcx, 'cstore> BlanketImplFinder <'a, 'tcx, 'rcx, 'cstore> {
Ok(eval_result) => eval_result.may_apply(),
Err(traits::OverflowError) => true, // overflow doesn't mean yes *or* no
};
debug!("get_blanket_impls: found applicable impl: {}\
for trait_ref={:?}, ty={:?}",
may_apply, trait_ref, ty);

if !may_apply {
return
}
Expand Down
31 changes: 31 additions & 0 deletions src/test/rustdoc/issue-55001.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Regression test for issue #55001. Previously, we would incorrectly
// cache certain trait selection results when checking for blanket impls,
// resulting in an ICE when we tried to confirm the cached ParamCandidate
// against an obligation.

pub struct DefaultAllocator;
pub struct Standard;
pub struct Inner;

pub trait Rand {}

pub trait Distribution<T> {}
pub trait Allocator<N> {}

impl<T> Rand for T where Standard: Distribution<T> {}

impl<A> Distribution<Point<A>> for Standard
where
DefaultAllocator: Allocator<A>,
Standard: Distribution<A> {}

impl Distribution<Inner> for Standard {}


pub struct Point<N>
where DefaultAllocator: Allocator<N>
{
field: N
}

fn main() {}