implement Move trait prototype#156018
Draft
nia-e wants to merge 2 commits into
Draft
Conversation
rustbot
added
S-waiting-on-author
Member
Author
|
@bors try @rust-timer queue |
This comment was marked as off-topic.
This comment was marked as off-topic.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Apr 30, 2026
implement `Move` trait prototype
Member
Author
|
(tests are broken, mostly due to diagnostics changing) |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
Contributor
|
the what i've done in https://github.com/rust-lang/rust/pull/146201/changes#diff-34d5893dd95fe09f9a0fd3341efacd1c21853bb34ba29d9d79bb9af26bb8a0a0 |
lcnr
reviewed
May 1, 2026
| let key = self.infcx.param_env.and(type_op::prove_predicate::ProvePredicate { predicate }); | ||
| let op = CustomTypeOp::new( | ||
| |ocx| { | ||
| let res = type_op::QueryTypeOp::perform_locally_with_next_solver(ocx, key, span); |
Contributor
There was a problem hiding this comment.
if you look at what perform_locally_with_next_solver, it should just be the thing you do in the error path anyways 🤔 🤷
actually, we could make scrape_region_constraints generic over the expected errors, and have a FallibleCustomTypeOp which uses it while expecting potential errors. Instead instead of always emitting a delayed bug if there are errors, scrape_region_constraints would just report the failures as proper type errors 🤔 that seems nicer to me
Member
Author
There was a problem hiding this comment.
right, ok - wasn't sure if that level of overhaul was on the table. it was kind of difficult to recreate the predicate in scrape_region_constraints (thus me doing it this way) but if i can mess with it that makes my life easier :D
lcnr
reviewed
May 1, 2026
lcnr
reviewed
May 1, 2026
lcnr
reviewed
May 1, 2026
lcnr
reviewed
May 1, 2026
| let relaxed_bounds = collect_relaxed_bounds(hir_bounds, context); | ||
| self.reject_duplicate_relaxed_bounds(relaxed_bounds); | ||
|
|
||
| let Some(move_did) = tcx.lang_items().move_trait() else { |
Contributor
There was a problem hiding this comment.
I'd just always require_lang_item :3 requiring core to provide the trait definition for Move seems innocent enough
Member
Author
There was a problem hiding this comment.
that's what I tried initially, but it means we need to add Move in a quadrillion tests that don't depend on core 🫠
Contributor
There was a problem hiding this comment.
we should have some mini_core somewhere that most such tests should use 😅
are there still proper #[no_core] tests that don't use an existing mini_core?
lcnr
reviewed
May 1, 2026
lcnr
reviewed
May 1, 2026
| } | ||
|
|
||
| // Don't use `add_implicit_bounds` directly to skip adding `Sized`. | ||
| self.add_implicit_move_bound( |
Contributor
There was a problem hiding this comment.
hmm, I'd prefer add_implicit_bound here, because otherwise we have to do that when adding Forget later on
lcnr
reviewed
May 1, 2026
| } | ||
|
|
||
| pub fn is_implicit_trait(self, def_id: DefId) -> bool { | ||
| self.is_default_trait(def_id) || matches!(self.as_lang_item(def_id), Some(LangItem::Move)) |
Contributor
There was a problem hiding this comment.
why are we not treating Sized as an implicit trait?
Member
Author
There was a problem hiding this comment.
Sized is weird enough (sometimes the bound is Sized, sometimes MetaSized etc) that i thought it made more sense to treat it separately. like, arguably MetaSized should also be an implicit trait but that would change behaviour at most callsites i think.
though i realise this is kinda confusing naming given that I also wrote add_implicit_trait_bounds that does add sized...
lcnr
reviewed
May 1, 2026
| } | ||
|
|
||
| // Backward compatibility for default auto traits. | ||
| // Backward compatibility for default auto traits & `Move`. |
Contributor
There was a problem hiding this comment.
this is somewhat meh, Move is a default auto trait 😅 ✨ , just not add default_auto_trait
Member
There was a problem hiding this comment.
In general, this PR doesn't seem to use the "infrastructure" provided by internal feature more_maybe_bounds unlike earlier experiments.
I lack the context, so I don't know why that's done this way. However, if the "proper" Move and Forget default auto traits won't use all the preexisting MMB lowering routines, then I'd rather remove that internal feature entirely since it's evidently being supplanted (and also won't be needed anymore anyway as its purpose was to experiment with new default bounds).
Maintaining two incredibly similar but still somehow distinct features (default vs. "implicit") is somewhat meh I dare say. We probably want to notify Vadim then who is the owner of MMB IIRC.
Contributor
There was a problem hiding this comment.
yeah, I agree. Ripping out that feature in favor of actually having individual traits seems good to me. Reusing these features doesn't simplify adding new traits. Having one non-sized trait is nice to properly handle things though
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
Author
|
curious what the perf hit is without doing any special handling. should actually work this time :D @bors try @rust-timer queue |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment has been minimized.
This comment has been minimized.
rust-bors
Bot
removed
the
S-waiting-on-review
Contributor
|
💔 Test for 1e6ab13 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
Author
|
@bors try |
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Jun 15, 2026
implement `Move` trait prototype
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (f978b70): comparison URL. Overall result: ❌✅ regressions and improvements - BENCHMARK(S) FAILEDBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @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 1.3%, secondary -4.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.9%, secondary 5.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.3%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 518.77s -> 533.733s (2.88%) |
rustbot
removed
the
S-waiting-on-perf
lcnr
reviewed
Jun 16, 2026
| } | ||
| } else if let PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) = context | ||
| && move_tr | ||
| { |
| Some(rustc_hir::LangItem::Move) | ||
| ) | ||
| }) | ||
| } else { |
Contributor
There was a problem hiding this comment.
this only filters the param_env if tcx.is_trait(def_id) is true
| hir_bounds: &[hir::GenericBound<'tcx>], | ||
| context: ImpliedBoundsContext<'tcx>, | ||
| span: Span, | ||
| including_sized: bool, |
Contributor
There was a problem hiding this comment.
please make this a newtype enum
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
|
@bors try @rust-timer queue |
Collaborator
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Jul 24, 2026
implement `Move` trait prototype
Contributor
|
💔 Test for b373540 failed: CI. Failed job:
|
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
Add a barebones implementation for
Move(#149607), pending some diagnostics changes & tests.TODO
feature(move_trait)Mover? lcnr