Make GeneratorState#_configure only write the options it is given - #1077
Merged
Merged
Conversation
The C extension walks the given hash and writes only the keys present (generator.c:1860), and since ruby#1076 the pure generator does the same. The Java one re-wrote every option with its class default, so any configure or merge silently reset max_nesting, allow_nan, ascii_only, script_safe, strict, buffer_initial_length, allow_duplicate_key, as_json and sort_keys. Re-enables the two tests ruby#1076 marked pending on JRuby.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1076, which aligned the pure generator and left the Java one, marking the gap with two
omit ... if RUBY_ENGINE == 'jruby'lines. This closes it and re-enables both tests.GeneratorState#_configurere-wrote every option with its class default, so anyconfigure/mergereset the options it was not given:The
allow_nanreset is the sharpest — a state that generatedNaNa moment earlier starts raisingJSON::GeneratorError. Through the public output,JSON.state.new(sort_keys: true)after anyconfiguregave{"b":1,"a":2}on JRuby and{"a":2,"b":1}elsewhere. A string option given as an explicitnilhad the mirror problem, keeping the previous value instead of clearing.configure_state_i(generator.c:1860) is anif/else ifchain overrb_hash_foreach, so it writes only the keys present. The non-string options now take the current field as their default, which is the minimal change that makes "absent" mean "leave alone":OptionsReader.getdelegates toRubyHash#delete: absent gives Javanull(keep current), present-but-nilgivesnil(falsy), which is whatlong_config/RTESTdo on the C side.as_jsonandsort_keysneed an explicithasKey, and the five string options need a small helper becausegetStringreturnsnullfor absent and falsy.hasKeydoes not delete, soensureEmpty()still rejects unknown keywords.Verification
Built with
rake create_jarinjruby:9.4-jdk21, every row rebuilt from scratch, each row'sGeneratorState.javamd5 distinct.masterhasKeyguardsort_keysunguardedmax_nestingback toDEFAULT_MAX_NESTINGas_jsonunguardedThe
as_jsonrow is why there is a third test: with only the two re-enabled tests that mutant survived, so the guard would have shipped uncovered. I checked the new assertion against the C extension first — it passes there unchanged.Java is not compiled on CRuby and the two
omits werejruby-guarded, so those rows already ran these tests. TruffleRuby loadslib/json/truffle_ruby/generator.rb, which #1076 already fixed; I did not run that engine.Not measured:
hasKeyadds a second hash lookup foras_json/sort_keys.configureis not the generate hot path, but I have not benchmarked it.If you would prefer the C shape here — iterate the given hash once and switch on the key — I am happy to redo it that way; it is larger because
ensureEmptyrelies ongetdeleting as it consumes.Disclosure: I used Claude (an AI assistant) while preparing this change, as on #1076. Every result above I ran and verified myself.