Update transmute_copy to ub_checks and ?Sized - #155989
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Currently, this UB is detected at run time when |
|
Yeah I agree, making this an non-unwinding panic seems like an obvious improvement, but disabling the check when UB checks are disabled seems strictly worse. The idea of UB checks being optional is that they can be expensive, but this check is constant and therefore guaranteed to be free, so why not always? |
|
The check isn't constant when the source is unsized. |
|
Also, the thing with the "well it's a 'free' check" is that that's only true if it's Plus, it's often not language UB to violate this (at least under TB), since often the reference is into a larger allocation anyway. The interesting and difficult part of the UB remains fundamentally uncheckable. The assert was useful before because people never noticed that they were doing something always-UB. But the ubcheck is absolutely sufficient to cover that need. If you have code that you never run in debug ever and you're pervasively getting your |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
|
||
| // If Dst has a higher alignment requirement, src might not be suitably aligned. | ||
| if align_of::<Dst>() > align_of::<Src>() { | ||
| if align_of::<Dst>() > align_of_val::<Src>(src) { |
There was a problem hiding this comment.
For unsized Src this comparison might not be optimized out. Is it even worth having both branches then?
There was a problem hiding this comment.
Hmm, interesting question. I guess I could condition it on is_val_statically_known...
(We can't specialize on Sized-ness, right?)
EDIT later: Note to self, include some codegen tests to demonstrate one way or the other.
There was a problem hiding this comment.
@RalfJung For my own edification, can you say more about why this comparison might not be optimized out in the unsized case?
There was a problem hiding this comment.
Because align_of_val might not be a constant if the unsized tail is dyn Trait. So this compiles to if some_const > value_loaded_from_vtable ... and I don't see how LLVM could optimize this.
There was a problem hiding this comment.
Yeah I think that makes sense. What is the alternative you're suggesting? You mention not having both branches, but which branch are you suggesting be eliminated?
(To be clear, I am nearly certain that these questions are coming from a lack of understanding on my part.)
There was a problem hiding this comment.
Originally I meant to suggest that we should just always use read_unaligned to avoid codegen'ing two read paths.
But as @scottmcm mentioned there's an alternative: use is_val_statically_known.
There was a problem hiding this comment.
Good news, even at low opt-levels LLVM is smart and merges the loads or the memcpys so even with the branch in the rust code the right thing happens 🎉 Test added.
(I still have doc fixes to do, though, so this PR is not ready yet.) Done.
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
2a3510c to
a3a8023
Compare
|
@bors r+ |
|
📋 This PR cannot be approved because it currently has the following label: |
|
@bors r=Mark-Simulacrum |
…=Mark-Simulacrum Update `transmute_copy` to ub_checks and `?Sized` The `assert!` in `transmute_copy` is from rust-lang#98839. However, it was controversial at the time because it's possible to write `transmute_copy`s that were UB by the description but didn't pass the documented conditions. It's also sub-optimal to have possible `panic`s in unsafe code where the panic can actually make things worse by dropping things in the middle of what the programmer expected to be panic-free. Now that we have the `UbChecks` possibility, though, we can do this better: this PR makes it a *non-unwinding* panic when not meeting the documented condition when UB checks are enabled, but which very clearly emphasizes that said check isn't a documented behaviour of the function. Also, I was staring at the signature and couldn't figure out why it needs `Src: Sized`. Since it *always* takes a reference, there seems to be no downside to accepting `Src: ?Sized`. That would also give it more of a reason to exist even if <https://doc.rust-lang.org/nightly/std/mem/fn.transmute_prefix.html> exists (as `transmute_prefix(x)` existing will no longer need the `transmute_copy(&ManuallyDrop::new(x))` polyfill, cc rust-lang/rfcs#3844). That definitely needs an FCP, though. (The change to `assert_unsafe_precondition` here doesn't technically need libs-api assent, but I'd still like to hear your thoughts since it's different from the "promise the assert" that IIRC you were also asked to consider elsewhere.) A partial replacement for rust-lang#154665
…=Mark-Simulacrum Update `transmute_copy` to ub_checks and `?Sized` The `assert!` in `transmute_copy` is from rust-lang#98839. However, it was controversial at the time because it's possible to write `transmute_copy`s that were UB by the description but didn't pass the documented conditions. It's also sub-optimal to have possible `panic`s in unsafe code where the panic can actually make things worse by dropping things in the middle of what the programmer expected to be panic-free. Now that we have the `UbChecks` possibility, though, we can do this better: this PR makes it a *non-unwinding* panic when not meeting the documented condition when UB checks are enabled, but which very clearly emphasizes that said check isn't a documented behaviour of the function. Also, I was staring at the signature and couldn't figure out why it needs `Src: Sized`. Since it *always* takes a reference, there seems to be no downside to accepting `Src: ?Sized`. That would also give it more of a reason to exist even if <https://doc.rust-lang.org/nightly/std/mem/fn.transmute_prefix.html> exists (as `transmute_prefix(x)` existing will no longer need the `transmute_copy(&ManuallyDrop::new(x))` polyfill, cc rust-lang/rfcs#3844). That definitely needs an FCP, though. (The change to `assert_unsafe_precondition` here doesn't technically need libs-api assent, but I'd still like to hear your thoughts since it's different from the "promise the assert" that IIRC you were also asked to consider elsewhere.) A partial replacement for rust-lang#154665
…=Mark-Simulacrum Update `transmute_copy` to ub_checks and `?Sized` The `assert!` in `transmute_copy` is from rust-lang#98839. However, it was controversial at the time because it's possible to write `transmute_copy`s that were UB by the description but didn't pass the documented conditions. It's also sub-optimal to have possible `panic`s in unsafe code where the panic can actually make things worse by dropping things in the middle of what the programmer expected to be panic-free. Now that we have the `UbChecks` possibility, though, we can do this better: this PR makes it a *non-unwinding* panic when not meeting the documented condition when UB checks are enabled, but which very clearly emphasizes that said check isn't a documented behaviour of the function. Also, I was staring at the signature and couldn't figure out why it needs `Src: Sized`. Since it *always* takes a reference, there seems to be no downside to accepting `Src: ?Sized`. That would also give it more of a reason to exist even if <https://doc.rust-lang.org/nightly/std/mem/fn.transmute_prefix.html> exists (as `transmute_prefix(x)` existing will no longer need the `transmute_copy(&ManuallyDrop::new(x))` polyfill, cc rust-lang/rfcs#3844). That definitely needs an FCP, though. (The change to `assert_unsafe_precondition` here doesn't technically need libs-api assent, but I'd still like to hear your thoughts since it's different from the "promise the assert" that IIRC you were also asked to consider elsewhere.) A partial replacement for rust-lang#154665
…uwer Rollup of 20 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #155989 (Update `transmute_copy` to ub_checks and `?Sized`) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)
…uwer Rollup of 19 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #155989 (Update `transmute_copy` to ub_checks and `?Sized`) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)
Rollup merge of #155989 - scottmcm:unsized-transmute-copy, r=Mark-Simulacrum Update `transmute_copy` to ub_checks and `?Sized` The `assert!` in `transmute_copy` is from #98839. However, it was controversial at the time because it's possible to write `transmute_copy`s that were UB by the description but didn't pass the documented conditions. It's also sub-optimal to have possible `panic`s in unsafe code where the panic can actually make things worse by dropping things in the middle of what the programmer expected to be panic-free. Now that we have the `UbChecks` possibility, though, we can do this better: this PR makes it a *non-unwinding* panic when not meeting the documented condition when UB checks are enabled, but which very clearly emphasizes that said check isn't a documented behaviour of the function. Also, I was staring at the signature and couldn't figure out why it needs `Src: Sized`. Since it *always* takes a reference, there seems to be no downside to accepting `Src: ?Sized`. That would also give it more of a reason to exist even if <https://doc.rust-lang.org/nightly/std/mem/fn.transmute_prefix.html> exists (as `transmute_prefix(x)` existing will no longer need the `transmute_copy(&ManuallyDrop::new(x))` polyfill, cc rust-lang/rfcs#3844). That definitely needs an FCP, though. (The change to `assert_unsafe_precondition` here doesn't technically need libs-api assent, but I'd still like to hear your thoughts since it's different from the "promise the assert" that IIRC you were also asked to consider elsewhere.) A partial replacement for #154665
…uwer Rollup of 19 pull requests Successful merges: - rust-lang/rust#158692 (Add release notes for 1.96.1) - rust-lang/rust#134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - rust-lang/rust#155932 (MIR Call terminator: evaluate destination place before arguments) - rust-lang/rust#155989 (Update `transmute_copy` to ub_checks and `?Sized`) - rust-lang/rust#156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - rust-lang/rust#157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - rust-lang/rust#157835 (expand free alias types in the auto-trait orphan check) - rust-lang/rust#157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - rust-lang/rust#158434 (delegation: refactor AST -> HIR lowering) - rust-lang/rust#158552 (make some tidy errors around python easier to understand) - rust-lang/rust#158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - rust-lang/rust#158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - rust-lang/rust#158711 (library: Comment on libtest's dicey internal soundness) - rust-lang/rust#158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - rust-lang/rust#158539 (Move `SizeHint` and `IoHandle` to `core::io`) - rust-lang/rust#158659 (refactor the normalization in `coerce_shared_info`) - rust-lang/rust#158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - rust-lang/rust#158698 (Update TypeVisitable implementation) - rust-lang/rust#158706 (Tweaks to MIR building scope API)
Pkgsrc changes: * Adapt to changes in vendored crate versions. * Version & checksum changes. Upstream changes: Version 1.99.0 (2026-10-01) ========================== Language -------- - [Add allow-by-default `raw_borrows_via_references` lint that checks for references that decay immediately into raw borrows](rust-lang/rust#138230) - [Extend `unconditional_panic` lint to function calls that panic when the chunks/windows size is zero] (rust-lang/rust#153563) - [Stabilize C-variadic function definitions] (rust-lang/rust#155697) - [Stabilize the ability to use `#[unsafe(naked)]` functions to define C-variadic functions (`#![feature(c_variadic_naked_functions)]`).] (rust-lang/rust#159746) - [Trait methods are now resolved on an adjusted never type (producing a FCW)] (rust-lang/rust#156047) - [Coerce from inference variables to trait objects if the inference variable is related via subtyping to a type that is known to be `Sized`] (rust-lang/rust#157820) - [Stabilize `#[my_macro] mod foo;`] (rust-lang/rust#157857). This allows outlined modules (`mod foo;`) anywhere in the body of a custom attribute or derive macro. - [Fix the `overflowing_literals` lint with repeated negation] (rust-lang/rust#158302). For instance, it will now no longer lint on `--128_i8`, which is already detected by the `arithmetic_overflow` lint. - [Add POSIX symbols to the `invalid_runtime_symbol_definitions` and `suspicious_runtime_symbol_definitions` lints] (rust-lang/rust#158522) - [Lint unused `#[path]` attributes on inline modules] (rust-lang/rust#158835) - [Enable `unreachable_cfg_select_predicates` lint as part of `unused` lint group] (rust-lang/rust#159179) - [Stabilize passing 128-bit integers via vector registers with `asm!` on x86] (rust-lang/rust#159525) - [Explicitly document that some allocations are allowed to grow in-place (but none are allowed to shrink)] (rust-lang/rust#159729) - We now [guarantee] (rust-lang/rust#159730) that the contents of an `UnsafeCell` can be accessed without going through `get` - [The `invalid_reference_casting` lint was adjusted accordingly] (rust-lang/rust#159960) - [Account for globally enabled target features in `global_asm!`] (rust-lang/rust#160594) - [Warn if an invalid `doc` attribute is used on a macro invocation] (rust-lang/rust#161003) Compiler -------- - [Convert `-Ctarget-cpu` into a target-modifier for AVR, AMDGCN and NVPTX] (rust-lang/rust#150732) - [Enable `static_position_independent_executables` on all gnu and musl targets] (rust-lang/rust#158510) - When providing a suggestion about a missing method, rustc now prefers an exactly matching name from a [doc alias attribute] (https://doc.rust-lang.org/rustdoc/advanced-features.html#add-aliases-for-an-item-in-documentation-search) over a similarity search from other method names. If your new users sometimes expect a method under a different name, adding a doc alias will now help them find it via rustc suggestions, in addition to helping them find it via rustdoc search: [When suggesting method names, prefer *exact* doc aliases over similar names](rust-lang/rust#160369) Platform Support ---------------- - [Promote `riscv64-unknown-linux-musl` to Tier 2 with host tools] (rust-lang/rust#158766) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. [platform-support-doc]: https://doc.rust-lang.org/rustc/platform-support.html Libraries --------- - Iteration on `RangeInclusive` (`a..=b` ranges) is now [optimized better in some circumstances] (rust-lang/rust#155114). As a side effect of this, the behavior of `RangeInclusive` values that has already been exhausted (as an iterator) has changed. For example, the return values of `start()` and `end()` on such ranges may return different values, and using such ranges as slice indexes may have different behavior. These behaviors were not guaranteed to be stable, so these changes are considered to not be breaking changes. - [Relax `transmute_copy` to accept `?Sized` types] (rust-lang/rust#155989) - [Update `transmute_copy` to use a non-unwinding panic] (rust-lang/rust#155989) - [Don't escape U+FF9E and U+FF9F in `escape_debug_ext`] (rust-lang/rust#158057) - [Re-export `core::fmt::NumBuffer` in `alloc` (and `std`)] (rust-lang/rust#161430) Stabilized APIs --------------- - [`IntoIterator` for `Box<[T; N]>`] (https://doc.rust-lang.org/stable/std/iter/trait.IntoIterator.html#impl-IntoIterator-for-Box%3C%5BT;+N%5D,+A%3E) - [`IntoIterator` for `&Box<[T; N]>`] (https://doc.rust-lang.org/stable/std/iter/trait.IntoIterator.html#impl-IntoIterator-for-%26Box%3C%5BT;+N%5D,+A%3E) - [`IntoIterator` for `&mut Box<[T; N]>`] (https://doc.rust-lang.org/stable/std/iter/trait.IntoIterator.html#impl-IntoIterator-for-%26mut+Box%3C%5BT;+N%5D,+A%3E) - [`VecDeque::retain_back`] (https://doc.rust-lang.org/stable/std/collections/struct.VecDeque.html#method.retain_back) - [`core::ffi::VaList`] (https://doc.rust-lang.org/stable/core/ffi/struct.VaList.html) - [`Box::into_non_null`] (https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_non_null) - [`Box::from_non_null`] (https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.from_non_null) - [`Vec::into_parts`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.into_parts) - [`Vec::from_parts`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.from_parts) - [`core::mem::size_of_val_raw`] (https://doc.rust-lang.org/stable/core/mem/fn.size_of_val_raw.html) - [`core::mem::align_of_val_raw`] (https://doc.rust-lang.org/stable/core/mem/fn.align_of_val_raw.html) - [`core::alloc::Layout::for_value_raw`] (https://doc.rust-lang.org/stable/core/alloc/struct.Layout.html#method.for_value_raw) - [`String::from_utf8_lossy_owned`] (https://doc.rust-lang.org/stable/std/string/struct.String.html#method.from_utf8_lossy_owned) - [`string::FromUtf8Error::into_utf8_lossy`] (https://doc.rust-lang.org/stable/std/string/struct.FromUtf8Error.html#method.into_utf8_lossy) - [`FusedIterator for StepBy<I>`] (https://doc.rust-lang.org/stable/std/iter/struct.StepBy.html#impl-FusedIterator-for-StepBy%3CI%3E) - [`std::fs::set_times`] (https://doc.rust-lang.org/stable/std/fs/fn.set_times.html) - [`std::fs::set_times_nofollow`] (https://doc.rust-lang.org/stable/std/fs/fn.set_times_nofollow.html) Cargo ----- - Add a new built-in profile `debug`. This is a preparation for transitioning the `dev` profile away from debugging to give a saner default for faster development iterations. Currently there is no difference between `dev` and `debug` profiles. [docs] (https://doc.rust-lang.org/nightly/cargo/reference/profiles.html#debug-1) [#17214] (rust-lang/cargo#17214) - Workspace members on edition 2024 or later can now override an inherited workspace dependency's `default-features` field. For example, `serde = { workspace = true, default-features = false }` now turns off default features even when the workspace definition enables them. On earlier editions, `default-features = false` is ignored with a warning. ([RFC 3945] (rust-lang/rfcs#3945)) [#17126](rust-lang/cargo#17126) - Incremental compilation is now disabled by default when running in CI. CI is detected via the CI environment variable. [#17220] (rust-lang/cargo#17220) See also the [full Cargo changelog] (https://doc.rust-lang.org/nightly/cargo/CHANGELOG.html#cargo-199-2026-10-01) Rustdoc ----- - [Add new `unused_footnote_definition` rustdoc lint] (rust-lang/rust#137858) - Smarter filtering of trait impls yields performance improvements of 20% on average and up to 40% on some real-world crates. ([1] (rust-lang/rust#159623), [2](rust-lang/rust#159721), [3](rust-lang/rust#159779), [4](rust-lang/rust#159854), [5](rust-lang/rust#159091)) Compatibility Notes ------------------- - [Fully deprecate the legacy integral modules] (rust-lang/rust#146882). For example, `std::i32::MAX` should be accessed via `i32::MAX` instead. - [Upgrade `no_mangle_generic_items` into hard error] (rust-lang/rust#154585) - [The `Pin::new_unchecked` has had its safety invariants changed slightly] (rust-lang/rust#156935) - [Do not promote references to extern statics] (rust-lang/rust#157641) - [Ensure that the inferred types of `let` patterns typecheck] (rust-lang/rust#157841) - [hermit/fs: Return `unsupported()` instead of `from_raw_os_error(22)`] (rust-lang/rust#158247) - [Fixed a bug where `#[repr(simd)]` was accidentally allowed on macro invocations on stable Rust] (rust-lang/rust#158523) - [Abort const-eval when there are generics in the type of the value being produced] (rust-lang/rust#159504) - [Attributes not applying to anything are now an error in code blocks in doc comments] (rust-lang/rust#159849) - [`Box::leak`: tell people to avoid unleaking] (rust-lang/rust#160323) - [PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian] (rust-lang/rust#160441) - [Do not take `doc(cfg())` into account when filtering doctests] (rust-lang/rust#159014) - [Infer anonymous lifetimes in the types of associated consts as `'static`] (rust-lang/rust#156508) - Macros that expand to a semicolon now produce a warning lint (`semicolon_in_expressions_from_non_local_macros`) even when the macro comes from another crate. Previously, such warnings only appeared for macros from the same crate, to avoid showing warnings that can't be fixed locally; however, this masked problems, as integration tests from the crate providing the macro get compiled as a separate crate, so tests often wouldn't reveal this issue. If you encounter a lint like this, please make sure to report it to the crate providing the macro so they can fix it; don't just silence it in your own crate. - [`semicolon_in_expressions_from_macros`: Lint on non-local macros too] (rust-lang/rust#159222) - [Split non-local `semicolon_in_expressions_from_macros` into a separate lint] (rust-lang/rust#159700) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Update to LLVM 23] (rust-lang/rust#158734)
View all comments
The
assert!intransmute_copyis from #98839. However, it was controversial at the time because it's possible to writetransmute_copys that were UB by the description but didn't pass the documented conditions. It's also sub-optimal to have possiblepanics in unsafe code where the panic can actually make things worse by dropping things in the middle of what the programmer expected to be panic-free. Now that we have theUbCheckspossibility, though, we can do this better: this PR makes it a non-unwinding panic when not meeting the documented condition when UB checks are enabled, but which very clearly emphasizes that said check isn't a documented behaviour of the function.Also, I was staring at the signature and couldn't figure out why it needs
Src: Sized. Since it always takes a reference, there seems to be no downside to acceptingSrc: ?Sized. That would also give it more of a reason to exist even if https://doc.rust-lang.org/nightly/std/mem/fn.transmute_prefix.html exists (astransmute_prefix(x)existing will no longer need thetransmute_copy(&ManuallyDrop::new(x))polyfill, cc rust-lang/rfcs#3844). That definitely needs an FCP, though.(The change to
assert_unsafe_preconditionhere doesn't technically need libs-api assent, but I'd still like to hear your thoughts since it's different from the "promise the assert" that IIRC you were also asked to consider elsewhere.)A partial replacement for #154665