Avoid repeated trait bounds in derived impls #27134
Conversation
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
|
|
||
| for field_ty in field_tys { | ||
| let tys = find_type_parameters(&*field_ty, &ty_param_names); | ||
|
|
||
| for ty in tys { | ||
| // if we have already handled this type, skip it | ||
| if let ast::TyPath(_, ref p) = ty.node { | ||
| if processed_types.contains(&p.segments) { continue } |
There was a problem hiding this comment.
This can be if !processed_types.insert(&p.segments) { continue }.
|
The old behaviour is because it was a very simple rule ( I don't really know where to go... maybe we can preserve the old behaviour while still removing the double trait bounds? (E.g. check if type parameters are in the |
|
(I'd r+ in a heartbeat if the patch just deduplicates, I'm unsure about the other changes... I want it, but it'd be nice to do it "properly", so that the examples I mention above are also handled but it's unclear how we can do this best.) |
|
Yeah, I can see the consistency argument. Fixing the bounds "properly" would be a much bigger change, and I'm not actually sure it can be done with |
It can sort-of be done by instead inserting
It's something that probably makes sense to handle in rustdoc too but this is a more general problem: it could be much better at converting source code into a "canonical" representation. |
|
@huonw Ok, I have amended my changes to keep the bounds on all type parameters. The example signature now looks as follows: impl <I: ::std::clone::Clone,
U: ::std::clone::Clone + IntoIterator,
F: ::std::clone::Clone>
::std::clone::Clone for FlatMap<I, U, F>
where U::IntoIter: ::std::clone::Clone { |
| // if we have already handled this type, skip it | ||
| if let ast::TyPath(_, ref p) = ty.node { | ||
| if p.segments.len() == 1 | ||
| && ty_param_names.contains(&p.segments[0].identifier.name) |
There was a problem hiding this comment.
Maybe we could just add these to the process_fields_types HashSet to begin with; processed_field_types.extend(ty_param_names.iter().cloned()).
There was a problem hiding this comment.
I don't think so, unless I'm misunderstanding your comment. ty_param_names are ast::Names, while the elements of processed_field_types are Vec<ast::PathSegment>. It's not clear to me that we can somehow convert one into the other for comparison.
|
@bors r+ |
|
📌 Commit 93e5a74 has been approved by |
|
⌛ Testing commit 93e5a74 with merge cde83e1... |
|
💔 Test failed - auto-mac-64-opt |
|
@bors: retry On Mon, Aug 3, 2015 at 10:35 AM, bors [email protected] wrote:
|
Fixes #25022 This adapts the deriving mechanism to not repeat bounds for the same type parameter. To give an example: for the following code: ```rust #[derive(Clone)] pub struct FlatMap<I, U: IntoIterator, F> { iter: I, f: F, frontiter: Option<U::IntoIter>, backiter: Option<U::IntoIter>, } ``` the latest nightly generates the following impl signature: ```rust impl <I: ::std::clone::Clone, U: ::std::clone::Clone + IntoIterator, F: ::std::clone::Clone> ::std::clone::Clone for FlatMap<I, U, F> where I: ::std::clone::Clone, F: ::std::clone::Clone, U::IntoIter: ::std::clone::Clone, U::IntoIter: ::std::clone::Clone ``` With these changes, the signature changes to this: ```rust impl <I, U: IntoIterator, F> ::std::clone::Clone for FlatMap<I, U, F> where I: ::std::clone::Clone, F: ::std::clone::Clone, U::IntoIter: ::std::clone::Clone ``` (Nothing in the body of the impl changes) Note that the second impl is more permissive, as it doesn't have a `Clone` bound on `U` at all. There was a compile-fail test that failed due to this. I don't understand why we would want the old behaviour (and nobody on IRC could tell me either), so please tell me if there is a good reason that I missed.
Fixes #25022
This adapts the deriving mechanism to not repeat bounds for the same type parameter. To give an example: for the following code:
the latest nightly generates the following impl signature:
With these changes, the signature changes to this:
(Nothing in the body of the impl changes)
Note that the second impl is more permissive, as it doesn't have a
Clonebound onUat all. There was a compile-fail test that failed due to this. I don't understand why we would want the old behaviour (and nobody on IRC could tell me either), so please tell me if there is a good reason that I missed.