Skip to content

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 24, 2022

I think that Closure/ParamTy/Opaque are all "opaque" enough that it's meaningful to suggest borrowing them instead of moving them at their usage sites when we see a move error. See the attached issue for example.

Is this suggestion too general? I could perhaps use the move site information to limit this to places like fn calls, but I don't know enough about mir borrowck to know if that's an easy change.

Fixes #90828

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 24, 2022
@rust-highfive
Copy link
Contributor

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2022
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your change looks good to me, can you add a comment why this has to happen only if needs_note is true? Or potentially even experiment with always adding this new suggestion?

fn report_use_of_moved_or_uninitialized sure is a horrible function '^^ would be nice to extract all of this into separate functions. While I don't expect you to do that in this PR, can you move your new changes into a separate one?

@compiler-errors
Copy link
Member Author

Yeah, needs_note doesn't mean anything to this suggestion. Also I refactored out two of the suggestions near the end of report_use_of_moved_or_uninitialized into their own helpers.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after the formatting fix for let chains

@bors delegate+

`needs_note` is false if we've already suggested why the type is Copy...
but that has nothing to do with the diagnostic.
@compiler-errors
Copy link
Member Author

compiler-errors commented Mar 29, 2022

I formatted the let chains with a patched rustfmt (rust-lang/rustfmt#5203), and also applied the let-else suggestions asked for.

@bors r=lcnr

@bors
Copy link
Collaborator

bors commented Mar 29, 2022

📌 Commit ac95e80 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2022
@bors
Copy link
Collaborator

bors commented Mar 29, 2022

⌛ Testing commit ac95e80 with merge abf0ec8...

@bors
Copy link
Collaborator

bors commented Mar 29, 2022

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing abf0ec8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2022
@bors bors merged commit abf0ec8 into rust-lang:master Mar 29, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 29, 2022
@bors bors mentioned this pull request Mar 29, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (abf0ec8): comparison url.

Summary: This benchmark run did not return any relevant results. 5 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@compiler-errors compiler-errors deleted the fn-borrow branch April 7, 2022 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When FnMut or Fn closure is moved and then used, the compiler should suggest borrowing it instead
6 participants