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

Misleading error with Drop and lifetime parameters #17858

Closed
bnoordhuis opened this issue Oct 7, 2014 · 10 comments
Closed

Misleading error with Drop and lifetime parameters #17858

bnoordhuis opened this issue Oct 7, 2014 · 10 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints

Comments

@bnoordhuis
Copy link
Contributor

struct S<'a>(&'a T);

struct T;

impl<'a> Drop for S<'a> {
    fn drop(&mut self) {}
}

fn main() {
    let _ = S(&T);
}
$ rustc -v
rustc 0.12.0-nightly (b5ba2f551 2014-10-06 20:27:14 +0000)

$ rustc dtor.rs 
dtor.rs:5:1: 7:2 error: cannot implement a destructor on a structure with type parameters [E0141]
dtor.rs:5 impl<'a> Drop for S<'a> {
dtor.rs:6     fn drop(&mut self) {}
dtor.rs:7 }
dtor.rs:5:1: 7:2 note: use "#[unsafe_destructor]" on the implementation to force the compiler to allow this
dtor.rs:5 impl<'a> Drop for S<'a> {
dtor.rs:6     fn drop(&mut self) {}
dtor.rs:7 }
error: aborting due to previous error

It's not clear to me why this program gets rejected.

@alexcrichton
Copy link
Member

I think the error message may just be a little misleading, if you opt into unsafe_destructor it should compile though

#![feature(unsafe_destructor)]

struct S<'a>(&'a T);

struct T;

#[unsafe_destructor]
impl<'a> Drop for S<'a> {
    fn drop(&mut self) {}
}

fn main() {
    let _ = S(&T);
}

@bnoordhuis
Copy link
Contributor Author

It does but I don't quite understand why the compiler rejects the program. Does the presence of the lifetime make it unsound?

@alexcrichton
Copy link
Member

Right now the compiler's handling of lifetimes and destructors aren't quite sound in all cases. For example, in your example, it should be required that the destructor of S run before the destructor to T (otherwise S may see uninitialized memory).

Whereas, in this example (which compiles today), the destructor for T runs first:

#![feature(unsafe_destructor)]

struct S<'a>(&'a T);

struct T;

impl<'a> S<'a> {
    fn foo(&self) {}
}

#[unsafe_destructor]
impl<'a> Drop for S<'a> {
    fn drop(&mut self) {
        println!("dropping S");
    }
}

impl Drop for T {
    fn drop(&mut self) {
        println!("dropping T");
    }
}

fn main() {
    {
        println!("before!");
        let t = T;
        S(&t).foo()
    };
    println!("after!");

}
$ rustc -v
rustc 0.12.0-nightly (b5ba2f551 2014-10-06 20:27:14 +0000)
$ rustc foo.rs 
$ ./foo 
before!
dropping T
dropping S
after!

@alexcrichton
Copy link
Member

And for a real world use case, you can see my comment which uses a standard RefCell to generate unsafe reads and writes.

@bnoordhuis
Copy link
Contributor Author

Thanks Alex, that makes perfect sense. Should I close the issue?

@alexcrichton alexcrichton changed the title impl Drop disallowed with E0141 for types with lifetimes Misleading error with Drop and lifetime parameters Oct 8, 2014
@alexcrichton
Copy link
Member

I think the error message could still use some improvement (mention lifetime parameters, for example), so I'm ok leaving this open to track that.

@huonw huonw added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 8, 2014
@Byron
Copy link
Member

Byron commented Feb 9, 2015

There is something I don't understand about the example though: We explicitly specify that the lifetime of the &T must match the one of the S instance which contains it, and as Rust assures that, the destructor should be safe. After all, we specified that the reference to T needs to live at least as long as S. It seems the actual problem is that we didn't actually express that instance of T shall outlive instance of S - as far as Rust is concerned, it only knows T must live as long as S, which might make the destructor a special case, as technically, an instance of S is about to decease.

After all, it appears there might be a problem with either expressiveness of lifetimes, or with the way destructors are handled, which is exactly what you said :) : "Right now the compiler's handling of lifetimes and destructors aren't quite sound in all cases.". Good I arrived at the same conclusion.

Another concern of mine is also stated in #11406/#8861: there may be an inflationary use of #![feature(unsafe_destructor)] of people who don't really understand the implications (which includes me, usually). Therefore I hope this will be fixed until 1.0 is released to prevent unsafe_destructors to pop up everywhere.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 9, 2015

@Byron this is exactly the topic of rust-lang/rfcs#769 , which has a prototype implementation (see #21972) essentially ready to land (and should land for 1.0), and that should remove the unsoundness discussed here.

@nham
Copy link
Contributor

nham commented May 12, 2015

I suspect this issue can be closed. Error E0141 is no longer in the code base, and the example code now produces this error:

E0141.rs:10:16: 10:17 error: borrowed value does not live long enough
E0141.rs:10     let _ = S(&T);
                           ^
E0141.rs:10:5: 10:19 note: reference must be valid for the destruction scope surrounding statement at 10:4...
E0141.rs:10     let _ = S(&T);
                ^~~~~~~~~~~~~~
E0141.rs:10:5: 10:19 note: ...but borrowed value is only valid for the statement at 10:4
E0141.rs:10     let _ = S(&T);
                ^~~~~~~~~~~~~~
E0141.rs:10:5: 10:19 help: consider using a `let` binding to increase its lifetime
E0141.rs:10     let _ = S(&T);
                ^~~~~~~~~~~~~~
error: aborting due to previous error

(rustc 1.1.0-nightly (dc630d01e 2015-05-09) (built 2015-05-10))

@pnkfelix pnkfelix self-assigned this May 12, 2015
@steveklabnik
Copy link
Member

Agree with @nham

lnicola pushed a commit to lnicola/rust that referenced this issue Sep 25, 2024
feat: render patterns in params for hovering

Fix rust-lang#17858

This PR introduces an option to [hir-def/src/body/pretty.rs](https://github.com/rust-lang/rust-analyzer/blob/08c7bbc2dbe4dcc8968484f1a0e1e6fe7a1d4f6d/crates/hir-def/src/body/pretty.rs) to render the result as a single line, which is then reused for rendering patterns in parameters for hovering.
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
Projects
None yet
Development

No branches or pull requests

7 participants