Skip to content

Conversation

@etiennebarrie
Copy link
Contributor

@etiennebarrie etiennebarrie commented Nov 28, 2025

Follow up to #903

When serializing an Array, and one of the elements of the Array requires calling to_json, if the depth is changed, it will be used for the next entries, which wasn't the case before
5abd434, and is not the case with TruffleRuby and JRuby.

Additionally, with TruffleRuby and JRuby the state's depth after the to_json call is used to close the Array, which isn't the case with CRuby.

When serializing an Array, and one of the elements of the Array requires
calling `to_json`, if the depth is changed, it will be used for the next
entries, which wasn't the case before
5abd434, and is not the case with
TruffleRuby and JRuby.

Additionally, with TruffleRuby and JRuby the state's depth after the
`to_json` call is used to close the Array, which isn't the case with
CRuby.
@byroot
Copy link
Member

byroot commented Nov 28, 2025

Additionally, with TruffleRuby and JRuby the state's depth after the to_json call is used to close the Array, which isn't the case with CRuby.

Can't we fix that behavior rather than to encode it in test?

@etiennebarrie
Copy link
Contributor Author

Yeah I wasn't sure, I have other things I'd like to change about depth and I though maybe we could put them all together with a deprecation cycle.

state = JSON::State.new
state.generate([].tap { _1 << _1 }) rescue nil
state.depth # => 100
obj = Object.new
def obj.to_json(state)
  state.depth += 1
  "1"
end
state.generate(obj)
state.depth # => 1

In both cases I think we should return 0.

@byroot
Copy link
Member

byroot commented Nov 28, 2025

Generally speaking, I want to stop having a mutable State object.

Also generally speaking, I don't think anyone really use the dept argument, so if the behavior is currently wonky I'm fine changing it without deprecation.

@byroot
Copy link
Member

byroot commented Nov 28, 2025

Generally speaking, I want to stop having a mutable State object.

Reminds me, I should deprecate all the State#attr= methods. You should only set the state config at initialization, and never mutate it later.

@etiennebarrie
Copy link
Contributor Author

Reminds me, I should deprecate all the State#attr= methods. You should only set the state config at initialization, and never mutate it later.

I think we'll need to keep depth mutable just for the old to_json(state) API maybe? Or it will be hard to get parity with TruffleRuby given it's all implemented by mutating depth.

@byroot
Copy link
Member

byroot commented Nov 29, 2025

Or it will be hard to get parity with TruffleRuby given it's all implemented by mutating depth.

Truffle can use an undocumented method like increment_depth / decrement_depth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants