Safe Transmute: Pass ErrorGuaranteed up to error reporting #110669
Conversation
This patch updates the `Answer` enum used by Safe Transmute to have a variant for errors, and includes the `ErrorGuaranteed` type so that we know that an error was already emitted. Before this change, we would return `Answer::Yes` for failures, which gives the same end-result, but is confusing because `Answer::Yes` is supposed to mean that safe transmutation is possible. Now, it's clear when we have an error, and we can decide what to do with it during error reporting. Also, we now know for sure that an error was emitted for `LayoutError` (previously, we just assumed without confirming).
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
| TypeError(ErrorGuaranteed), | ||
| } | ||
|
|
||
| impl<'tcx> From<LayoutError<'tcx>> for Err { | ||
| fn from(err: LayoutError<'tcx>) -> Self { | ||
| let guar = rustc_middle::ty::tls::expect_compilation_to_fail(); |
There was a problem hiding this comment.
A layout error does not imply that compilation is going to fail. This can happen when the type is sufficiently polymorphic, such as &T where T: ?Sized.
There was a problem hiding this comment.
This would ICE, for example, when we relax this requirement:
rust/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs
Lines 778 to 780 in 80a2ec4
There was a problem hiding this comment.
If I'm understanding correctly, you're saying that a layout error may or may not cause compilation to fail? If so, I can change it to Unknown(Option<ErrorGuaranteed>), and use if let Some(guar) = with(|tcx| tcx.sess.is_compilation_going_to_fail()) to decide what to return in this From impl
There was a problem hiding this comment.
No, I don't really think that relying on global state such as that for trait solver behavior is a good idea.
There was a problem hiding this comment.
Gotcha, I can undo the changes in this section then
| (Err(Err::TypeError(_)), _) => Err(Answer::Yes), | ||
| (_, Err(Err::TypeError(_))) => Err(Answer::Yes), | ||
| (Err(Err::Unknown), _) => Err(Answer::Yes), | ||
| (_, Err(Err::Unknown)) => Err(Answer::Yes), |
There was a problem hiding this comment.
Why not just return No here? If this is just to not annoy users with errors, we should be doing diagnostic deduplication on the error reporting side.
There was a problem hiding this comment.
Yeah, I added deduplication here
rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Lines 743 to 754 in 0d74752
Is that what you mean?
The reason I made a separate Answer variant instead of reusing Answer::No is cause there is a semantic difference between safe transmute not being possible (Answer::No) and not knowing if safe transmute is possible because of an error that occurred when trying to get an answer (Answer::Err).
There was a problem hiding this comment.
Well in the case of a layout error, it seems like you should be modeling this case with an additional variant that signifies "unknown".
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #110789) made this pull request unmergeable. Please resolve the merge conflicts. |
|
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks! FYI: when a PR is ready for review, send a message containing |
|
I'm gonna close since I integrated some of these changes into other PRs that I merged. No longer needed. |
This patch updates the
Answerenum used by Safe Transmute to have a variant for errors, and includes theErrorGuaranteedtype so that we know that an error was already emitted. Before this change, we would returnAnswer::Yesfor failures, which gives the same end-result, but is confusing becauseAnswer::Yesis supposed to mean that safe transmutation is possible. Now, it's clear when we have an error, and we can decide what to do with it during error reporting. Also, we now know for sure that an error was emitted forLayoutError(previously, we just assumed without confirming).