Fix generic Drop impls with trait bounds. #12403

eddyb · 2014-02-19T20:15:30Z

Fix generic Drop impls with trait bounds.
Fixes #4252.

flaper87 · 2014-02-19T20:55:31Z

👍

nikomatsakis · 2014-02-19T21:08:04Z

src/librustc/middle/typeck/check/vtable.rs

+    if has_trait_bounds(*type_param_defs) {
+        let vcx = VtableContext {
+            infcx: &infer::new_infer_ctxt(tcx),
+            param_env: &ty::construct_parameter_environment(tcx, None, [], [], [], id)


Why are you passing all empty vectors here? That doesn't seem right to me. Themethod_type_params and method_region_params should be empty, yes, but not the item ones, those should be drawn from generics

For normal type checking, those are taken from the enclosing function, which in this case isn't the callee, but the caller, a monomorphized function that is being translated. Possibly even a drop glue (maybe only drop glues?).

Yeah, right. Sounds good.

sfackler · 2014-02-19T21:09:21Z

Hurray!

huonw · 2014-02-19T21:18:01Z

src/librustc/middle/typeck/check/demand.rs

@@ -20,17 +20,17 @@ use syntax::codemap::Span;

 // Requires that the two types unify, and prints an error message if they
 // don't.
-pub fn suptype(fcx: @FnCtxt, sp: Span, expected: ty::t, actual: ty::t) {
+pub fn suptype(fcx: &FnCtxt, sp: Span, expected: ty::t, actual: ty::t) {


It'd be really nice to keep these in separate commits/PRs ;)

I've already split this into 3 commits, I have to take it one step at a time :).

…ecking a bit.

nikomatsakis · 2014-02-19T22:08:16Z

src/librustc/middle/trans/base.rs

+        let tsubsts = ty::substs {
+            regions: ty::ErasedRegions,
+            self_ty: None,
+            tps: /*bad*/ substs.to_owned()


Nit: Please remove this /* bad */. No need to flagellate ourselves for every copy.

… bounds.

nikomatsakis · 2014-02-19T22:13:06Z

This looks good (modulo nits) -- but there is something very questionable about supporting type parameter bounds in drop to begin with. I guess any such def'n requires #[unsafe_dtor]? The problem is, imagine I define a destructor like

impl<T:Foo> Drop for S<T> { ... }

and now I have s: S<U> where U does not implement Foo -- what happens when s is dropped? Wacko.

It only makes sense if we can guarantee that all instances of S are instantiated with types that meet Foo. Therefore, I think permitting bounds on destructors only really makes sense if we permit them on structure definitions as well. Which we ought to do. But that's a separate PR, I guess.

Long story short: r+ with nits addressed.

glaebhoerl · 2014-02-20T10:21:33Z

@nikomatsakis:

I have s: S<U> where U does not implement Foo -- what happens when s is dropped? Wacko.

It only makes sense if we can guarantee that all instances of S are instantiated with types that meet Foo.

I think there's another interpretation that's equally reasonable.

If you have

struct S<T> { a: Rc<int>, b: Option<~S<T>>, c: T }

without a Drop impl for S, then S still has a destructor, which will run the destructors of its fields.

Following similar logic, if you have

impl<T: Foo> Drop for S<T> { ... }

you could say that for A: Foo, the destructor for S<A> consists of the Drop impl you wrote plus the destructors of its fields, while for B: !Foo, the destructor for S<B> consists only of the latter (as if you had not written a Drop impl).

Along the same lines, you could also write things such as

impl Drop for S<bool> { ... }
impl Drop for S<char> { ... }

in which case the destructors for S<bool> and S<char> would run different user-provided code followed by the same field-destructors, and S<double> would only run the field destructors. The same coherence rules would presumably apply as for all other traits (so you couldn't have the impl<T: Foo> and the impls for individual types, because they potentially overlap).

The basic insight is that while they run together, the programmer-defined Drop impl and the compiler-generated drop glue for the fields are logically distinct entities. See #11855 for more thoughts along similar lines.

As in that other issue, while I believe all of this checks out on a logical/theoretical level, a big question is how it squares with the code humans might actually want to write, and their expectations. But even under the above semantics, if the programmer wants to rule out any S<B> where B: !Foo, she can just make its fields private and only export a smart constructor fn new<T: Foo>() -> S<T>, so the two things are at least somewhat orthogonal.

nikomatsakis · 2014-02-20T12:22:08Z

On Thu, Feb 20, 2014 at 02:21:36AM -0800, Gábor Lehel wrote:

I think there's another interpretation that's equally reasonable.

Yes, I considered this interpretation and rejected it, though it does
seem like it logically works. I mostly rejected it because it made
things in the compiler more complicated than I wanted them to be and I
didn't want to dive into thinking it all the way through. Lazy on my
part. It probably does work out, though, at least I can't see a reason
that it wouldn't right now.

…tsakis Fix generic Drop impls with trait bounds. Fixes #4252.

One of the most common ways to use the stdin stream is to read it line by line for a small program. In order to facilitate this common usage pattern, this commit changes the stdin() function to return a BufferedReader by default. A new `stdin_raw()` method was added to get access to the raw unbuffered stream. I have not changed the stdout or stderr methods because they are currently unable to flush in their destructor, but rust-lang#12403 should have just fixed that.

One of the most common ways to use the stdin stream is to read it line by line for a small program. In order to facilitate this common usage pattern, this commit changes the stdin() function to return a BufferedReader by default. A new `stdin_raw()` method was added to get access to the raw unbuffered stream. I have not changed the stdout or stderr methods because they are currently unable to flush in their destructor, but #12403 should have just fixed that.

Pointers cannot be converted to integers at compile time Fix rust-lang#12402 changelog: [`transmutes_expressible_as_ptr_casts`]: do not suggest invalid const casts

nikomatsakis reviewed Feb 19, 2014
View reviewed changes

huonw reviewed Feb 19, 2014
View reviewed changes

eddyb added 2 commits February 19, 2014 23:26

Don't recurse twice into ExprParen in vtable checking.

1ca1ff2

Replaced method_map_entry with method_origin and cleaned up vtable ch…

efef078

…ecking a bit.

nikomatsakis reviewed Feb 19, 2014
View reviewed changes

Resolve the vtables for method calls to generic Drop impls with trait…

efaa1ea

… bounds.

bors added a commit that referenced this pull request Feb 20, 2014

auto merge of #12403 : eddyb/rust/generic-dtors-with-bounds, r=nikoma…

06e1281

…tsakis Fix generic Drop impls with trait bounds. Fixes #4252.

bors closed this Feb 20, 2014

bors merged commit efaa1ea into rust-lang:master Feb 20, 2014

alexcrichton mentioned this pull request Feb 20, 2014

Return a buffered stdin by default. #12422

Merged

eddyb deleted the generic-dtors-with-bounds branch February 22, 2014 17:32

bluss mentioned this pull request Jun 6, 2014

ICE if generic destructor has several type parameters #14695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix generic Drop impls with trait bounds. #12403

Fix generic Drop impls with trait bounds. #12403

eddyb commented Feb 19, 2014

flaper87 commented Feb 19, 2014

nikomatsakis Feb 19, 2014

eddyb Feb 19, 2014

nikomatsakis Feb 19, 2014

sfackler commented Feb 19, 2014

huonw Feb 19, 2014

eddyb Feb 19, 2014

nikomatsakis Feb 19, 2014

nikomatsakis Feb 19, 2014

nikomatsakis commented Feb 19, 2014

glaebhoerl commented Feb 20, 2014

nikomatsakis commented Feb 20, 2014

Fix generic Drop impls with trait bounds. #12403

Fix generic Drop impls with trait bounds. #12403

Conversation

eddyb commented Feb 19, 2014

flaper87 commented Feb 19, 2014

nikomatsakis Feb 19, 2014

Choose a reason for hiding this comment

eddyb Feb 19, 2014

Choose a reason for hiding this comment

nikomatsakis Feb 19, 2014

Choose a reason for hiding this comment

sfackler commented Feb 19, 2014

huonw Feb 19, 2014

Choose a reason for hiding this comment

eddyb Feb 19, 2014

Choose a reason for hiding this comment

nikomatsakis Feb 19, 2014

Choose a reason for hiding this comment

nikomatsakis Feb 19, 2014

Choose a reason for hiding this comment

nikomatsakis commented Feb 19, 2014

glaebhoerl commented Feb 20, 2014

nikomatsakis commented Feb 20, 2014