Add detailed error explanation for E0509 #33383

cramertj · 2016-05-03T17:26:24Z

rust-highfive · 2016-05-03T17:26:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

GuillaumeGomez · 2016-05-03T18:03:19Z

src/librustc_borrowck/diagnostics.rs

+Here, we tried to move a field out of a struct of type `DropStruct` which
+implements the `Drop` trait.
+
+`Drop` types have an implicit destructor that gets called when they go out of


This expression is a bit unwieldy. The correct explanation would be that a struct cannot be dropped if one of its field(s) is borrowed.

Also, I don't like the expression "Drop types". I prefer to use "Types which implement Drop trait".

Thanks! I was trying to explain exactly why a struct couldn't be dropped if one of its field(s) is borrowed, but I agree it made the overall explanation harder to understand. I'll push a commit fixing that in a minute.

Also, I think we want "moved", not "borrowed" 👍.

GuillaumeGomez · 2016-05-03T18:04:13Z

Except for the problem I wrote about, it's very good. Thanks! Ping me once fixed please.

apasel422 · 2016-05-03T18:46:30Z

src/librustc_borrowck/diagnostics.rs

@@ -429,6 +429,93 @@ You can find more information about borrowing in the rust-book:
 http://doc.rust-lang.org/stable/book/references-and-borrowing.html
 "##,

+E0509: r##"
+This error occurs when an attempt is made to move out of a container that


I think this would be clearer as "move out of a value whose type implements the Drop trait", as "container" is a bit overloaded.

"...a value whose type implements..." sounds kind of awkward to me, but I agree it's less ambiguous.

cramertj · 2016-05-04T02:40:43Z

@GuillaumeGomez

GuillaumeGomez · 2016-05-04T07:29:20Z

Please squash your commits.

cramertj · 2016-05-04T23:22:40Z

@GuillaumeGomez

GuillaumeGomez · 2016-05-05T10:31:33Z

Thanks!

r=me @steveklabnik @Manishearth

Manishearth · 2016-05-05T10:34:22Z

src/librustc_borrowck/diagnostics.rs

+
+Here, we tried to move a field out of a struct of type `DropStruct` which
+implements the `Drop` trait. However, a struct cannot be dropped if one or
+more of its fields have been moved.


Could you explain why this is the case here?

