Change Copy to Clone in trait requirements as appropriate #65331

Xaeroxe · 2019-10-12T10:40:12Z

I spotted a few places in std requiring Copy where we didn't appear to be getting any benefit from requiring it over Clone. Clone is implemented for more types, so it seems preferable to use it where we can.

This won't break any backwards compatibility as Clone is a requirement of Copy.

rust-highfive · 2019-10-12T10:40:22Z

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2019-10-12T10:53:29Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-12T10:46:32.4077856Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-12T10:46:32.4161885Z ##[command]git config gc.auto 0
2019-10-12T10:46:32.4244545Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-12T10:46:32.4328393Z ##[command]git config --get-all http.proxy
2019-10-12T10:46:32.4467313Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65331/merge:refs/remotes/pull/65331/merge
---
2019-10-12T10:51:50.2216512Z     Checking backtrace v0.3.37
2019-10-12T10:51:51.4588590Z error[E0507]: cannot move out of a shared reference
2019-10-12T10:51:51.4589089Z     --> src/liballoc/collections/btree/map.rs:1766:43
2019-10-12T10:51:51.4589334Z      |
2019-10-12T10:51:51.4589666Z 1766 |         self.extend(iter.into_iter().map(|(&key, &value)| (key.clone(), value.clone())));
2019-10-12T10:51:51.4590039Z      |                                           ^^---^^^^^^^^^
2019-10-12T10:51:51.4590642Z      |                                             data moved here
2019-10-12T10:51:51.4590642Z      |                                             data moved here
2019-10-12T10:51:51.4591084Z      |                                             move occurs because `key` has type `K`, which does not implement the `Copy` trait
2019-10-12T10:51:51.4591848Z error[E0507]: cannot move out of a shared reference
2019-10-12T10:51:51.4592217Z     --> src/liballoc/collections/btree/map.rs:1766:43
2019-10-12T10:51:51.4592440Z      |
2019-10-12T10:51:51.4592440Z      |
2019-10-12T10:51:51.4592771Z 1766 |         self.extend(iter.into_iter().map(|(&key, &value)| (key.clone(), value.clone())));
2019-10-12T10:51:51.4593460Z      |                                                   |
2019-10-12T10:51:51.4593798Z      |                                                   data moved here
2019-10-12T10:51:51.4593798Z      |                                                   data moved here
2019-10-12T10:51:51.4594362Z      |                                                   move occurs because `value` has type `V`, which does not implement the `Copy` trait
2019-10-12T10:51:52.0041920Z error: aborting due to 2 previous errors
2019-10-12T10:51:52.0042047Z 
2019-10-12T10:51:52.0042386Z For more information about this error, try `rustc --explain E0507`.
2019-10-12T10:51:52.0304569Z error: could not compile `alloc`.
---
2019-10-12T10:51:52.0388921Z == clock drift check ==
2019-10-12T10:51:52.0408478Z   local time: Sat Oct 12 10:51:52 UTC 2019
2019-10-12T10:51:52.1902685Z   network time: Sat, 12 Oct 2019 10:51:52 GMT
2019-10-12T10:51:52.1905801Z == end clock drift check ==
2019-10-12T10:51:53.1873022Z ##[error]Bash exited with code '1'.
2019-10-12T10:51:53.1915716Z ##[section]Starting: Checkout
2019-10-12T10:51:53.1917568Z ==============================================================================
2019-10-12T10:51:53.1917645Z Task         : Get sources
2019-10-12T10:51:53.1917694Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2019-10-12T11:06:35Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-12T10:59:23.9720162Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-12T10:59:23.9737706Z ##[command]git config gc.auto 0
2019-10-12T10:59:23.9743024Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-12T10:59:23.9746918Z ##[command]git config --get-all http.proxy
2019-10-12T10:59:23.9752807Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65331/merge:refs/remotes/pull/65331/merge
---
2019-10-12T11:05:00.5455230Z     Checking panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
2019-10-12T11:05:03.4322003Z error[E0599]: no method named `cloned` found for type `T` in the current scope
2019-10-12T11:05:03.4324550Z     --> src/libstd/collections/hash/map.rs:2435:31
2019-10-12T11:05:03.4325512Z      |
2019-10-12T11:05:03.4326718Z 2435 |         self.base.extend(iter.cloned())
2019-10-12T11:05:03.4327280Z      |                               ^^^^^^ method not found in `T`
2019-10-12T11:05:03.4327896Z      = note: the method `cloned` exists but the following trait bounds were not satisfied:
2019-10-12T11:05:03.4327896Z      = note: the method `cloned` exists but the following trait bounds were not satisfied:
2019-10-12T11:05:03.4328133Z              `&mut T : core::iter::Iterator`
2019-10-12T11:05:03.4328524Z      = help: items from traits can only be used if the type parameter is bounded by the trait
2019-10-12T11:05:03.4328848Z help: the following trait defines an item `cloned`, perhaps you need to restrict type parameter `T` with it:
2019-10-12T11:05:03.4329270Z      |
2019-10-12T11:05:03.4329609Z 2434 |     fn extend<T: core::iter::Iterator + IntoIterator<Item = (&'a K, &'a V)>>(&mut self, iter: T) {
2019-10-12T11:05:03.4329932Z 
2019-10-12T11:05:04.9136817Z error: aborting due to previous error
2019-10-12T11:05:04.9136938Z 
2019-10-12T11:05:04.9137292Z For more information about this error, try `rustc --explain E0599`.
---
2019-10-12T11:05:04.9651627Z == clock drift check ==
2019-10-12T11:05:04.9676728Z   local time: Sat Oct 12 11:05:04 UTC 2019
2019-10-12T11:05:05.0573289Z   network time: Sat, 12 Oct 2019 11:05:05 GMT
2019-10-12T11:05:05.0573483Z == end clock drift check ==
2019-10-12T11:05:05.9299864Z ##[error]Bash exited with code '1'.
2019-10-12T11:05:05.9344776Z ##[section]Starting: Checkout
2019-10-12T11:05:05.9346883Z ==============================================================================
2019-10-12T11:05:05.9346957Z Task         : Get sources
2019-10-12T11:05:05.9347004Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Centril · 2019-10-12T16:54:51Z

(Have you double checked that there's no unsafe { ... } code somewhere in the standard library making use of Copy for soundness?)

bluss · 2019-10-12T17:05:19Z

This is about Extend<&T> where T: Copy (and similar), those have been conservatively chosen to use Copy in the past, but it can of course be revisited. I don't have issue links.

rust-highfive · 2019-10-12T19:52:44Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-12T19:43:15.3873268Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-12T19:43:15.3980735Z ##[command]git config gc.auto 0
2019-10-12T19:43:15.4046268Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-12T19:43:15.4101245Z ##[command]git config --get-all http.proxy
2019-10-12T19:43:15.4238147Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65331/merge:refs/remotes/pull/65331/merge
---
2019-10-12T19:49:02.8075491Z     Checking panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
2019-10-12T19:49:05.2374083Z error[E0599]: no method named `cloned` found for type `T` in the current scope
2019-10-12T19:49:05.2375854Z     --> src/libstd/collections/hash/map.rs:2435:31
2019-10-12T19:49:05.2376158Z      |
2019-10-12T19:49:05.2376771Z 2435 |         self.base.extend(iter.cloned())
2019-10-12T19:49:05.2377065Z      |                               ^^^^^^ method not found in `T`
2019-10-12T19:49:05.2377551Z      = note: the method `cloned` exists but the following trait bounds were not satisfied:
2019-10-12T19:49:05.2377551Z      = note: the method `cloned` exists but the following trait bounds were not satisfied:
2019-10-12T19:49:05.2378032Z              `&mut T : core::iter::Iterator`
2019-10-12T19:49:05.2378398Z      = help: items from traits can only be used if the type parameter is bounded by the trait
2019-10-12T19:49:05.2378676Z help: the following trait defines an item `cloned`, perhaps you need to restrict type parameter `T` with it:
2019-10-12T19:49:05.2379024Z      |
2019-10-12T19:49:05.2381662Z 2434 |     fn extend<T: core::iter::Iterator + IntoIterator<Item = (&'a K, &'a V)>>(&mut self, iter: T) {
2019-10-12T19:49:05.2382515Z 
2019-10-12T19:49:06.5642083Z error: aborting due to previous error
2019-10-12T19:49:06.5642896Z 
2019-10-12T19:49:06.5643562Z For more information about this error, try `rustc --explain E0599`.
---
2019-10-12T19:49:06.6089719Z == clock drift check ==
2019-10-12T19:49:06.6107562Z   local time: Sat Oct 12 19:49:06 UTC 2019
2019-10-12T19:49:06.7610721Z   network time: Sat, 12 Oct 2019 19:49:06 GMT
2019-10-12T19:49:06.7617364Z == end clock drift check ==
2019-10-12T19:49:07.9916693Z ##[error]Bash exited with code '1'.
2019-10-12T19:49:07.9959564Z ##[section]Starting: Checkout
2019-10-12T19:49:07.9960993Z ==============================================================================
2019-10-12T19:49:07.9961036Z Task         : Get sources
2019-10-12T19:49:07.9961077Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

sinkuu · 2019-10-12T21:27:48Z

RFC 839 states Copy bound here was a purposeful choice: https://rust-lang.github.io/rfcs/0839-embrace-extend-extinguish.html

rust-highfive · 2019-10-14T01:19:19Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-14T00:00:07.0051516Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-14T00:00:07.0131602Z ##[command]git config gc.auto 0
2019-10-14T00:00:07.0210349Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-14T00:00:07.0259522Z ##[command]git config --get-all http.proxy
2019-10-14T00:00:07.0386861Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65331/merge:refs/remotes/pull/65331/merge
---
2019-10-14T00:08:40.4578865Z     Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
2019-10-14T00:08:42.5615283Z error[E0282]: type annotations needed for `&&T`
2019-10-14T00:08:42.5616241Z     --> src/librustdoc/html/render.rs:2515:25
2019-10-14T00:08:42.5616694Z      |
2019-10-14T00:08:42.5617474Z 2515 |             .partition(|i| i.inner_impl().synthetic);
2019-10-14T00:08:42.5618156Z      |                         ^ consider giving this closure parameter the explicit type `&&T`, where the type parameter `T` is specified
2019-10-14T00:08:42.5619100Z      = note: type must be known at this point
2019-10-14T00:08:42.5619341Z 
2019-10-14T00:08:42.5619341Z 
2019-10-14T00:08:42.7102286Z error[E0277]: the trait bound `std::vec::Vec<&&html::render::Impl>: std::iter::Extend<&html::render::Impl>` is not satisfied
2019-10-14T00:08:42.7102633Z     --> src/librustdoc/html/render.rs:3202:14
2019-10-14T00:08:42.7102905Z      |
2019-10-14T00:08:42.7108331Z 3202 |             .partition(|t| t.inner_impl().synthetic);
2019-10-14T00:08:42.7108993Z      |              ^^^^^^^^^ the trait `std::iter::Extend<&html::render::Impl>` is not implemented for `std::vec::Vec<&&html::render::Impl>`
2019-10-14T00:08:42.7109756Z      = help: the following implementations were found:
2019-10-14T00:08:42.7109756Z      = help: the following implementations were found:
2019-10-14T00:08:42.7110857Z                <std::vec::Vec<T> as std::iter::Extend<&'a T>>
2019-10-14T00:08:42.7111302Z                <std::vec::Vec<T> as std::iter::Extend<T>>
2019-10-14T00:08:43.1234277Z error: aborting due to 2 previous errors
2019-10-14T00:08:43.1234387Z 
2019-10-14T00:08:43.1234619Z Some errors have detailed explanations: E0277, E0282.
2019-10-14T00:08:43.1234853Z For more information about an error, try `rustc --explain E0277`.
---
2019-10-14T00:08:43.1661574Z == clock drift check ==
2019-10-14T00:08:43.1677567Z   local time: Mon Oct 14 00:08:43 UTC 2019
2019-10-14T00:08:43.3181226Z   network time: Mon, 14 Oct 2019 00:08:43 GMT
2019-10-14T00:08:43.3181328Z == end clock drift check ==
2019-10-14T00:08:44.2868646Z ##[error]Bash exited with code '1'.
2019-10-14T00:08:44.3037646Z ##[section]Starting: Checkout
2019-10-14T00:08:44.3039684Z ==============================================================================
2019-10-14T00:08:44.3040481Z Task         : Get sources
2019-10-14T00:08:44.3041186Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Xaeroxe · 2019-10-14T01:29:28Z

This appears to have introduced a minor backwards compatibility problem due to the new ambiguity between &T and T extend calls. New trait implementations is oftentimes considered acceptable breakage, however I don't feel like I have enough background knowledge to say whether this is or not.

TimNN · 2019-10-14T16:08:13Z

cc @rust-lang/libs

JohnCSimon · 2019-10-26T02:46:29Z

Pinging from triage - please review, this has sat idle for the past 11 days.
@rust-lang/libs
CC: @TimNN @Xaeroxe
Thanks.

SimonSapin · 2019-10-26T06:41:10Z

With #65331 (comment) on how the status quo was a design choice and #65331 (comment) on how this change introduces introduces compat issues, I think we’ll want more motivation than “it seems preferable”.

What are some concrete use cases that this enables? Can they all also be achieved today with an extra .cloned() call in an iterator chain? Are they worth doing the crater dance and dealing with regressions?

Xaeroxe · 2019-10-26T08:01:59Z

These are all excellent questions, whose answers do not motivate merging this right now. I just spotted an arbitrary improvement I could make, and made it. I have no use case, and no motivation for this other than "why not?"

With that said, I'm closing this.

SimonSapin · 2019-10-26T08:15:29Z

Thank you for looking into this, even if it it didn’t work out!

rust-highfive assigned TimNN Oct 12, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2019

Xaeroxe force-pushed the rm-copy branch from 10558f1 to 3e769c1 Compare October 12, 2019 10:45

Xaeroxe force-pushed the rm-copy branch 2 times, most recently from 7967870 to 6e82c19 Compare October 12, 2019 10:58

bluss added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 12, 2019

Xaeroxe force-pushed the rm-copy branch from 6e82c19 to b5f5aab Compare October 12, 2019 18:45

Change Copy to Clone in trait requirements as appropriate

1d51a8a

Xaeroxe force-pushed the rm-copy branch from b5f5aab to 1d51a8a Compare October 13, 2019 23:59

Xaeroxe closed this Oct 26, 2019

Xaeroxe deleted the rm-copy branch August 3, 2020 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Copy to Clone in trait requirements as appropriate #65331

Change Copy to Clone in trait requirements as appropriate #65331

Xaeroxe commented Oct 12, 2019

rust-highfive commented Oct 12, 2019

rust-highfive commented Oct 12, 2019

rust-highfive commented Oct 12, 2019

Centril commented Oct 12, 2019

bluss commented Oct 12, 2019 •

edited

Loading

rust-highfive commented Oct 12, 2019

sinkuu commented Oct 12, 2019 •

edited

Loading

rust-highfive commented Oct 14, 2019

Xaeroxe commented Oct 14, 2019

TimNN commented Oct 14, 2019

JohnCSimon commented Oct 26, 2019

SimonSapin commented Oct 26, 2019

Xaeroxe commented Oct 26, 2019

SimonSapin commented Oct 26, 2019

Change Copy to Clone in trait requirements as appropriate #65331

Change Copy to Clone in trait requirements as appropriate #65331

Conversation

Xaeroxe commented Oct 12, 2019

rust-highfive commented Oct 12, 2019

rust-highfive commented Oct 12, 2019

rust-highfive commented Oct 12, 2019

Centril commented Oct 12, 2019

bluss commented Oct 12, 2019 • edited Loading

rust-highfive commented Oct 12, 2019

sinkuu commented Oct 12, 2019 • edited Loading

rust-highfive commented Oct 14, 2019

Xaeroxe commented Oct 14, 2019

TimNN commented Oct 14, 2019

JohnCSimon commented Oct 26, 2019

SimonSapin commented Oct 26, 2019

Xaeroxe commented Oct 26, 2019

SimonSapin commented Oct 26, 2019

bluss commented Oct 12, 2019 •

edited

Loading

sinkuu commented Oct 12, 2019 •

edited

Loading