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

overlook overflows in rustdoc trait solving #54199

Merged
merged 5 commits into from Sep 26, 2018

Conversation

@nikomatsakis
Copy link
Contributor

commented Sep 13, 2018

Context:

The new rustdoc "auto trait" feature walks across impls and tries to run trait solving on them with a lot of unconstrained variables. This is prone to overflows. These overflows used to cause an ICE because of a caching bug (fixed in this PR). But even once that is fixed, it means that rustdoc causes an overflow rather than generating docs.

This PR therefore adds a new helper that propagates the overflow error out. This requires rustdoc to then decide what to do when it encounters such an overflow: technically, an overflow represents neither "yes" nor "no", but rather a failure to make a decision. I've decided to opt on the side of treating this as "yes, implemented", since rustdoc already takes an optimistic view. This may prove to include too many items, but I suspect not.

We could probably reduce the rate of overflows by unifying more of the parameters from the impl -- right now we only seem to consider the self type. Moreover, in the future, as we transition to Chalk, overflow errors are expected to just "go away" (in some cases, though, queries might return an ambiguous result).

Fixes #52873

cc @QuietMisdreavus -- this is the stuff we were talking about earlier
cc @GuillaumeGomez -- this supersedes #53687

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2018

r? @eddyb

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

Er, dang it, I should try to make a test for this. Now that I know what's going wrong I may be able to do it.

@nikomatsakis nikomatsakis changed the title overlook overflows in rustdoc trait solving [WIP] overlook overflows in rustdoc trait solving Sep 13, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Oh this is perfect! Thanks a lot!

On rustdoc side, it'll be "easy". I already have something in mind: if we get an overflow, we just continue to check if there is an other matching trait, if not, then we consider this one was a matching one. How does that sound for you?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

On rustdoc side, it'll be "easy". I already have something in mind: if we get an overflow, we just continue to check if there is an other matching trait, if not, then we consider this one was a matching one. How does that sound for you?

I'm not sure I understand exactly what you are proposing. Do you mean "continue to look for any other matching impl"?

@eddyb

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

r=me with tests added

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

@nikomatsakis Yes I do! (sorry for the answer delay...)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Argh. So I spent a while trying to reproduce this error with a self-contained test case but I've failed thus far.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I guess I will start by paring down the existing failure.

@nikomatsakis nikomatsakis changed the title [WIP] overlook overflows in rustdoc trait solving overlook overflows in rustdoc trait solving Sep 25, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

Like I said: seems to appear starting a certain number of possibilities. Thanks a lot in any case!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

📌 Commit a3997f7 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

⌛️ Testing commit a3997f7 with merge 492e48d...

bors added a commit that referenced this pull request Sep 26, 2018
Auto merge of #54199 - nikomatsakis:predicate_may_hold-failure, r=eddyb
overlook overflows in rustdoc trait solving

Context:

The new rustdoc "auto trait" feature walks across impls and tries to run trait solving on them with a lot of unconstrained variables. This is prone to overflows. These overflows used to cause an ICE because of a caching bug (fixed in this PR). But even once that is fixed, it means that rustdoc causes an overflow rather than generating docs.

This PR therefore adds a new helper that propagates the overflow error out. This requires rustdoc to then decide what to do when it encounters such an overflow: technically, an overflow represents neither "yes" nor "no", but rather a failure to make a decision. I've decided to opt on the side of treating this as "yes, implemented", since rustdoc already takes an optimistic view. This may prove to include too many items, but I *suspect* not.

We could probably reduce the rate of overflows by unifying more of the parameters from the impl -- right now we only seem to consider the self type. Moreover, in the future, as we transition to Chalk, overflow errors are expected to just "go away" (in some cases, though, queries might return an ambiguous result).

Fixes #52873

cc @QuietMisdreavus -- this is the stuff we were talking about earlier
cc @GuillaumeGomez -- this supersedes #53687
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

💔 Test failed - status-travis

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

⌛️ Testing commit a3997f7 with merge e783d2b...

