add a FCW for overflow errors with the next solver#159224
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[WIP] add FCW for overflow with the next solver
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (6eb68d9): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.1%, secondary -0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -4.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 487.42s -> 490.943s (0.72%) |
725357d to
4a67928
Compare
This comment has been minimized.
This comment has been minimized.
4a67928 to
7640bb3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7640bb3 to
a69af77
Compare
This comment has been minimized.
This comment has been minimized.
a69af77 to
20e6271
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
20e6271 to
d817881
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
I don't like next_trait_solver_overflow as a lint name 🤔 it feels too tied to the implementation. exceeded_recursion_limit or sth feels better
I think we should also mention the predicate that caused overflow, to allow the user to fix the overflow by e.g. manually implementing Send for one of their types
| self.evaluate_root_goal_for_proof_tree_raw((canonical_goal, root_depth)) | ||
| } | ||
|
|
||
| fn emit_next_solver_overflow_fcw(self, span: Option<Span>) { |
There was a problem hiding this comment.
take a Span and supply a DUMMY_SP in the caller where necessary
| fn emit_next_solver_overflow_fcw(self, span: Option<Span>) { | ||
| self.emit_node_span_lint( | ||
| rustc_session::lint::builtin::NEXT_TRAIT_SOLVER_OVERFLOW, | ||
| CRATE_HIR_ID, |
There was a problem hiding this comment.
ah yeah, I guess that works 👍
we should somehow adjust the error message to say that this lint is always attached to the whole crate and can't be disabled on a per function basis
| let eval_with_recursion_limit = |limit| { | ||
| EvalCtxt::enter_root(self, limit, span, |ecx| { | ||
| ecx.evaluate_goal_no_fast_paths(GoalSource::Misc, goal) | ||
| }) | ||
| }; | ||
| let is_overflow_and_has_no_stalled_infers = |eval_result: &Result<GoalEvaluation<I>, _>| { | ||
| let predicate = match eval_result { | ||
| Err(_) => return false, | ||
| Ok(goal_evaluation) if !goal_evaluation.certainty.is_overflow() => return false, | ||
| Ok(goal_evaluation) => goal_evaluation.goal.predicate, | ||
| }; | ||
|
|
||
| let has_no_stalled_infers = match predicate.kind().skip_binder() { | ||
| ty::PredicateKind::Clause(ty::ClauseKind::Projection(projection)) => { | ||
| !projection.projection_term.has_non_region_infer() | ||
| } | ||
| _ => !predicate.has_non_region_infer(), | ||
| }; | ||
| has_no_stalled_infers | ||
| }; | ||
|
|
||
| // The old solver doesn't check depth requirement when looking up cache | ||
| // while the next solver does so. Thus the next solver is more prone to | ||
| // overflow. We emit a FCW for this. | ||
| let mut result = eval_with_recursion_limit(self.cx().recursion_limit()); | ||
| if is_overflow_and_has_no_stalled_infers(&result) { | ||
| let new_result = eval_with_recursion_limit(self.cx().recursion_limit() * 2); | ||
| if let Ok(goal_evaluation) = &new_result | ||
| && goal_evaluation.certainty.is_yes() | ||
| { | ||
| self.cx().emit_next_solver_overflow_fcw(Some(span)); | ||
| result = new_result; | ||
| } | ||
| } |
There was a problem hiding this comment.
can you move this into a separate functions, e.g. is_overflow_and_has_no_stalled_infer can probably be shared between the call sites
can you do the reevaluation in a commit_if_ok so that we don't apply the inference etc constraints of the reevaluation while ignoring its result?
|
|
||
| // The old solver doesn't check depth requirement when looking up cache | ||
| // while the next solver does so. Thus the next solver is more prone to | ||
| // overflow. We emit a FCW for this. |
There was a problem hiding this comment.
link to an issue/the lint track issue/somewhere here
|
ah, and please limit this to not apply in we should do a next solver crater run with this to see how many crates are impacted by this |
The old solver doesn't check cache entry's depth requirement when looking up cache while the next solver correctly does so. It's necessary to behave correctly with respect to incremental compilation.
As a result, the next solver is more likely to run into overflow errors.
We try to catch such breakages by rerun the evaluation with doubled recursion limit and if that succeeds we emit a FCW.
This PR is being discussed on zulip
r? @lcnr