Skip to content

Make GeneratorState#_configure only write the options it is given - #1077

Merged
byroot merged 1 commit into
ruby:masterfrom
youdie006:jruby-configure-only-given-options
Sep 11, 2026
Merged

Make GeneratorState#_configure only write the options it is given#1077
byroot merged 1 commit into
ruby:masterfrom
youdie006:jruby-configure-only-given-options

Conversation

@youdie006

@youdie006 youdie006 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

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#_configure re-wrote every option with its class default, so any configure/merge reset the options it was not given:

state = JSON.state.new(max_nesting: 3, allow_nan: true, ascii_only: true, script_safe: true)
state.configure(indent: '1')
# JRuby: max_nesting 100, allow_nan false, ascii_only false, script_safe false
# C ext and pure generator: 3, true, true, true

The allow_nan reset is the sharpest — a state that generated NaN a moment earlier starts raising JSON::GeneratorError. Through the public output, JSON.state.new(sort_keys: true) after any configure gave {"b":1,"a":2} on JRuby and {"a":2,"b":1} elsewhere. A string option given as an explicit nil had the mirror problem, keeping the previous value instead of clearing.

configure_state_i (generator.c:1860) is an if/else if chain over rb_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":

maxNesting = opts.getInt("max_nesting", maxNesting);

OptionsReader.get delegates to RubyHash#delete: absent gives Java null (keep current), present-but-nil gives nil (falsy), which is what long_config/RTEST do on the C side. as_json and sort_keys need an explicit hasKey, and the five string options need a small helper because getString returns null for absent and falsy. hasKey does not delete, so ensureEmpty() still rejects unknown keywords.

Verification

Built with rake create_jar in jruby:9.4-jdk21, every row rebuilt from scratch, each row's GeneratorState.java md5 distinct.

row result
the two re-enabled tests on master 4 failures
with the fix 8 tests, 35 assertions, 0 failures
helper drops its hasKey guard 3 failures
sort_keys unguarded 1 failure
max_nesting back to DEFAULT_MAX_NESTING 1 failure
as_json unguarded 1 failure

The as_json row 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.

engine result
JRuby 9.4.15.0 589 tests, 3000 assertions, 0 failures
CRuby 3.2.11, C extension 595 tests, 3423 assertions, 0 failures

Java is not compiled on CRuby and the two omits were jruby-guarded, so those rows already ran these tests. TruffleRuby loads lib/json/truffle_ruby/generator.rb, which #1076 already fixed; I did not run that engine.

Not measured: hasKey adds a second hash lookup for as_json/sort_keys. configure is 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 ensureEmpty relies on get deleting 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.

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.
@byroot
byroot merged commit bbc73fb into ruby:master Sep 11, 2026
42 checks passed
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