Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make std::io::copy take Read and Write by value instead of by reference #111063

Closed
wants to merge 2 commits into from

Conversation

est31
Copy link
Member

@est31 est31 commented May 1, 2023

The traits are also implemented on mutable references of types implementing that trait.

The initial goal of this PR is to get a crater run done so that impact of breakage can be estimated, then a decision can be made on #105643.

@rustbot
Copy link
Collaborator

rustbot commented May 1, 2023

r? @thomcc

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@est31
Copy link
Member Author

est31 commented May 1, 2023

@bors try

@bors
Copy link
Contributor

bors commented May 1, 2023

⌛ Trying commit 8a7c47d21d4b3af4bb15c663615a6fac7bb8c32d with merge 57441dc4961028bc64c66456cc33e98669c830a9...

@rust-log-analyzer

This comment has been minimized.

@est31 est31 force-pushed the io_copy_by_val branch from 8a7c47d to 54bebd5 Compare May 1, 2023 14:00
@rust-log-analyzer

This comment has been minimized.

@est31 est31 force-pushed the io_copy_by_val branch from 54bebd5 to d63f5ff Compare May 1, 2023 19:18
@est31
Copy link
Member Author

est31 commented May 1, 2023

@bors try

@bors
Copy link
Contributor

bors commented May 1, 2023

⌛ Trying commit d63f5ff4dc0993baa817f360ba578eec0374d36e with merge 5cc6cc22f274979bcd2a65b62b719ae3a4f42519...

@bors
Copy link
Contributor

bors commented May 1, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2023
@est31
Copy link
Member Author

est31 commented May 1, 2023

This has a good start... 😆

 Documenting stage2 cargo (x86_64-unknown-linux-gnu)
[...]
    Checking gix-odb v0.42.0
error[E0382]: borrow of moved value: `buf`
  --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/gix-config-0.18.0/src/file/init/from_paths.rs:71:13
   |
57 |         buf: &mut Vec<u8>,
   |         --- move occurs because `buf` has type `&mut Vec<u8>`, which does not implement the `Copy` trait
...
71 |             buf.clear();
   |             ^^^^^^^^^^^ value borrowed here after move
...
78 |                 buf,
   |                 --- value moved here, in previous iteration of loop

error[E0382]: borrow of moved value: `buf`
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/gix-config-0.18.0/src/file/includes/mod.rs:101:9
    |
88  |     buf: &mut Vec<u8>,
    |     --- move occurs because `buf` has type `&mut Vec<u8>`, which does not implement the `Copy` trait
89  | ) -> Result<(), Error> {
90  |     for (section_id, config_path) in section_ids_and_include_paths {
    |     -------------------------------------------------------------- inside of this loop
...
101 |         buf.clear();
    |         ^^^^^^^^^^^ value borrowed here after move
102 |         std::io::copy(&mut std::fs::File::open(&config_path)?, buf)?;
    |                                                                --- value moved here, in previous iteration of loop

@rustbot
Copy link
Collaborator

rustbot commented May 2, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@est31 est31 marked this pull request as draft May 2, 2023 01:51
@est31
Copy link
Member Author

est31 commented May 2, 2023

@bors try

@bors
Copy link
Contributor

bors commented May 2, 2023

⌛ Trying commit 8ea0e58e58d1f65761c3ee23802b37d9daf77ff1 with merge 98822f45d8d7b85b11ab1d06265216f1eae4b469...

@bors
Copy link
Contributor

bors commented May 2, 2023

💔 Test failed - checks-actions

@est31 est31 force-pushed the io_copy_by_val branch from 8ea0e58 to bebcd58 Compare May 2, 2023 19:12
@est31
Copy link
Member Author

est31 commented May 2, 2023

@bors try

@bors
Copy link
Contributor

bors commented May 2, 2023

⌛ Trying commit bebcd5866b468f95748402852fde98f130e346ca with merge 275392a4242c37cea20935e91a1f83a43ce59ccc...

@bors
Copy link
Contributor

bors commented May 2, 2023

💔 Test failed - checks-actions

est31 added 2 commits May 4, 2023 06:25
The traits are also implemented on mutable references of types implementing that trait.
@est31 est31 force-pushed the io_copy_by_val branch from bebcd58 to 81617fe Compare May 4, 2023 04:45
@est31
Copy link
Member Author

est31 commented May 4, 2023

@bors try

@bors
Copy link
Contributor

bors commented May 4, 2023

⌛ Trying commit 81617fe with merge a326ccb4ce868265c4f69a0270f3033164acc4f9...

@bors
Copy link
Contributor

bors commented May 4, 2023

☀️ Try build successful - checks-actions
Build commit: a326ccb4ce868265c4f69a0270f3033164acc4f9 (a326ccb4ce868265c4f69a0270f3033164acc4f9)

@Amanieu
Copy link
Member

Amanieu commented May 4, 2023

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-111063 created and queued.
🤖 Automatically detected try build a326ccb4ce868265c4f69a0270f3033164acc4f9
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 4, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-111063 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-111063 is completed!
📊 1091 regressed and 3 fixed (266148 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 5, 2023
@est31
Copy link
Member Author

est31 commented May 5, 2023

It seems that the large impact of a thousand regressed crates is caused by just one crate on its own, exr (in various versions of that crate; 1.4.1, 1.4.2, 1.5.0, 1.5.1, 1.5.2, 1.5.3, 1.6.3). The other regressions are fairly minor. Thankfully the cargo published to crates.io doesn't use gitoxide yet, otherwise all the tools using cargo would be regressed, too.

@Mark-Simulacrum
Copy link
Member

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@lqd
Copy link
Member

lqd commented May 5, 2023

@craterbot
Copy link
Collaborator

👌 Experiment pr-111063-1 created and queued.
🤖 Automatically detected try build a326ccb4ce868265c4f69a0270f3033164acc4f9
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-111063-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-111063-1 is completed!
📊 1002 regressed and 0 fixed (1091 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 6, 2023
@est31
Copy link
Member Author

est31 commented May 6, 2023

Similar results as above, but thankfully now some of the noise got cleaned up.

Some analysis: of the 1002 regressions, 881 are caused by different versions of exr. The remaining 120 are either root regressions (42), or other crates used by those. The biggest two groups are cfb (17 regressions), and various gix crates (21 regressions, but I expect this to increase the time the gix adoption in cargo hits stable).

Sometimes a good suggestion is shown, but most of the time, there is no suggestion.

In general, I think if one wants to patch this, one has to release patched releases for both multiple versions of gix as well as for exr.

@est31
Copy link
Member Author

est31 commented May 6, 2023

I've obtained the information that I wanted to obtain with this PR: the results from the crater run. If the libs team decides to go towards the direction of doing this, then we can open a new PR that isn't as full with bot spam as this one :).

@est31 est31 closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants