remove InPlaceIterable marker from Peekable due to unsoundness #85340
Conversation
|
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I think you could also add a regression test, just to make sure this bug is catched if someone will try to implement |
|
Right, will do. I might even try to salvage the optimization later so it's good to have a test. |
the8472
force-pushed
the
no-inplaceiterable-on-peekable
branch
from
384b725
to
7d141e1
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 7d141e1891b1664b804b0b1a133ffd48fec85a9d with merge e3cdb041f816aa5fee61e1c9e066a4d6f1fd23a0... |
|
☀️ Try build successful - checks-actions |
|
Queued e3cdb041f816aa5fee61e1c9e066a4d6f1fd23a0 with parent eac3c7c, future comparison URL. |
|
Finished benchmarking try commit (e3cdb041f816aa5fee61e1c9e066a4d6f1fd23a0): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
Perf looks neutral. @rustbot label T-libs-impl |
rustbot
added
the
T-libs
the8472
force-pushed
the
no-inplaceiterable-on-peekable
branch
from
e064962
to
6cb81a6
Compare
|
☔ The latest upstream changes (presumably #84767) made this pull request unmergeable. Please resolve the merge conflicts. |
The unsoundness is not in Peekable per se, it rather is due to the interaction between Peekable being able to hold an extra item and vec::IntoIter's clone implementation shortening the allocation. An alternative solution would be to change IntoIter's clone implementation to keep enough spare capacity available.
This also checks the contents and not only the capacity in case IntoIter's clone implementation is changed to add capacity at the end. Extra capacity at the beginning would be needed to make InPlaceIterable work. Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>
the8472
force-pushed
the
no-inplaceiterable-on-peekable
branch
from
6cb81a6
to
7cb4e51
Compare
|
Thank you very much for the fix. @bors r+ |
|
📌 Commit 7cb4e51 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
pietroalbini
added
the
beta-nominated
|
Nominating to beta backport as requested on Zulip. |
rustbot
added
the
beta-accepted
Mark-Simulacrum
removed
the
beta-nominated
…ulacrum [beta] backports * Backport 1.52.1 release notes rust-lang#85404 * remove InPlaceIterable marker from Peekable due to unsoundness rust-lang#85340 * rustdoc: Call initSidebarItems in root module of crate rust-lang#85304 * Update LLVM submodule rust-lang#85236 * Do not ICE on invalid const param rust-lang#84913 * Disallows #![feature(no_coverage)] on stable and beta (using standard crate-level gating) rust-lang#84871 * Ensure TLS destructors run before thread joins in SGX rust-lang#84409
The unsoundness is not in Peekable per se, it rather is due to the
interaction between Peekable being able to hold an extra item
and vec::IntoIter's clone implementation shortening the allocation.
An alternative solution would be to change IntoIter's clone implementation
to keep enough spare capacity available.
fixes #85322