-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix issue #78496 #79882
Fix issue #78496 #79882
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
r? @oli-obk |
yes, I think going with the conservative scheme is for the best. It's not just downcast projections. Deref and index projections have the same issue. @bors r+ |
📌 Commit 3812f70 has been approved by |
Fix issue rust-lang#78496 EarlyOtherwiseBranch finds MIR structures like: ``` bb0: { ... _2 = discriminant(X) ... switchInt(_2) -> [1_isize: bb1, otherwise: bb3] } bb1: { ... _3 = discriminant(Y) ... switchInt(_3) -> [1_isize: bb2, otherwise: bb3] } bb2: {...} bb3: {...} ``` And transforms them into something like: ``` bb0: { ... _2 = discriminant(X) _3 = discriminant(Y) _4 = Eq(_2, _3) switchInt(_4) -> [true: bb4, otherwise: bb3] } bb2: {...} // unchanged bb3: {...} // unchanged bb4: { switchInt(_2) -> [1_isize: bb2, otherwise: bb3] } ``` But that is not always a safe thing to do -- sometimes the early `otherwise` branch is necessary so the later block could assume the value of `discriminant(X)`. I am not totally sure what's the best way to detect that, but fixing rust-lang#78496 should be easy -- we just check if `X` is a sub-expression of `Y`. A more precise test might be to check if `Y` contains a `Downcast(1)` of `X`, but I think this might be good enough. Fix rust-lang#78496
Fix issue rust-lang#78496 EarlyOtherwiseBranch finds MIR structures like: ``` bb0: { ... _2 = discriminant(X) ... switchInt(_2) -> [1_isize: bb1, otherwise: bb3] } bb1: { ... _3 = discriminant(Y) ... switchInt(_3) -> [1_isize: bb2, otherwise: bb3] } bb2: {...} bb3: {...} ``` And transforms them into something like: ``` bb0: { ... _2 = discriminant(X) _3 = discriminant(Y) _4 = Eq(_2, _3) switchInt(_4) -> [true: bb4, otherwise: bb3] } bb2: {...} // unchanged bb3: {...} // unchanged bb4: { switchInt(_2) -> [1_isize: bb2, otherwise: bb3] } ``` But that is not always a safe thing to do -- sometimes the early `otherwise` branch is necessary so the later block could assume the value of `discriminant(X)`. I am not totally sure what's the best way to detect that, but fixing rust-lang#78496 should be easy -- we just check if `X` is a sub-expression of `Y`. A more precise test might be to check if `Y` contains a `Downcast(1)` of `X`, but I think this might be good enough. Fix rust-lang#78496
Fix issue rust-lang#78496 EarlyOtherwiseBranch finds MIR structures like: ``` bb0: { ... _2 = discriminant(X) ... switchInt(_2) -> [1_isize: bb1, otherwise: bb3] } bb1: { ... _3 = discriminant(Y) ... switchInt(_3) -> [1_isize: bb2, otherwise: bb3] } bb2: {...} bb3: {...} ``` And transforms them into something like: ``` bb0: { ... _2 = discriminant(X) _3 = discriminant(Y) _4 = Eq(_2, _3) switchInt(_4) -> [true: bb4, otherwise: bb3] } bb2: {...} // unchanged bb3: {...} // unchanged bb4: { switchInt(_2) -> [1_isize: bb2, otherwise: bb3] } ``` But that is not always a safe thing to do -- sometimes the early `otherwise` branch is necessary so the later block could assume the value of `discriminant(X)`. I am not totally sure what's the best way to detect that, but fixing rust-lang#78496 should be easy -- we just check if `X` is a sub-expression of `Y`. A more precise test might be to check if `Y` contains a `Downcast(1)` of `X`, but I think this might be good enough. Fix rust-lang#78496
Rollup of 11 pull requests Successful merges: - rust-lang#79051 (Implement if-let match guards) - rust-lang#79877 (Allow `since="TBD"` for rustc_deprecated) - rust-lang#79882 (Fix issue rust-lang#78496) - rust-lang#80026 (expand-yaml-anchors: Make the output directory separator-insensitive) - rust-lang#80039 (Remove unused `TyEncoder::tcx` required method) - rust-lang#80069 (Test that `core::assert!` is valid) - rust-lang#80072 (Fixed conflict with drop elaboration and coverage) - rust-lang#80073 (Add support for target aliases) - rust-lang#80082 (Revert rust-lang#78790 - rust-src vendoring) - rust-lang#80097 (Add `popcount` and `popcnt` as doc aliases for `count_ones` methods.) - rust-lang#80103 (Remove docs for non-existent parameters in `rustc_expand`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
EarlyOtherwiseBranch finds MIR structures like:
And transforms them into something like:
But that is not always a safe thing to do -- sometimes the early
otherwise
branch is necessary so the later block could assume the value ofdiscriminant(X)
.I am not totally sure what's the best way to detect that, but fixing #78496 should be easy -- we just check if
X
is a sub-expression ofY
. A more precise test might be to check ifY
contains aDowncast(1)
ofX
, but I think this might be good enough.Fix #78496