Consolidate and improve error messaging for CoerceUnsized and DispatchFromDyn
#137289
Conversation
rustbot
added
S-waiting-on-review
compiler-errors
force-pushed
the
coerce-unsized-errors
branch
2 times, most recently
from
a72f80f to
ba4f02f
Compare
There was a problem hiding this comment.
The only major design choice I took with this is that I'm not hiding the names CoerceUnsized and DispatchFromDyn from the user when they encounter errors from derive(CoercePointee).
I think the fact that the derive expands into these two trait impls is an unavoidable fact, and tailoring the message to hide the "implementation detail" (which is not even really an implementation detail) requires unnecessary complexity and also obscures to more curious users about why they're encountering the issues they'd be encountering.
| @@ -1,4 +1,7 @@ | |||
| `CoerceUnsized` was implemented on something that isn't a struct. | |||
| #### Note: this error code is no longer emitted by the compiler. | |||
There was a problem hiding this comment.
E0376 was folded into E0377, since it's a bit strange to split out the error that would be issued for impl<T, U> CoerceUnsized<U> for W<T> and impl<T, U> CoerceUnsized<X<U>> for W<T>. The root cause of both is that they need to be of the same ADT.
| if !errors.is_empty() { | ||
| infcx.err_ctxt().report_fulfillment_errors(errors); | ||
| if is_from_coerce_pointee_derive(tcx, span) { | ||
| return Err(tcx.dcx().emit_err(errors::CoerceFieldValidity { |
There was a problem hiding this comment.
We could perhaps look at the fulfillment error to "drill down" a better reason for why this goal doesn't hold, but I'm still not totally clear what the wording here should even be in that case.
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
coerce-unsized-errors
branch
from
ba4f02f to
1c44fb3
Compare
Firstly, this PR consolidates and reworks the error diagnostics for
CoercePointeeandDispatchFromDyn. There was a ton of duplication for no reason -- this reworks both the errors and also the error codes, since they can be shared between both traits since they report the same thing.Secondly, when encountering a struct with multiple fields that must be coerced, point out the field spans, rather than mentioning the fields by name. This makes the error message clearer, but also means that we don't mention the
__Sdummy parameter forderive(CoercePointee).Thirdly, emit a custom error message when we encounter a trait error that comes from the recursive field
CoerceUnsized/DispatchFromDyntrait check. Note: This is the only one I'm not too satisfied with -- I think it could use some more refinement, but ideally it explains that the field must be an unsize-able pointer... Feedback welcome.Finally, don't emit
DispatchFromDynvalidity errors if we detectCoerceUnsizedvalidity errors from an impl of the same ADT.This is best reviewed per commit.
r? @oli-obk perhaps?
cc @dingxiangfei2009 -- sorry for making my own attempt at this PR, but I wanted to see if I could implement a fix for #136796 in a less complicated way, since communicating over github review comments can be a bit slow. I'll leave comments inline to explain my thinking about the diagnostics changes.