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

Note when a a move/borrow error is caused by a deref coercion #75304

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

Aaron1011
Copy link
Member

Fixes #73268

When a deref coercion occurs, we may end up with a move error if the
base value has been partially moved out of. However, we do not indicate
anywhere that a deref coercion is occuring, resulting in an error
message with a confusing span.

This PR adds an explicit note to move errors when a deref coercion is
involved. We mention the name of the type that the deref-coercion
resolved to, as well as the Deref::Target associated type being used.

@rust-highfive
Copy link
Collaborator

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 Aug 8, 2020
@lcnr
Copy link
Contributor

lcnr commented Aug 8, 2020

hmm, I don't see any obvious problems here, but am not really familiar enough with what's touched here,

maybe r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned lcnr Aug 8, 2020
|
LL | type Target = NotCopy;
| ^^^^^^^^^^^^^^^^^^^^^^
= note: move occurs because `val.first` has type `NotCopy`, which does not implement the `Copy` trait
Copy link
Contributor

Choose a reason for hiding this comment

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

This note seems like it could go as the label of the main span in the previous note.

There's something missing in the new output, but I can't quite put my finger on it. Having said that, we can land this PR and improve it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should attach it to the span of val.inner;, since that's referring to the borrow. However, I think it would make sense to reorder it to go before the 'deref defined here' note.

@Aaron1011 Aaron1011 force-pushed the feature/diag-deref-move-out branch from 0c52521 to 3cf4bbc Compare August 12, 2020 15:05
@Aaron1011
Copy link
Member Author

@estebank I've addressed your comments

@Aaron1011
Copy link
Member Author

@estebank Are there any changes that you'd like me to make?

@bors

This comment has been minimized.

@Aaron1011 Aaron1011 force-pushed the feature/diag-deref-move-out branch from 6c1030e to 6893db8 Compare August 31, 2020 17:23
@Aaron1011
Copy link
Member Author

@estebank: I've resolved the merge conflicts.

@bors

This comment has been minimized.

@Aaron1011 Aaron1011 force-pushed the feature/diag-deref-move-out branch 2 times, most recently from 57f8be6 to 67d9ee6 Compare September 5, 2020 17:25
@bors

This comment has been minimized.

Fixes rust-lang#73268

When a deref coercion occurs, we may end up with a move error if the
base value has been partially moved out of. However, we do not indicate
anywhere that a deref coercion is occuring, resulting in an error
message with a confusing span.

This PR adds an explicit note to move errors when a deref coercion is
involved. We mention the name of the type that the deref-coercion
resolved to, as well as the `Deref::Target` associated type being used.
@Aaron1011 Aaron1011 force-pushed the feature/diag-deref-move-out branch from 67d9ee6 to d18b4bb Compare September 11, 2020 00:56
@Aaron1011
Copy link
Member Author

@estebank: This is now ready for review

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 15, 2020
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 15, 2020

📌 Commit d18b4bb has been approved by estebank

@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 Sep 15, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#73955 (deny(unsafe_op_in_unsafe_fn) in libstd/process.rs)
 - rust-lang#75146 (Detect overflow in proc_macro_server subspan)
 - rust-lang#75304 (Note when a a move/borrow error is caused by a deref coercion)
 - rust-lang#75749 (Consolidate some duplicate code in the sys modules.)
 - rust-lang#75882 (Use translated variable for test string)
 - rust-lang#75886 (Test that bounds checks are elided for [..index] after .position())
 - rust-lang#76048 (Initial support for riscv32gc_unknown_linux_gnu)
 - rust-lang#76198 (Make some Ordering methods const)
 - rust-lang#76689 (Upgrade to pulldown-cmark 0.8.0)
 - rust-lang#76763 (Update cargo)

Failed merges:

r? `@ghost`
@bors bors merged commit fa4cfeb into rust-lang:master Sep 16, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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.

Confusing error message on deref-coercion after partial move out
7 participants