Fix Rustdoc ICE when checking blanket impls #55258

Aaron1011 · 2018-10-22T04:51:54Z

Previously, SelectionContext would unconditionally cache the selection
result for an obligation. This worked fine for most users of
SelectionContext, but it caused an issue when used by Rustdoc's blanket
impl finder.

The issue occured when SelectionContext chose a ParamCandidate which
contained inference variables. Since inference variables can change
between calls to select(), it's not safe to cache the selection result -
the chosen candidate might not be applicable for future results, leading
to an ICE when we try to run confirmation.

This commit prevents SelectionContext from caching any ParamCandidate
that contains inference variables. This should always be completely
safe, as trait selection should never depend on a particular result
being cached.

I've also added some extra debug!() statements, which I found helpful in
tracking down this bug.

rust-highfive · 2018-10-22T04:52:16Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Fixes rust-lang#55001, rust-lang#54744 Previously, SelectionContext would unconditionally cache the selection result for an obligation. This worked fine for most users of SelectionContext, but it caused an issue when used by Rustdoc's blanket impl finder. The issue occured when SelectionContext chose a ParamCandidate which contained inference variables. Since inference variables can change between calls to select(), it's not safe to cache the selection result - the chosen candidate might not be applicable for future results, leading to an ICE when we try to run confirmation. This commit prevents SelectionContext from caching any ParamCandidate that contains inference variables. This should always be completely safe, as trait selection should never depend on a particular result being cached. I've also added some extra debug!() statements, which I found helpful in tracking down this bug.

Aaron1011 · 2018-10-23T18:42:02Z

r? @GuillaumeGomez - they originally wrote the blanket impl finder code in #52585

GuillaumeGomez · 2018-10-23T21:16:12Z

This is awesome! Thanks a lot!

@bors: r+

bors · 2018-10-23T21:16:13Z

📌 Commit 4f2624c has been approved by GuillaumeGomez

…illaumeGomez Fix Rustdoc ICE when checking blanket impls Fixes rust-lang#55001, rust-lang#54744 Previously, SelectionContext would unconditionally cache the selection result for an obligation. This worked fine for most users of SelectionContext, but it caused an issue when used by Rustdoc's blanket impl finder. The issue occured when SelectionContext chose a ParamCandidate which contained inference variables. Since inference variables can change between calls to select(), it's not safe to cache the selection result - the chosen candidate might not be applicable for future results, leading to an ICE when we try to run confirmation. This commit prevents SelectionContext from caching any ParamCandidate that contains inference variables. This should always be completely safe, as trait selection should never depend on a particular result being cached. I've also added some extra debug!() statements, which I found helpful in tracking down this bug.

Rollup of 21 pull requests Successful merges: - #54816 (Don't try to promote already promoted out temporaries) - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`) - #54921 (Add line numbers option to rustdoc) - #55167 (Add a "cheap" mode for `compute_missing_ctors`.) - #55258 (Fix Rustdoc ICE when checking blanket impls) - #55264 (Compile the libstd we distribute with -Ccodegen-unit=1) - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators) - #55292 (Macro diagnostics tweaks) - #55298 (Point at macro definition when no rules expect token) - #55301 (List allowed tokens after macro fragments) - #55302 (Extend the impl_stable_hash_for! macro for miri.) - #55325 (Fix link to macros chapter) - #55343 (rustbuild: fix remap-debuginfo when building a release) - #55346 (Shrink `Statement`.) - #55358 (Remove redundant clone (2)) - #55370 (Update mailmap for estebank) - #55375 (Typo fixes in configure_cmake comments) - #55378 (rustbuild: use configured linker to build boostrap) - #55379 (validity: assert that unions are non-empty) - #55383 (Use `SmallVec` for the queue in `coerce_unsized`.) - #55391 (bootstrap: clean up a few clippy findings)

QuietMisdreavus · 2018-11-07T19:43:51Z

Nominating for beta backport, this fixes an ICE that is currently on stable. @rust-lang/compiler @rust-lang/rustdoc

estebank · 2018-11-07T19:49:48Z

Accepting for beta promotion.

arielb1 · 2018-11-10T21:24:27Z

Since when parameter environments with inference variables exist?

cc @nikomatsakis @rust-lang/wg-traits .

@nrc