bors added a commit that referenced this pull request Sep 26, 2018
Auto merge of #54199 - nikomatsakis:predicate_may_hold-failure, r=eddyb
overlook overflows in rustdoc trait solving

Context:

The new rustdoc "auto trait" feature walks across impls and tries to run trait solving on them with a lot of unconstrained variables. This is prone to overflows. These overflows used to cause an ICE because of a caching bug (fixed in this PR). But even once that is fixed, it means that rustdoc causes an overflow rather than generating docs.

This PR therefore adds a new helper that propagates the overflow error out. This requires rustdoc to then decide what to do when it encounters such an overflow: technically, an overflow represents neither "yes" nor "no", but rather a failure to make a decision. I've decided to opt on the side of treating this as "yes, implemented", since rustdoc already takes an optimistic view. This may prove to include too many items, but I *suspect* not.

We could probably reduce the rate of overflows by unifying more of the parameters from the impl -- right now we only seem to consider the self type. Moreover, in the future, as we transition to Chalk, overflow errors are expected to just "go away" (in some cases, though, queries might return an ambiguous result).

Fixes #52873

cc @QuietMisdreavus -- this is the stuff we were talking about earlier
cc @GuillaumeGomez -- this supersedes #53687
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing e783d2b to master...

@bors bors merged commit a3997f7 into rust-lang:master Sep 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

The linked issue is still occurring on beta; should we backport this PR?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Didn't you disable blanket impls doc generation on beta? If so, it shouldn't be happening... Otherwise, it means the bug was not from there and that we should backport this pr and re-enable blanket impl doc generation.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

I disabled it in beta, right before it was promoted to stable. Then a new beta was created, so the problem still exists. See the discussion starting at #53687 (comment).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Weird...

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

@rust-lang/rustdoc Since this never got a beta backport, now that ICE is on stable. I've gotten a couple confirmations that 1.30.0 fails to build docs for typenum. If the release team decides to publish a point release, should we backport this?

@rfcbot poll [T-rustdoc] Stable backport?

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

If it helps the decision, someone was able to get this branch cleanly rebased onto the stable branch, so it should work out. stable...joelgallant:predicate_may_hold-failure

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Urf, trying to get the rfcbot poll to work. One last try, then i'm switching to manual ticky-boxes...

Here are some manual check-boxes for the rustdoc team, so we have some kind of official record-keeping of actually coming together for the decision. The question, once again, is Should we backport this to stable, if a point release is put together? Please check your box if you agree or comment in this issue with your thoughts.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

ping @rust-lang/rustdoc, i manually edited checkboxes into the previous comment

@pietroalbini

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

There seems to be consensus from the rustdoc team, marking this as stable-accepted.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Nov 7, 2018
Auto merge of rust-lang#54199 - nikomatsakis:predicate_may_hold-failu…
…re, r=eddyb

overlook overflows in rustdoc trait solving

Context:

The new rustdoc "auto trait" feature walks across impls and tries to run trait solving on them with a lot of unconstrained variables. This is prone to overflows. These overflows used to cause an ICE because of a caching bug (fixed in this PR). But even once that is fixed, it means that rustdoc causes an overflow rather than generating docs.

This PR therefore adds a new helper that propagates the overflow error out. This requires rustdoc to then decide what to do when it encounters such an overflow: technically, an overflow represents neither "yes" nor "no", but rather a failure to make a decision. I've decided to opt on the side of treating this as "yes, implemented", since rustdoc already takes an optimistic view. This may prove to include too many items, but I *suspect* not.

We could probably reduce the rate of overflows by unifying more of the parameters from the impl -- right now we only seem to consider the self type. Moreover, in the future, as we transition to Chalk, overflow errors are expected to just "go away" (in some cases, though, queries might return an ambiguous result).

Fixes rust-lang#52873

cc @QuietMisdreavus -- this is the stuff we were talking about earlier
cc @GuillaumeGomez -- this supersedes rust-lang#53687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.