Move trait prototype

[zannabianca1997]

View all comments

Finishing the work started by @nia-e in #156018 :

Add a barebones implementation for Move (#149607), pending some diagnostics changes & tests.

TODO

• next solver currently errors when not enabling feature(move_trait)
• change printing of trait objects to not show Move
• change printing of opaque types to not show Move
• fix mangling of Move bounds
• how to handle empty trait object that now are dyn Move
• relationship between PhantomData<T>: Move and T: Move
• also make sure rustdoc handles move correctly
• valuable to look into why diesel regresses

r? lcnr

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @lcnr (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions for more information.

[zannabianca1997]

A quick ./x.py test tests/ui locally to have a baseline

test result: FAILED. 9 passed; 206 failed; 21862 ignored; 0 measured; 0 filtered out; finished in 1.14s

[zannabianca1997]

test result: FAILED. 21801 passed; 32 failed; 244 ignored; 0 measured; 0 filtered out; finished in 168.70s

wow that were a lot of them. looks like the next batch is a bunch of fn that now has fn() + Move

[zannabianca1997]

looking at the errors, there is also a bunch of (dyn + 'static) printing.

Those are all checks going around the trait_alias feature, that created before empty trait object and now creates what are effectively dyn Move, that are being misprinted.

That sure raises a question - can we handle them now? ummm - putting this aside in favour of lower hanging fruits

[zannabianca1997]

@lcnr this last one (520bc2f)

it's better to break the ABI and accept all aymbol mangling containing Move or we have to decide a way to mangle ?Move? I know rust has no stable abi but to put it in all symbols seems a bit too much

[cramertj]

Apologies for the drive-by, but it's surprising to me that PhantomData would unconditionally implement Move, rather than implementing it only when T: Move. Other marker trait impls for PhantomData are conditional on T: https://doc.rust-lang.org/std/marker/struct.PhantomData.html#synthetic-implementations

View changes since the review

[zannabianca1997]

ummm...

I often saw phantomdata as a way to maintain pointer type info, so having &'a T be somewhat equivalent to (*const (), PhantomData<&'a T>). Wouldn't not implementing Move for it make this kind of pointers immovable? The moveability of the pointer should not be related to the one of the pointee

[zannabianca1997]

generally I see in the future a proliferation of Phantom* types (like #135806) to represent different aspects you want to inherit from the type

[lcnr]

for perf reasons, it might actually be faster to do

if !tcx.features().move_trait() {
    let move_trait = tcx.get_lang_item(LangItem::Move);
    // filter_by_comparing to that :>
}

might be worth another perf run with that change 😁

View changes since the review

[zannabianca1997]

Oh that's a get for each clause - totally missed that. I guess that T: Move clause would be common enough to make caching that at least equal in performance

You should call the perf run right? Also need to check diesel as I am hopefully almost done with fixing the tests

[rust-log-analyzer]

The job x86_64-gnu-gcc-core-tests failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

---- mem::type_info::test_dynamic_traits stdout ----
---- mem::type_info::test_dynamic_traits stderr ----

thread 'main' (19197) panicked at library/coretests/tests/mem/type_info.rs:364:9:
assertion `left == right` failed: unexpected ids.
actual: [TypeId(0x9b192d704cbe10d507bc1e5256cef3a9), TypeId(0x19e931f02a5065b2fd07215b8865ce8c)]
expected: [TypeId(0xa73276c55ec593447079ff75a7990bc5)]
  left: {TypeId(0x19e931f02a5065b2fd07215b8865ce8c), TypeId(0x9b192d704cbe10d507bc1e5256cef3a9)}
 right: {TypeId(0xa73276c55ec593447079ff75a7990bc5)}
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- mem::type_info::test_dynamic_traits stdout end ----

failures:
    mem::type_info::test_dynamic_traits

Important

For more information how to resolve CI failures of this job, visit this link.