Add long diagnostics for "bind by-ref and by-move" #24482
Conversation
|
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -112,6 +112,40 @@ reference when using guards or refactor the entire expression, perhaps by | |||
| putting the condition inside the body of the arm. | |||
| "##, | |||
|
|
|||
| E0009: r##" | |||
| In a pattern, all values that don't implement the Copy trait have to be binded the same way | |||
There was a problem hiding this comment.
nit: s/binded/bound
Preferably keep "Copy" between backticks
There was a problem hiding this comment.
I think it's also more natural if we leave off the "than the others".
|
I'd prefer if code was marked as such, both with inline backticks and fences. It makes it easier to read. Aside from that and a couple of nits, r=me |
|
@Manishearth, @michaelsproul: Thanks for your comments ! I corrected what you pointed. Do you see remaining things that could be improved ? |
| @@ -112,6 +112,49 @@ reference when using guards or refactor the entire expression, perhaps by | |||
| putting the condition inside the body of the arm. | |||
| "##, | |||
|
|
|||
| E0009: r##" | |||
| In a pattern, all values that don't implement the `Copy` trait have to be bound | |||
| the same way. The goal here is to avoid to bind by-move and by-ref at the same | |||
There was a problem hiding this comment.
"The goal here is to avoid binding simultaneous by-move and by-ref"
|
teensy bit |
|
I feel like this doesn't give a good reason for why bind by-move and by-ref are disallowed in the same pattern. Does anyone know? I've been unable to come up with an example that would violate memory safety. Partial moves are allowed by the compiler, which seem almost identical. Is this error due to a limitation in the compiler's ability to reason about data-overlap in patterns? |
So if we have |
|
@michaelsproul: Hum... Don't you think it would be too much ? I thought here was to give a help to the developer in order to make him understand what he made wrong and how to fix it. |
|
@GuillaumeGomez: I think meaningful justifications both help people learn and make them happier to follow recommendations. @Manishearth: I thought that too, but essentially the same thing works if you do it step by step: fn main() {
let p = ("x".to_string(), "y".to_string());
let (_, ref y) = p;
let (x, _) = p;
println!("{} {}", x, y);
} |
|
Huh. That should work then. |
| } | ||
| ``` | ||
|
|
||
| You have two solutions: either you implement the `Copy` trait on the X structure: |
There was a problem hiding this comment.
This line is longer than 80 characters, might be best flowing across the next line:
There are two possible solutions.
1. Implement the `Copy` trait for the X structure:
// Code here
2. Bind the pattern's names the same way:
// Code here
(there's also a "the the" in the current version of the second case)
|
Thanks for doing this, sorry about the endless comments! 😄 (I avoided this error specifically because it looked hard) |
|
@michaelsproul: No problem, it's very kind of you to help me like this ! I actually took this error because I didn't know what it was, I discovered a whole new type of errors haha. |
| ``` | ||
|
|
||
| You have two solutions: | ||
| 1. Implement the `Copy` trait for the X structure: |
There was a problem hiding this comment.
I don't think this is a particularly good piece of advice. While it does indeed make the code in question compile, implementing Copy is often undesirable (such as for wide structures) and has its own set of ramifications that we should perhaps expand on here, or link to a document that does that.
There was a problem hiding this comment.
I see your point. Should I precise that it would be better to do the second solution ?
There was a problem hiding this comment.
Yes, and make the second solution the first solution.
|
@jakub- @Manishearth: Done. Do you see anything else ? |
|
And it's good. If anyone does see something else to change, please just ask ! |
|
Thanks! Could you squash the commits together as well? |
GuillaumeGomez
force-pushed
the
error-explanation
branch
2 times, most recently
from
bd1e778 to
9a15234
Compare
|
@alexcrichton: Done ! |
|
@bors: rollup |
|
☔ The latest upstream changes (presumably #24512) made this pull request unmergeable. Please resolve the merge conflicts. |
…=alexcrichton Part of rust-lang#24407.
GuillaumeGomez
force-pushed
the
error-explanation
branch
from
9a15234 to
cf53f03
Compare
|
I resolved the conflicts. |
|
Closing in favor of the merged version at #24542 |
I did a manual merge of all the extended error PRs as we were getting merge conflicts yesterday. I think this is preferable to merging separately as I ended up having to manually merge @nham and @GuillaumeGomez's commits. Rollup of rust-lang#24458, rust-lang#24482 and rust-lang#24488. rust-lang#24482 and rust-lang#24488 were already re-approved, and would need to be cancelled if this is merged instead.
Part of #24407.