beta backport rollup Backports of some beta-approved PRs - [x] #55385: NLL: cast causes failure to promote to static - [x] #56043: remove "approx env bounds" if we already know from trait - [x] #56003: do not propagate inferred bounds on trait objects if they involve `Self` - [x] #55852: Rewrite `...` as `..=` as a `MachineApplicable` 2018 idiom lint - [x] #55804: rustdoc: don't inline `pub use some_crate` unless directly asked to - [x] #56059: Increase `Duration` approximate equal threshold to 1us - [x] Keep resolved defs in path prefixes and emit them in save-analysis #54145 - [x] Adjust Ids of path segments in visibility modifiers #55487 - [x] save-analysis: bug fix and optimisation. #55521 - [x] save-analysis: be even more aggressive about ignorning macro-generated defs #55936 - [x] save-analysis: fallback to using path id #56060 - [x] save-analysis: Don't panic for macro-generated use globs #55879 - [x] Add temporary renames to manifests for rustfmt/clippy #56081 - [x] Revert #51601 #56049 - [x] Fix stability hole with `static _` #55983 - [x] #56077 - [x] Fix Rustdoc ICE when checking blanket impls #55258 - [x] Updated RELEASES.md for 1.31.0 #55678 - [x] ~~#56061~~ #56111 - [x] Stabilize `extern_crate_item_prelude` #56032 Still running tests locally, and I plan to backport @nrc's other PRs too (cc @petrochenkov -- thanks for the advice)

eddyb · 2020-02-24T14:52:05Z

src/librustc/traits/select.rs

+     ) -> 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()))


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

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)

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

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

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).

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.

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.

It seems like it just screens out lifetime parameters

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

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.

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

rust-highfive assigned eddyb Oct 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2018

Aaron1011 force-pushed the fix/rustdoc-blanket branch from 1d9d2bd to ace4af8 Compare October 22, 2018 05:31

Aaron1011 force-pushed the fix/rustdoc-blanket branch from ace4af8 to 4f2624c Compare October 22, 2018 23:40

rust-highfive assigned GuillaumeGomez and unassigned eddyb Oct 23, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2018

pietroalbini mentioned this pull request Oct 23, 2018

Rollup of 19 pull requests #55295

Closed

kennytm mentioned this pull request Oct 24, 2018

Rollup of 28 pull requests #55320

Closed

kennytm mentioned this pull request Oct 26, 2018

Rollup of 15 pull requests #55371

Closed

kennytm mentioned this pull request Oct 26, 2018

Rollup of 21 pull requests #55382

Merged

bors merged commit 4f2624c into rust-lang:master Oct 26, 2018

jeremystucki mentioned this pull request Oct 28, 2018

Build doc dependencies myelin-ai/myelin#181

Merged

jesskfullwood mentioned this pull request Oct 28, 2018

Rustc panicked at Box<Any>: impossible case reached #54002

Closed

pietroalbini mentioned this pull request Oct 29, 2018

[beta]: Prepare the 1.31.0 beta release #55405

Merged

memoryruins mentioned this pull request Oct 29, 2018

cargo-doc: compiler panicked at 'Box<Any>' impossible case reached #55489

Closed

QuietMisdreavus mentioned this pull request Nov 7, 2018

Cargo doc explodes with ICE (rest of the tests seem to work?) #55690

Closed

QuietMisdreavus added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 7, 2018

estebank added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Nov 7, 2018

pietroalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 20, 2018

nikomatsakis mentioned this pull request Nov 20, 2018

beta backport rollup #56102

Merged

20 tasks

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 20, 2018

eddyb reviewed Feb 24, 2020

View reviewed changes

eddyb mentioned this pull request Feb 24, 2020

[WIP] traits/select: use global vs per-infcx caches more uniformly. #69294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Rustdoc ICE when checking blanket impls #55258

Fix Rustdoc ICE when checking blanket impls #55258

Aaron1011 commented Oct 22, 2018

rust-highfive commented Oct 22, 2018

Aaron1011 commented Oct 23, 2018

GuillaumeGomez commented Oct 23, 2018

bors commented Oct 23, 2018

QuietMisdreavus commented Nov 7, 2018

estebank commented Nov 7, 2018

arielb1 commented Nov 10, 2018

eddyb Feb 24, 2020

nikomatsakis Feb 25, 2020

nikomatsakis Feb 25, 2020

nikomatsakis Feb 25, 2020

eddyb Feb 25, 2020

eddyb Feb 25, 2020

nikomatsakis Feb 25, 2020

eddyb Feb 25, 2020

nikomatsakis Feb 26, 2020

nikomatsakis Feb 26, 2020

Fix Rustdoc ICE when checking blanket impls #55258

Fix Rustdoc ICE when checking blanket impls #55258

Conversation

Aaron1011 commented Oct 22, 2018

rust-highfive commented Oct 22, 2018

Aaron1011 commented Oct 23, 2018

GuillaumeGomez commented Oct 23, 2018

bors commented Oct 23, 2018

QuietMisdreavus commented Nov 7, 2018

estebank commented Nov 7, 2018

arielb1 commented Nov 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment