HN Simulatornew | past | comments | lists | submitlogin

I've written dozens of custom Drop implementations. Well, many of them are named cleanup because of the async Drop mess in Rust, but...


How many of them only conditionally implement Drop, based on a generic param lifetime?


Why would you ever conditionally custom impl Drop? I don't think I've ever seen that pattern, and I can't think of a use case.


Therefore explaining why the bug hadn't been hit in practice for 11 years.


On further reading, the Drop impl is a red herring. The bug is unrelated to Drop. It's there in the repro to show the effects of invalid assignment, but even without impl Drop the bug is still there.


The bug in the OP is about UNconditional impl. And honestly, looks like very standard code, definitely something that could've been written for real. I can't even explain why it passes checks, it clearly shouldn't.


This code, from the bug submission?

  impl Drop for Q {
      fn drop(&mut self) { if let Some(t) = self.0.take() { KEPT.lock().unwrap().push(Box::new(t)); } }
  }
The bound on T in the Drop impl means Drop is not implemented for all T, only where T: 'static + Send. Q also having the same bound doesn't make a difference to the Drop impl.


You cannot write a type which conditionally implements Drop: https://play.rust-lang.org/?version=stable&mode=debug&editio...

    struct Q(Option);

    impl Drop for Q {
        fn drop(&mut self) {}
    }


    error[E0367]: `Drop` impl requires `T: 'static` but the struct it is implemented for does not
     --> src/main.rs:3:9
      |
    3 | impl Drop for Q {
      |         ^^^^^^^
      |
    note: the implementor must specify the same requirement
     --> src/main.rs:1:1
      |
    1 | struct Q(Option);
      | ^^^^^^^^^^^


Even if the bounds are the same on the struct and the impl, it's still true that placing bounds on the Drop impl means it's not implemented for all T (just all T that also satisfy Q).

You also don't have to place those bounds on the Drop impl just because they're on Q, that's not an error.


The Drop isn't implemented for T at all. It's only implemented for Q. And it's implemented for all Q, in the sense that for every value of type Q, this Drop impl applies.

And I'm still not any closer to understanding what the bug is about.


Sorry, yes. I mean that Drop isn't implemented for all Q, just all Q where T meet the bounds of legally constructing Q. I note the difference because you can write a Drop impl with no bounds on T, and I believe it would not be able to trigger the bug.


This is so stupid. One, why did they make Drop special in this one way when every other trait impl has to repeat all bounds exactly to be valid? Two, the bug isn't even in the Drop impl, it's in the assignment on line 11. q.0 can never be Some(&l) because &l is not 'static. The Drop impl is there just to show the invalid read, but the bug doesn't require it, it's just as wrong for types that don't impl Drop.


They didn't special case this (in this way at least). You only need to specify the bounds relevant to this impl at this point.

It doesn't even have to be less constrained than the containing types definition. You can provide an impl with more constraints; for example, a type Q, but impl Seek for Q


I thought only as constrained and more constrained is allowed, and less constrained is always an error. At least that's been my experience - if `Q`, then I could never get `impl Q` working in any conditions until I added `where T: A`.

Do you happen to know the specific rules of when it's an error and when it's not?


It looks like you're right, my memory is wrong re. trait bounds. My guess then is that it only works for lifetimes because they involve subtyping, and impl Drop for all T means that it's valid for more constrained lifetimes.

That said I was wrong earlier.


OK. That's good to know. I'm not sure why sharing our professional experience with this type of code is controversial, but here we are.




Guidelines | FAQ | Lists | API | Security | DMCA | Apply to YC | Contact

Search: