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 |
|
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 |
| } | ||
|
|
||
| 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
There was a problem hiding this comment.
Yeah, it seems check_potentially_region_dependent_goals is currently not exercised, since we silently ignore ambiguity.
But with #164055 , it'll be used.
There was a problem hiding this comment.
We won't have duplicate errors but the control flow is a bit convoluted. Could you comment in this and_then closure that this is to report non-region dependent errors, or specifically for the FallableCustomTypeOp?
Can Move obligations have nested goals that're region dependent? Then we'll have duplicate errors.
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
a96573c to
c5ec872
Compare
|
@rustbot review |
This comment has been minimized.
This comment has been minimized.
c5ec872 to
616cf33
Compare
There was a problem hiding this comment.
Thank you for added comments!
Now I get the motivation is to report Move trait fulfillment errors in borrowck and those are not related to region uniquification.
This seems to violate the invariant that we shall not have obligation errors in borrowck if HIR typeck succeeds. But if it's what the Move design requires, then no problem from me.
|
We cannot know these bounds earlier, since it depends on whether we move a value, which we only know in mir. |
616cf33 to
4400802
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. |
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