`Value`s blow the stack on drop when deeply nested

[brson]

It seems like Value has the default recursive Drop implementation, and if it gets too deep then Drop hits the end of the stack.

I recently had to write a wrapper type that took apart a serde_json::Value to avoid it recursing. Seems like this could be something serde_json might do on its own.

Didn't see an existing issue on this.

[dtolnay]

Wow, thanks. Can you share the code for your wrapper or send a PR?

[brson]

@dtolnay I took a stab at a patch, but unfortunately it's complicated to do in-tree because implementing Drop makes it impossible to destructure Value.

Here's my branch anyway: https://github.com/brson/json/tree/drop

And the commit: brson@25a12c7

This is similar to what I implemented privately in a wrapper but different because I didn't need to handle the case when Array is the root, so IDK if it's quite right. That's the gist anyway.

[brson]

Here's a selection of the build errors that happen when adding Drop:

    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
308 |     deserialize_prim_number!(deserialize_i32);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
309 |     deserialize_prim_number!(deserialize_i64);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
310 |     deserialize_prim_number!(deserialize_u8);
    |     ----------------------------------------- in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
311 |     deserialize_prim_number!(deserialize_u16);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
312 |     deserialize_prim_number!(deserialize_u32);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
313 |     deserialize_prim_number!(deserialize_u64);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
314 |     deserialize_prim_number!(deserialize_f32);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:234:17
    |
234 |                 Value::Number(n) => n.deserialize_any(visitor),
    |                 ^^^^^^^^^^^^^^-^
    |                 |             |
    |                 |             hint: to prevent move, use `ref n` or `ref mut n`
    |                 cannot move out of here
...
315 |     deserialize_prim_number!(deserialize_f64);
    |     ------------------------------------------ in this macro invocation

error[E0509]: cannot move out of type `value::Value`, which implements the `Drop` trait
   --> src/value/de.rs:299:13
    |
299 |             Value::Number(n) => n.deserialize_any(visitor),
    |             ^^^^^^^^^^^^^^-^
    |             |             |
    |             |             hint: to prevent move, use `ref n` or `ref mut n`
    |             cannot move out of here

[dtolnay]

Hmm, that's unfortunate all around. We cannot implement Drop for Value because it breaks existing destructuring code, we cannot implement Drop for Vec<Value> because it overlaps Vec<T>'s impl, we cannot implement Drop for serde_json::Map<String, Value> because implementations of Drop cannot apply to a specialized choice of type parameters, and we cannot implement Drop for serde_json::Map<K, V> because that requires a recursive call to V's drop.

If this were implemented as a method, would that solve your use case or do you really need there to be a wrapper?

impl Value {
    fn drop(self) { /* the clever */ }
}

[brson]

@dtolnay Having that code in-tree would be marginally better than not just because it would be upstream. I'd still root my Value in a wrapper type on my end to avoid mistakes though.

It is unfortunate.

[brson]

Hm, well actually, having that by-value drop would probably just make it harder to implement the wrapper type downstream, because then the wrapper has to use a RefCell or something to make its own dtor work.

[remexre]

I wrote up a simple implementation of this; tested with n in 0..(2<<24) on my own machine and it works flawlessly (if a bit slowly); the playground doesn't let me allocate that much memory though.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=1b68b66537ab03b160e443f692ce7af8

EDIT: Should've read the whole issue, ignore me :P

[Yoric]

Hmm, that's unfortunate all around. We cannot implement Drop for Value because it breaks existing destructuring code, we cannot implement Drop for Vec because it overlaps Vec's impl, we cannot implement Drop for serde_json::Map<String, Value> because implementations of Drop cannot apply to a specialized choice of type parameters, and we cannot implement Drop for serde_json::Map<K, V> because that requires a recursive call to V's drop.

Do you actually need to expose a (forall K, V) Map<K, V> in serde-json? If Value defined

pub enum Value {
  ...
  Object(Object)
}

pub struct Object(Map<String, Value>);
impl Drop for Object {
   ...
}

This would already solve the problem in most cases, right?