Generate non-frozen strings on TruffleRuby for consistency#963
Generate non-frozen strings on TruffleRuby for consistency#963byroot merged 1 commit intoruby:masterfrom
Conversation
| if empty? | ||
| state.depth -= 1 | ||
| return '{}' | ||
| return +'{}' |
There was a problem hiding this comment.
It would be faster to do the +/.dup in .dump vs in every "empty case" as most of these do not need to be mutable.
Probably just .dup here would be enough:
https://github.com/rwstauner/json/blob/cd2f0195a2824bcb88e2173f1f4ecf7ec68528cc/lib/json/truffle_ruby/generator.rb#L352
I'll try it.
Though that wouldn't work if people expect obj.to_json to return a mutable String, but not sure there is such an expectation. Notably true.to_s is already frozen?, even on CRuby.
There was a problem hiding this comment.
Though that wouldn't work if people expect
obj.to_jsonto return a mutable String, but not sure there is such an expectation.
It's best to stay consistent with the C version that is largely used, and it does always return a mutable string.
There was a problem hiding this comment.
Mmh, OTOH it's natural to have frozen strings when implementing in Ruby and rather unusual to have frozen strings in C, so I wonder if the semantics here are more influenced by that than intentional. We have some differences in core classes in TruffleRuby like frozen exception messages and we made the choice to mostly keep them frozen.
I think it's worth doing #964, but if you disagree let me know and I'll just drop the first commit.
The second commit is safe regardless.
There was a problem hiding this comment.
so I wonder if the semantics here are more influenced by that than intentional.
Of course, but my point is that regardless of the intent, it has been this way for a long time and may cause a compatibility issue.
Which is worse because it means to_json would sometimes return frozen string sometimes mutable ones, meaning it's very easy to miss that case when testing. So I'm not in favor of method returning either, it has to be consistent.
It's not that rare to have code such as object.to_json << "\n".
…o_json method * To avoid extra unnecessary allocations. * See ruby#963
3 participants
I encountered an error from activesupport because it is expecting generated json to not be frozen:
https://github.com/rails/rails/blob/4df808965b4a1e42cbd4a3ad0764d30a18a5e1b1/activesupport/lib/active_support/json/encoding.rb#L188-L192
While we might want to protect against that there, this also seemed unintentional and inconsistent so I figured I would start here.