-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[WIP] Fix evaluation overflow #53687
[WIP] Fix evaluation overflow #53687
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@nikomatsakis I'll need your help on this one (@eddyb said you were the one to ask for) |
@@ -175,7 +175,7 @@ impl<'a, 'tcx> Lift<'tcx> for traits::SelectionError<'a> { | |||
super::ConstEvalFailure(ref err) => tcx.lift(&**err).map(|err| super::ConstEvalFailure( | |||
err.into(), | |||
)), | |||
super::Overflow => bug!(), // FIXME: ape ConstEvalFailure? |
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, I'm .. not entirely sure why this was bug!
to begin with. It seems like it could just be Some(super::Overflow)
, instead?
a72286a
to
632f9c4
Compare
@nikomatsakis I made the replacement, and as expected: it still fails at the same place but with the following error message:
|
hmm. |
Ping @nikomatsakis @GuillaumeGomez! 1.29 is going to be released in a bit more than a week! |
Still waiting for @nikomatsakis at last news. Don't know if he had time to look at it. |
@GuillaumeGomez I was waiting on you to minimize a test case =) |
Seems like we miscommunicated! |
Woups, indeed, sorry about that! |
@nikomatsakis After a long try, it seems that it comes more from the number of possibilities than anything specific. With this small sample of code, it works as expected: use std::marker::PhantomData;
pub trait Unsigned {}
pub trait PrivatePow<Y, N> {
type Output;
}
pub type PrivatePowOut<A, Y, N> = <A as PrivatePow<Y, N>>::Output;
pub trait Pow<N> {
type Output;
fn powi(self, x: N) -> Self::Output;
}
pub struct UTerm;
pub struct B1;
pub struct UInt<U, B> {
_marker: PhantomData<(U, B)>,
}
pub type U1 = UInt<UTerm, B1>;
impl<X: Unsigned, N: Unsigned> Pow<N> for X
where
X: PrivatePow<U1, N>,
{
type Output = PrivatePowOut<X, U1, N>;
fn powi(self, _: N) -> Self::Output {
unsafe { ::core::mem::uninitialized() }
}
} Adding a few more types matching the blanket impl will certainly allow to get the ICE again but couldn't find how much exactly... |
FYI, beta->stable promotion should happen on Monday, so if we want to get the regression fixed we need to ideally get this landed on master and approved for backport by Sunday. |
@nikomatsakis @GuillaumeGomez @rust-lang/rustdoc This is highly likely to slip into the 1.29 release at this point unless we can get an implementation and review in the next ~12-15 hours. Is that acceptable? I'm not sure how serious this issue is. |
If the bug is happening while evaluating blanket impls, the solution for beta is to disable blanket impls. That buys us another release to get this sorted properly. |
I will likely go to sleep before my build can finish, so if someone wants to beat me to it, blanket impls can be disabled on beta by commenting/removing this line (and possibly the related |
Sorry for my inattention here. I'm still feeling uncertain about the changes here. I don't really understand what is happening. I was also unable to minimize. I will investigate for a few hours today and make a call one way or the other on what to do. |
Backporting a quick-n-dirty patch to fix up rustdoc may be fine, though. |
discussed at compiler team meeting. Decided we are unlikely to accept this PR into beta even if it is r+'ed in its current state. So, removing beta-nomination tag. If a reviewer decides that some future form of the PR is worth considering for backport, please renominate. |
(If that's what you were suggesting @QuietMisdreavus) |
This was already backported in #54104, FYI. |
@pietroalbini called it, I wrote the "quick and dirty" patch for beta and it was able to get it merged in before it was promoted to today's release. |
To be clear, what was backported is not a tentative fix, but rather disabling the feature that still needs to be fixed, so people don't get ICEs, the feature is just "delayed" for another release cycle? |
@eddyb Correct. As the feature had not landed on stable yet, i "delayed its introduction" by removing it from the beta branch as it was being promoted to stable. |
Seems good. |
Closing in favor of #54199 |
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
…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
Fixes #52873.
Currently, it fails to evaluate this code. By replacing this line, instead of a bug we get a nice infinite recursion:
So I assume the simplification is failing for some reasons in predicate_may_hold.
cc @eddyb @nikomatsakis