Repository navigation
Fallible type ops - #165335
Fallible type ops#165335jdonszelmann wants to merge 1 commit into
Conversation
|
Some changes occurred in engine.rs, potentially modifying the public API of cc @lcnr |
rustbot
added
S-waiting-on-review
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @lcnr or anyone in @rust-lang/initiative-trait-system-refactor |
|
r? @rust-lang/initiative-trait-system-refactor (lcnr isn't on rotation right now, confirmed with them) |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
r? project-trait-system-refactor |
|
|
||
| /// Certain proofs (e.g. `Move`) may error during MIR typeck, so handle them separately. | ||
| #[allow(unused)] | ||
| pub(super) fn prove_fallible_predicate( |
There was a problem hiding this comment.
How is this different from prove_clause above? 🤔
There was a problem hiding this comment.
that it uses FallibleTypeOp internally, which can return errors that are reported
There was a problem hiding this comment.
importantly, prove_clause is effectively an assertion: it produces a span_delayed_bug if the clause does not hold i.e. because it wasn't rejected earlier. Fallible ops can cause new errors unrelated to what is or isn't already proven. Let me add a comment about that.
| } | ||
|
|
||
| impl<'tcx> EngineError<'tcx> for ScrubbedTraitError<'tcx> { | ||
| fn try_report_errors(_infcx: &InferCtxt<'tcx>, _errors: ThinVec<Self>) {} |
There was a problem hiding this comment.
Surprising that try_report_errors doesn't actually report anything?
There was a problem hiding this comment.
it doesn't in this impl of EngineError but it does for FulfullmentError. I've left a comment in the source.
| let errors = ocx.evaluate_obligations_error_on_ambiguity(); | ||
| match errors { | ||
| TraitErrors::HasErrors(errors) => { | ||
| E::try_report_errors(infcx, errors); |
There was a problem hiding this comment.
We would duplicate the reporting? try_report_errors and check_potentially_region_dependent_goals can report similar errors 🤔
There was a problem hiding this comment.
when check_potentially_region_dependent_goals emits errors, it returnsErr(). I have not been able to find a single case in which this actually happens, either manually or in the test suite. I think it's a separate concern that maybe, since that's highly dubious....
There was a problem hiding this comment.
it probably needs a bug!() or something, or potentially we can just remove this entire call
There was a problem hiding this comment.
but at least I don't think we can get duplicate errors
| /// Processes the operation and all resulting obligations, | ||
| /// returning the final result along with any region constraints | ||
| /// (they will be given over to the NLL region solver). | ||
| fn fully_perform( |
There was a problem hiding this comment.
How is the new TypeOp different from CustomTypeOp except that it uses FulfillmentError?
There was a problem hiding this comment.
Well, that is quite a big difference. FulfillmentError reports its errors
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
a96573c to
c5ec872
Compare
|
@rustbot review |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
c5ec872 to
616cf33
Compare
View all comments
This is a single commit extracted from #156018 and #161457. It adds a currently unused api to mir typeck, that's required to implement the move trait, but is on its own also useful for other changes like resolving all cases of #132279
By extracting it, we can lower the review burden on the other PRs
cc: @zannabianca1997