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

Add detailed error explanation for E0509 #33383

Merged
merged 1 commit into from
May 10, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 95 additions & 1 deletion src/librustc_borrowck/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -429,6 +429,101 @@ 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 value whose type
implements the `Drop` trait.

Example of erroneous code:

```compile_fail
struct FancyNum {
num: usize
}

struct DropStruct {
fancy: FancyNum
}

impl Drop for DropStruct {
fn drop(&mut self) {
// Destruct DropStruct, possibly using FancyNum
}
}

fn main() {
let drop_struct = DropStruct{fancy: FancyNum{num: 5}};
let fancy_field = drop_struct.fancy; // Error E0509
println!("Fancy: {}", fancy_field.num);
// implicit call to `drop_struct.drop()` as drop_struct goes out of scope
}
```

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.
Copy link
Member

Choose a reason for hiding this comment

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

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)

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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."

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'm fine with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change made.

Copy link
Member

@Manishearth Manishearth May 6, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@GuillaumeGomez GuillaumeGomez May 8, 2016

Choose a reason for hiding this comment

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

"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.

Copy link
Member

Choose a reason for hiding this comment

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

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


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.

This error can be fixed by creating a reference to the fields of a struct,
enum, or tuple using the `ref` keyword:

```
struct FancyNum {
num: usize
}

struct DropStruct {
fancy: FancyNum
}

impl Drop for DropStruct {
fn drop(&mut self) {
// Destruct DropStruct, possibly using FancyNum
}
}

fn main() {
let drop_struct = DropStruct{fancy: FancyNum{num: 5}};
let ref fancy_field = drop_struct.fancy; // No more errors!
println!("Fancy: {}", fancy_field.num);
// implicit call to `drop_struct.drop()` as drop_struct goes out of scope
}
```

Note that this technique can also be used in the arms of a match expression:

```
struct FancyNum {
num: usize
}

enum DropEnum {
Fancy(FancyNum)
}

impl Drop for DropEnum {
fn drop(&mut self) {
// Destruct DropEnum, possibly using FancyNum
}
}

fn main() {
// Creates and enum of type `DropEnum`, which implements `Drop`
let drop_enum = DropEnum::Fancy(FancyNum{num: 10});
match drop_enum {
// Creates a reference to the inside of `DropEnum::Fancy`
DropEnum::Fancy(ref fancy_field) => // No error!
println!("It was fancy-- {}!", fancy_field.num),
}
// implicit call to `drop_enum.drop()` as drop_enum goes out of scope
}
```
"##,

}

register_diagnostics! {
@@ -443,6 +538,5 @@ register_diagnostics! {
E0505, // cannot move out of `..` because it is borrowed
E0506, // cannot assign to `..` because it is borrowed
E0508, // cannot move out of type `..`, a non-copy fixed-size array
E0509, // cannot move out of type `..`, which defines the `Drop` trait
E0524, // two closures require unique access to `..` at the same time
}