-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Safe Transmute: Pass ErrorGuaranteed up to error reporting #110669
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -75,12 +75,10 @@ mod rustc { | |||||||||||||||||||||||||
let dst = Tree::from_ty(dst, context); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
match (src, dst) { | ||||||||||||||||||||||||||
// Answer `Yes` here, because 'unknown layout' and type errors will already | ||||||||||||||||||||||||||
// be reported by rustc. No need to spam the user with more errors. | ||||||||||||||||||||||||||
(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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well in the case of a layout error, it seems like you should be modeling this case with an additional variant that signifies "unknown". |
||||||||||||||||||||||||||
(Err(Err::TypeError(guar)), _) => Err(Answer::Err(guar)), | ||||||||||||||||||||||||||
(_, Err(Err::TypeError(guar))) => Err(Answer::Err(guar)), | ||||||||||||||||||||||||||
(Err(Err::Unknown(guar)), _) => Err(Answer::Err(guar)), | ||||||||||||||||||||||||||
(_, Err(Err::Unknown(guar))) => Err(Answer::Err(guar)), | ||||||||||||||||||||||||||
(Err(Err::Unspecified), _) => Err(Answer::No(Reason::SrcIsUnspecified)), | ||||||||||||||||||||||||||
(_, Err(Err::Unspecified)) => Err(Answer::No(Reason::DstIsUnspecified)), | ||||||||||||||||||||||||||
(Ok(src), Ok(dst)) => Ok((src, dst)), | ||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A layout error does not imply that compilation is going to fail. This can happen when the type is sufficiently polymorphic, such as
&T
whereT: ?Sized
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 useif let Some(guar) = with(|tcx| tcx.sess.is_compilation_going_to_fail())
to decide what to return in thisFrom
implThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I can undo the changes in this section then