Consolidate and improve error messaging for CoerceUnsized and DispatchFromDyn
#137289
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
compiler-errors
force-pushed
the
coerce-unsized-errors
branch
2 times, most recently
from
a72f80f to
ba4f02f
Compare
compiler-errors
commented
Feb 19, 2025
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.
oli-obk
approved these changes
Feb 20, 2025
compiler-errors
force-pushed
the
coerce-unsized-errors
branch
from
ba4f02f to
1c44fb3
Compare
|
@oli-obk is this r=you or is it waiting on another review? |
compiler-errors
force-pushed
the
coerce-unsized-errors
branch
from
1c44fb3 to
b46acc0
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 25, 2025
…mpiler-errors Rollup of 11 pull requests Successful merges: - rust-lang#136522 (Remove `feature(dyn_compatible_for_dispatch)` from the compiler) - rust-lang#137289 (Consolidate and improve error messaging for `CoerceUnsized` and `DispatchFromDyn`) - rust-lang#137321 (Correct doc about `temp_dir()` behavior on Android) - rust-lang#137417 (rustc_target: Add more RISC-V atomic-related features) - rust-lang#137489 (remove `#[rustc_intrinsic_must_be_overridde]`) - rust-lang#137530 (DWARF mixed versions with LTO on MIPS) - rust-lang#137543 (std: Fix another new symlink test on Windows) - rust-lang#137548 (Pass correct `TypingEnv` to `InlineAsmCtxt`) - rust-lang#137550 (Don't immediately panic if dropck fails without returning errors) - rust-lang#137552 (Update books) - rust-lang#137556 (rename simd_shuffle_generic → simd_shuffle_const_generic) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 25, 2025
Rollup merge of rust-lang#137289 - compiler-errors:coerce-unsized-errors, r=oli-obk Consolidate and improve error messaging for `CoerceUnsized` and `DispatchFromDyn` Firstly, this PR consolidates and reworks the error diagnostics for `CoercePointee` and `DispatchFromDyn`. 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 `__S` dummy parameter for `derive(CoercePointee)`. Thirdly, emit a custom error message when we encounter a trait error that comes from the recursive field `CoerceUnsized`/`DispatchFromDyn` trait 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 `DispatchFromDyn` validity errors if we detect `CoerceUnsized` validity 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 rust-lang#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.
90 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.