Ensure ErasedData only implements appropriate auto traits#154229
Ensure ErasedData only implements appropriate auto traits#154229Zoxc wants to merge 1 commit intorust-lang:mainfrom
ErasedData only implements appropriate auto traits#154229Conversation
rustbot
added
S-waiting-on-review
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ensure `ErasedData` only implements appropriate auto traits
rustbot
added
the
S-waiting-on-perf
There was a problem hiding this comment.
Seems ok to me. I'm not super familiar with the type erasing and DynSend/DynSync stuff so I will wait a bit in case others have anything to add.
cc @zetanumbers
| #[inline(always)] | ||
| #[define_opaque(Erased)] | ||
| pub fn erase_val<T: Erasable>(value: T) -> Erased<T> { | ||
| pub fn erase_val<T: Erasable + DynSend + DynSync>(value: T) -> Erased<T> { |
There was a problem hiding this comment.
Please add a comment here referring back to the unsafe DynSend/DynSync impls for ErasedData.
| data: MaybeUninit<Storage>, | ||
| /// `Storage` is an erased type, so we use an external type here to opt-out of auto traits | ||
| /// as those would be incorrect. | ||
| hide: PhantomData<Hide>, |
There was a problem hiding this comment.
A verb is strange for a field name/type name. hidden/Hidden would be better than hide/Hide, I think.
There was a problem hiding this comment.
I find the name puzzling either way; I don't see what is being “hidden” here.
If the goal is to not implement auto-traits, having something like _no_auto_traits: PhantomData<NoAutoTraits> seems a lot more straightforward to me.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (87b89de): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -6.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 485.484s -> 484.192s (-0.27%) |
rustbot
removed
the
S-waiting-on-perf
| // SAFETY: The bounds on `erase_val` ensure the types we erase are `DynSync` and `DynSend` | ||
| unsafe impl<Storage: Copy> DynSync for ErasedData<Storage> {} | ||
| unsafe impl<Storage: Copy> DynSend for ErasedData<Storage> {} |
There was a problem hiding this comment.
Storage should be DynSync and DynSend too
There was a problem hiding this comment.
We don't store Storage or create any instance of that type.
|
I just noticed that this overlaps with the experiment in #151715, interesting. |
Yeah, experimentally removing value erasure exposed all the places where we accidentally erase things that aren’t actually DynSend/DynSync, though in practice it’s all just dyn trait types that can be updated to include the appropriate bounds. |
|
I see, this PR is basically fixing the FIXME comment on |
6 participants
This uses an external type to prevent auto traits to be inferred on
ErasedData. That inference is incorrect since it may not store the type it declares.This also implements
DynSendandDynSynconErasedDatawhich are checked by bounds onerase_val.Some diagnostics bounds were missing
DynSync, which is also added here.