-
Notifications
You must be signed in to change notification settings - Fork 13.8k
TransmuteFrom: Gracefully handle unnormalized types and normalization errors #131112
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
Conversation
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.
So I think this PR is overengineered 😅 I left some inline comments, but realized that they're kinda ignoring the fact that this isn't the right general fix, I think.
Factoring out the confirmation code may be useful, but right now it's tied together with unnecessary/incorrect normalization calls which mask the real issue here:
The correct fix is just to add checks in get_safe_transmute_error_and_reason for the invariants that the transmutability code currently relies on. Specifically, we should be checking the same things as we do in assemble_candidates_for_transmutability -- i.e.
if obligation.predicate.has_non_region_param()
since right now, the transmutability code doesn't handle type/const parameters, and
if obligation.has_non_region_infer()
which implies ambiguity.
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.
Making a new inference context is definitely not the right approach.
Also unrelated, I believe the transmutability code should be resilient to NormalizationFailure layout errors. This PR doesn't, afaict, fix this ICE:
#![feature(transmutability)] trait A { type AssocA; } trait B { type AssocB: std::mem::TransmuteFrom<()>; } impl<T> B for T where for<'a> &'a i32: A, { type AssocB = <&'static i32 as A>::AssocA; }
This comment has been minimized.
This comment has been minimized.
eaee913 to
5b1a2b8
Compare
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.
Could use a comment
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.
I'm personally fine to punt on making this error message prettier — my top priority is making TransmuteFrom work — but happy to try to improve this further if you'd like to block the PR on it.
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.
No, it's fine. But yeah, it could be customized to say something like "cannot transmute types with ty/ct params in it".
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.
But happy to do that later.
@compiler-errors, you're right, that was a lot more simple: https://github.com/rust-lang/rust/pull/131112/files
TransmuteFrom: normalize types, unify confirmation and error reporting (削除ここまで)Also please run ./x.py test crashes, you need to remove the relevant crashes tests.
This comment has been minimized.
This comment has been minimized.
@rustbot author
...iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131024 (Don't give method suggestions when method probe fails due to bad implementation of `Deref`) - rust-lang#131112 (TransmuteFrom: Gracefully handle unnormalized types and normalization errors) - rust-lang#131176 (.gitignore files for nix) - rust-lang#131183 (Refactoring to `OpaqueTyOrigin`) - rust-lang#131187 (Avoid ICE in coverage builds with bad `#[coverage(..)]` attributes) - rust-lang#131192 (Handle `rustc_query_impl` cases of `rustc::potential_query_instability` lint) - rust-lang#131197 (Avoid emptiness check in `PeekMut::pop`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131112 - jswrenn:fix-130413, r=compiler-errors TransmuteFrom: Gracefully handle unnormalized types and normalization errors ~~Refactor to share code between `TransmuteFrom`'s trait selection and error reporting code paths. Additionally normalizes the source and destination types, and gracefully handles normalization errors.~~ Fixes rust-lang#130413 r? `@compiler-errors`
Uh oh!
There was an error while loading. Please reload this page.
(削除) Refactor to share code betweenTransmuteFrom's trait selection and error reporting code paths. Additionally normalizes the source and destination types, and gracefully handles normalization errors. (削除ここまで)Fixes #130413
r? @compiler-errors