Gracefully handle overflow errors in impl rematching #122539
Conversation
rustbot
added
S-waiting-on-review
| Ok(term) => term.ty().unwrap(), | ||
| Err(oflo) => { | ||
| self.overflowed = Err(oflo); | ||
| return ty; |
There was a problem hiding this comment.
slight behaviour change, before this PR the normalize_projection_type would replace the entire projection with an inference variable and set up the correct obligations. I can replicate the previous behaviour, but afaict we're in a doomed normalization anyway.
| } | ||
| Err(ProjectionNormalizationFailure::Overflow(oflo)) => { | ||
| self.overflowed = Err(oflo); | ||
| return ty; |
There was a problem hiding this comment.
we don't even bother to recurse with super_fold_with, could probably do that, as it's what callers expect.
| @@ -925,6 +925,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | |||
| // since we don't actually use them. | |||
| &mut vec![], | |||
| ) | |||
| .ok()? | |||
There was a problem hiding this comment.
if normalization overflowed, we will not do the migration. To combine both infallible overflowing normalization and this migration, some truly cursed shenanigans are necessary, if possible at all.
| @@ -40,7 +40,8 @@ fn normalize_canonicalized_projection_ty<'tcx>( | |||
| cause, | |||
| 0, | |||
| &mut obligations, | |||
| ); | |||
| ) | |||
| .map_err(|_| NoSolution)?; | |||
There was a problem hiding this comment.
The overflow would have errored out below anyway, causing NoSolution, so we're just doing this early now
| ReservationImpl, | ||
| #[allow(dead_code)] | ||
| TypeError(TypeError<'tcx>), | ||
| #[allow(dead_code)] |
|
I believe this to be necessary because While your fix tampers over this, imo the underlying issue is not fixeable. E.g. the change in idk, I am annoyed by this and would like to just keep the ICE around but do accept that apart from your approach, there's nothing feasible we can do here. Disabling the cache when hitting the |
|
Let's keep the ICE around. There are useful errors in addition to the ICE and the new solver fixes it correctly |
fixes #122529
I believe the second commit is entirely sound, because all we're doing is turning an ICE into a hard error.
The first commit is where we now enable more situations to have non-fatal overflow. There are multiple fishy things, that I will leave comments on directly in the diff.
r? @lcnr