`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: https://github.com/brson/json/commit/25a12c71f14f4b7112ed9f016fed31e966d0c466

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

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?