Repository navigation
fix unsoundness when compiling [diverging_const; _] - #165497
Conversation
The previous fix would still let `[...; _]` be unsound :c
rustbot
added
S-waiting-on-review
|
|
WaffleLapkin
added
the
beta-nominated
|
Beta nominating for the same reason as #165416 -- this (hopefully now actually) fixes an unsoundness with the never type that's about to be stable. |
There was a problem hiding this comment.
So the problem here is that in [CONST; 0] we dont actually evaluate CONST so even though the expr CONST diverges, the function as a whole does not?
And you fix this by making it so that if we aren't guaranteed that the repeat count is greater than zero, then we treat the array repeat expression as being non-diverging?
And the thing talked about in the original issue that another fix might be to instead actually evaluate CONST unconditionally? But we're not doing that rn because its more involved than papering over the issue?
does that all sounds like a correct understanding? if so lgtm r=me
|
I guess this also assumes that conservatively not considering things to be diverging is ok, is that the case xd |
|
Just to be sure again: Does this require a crater run? |
|
@BoxyUwU yes, I think everything you said is correct.
To the best of my knowledge, yes. Assumption of divergence allows the compiler to do a lot of things (such as removing code presumed unreachable). The opposite isn't true, if the compiler doesn't know if something diverges, that does not give any information to it. Note how you can always wrap a diverging expression in a function to hide (from the type system) the fact that it diverges. |
|
@theemathas I think this is still fine. To have a problem with this you need:
I think this is very improbable, and I don't feel like there is a need for crater. |
|
beta backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels are handled by them. |
rustbot
added
the
beta-accepted
| // Issue: <https://github.com/rust-lang/rust/issues/165323> | ||
| // | ||
| //@ build-fail | ||
| #![feature(generic_const_exprs)] |
There was a problem hiding this comment.
@BoxyUwU the only way I found to make a test with T::ASSOC work is to use GCE, is this what you wanted?
|
@bors r=BoxyUwU |
rust-bors
Bot
added
S-waiting-on-bors
fix unsoundness when compiling `[diverging_const; _]` Follow up to rust-lang#165416, because I [missed a case](rust-lang#165416 (comment)). r? @BoxyUwU
…uwer Rollup of 8 pull requests Successful merges: - #165496 (c-variadic: remove stale `cfg(bootstrap)` comment) - #164716 (Lower UEFI targets through LLVM's *uefi triple) - #165497 (fix unsoundness when compiling `[diverging_const; _]`) - #165275 (Build TPDE with local Clang when available) - #165467 (Use `emit_ptr_va_arg` for `va_arg` on MSP430) - #165483 (compiletest: Fix the edition of minicore to the latest stable one) - #165498 (LoongArch: Fix direct-access-external-data test) - #165501 (rustdoc: Don't add a border on an empty `:target` element)
|
@bors p=10 |
JonathanBrouwer
removed
the
beta-nominated
Rollup merge of #165497 - WaffleLapkin:_, r=BoxyUwU fix unsoundness when compiling `[diverging_const; _]` Follow up to #165416, because I [missed a case](#165416 (comment)). r? @BoxyUwU
[beta] backports Cherry picked: * #165497 * #165499 - rust-lang/rust-clippy#17752 (was already last beta, but we didn't manage to get the proper fixed merged in time like we planned) - rust-lang/rust-clippy#17828 - rust-lang/rust-clippy#17941 r? @ghost
[beta] backports Cherry picked: * #165497 * #165499 - rust-lang/rust-clippy#17752 (was already last beta, but we didn't manage to get the proper fixed merged in time like we planned) - rust-lang/rust-clippy#17828 - rust-lang/rust-clippy#17941 r? @ghost
Follow up to #165416, because I missed a case. r? @BoxyUwU