(Moving a field out of a struct invalidates that field, but it might be used during the drop call. Drop structs should be thought of as a single unit which can't have its fields moved)

I actually gave an explanation similar to that in an earlier version of this PR. @GuillaumeGomez and I agreed that it was a bit unwieldy, so I replaced it with a simpler explanation. @GuillaumeGomez also had an issue with calling things Drop types or Drop structs, and preferred the language "a value whose type implements the Drop trait." I'll try to reconcile the two-- what do you think of this language:

"Structs implementing the Drop trait have an implicit destructor that gets called when they go out of scope. This destructor may use the fields of the struct, so moving out of the struct could make it impossible to run the destructor. Therefore, we must think of all values whose type implements the Drop trait as single units whose fields cannot be moved."

Definitely more complicated, but I think it captures both messages.

I propose: "Structs implementing the Drop trait have an implicit destructor that gets called when they go out of scope. However, the struct cannot be destroyed if one of its field is borrowed because it would make it unavailable and we'd have to face unsafety. Therefore, we must think of all values whose type implements the Drop trait as single units whose fields cannot be moved."

I'm fine with that.

Change made.

There's nothing wrong with a field being borrowed, though (the borrow ends befire the drop anyway), the issue is with a field being moved.

I prefer @cramertj's suggested text, though Guillaume's text works with s/borrow/move too.

@Manishearth Good point. I glossed over that. @GuillaumeGomez Shall I switch back to my original proposal, or would you rather go with a combination of the two (or your proposal with "borrowed" changed to "moved")? Personally, I'm not a huge fan of the "we'd have to face unsafety" language, but I recognize I'm the newcomer here so I'm prepared to give concessions 😄

@GuillaumeGomez @Manishearth How do you feel about the current language?

"This destructor may use the fields of the struct, so moving out of the struct could make it impossible to run the destructor." -> This part is still incorrect.

EDIT: Maybe more details to help you understand why: That's not the fact that the destructor might use the field which makes the borrowing an issue but the fact that once the structure is destroyed, if you have a reference to one of its fields, this reference becomes invalid and unsafe.

What? No, that's not the case, the destructor using it is exactly the reason, no more. Borrowck prevents references escaping.

Edited the error explanation for E0509 to clarify dropping of moved fields Edited the error explanation for E0509 to clarify move out of Drop value language Fixed typeo in last commit to E0509 Switched to erroneous code wording

Manishearth · 2016-05-08T13:06:20Z

@bors r+

bors · 2016-05-08T13:06:21Z

📌 Commit 5071728 has been approved by Manishearth

Manishearth · 2016-05-08T13:12:23Z

@bors rollup

GuillaumeGomez · 2016-05-08T13:45:40Z

@Manishearth: You agree with the sentence: "This destructor may use the fields of the struct, so moving out of the struct could make it impossible to run the destructor."?

Manishearth · 2016-05-08T13:45:52Z

Yes.

GuillaumeGomez · 2016-05-08T13:49:40Z

Fine then.

Add detailed error explanation for E0509 Part of rust-lang#32777

Rollup of 9 pull requests - Successful merges: #32900, #33129, #33365, #33383, #33474, #33478, #33480, #33484, #33493 - Failed merges: #33360

Add detailed error explanation for E0509 Part of rust-lang#32777

Rollup of 10 pull requests - Successful merges: #33129, #33224, #33370, #33383, #33431, #33474, #33480, #33496, #33509, #33514 - Failed merges:

Add detailed error explanation for E0509 Part of rust-lang#32777

Rollup of 10 pull requests - Successful merges: #33129, #33224, #33370, #33383, #33431, #33474, #33480, #33496, #33509, #33514 - Failed merges:

rust-highfive assigned Aatch May 3, 2016

GuillaumeGomez reviewed May 3, 2016
View reviewed changes

GuillaumeGomez assigned GuillaumeGomez and unassigned Aatch May 3, 2016

apasel422 reviewed May 3, 2016
View reviewed changes

cramertj force-pushed the E0509 branch 2 times, most recently from 5c1fe56 to e732fa0 Compare May 3, 2016 19:31

cramertj force-pushed the E0509 branch from e732fa0 to e052dcc Compare May 4, 2016 17:55

cramertj force-pushed the E0509 branch from e052dcc to b02a7a7 Compare May 5, 2016 00:05

Manishearth reviewed May 5, 2016
View reviewed changes

cramertj force-pushed the E0509 branch 2 times, most recently from e10bfdd to 0cf96d8 Compare May 6, 2016 17:44

Add detailed error explanation for E0509

5071728

Edited the error explanation for E0509 to clarify dropping of moved fields Edited the error explanation for E0509 to clarify move out of Drop value language Fixed typeo in last commit to E0509 Switched to erroneous code wording

cramertj force-pushed the E0509 branch from 0cf96d8 to 5071728 Compare May 6, 2016 17:45

Manishearth mentioned this pull request May 8, 2016

Rollup of 9 pull requests #33501

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request May 8, 2016

Rollup merge of rust-lang#33383 - cramertj:E0509, r=Manishearth

a44bbee

Add detailed error explanation for E0509 Part of rust-lang#32777

bors added a commit that referenced this pull request May 8, 2016

Auto merge of #33501 - Manishearth:rollup, r=Manishearth

6d369a8

Rollup of 9 pull requests - Successful merges: #32900, #33129, #33365, #33383, #33474, #33478, #33480, #33484, #33493 - Failed merges: #33360

Manishearth added a commit to Manishearth/rust that referenced this pull request May 9, 2016

Rollup merge of rust-lang#33383 - cramertj:E0509, r=Manishearth

a6d9261

Add detailed error explanation for E0509 Part of rust-lang#32777

Manishearth added a commit to Manishearth/rust that referenced this pull request May 9, 2016

Rollup merge of rust-lang#33383 - cramertj:E0509, r=Manishearth

8540e44

Add detailed error explanation for E0509 Part of rust-lang#32777

Manishearth mentioned this pull request May 9, 2016

Rollup of 10 pull requests #33523

Merged

bors added a commit that referenced this pull request May 9, 2016

Auto merge of #33523 - Manishearth:rollup, r=Manishearth

9d68267

Rollup of 10 pull requests - Successful merges: #33129, #33224, #33370, #33383, #33431, #33474, #33480, #33496, #33509, #33514 - Failed merges:

Manishearth added a commit to Manishearth/rust that referenced this pull request May 9, 2016

Rollup merge of rust-lang#33383 - cramertj:E0509, r=Manishearth

57fa783

Add detailed error explanation for E0509 Part of rust-lang#32777

bors added a commit that referenced this pull request May 9, 2016

Auto merge of #33523 - Manishearth:rollup, r=Manishearth

e0fd34b

Rollup of 10 pull requests - Successful merges: #33129, #33224, #33370, #33383, #33431, #33474, #33480, #33496, #33509, #33514 - Failed merges:

bors merged commit 5071728 into rust-lang:master May 10, 2016

dfockler added a commit to dfockler/rust that referenced this pull request May 12, 2016

Add error explanations for E0374, E0375, E0376 on issue rust-lang#33383

1e901de

cramertj deleted the E0509 branch May 25, 2016 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detailed error explanation for E0509 #33383

Add detailed error explanation for E0509 #33383

cramertj commented May 3, 2016

rust-highfive commented May 3, 2016

GuillaumeGomez May 3, 2016

cramertj May 3, 2016

cramertj May 3, 2016 •

edited

Loading

GuillaumeGomez commented May 3, 2016

apasel422 May 3, 2016

cramertj May 3, 2016

cramertj commented May 4, 2016

GuillaumeGomez commented May 4, 2016

cramertj commented May 4, 2016

GuillaumeGomez commented May 5, 2016

Manishearth May 5, 2016

cramertj May 5, 2016

GuillaumeGomez May 5, 2016

cramertj May 5, 2016

cramertj May 5, 2016

Manishearth May 6, 2016 •

edited

Loading

cramertj May 6, 2016

cramertj May 7, 2016

GuillaumeGomez May 8, 2016 •

edited

Loading

Manishearth May 8, 2016

Manishearth commented May 8, 2016

bors commented May 8, 2016

Manishearth commented May 8, 2016

GuillaumeGomez commented May 8, 2016

Manishearth commented May 8, 2016

GuillaumeGomez commented May 8, 2016

Add detailed error explanation for E0509 #33383

Add detailed error explanation for E0509 #33383

Conversation

cramertj commented May 3, 2016

rust-highfive commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj May 3, 2016 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented May 4, 2016

GuillaumeGomez commented May 4, 2016

cramertj commented May 4, 2016

GuillaumeGomez commented May 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth May 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez May 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented May 8, 2016

bors commented May 8, 2016

Manishearth commented May 8, 2016

GuillaumeGomez commented May 8, 2016

Manishearth commented May 8, 2016

GuillaumeGomez commented May 8, 2016

cramertj May 3, 2016 •

edited

Loading

Manishearth May 6, 2016 •

edited

Loading

GuillaumeGomez May 8, 2016 •

edited

Loading