Move trait prototype - #161457
Conversation
it should give some info tho...
|
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. |
This comment has been minimized.
This comment has been minimized.
|
A quick |
Oh wow that got done with A LOT of tests
wow that were a lot of them. looks like the next batch is a bunch of |
This comment has been minimized.
This comment has been minimized.
|
looking at the errors, there is also a bunch of Those are all checks going around the That sure raises a question - can we handle them now? ummm - putting this aside in favour of lower hanging fruits |
This is one of the two options, the other being break ABI. I suspect we can do it, Rust not having a stable one (?)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mostly to avoid adding to every single dynamic symbol the `+ Move`
the output uses the debug print and parses it with regex fixed the regex to fetch the first trait, instead of Move
33dfdac to
30609e6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| // We don't support empty trait objects. | ||
| if regular_traits.is_empty() && auto_traits.is_empty() { | ||
| if regular_traits.iter().all(|t| tcx.is_implicit_trait(t.0.skip_binder().def_id(), false)) |
There was a problem hiding this comment.
hmm, feel like this can just be regular_traits.chain(auto_traits).all(is_implicit_trait)
also existing, but can you replace the bool of is_implicit_trait wth an enum, e.g.
enum WhateverThisfunctionwants {
Yes,
No,
}
*[View changes since the review](https://triagebot.infra.rust-lang.org/gh-changes-since/rust-lang/rust/161457/9a4ad59ae3073b013cd62f53f8349ddc61a012e8..5a6f61532811493d3a1fd88aecf71ce42594ec2e)*There was a problem hiding this comment.
Ops, wanted to clean that up but forgot. Will do. Also yeah, that would make much sense in readability. I can see it being extended in the future so will probably be more beneficial after
| write!(self, " + ")?; | ||
| } | ||
| first = false; | ||
| write!(self, "PointeeSized")?; |
There was a problem hiding this comment.
do we want somethign like... we do repeat this pattern a lot
let mut first = false;
let mut print_bound = |bound| {
if !first {
write!(self, " + ")?;
first = false;
}
self.write_str(bound)
};
*[View changes since the review](https://triagebot.infra.rust-lang.org/gh-changes-since/rust-lang/rust/161457/9a4ad59ae3073b013cd62f53f8349ddc61a012e8..5a6f61532811493d3a1fd88aecf71ce42594ec2e)*There was a problem hiding this comment.
Addressed - there are probably a ton of other similar cases of sparse printing with a separator, so i just wrote a helper struct to make code cleaner
| .collect() | ||
| } else { | ||
| clauses.into_iter().collect() | ||
| }; |
There was a problem hiding this comment.
that one is unfortunately somewhat problematic. gather_explicit_clauses_of is used by a query whose result we write to crate metadata, so this would erase the Move bound from upstream crates which don't have the feature enabled.
Why is this needed
There was a problem hiding this comment.
Of the changes that I made this is the one i was less sure of
I was trying to fix this test
The original test expected
error[E0091]: type parameter `N` is never used
--> $DIR/unused-type-param-suggestion.rs:25:8
|
LL | type D<N: ?Sized> = ();
| ^ unused type parameter
|
= help: consider removing `N` or referring to it in the body of the type alias
and after the change with Move it appeared a new help message = help: if you intended Nto be a const parameter, useconst N: /* Type */ instead
now, the help should not appear as there is a bound - so i checked from where it came, and got to read the check_type_alias_type_params_are_used that uses some sort of euristic to split huser written bounds and the sized hierarchy (?)
I honestly lost the plot there, but noticed that it was calling the _explicit_ version. And I saw the doc comment of gather_explicit_clauses_of noting that implied and inferred constraints should not appear there, and Move is indeed implicit in that case, so I just aligned it
There was a problem hiding this comment.
And I saw the doc comment of
gather_explicit_clauses_ofnoting that implied and inferred constraints should not appear there, andMoveis indeed implicit in that case, so I just aligned it
Default bounds (or what this PR now also calls "implicit bounds") are excluded from that. Without knowing all the places where we'll perform Move elaboration I would probably revert that.
The comment on gather_explicit_clauses_of doesn't consider these bounds to be potential implied bounds (in the sense of implied supertrait bounds, implicit outlives-bounds, etc.). You can see that gather_explicit_clauses_of does elaborate sizedness & other default bounds.
That's because default bounds plus relaxed bounds are just considered syntax sugar / an early syntactic transformation.
There was a problem hiding this comment.
now, the help should not appear as there is a bound - so i checked from where it came, and got to read the
check_type_alias_type_params_are_usedthat uses some sort of euristic to split huser written bounds and the sized hierarchy (?)
Regarding the heuristic, it utilizes the fact that implicitly added Sized (etc.) bounds have the same span as the type parameter they annotate (in order to identify which bounds to "count").
So for (pseudo) <T>, the span of the implicit T: Sized is identical to the span of type parameter T. The span that's used comes from explicit_clauses_of / gather_explicit_clauses_of.
I would check what the span we use for implicit T: Move bounds (T type param). If it indeed shares the span with the type param, then there must be something else going on.
There was a problem hiding this comment.
Okkkkkkkk
managed to find the correct solution!
it was a fixme that needed handling at the time (cause spans for different lines were not able to be merged
so yeah. now it works with c372a25
This comment has been minimized.
This comment has been minimized.
429ec3c to
a068654
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0acfa71 to
c51c5c0
Compare
This comment has been minimized.
This comment has been minimized.
…eature is" This reverts commit eda1c34.
handled Move as an additional bound instead of removing it from the gathered explicit clauses addressed an old FIXME that gave me the direction reverted the changes of a couple of tests that now see the Move
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
| {T: PointeeSized + ?Move} *mut T, | ||
| {T: PointeeSized + ?Move} &T, | ||
| {T: PointeeSized + ?Move} &mut T, | ||
| {T: PointeeSized + ?Move} PhantomData<T>, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
generally I see in the future a proliferation of Phantom* types (like #135806) to represent different aspects you want to inherit from the type
| result.clauses = tcx.arena.alloc_from_iter(result.clauses.iter().copied().filter(|p| { | ||
| !p.0.as_trait_clause().is_some_and(|p| { | ||
| p.polarity() == ClausePolarity::Positive | ||
| && matches!(tcx.as_lang_item(p.def_id()), Some(LangItem::Move)) |
There was a problem hiding this comment.
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 😁
| hir_bounds, | ||
| ImpliedBoundsContext::AssociatedTypeOrImplTrait, | ||
| span, | ||
| true, |
There was a problem hiding this comment.
that bool should also be a newtype enum 🤣 no clue what it means without looking at the function definition
View all comments
Finishing the work started by @nia-e in #156018 :
TODO
feature(move_trait)MoveMoveMoveboundsdyn MovePhantomData<T>: MoveandT: Movedieselregressesr? lcnr