[do not merge] Add a Move marker trait
#144679
Open
Conversation
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
impl Move for more types
rustbot
added
S-waiting-on-review
lcnr
reviewed
Jul 30, 2025
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors2 try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rust-bors bot
added a commit
that referenced
this pull request
Jul 30, 2025
[do not merge] Add a `Move` marker trait
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
Ohh, I think we've hit an ICE here? |
Ye, more specifically: https://triage.rust-lang.org/gha-logs/rust-lang/rust/47029834962#2025-07-30T12:37:47.0296013Z ( |
|
Talked with @lcnr on the Zulip thread, and they recommended adding |
|
💔 Test failed (CI). Failed jobs:
|
This comment has been minimized.
This comment has been minimized.
|
@yoshuawuyts can this PR build core now? If so I'll send to the perfbot, but from the CI results I'm not sure it does. |
|
That would be: |
|
Oh, that was quick. No, it doesn't compile. It fails with the same error we're seeing in CI: |
|
cc/ @Bryanskiy as the author of #120706, this might be interesting to you too |
|
Here is a predicate that the compiler tries to prove:
The documentation also says:
The situation is that there is an implicit To fix it you have to add #[lang = "dispatch_from_dyn"]
pub trait DispatchFromDyn<T: ?Move> {} |
This comment has been minimized.
This comment has been minimized.
|
Ouch, we also need #[lang = "unsize"]
pub trait Unsize<T: ?Sized + ?Move> {
// Empty.
}And maybe something else. I need to take a closer look. |
|
I've relaxed the bounds on |
This comment has been minimized.
This comment has been minimized.
|
The next step is to fix impl<'a, T: PointeeSized + Unsize<U>, U: PointeeSized> DispatchFromDyn<&'a U> for &'a T {}We also have implicit Imagine we have a trait To solve the problem, impl<'a, T: PointeeSized + Unsize<U> + ?Move, U: PointeeSized + ?Move> DispatchFromDyn<&'a U> for &'a T {}Same for other impls. |
This comment has been minimized.
This comment has been minimized.
|
@Bryanskiy thank you so much! I think we're getting closer. I started by adding the bounds you pointed out, and kept adding them to types until the dyn diagnostic errors stopped. Unfortunately this patch is still causing an ICE locally, and I'm not sure what to do about that. I've pushed the patch up, and expect CI to fail with the same error. But just in case, here are my local logs. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I believe we are hitting this check, which was added very recently: rust/compiler/rustc_errors/src/diagnostic.rs Lines 400 to 404 in adcb3d3 My suspicion is that this is picking up an existing issue in the implementation - which we weren't aware of because we've not been actively exercising the impl. If I disable the check, we get a lot further with compilation. But eventually it bottoms out with this error: logs. I'm not sure whether this is me doing something wrong, or that disabling the check was actually really bad. I suspect a bit of both - but figured I'd include it either way. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
I think triggering this check is a bug which may already exist and is reachable now. However, it's on the error path, so when hitting this error, we're already emitting a diagnostic, so the issue should already be something before this as if it were to compile, we'd never reach this 🤔 |
|
oh :x yeah ofc the issue is just candidate preference, before i explain this shit here, gonna write a rustc-dev-guide chapter about it, pls annoy me if i don't get to it by next tuesday i've had to link to FCP comments or explain this myself a few times. We should just have a single doc for it |
|
The issue is due to https://rustc-dev-guide.rust-lang.org/solve/candidate-preference.html#preference-over-impl-candidates And the reason this happens is that we add I think this breakage is pretty much unavoidable with this approach until we support OR-region constraints, if we ever do. |
|
In this case, should we revert to the default supertraits? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR implements the
Movemarker trait for the purposes of testing potential performance regressions in combination with #120706. For context about this experiment see this Zulip thread.r? @lcnr