implement Move trait prototype#156018
Conversation
rustbot
added
S-waiting-on-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.
implement `Move` trait prototype
|
(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.
|
the |
| 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); |
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
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
| if find_attr!(tcx, crate, RustcNoImplicitBounds) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
want to move this out into add_implicit_bounds?
| 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 { |
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
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 🫠
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?
|
|
||
| if let Res::Def(DefKind::Trait, def_id) = trait_ref.path.res | ||
| && (tcx.is_lang_item(def_id, hir::LangItem::Sized) || tcx.is_default_trait(def_id)) | ||
| && (tcx.is_lang_item(def_id, hir::LangItem::Sized) || tcx.is_implicit_trait(def_id)) |
There was a problem hiding this comment.
isn't Sized an implicit trait
| } | ||
|
|
||
| // Don't use `add_implicit_bounds` directly to skip adding `Sized`. | ||
| self.add_implicit_move_bound( |
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
| } | ||
|
|
||
| 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)) |
There was a problem hiding this comment.
why are we not treating Sized as an implicit trait?
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...
| } | ||
|
|
||
| // Backward compatibility for default auto traits. | ||
| // Backward compatibility for default auto traits & `Move`. |
There was a problem hiding this comment.
this is somewhat meh, Move is a default auto trait 😅 ✨ , just not add default_auto_trait
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.
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.
bare min core support
This comment has been minimized.
This comment has been minimized.
|
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.
implement `Move` trait prototype
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9c60331): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking 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 2.6%, secondary 178.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 8.9%, secondary 407.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.4%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 501.019s -> 515.163s (2.82%) |
rustbot
added
perf-regression
| } | ||
| } else if let PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) = context | ||
| && move_tr | ||
| { |
There was a problem hiding this comment.
hmm, could u do a separate if tcx.features().move_trait() { match context { exhaustivethingy } }
| let mut sized_candidates = candidates.iter().filter(|c| { | ||
| matches!(c.candidate, SizedCandidate) | ||
| || (is_default_auto_trait | ||
| && matches!(c.candidate, AutoImplCandidate | ImplCandidate(..))) | ||
| }); |
There was a problem hiding this comment.
yeah 👍
please add a UI test for this and add use revisions to also test it in the new trait solver (and also add this preference there)
i think maintainability-wise, do we want to do
is_default_auto_trait && c.candidate.is_impl_candidate()`
| fn winnow_candidates( | ||
| &mut self, | ||
| has_non_region_infer: bool, | ||
| is_default_auto_trait: bool, |
There was a problem hiding this comment.
merge with CandidatePreferenceMode
6 participants
View all comments
Add a barebones implementation for
Move(#149607), pending some diagnostics changes & tests.r? lcnr