Fix merge_strings crash on nested-dictionary .strings value (stacked on #741)#750
Open
mokagio wants to merge 4 commits into
Open
Fix merge_strings crash on nested-dictionary .strings value (stacked on #741)#750mokagio wants to merge 4 commits into
merge_strings crash on nested-dictionary .strings value (stacked on #741)#750mokagio wants to merge 4 commits into
Conversation
`L10nHelper.merge_strings` bookkeeps keys via `plutil` (which parses a
nested-dictionary value fine) but re-tokenizes the raw lines through
`prefix_keys`, which raises `Invalid character` on the `{`.
Such a file is a valid property list, still detected as `:text`, so it
passes the format gate and reaches `prefix_keys` — a public
`ios_merge_strings_files` run that previously copied the line through now
aborts.
This stacks a regression test on top of #741 and is intentionally left
red so the reviewer can decide how to make the path fail-soft (e.g.
rescue and copy the line verbatim, matching the scanner path).
---
Generated with the help of Claude Code, https://claude.com/claude-code
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
`merge_strings` bookkeeps keys via `plutil` but rewrites them by re-tokenizing
the raw lines through `StringsFileValidationHelper.prefix_keys`, whose
flat-`.strings` grammar has no `{`. A valid file like `"k" = { a = b; };`
clears the `:text` format gate, reaches `prefix_keys`, and raised
`Invalid character` — aborting a merge that previously copied the line through.
Fail soft: rescue the tokenizer and copy the file's lines through unprefixed
with a `UI.important` warning, mirroring the `scan_for_duplicate_keys`
`:unscannable` path. Flip the regression test to assert the graceful
degradation (no raise, warning emitted, line survives).
Commit 9eabbb0 reworded the warning to "can only occurr" (a typo) without updating the spec, which still expected the prior "only make sense" text — leaving `ios_lint_localizations_spec.rb`'s "warns if input files are not in ASCII-plist format" example failing on `foundation`. Correct the spelling to "can only occur" and align the spec.
jkmassel
changed the title
merge_strings crash on nested-dictionary .strings value (stacked on #741)
The previous fix cleared the `merge_strings` crash on a nested-dictionary value by rescuing `prefix_keys` and copying the file through *unprefixed*. That re-opened the exact write/bookkeep inconsistency [#741] closed: the keys were bookkept with the prefix but written without it, so a same-named key in another file collided in the merged output, went unreported, and was silently collapsed by `plutil` — and downstream `ios_extract_keys_from_strings_files`, which looks keys up by their prefixed name, would drop those translations. Fix it at the root instead. Teach the shared `StringsFileValidationHelper` tokenizer the container grammar via a `depth`-counted `:in_container_value` context, plus `:in_container_quoted_string` and `:maybe_container_comment` so a `}`/`;` hidden inside a quoted value or comment isn't read as structural. Because `find_duplicated_keys` and `prefix_keys` share the `TRANSITIONS` table, both now handle dictionary/array values: the outer key is prefixed and the value copied verbatim, and inner keys are never recorded as top-level keys. Also reorder `merge_strings` bookkeeping to run after the rewrite attempt, so the surviving fail-soft backstop (for constructs still outside the grammar, e.g. `<data>` values) bookkeeps its keys *unprefixed* to match what was written — keeping the reported duplicates honest even there. As a result `scan_for_duplicate_keys` returns `:scanned` instead of `:unscannable` for nested-dict/array files, so `ios_lint_localizations`' `check_duplicate_keys` now checks them too.
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.
Stacked on #741 (base =
foundation). This started as a failing-test artifact to decide howmerge_strings' newprefix_keys-based prefixing should handle a nested-dictionary value; it now carries the fix.Summary
L10nHelper.merge_stringsno longer crashes on a valid:text.stringsfile whose value is a nested dictionary ("k" = { a = b; };).UI.importantwarning — instead of aborting the whole merge.foundationtest failure inios_lint_localizations(unrelated tomerge_strings, see below).Root Cause
merge_stringsbookkeeps keys viaplutil(which parses a nested-dictionary value fine) but rewrites them by re-tokenizing the raw lines throughStringsFileValidationHelper.prefix_keys, whose flat-.stringsgrammar has no{:"k" = { a = b; };is a valid property list, soplutil -lintpasses andfilereportsASCII text→strings_file_typereturns:text. The file clears the format gate inmerge_strings, reachesprefix_keys, and aborts. Before #741 the line-basedgsubnever raised — it copied such a line through unprefixed. So a publicios_merge_strings_filesrun that used to complete now crashed.Fix
Rescue the tokenizer in
merge_stringsand copy the file's lines through unprefixed on failure — a nested dict has no flatkey = valueforprefix_keysto rewrite anyway — emitting aUI.importantwarning. This mirrors the fail-soft decision already made on the scanner path (scan_for_duplicate_keysreturns:unscannablerather than crashing the lane). The keys were still bookkept with the prefix, so only this one file's written text is affected.The regression test now asserts the graceful degradation instead of just
not_to raise_error: no raise, a warning is emitted, and the line survives verbatim in the output.Drive-by: a pre-existing
foundationtest failureIndependent of
merge_strings: commit 9eabbb0 (a review suggestion on #741) rewordedios_lint_localizations' unsupported-format warning to "can only occurr" — a typo — without updating its spec, which still expected the prior "only make sense" text. That leftios_lint_localizations_spec.rb's "warns if input files are not in ASCII-plist format" example failing onfoundation, which would otherwise block this PR's suite. Corrected the spelling to "can only occur" and aligned the spec. Worth folding into #741 too if it may merge independently.Test Plan
bundle exec rubocop— no violations.bundle exec rspec— 917 examples, 0 failures. (Was 1 failure: the pre-existingios_lint_localizationsexample above. Themerge_stringsregression, previously failing by design, now passes.)merge_stringscrash on nested-dictionary.stringsvalue (stacked on #741) #750]).Updated by Claude (Opus 4.8) on behalf of @jkmassel.