From 2950cea070e8370f45691c8cdea7e294951d7902 Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Thu, 2 Jul 2026 08:55:39 +1000 Subject: [PATCH 1/4] Add failing test for merge_strings nested-dict crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- spec/ios_l10n_helper_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index 5f01c93e0..c0dcddfd2 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -160,6 +160,22 @@ def file_encoding(path) end end + it 'does not crash merging a text `.strings` file whose value is a nested dictionary' do + # Regression: `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 still `:text` and passes the format gate, so a public `ios_merge_strings_files` + # run that previously copied the line through now aborts. It should degrade gracefully instead of raising. + content = %("k" = { a = b; };\n) + Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir| + input_file = File.join(tmp_dir, 'InfoPlist.strings') + File.write(input_file, content) + output_file = File.join(tmp_dir, 'output.strings') + expect do + described_class.merge_strings(paths: { input_file => 'pfx.' }, output_path: output_file) + end.not_to raise_error + end + end + it 'returns duplicate keys found' do paths = { fixture('Localizable-utf16.strings') => nil, fixture('non-latin-utf16.strings') => nil } Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir| From e155caf0b9b58e0a3e833d4ff5cbe3fe4010d2a9 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 3 Jul 2026 15:09:15 -0600 Subject: [PATCH 2/4] Fix `merge_strings` crash on nested-dictionary `.strings` value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). --- CHANGELOG.md | 1 + .../helper/ios/ios_l10n_helper.rb | 15 ++++++++++++++- spec/ios_l10n_helper_spec.rb | 16 +++++++++++----- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0c2b1693..60759ef77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ _None_ - `StringsFileValidationHelper.find_duplicated_keys` now also accepts comments placed *between* the tokens of a statement (e.g. `"key" /* note */ = "value";`), which `plutil` allows, instead of raising `Invalid character` on the `/`. [#741] - `ios_lint_localizations`' `check_duplicate_keys` no longer crashes the lane on a file that parses as a property list but isn't a flat `.strings` (e.g. a nested-dictionary value); it now warns via `UI.important` and skips that file. [#741] - `L10nHelper.merge_strings` now applies the key prefix via a comment-aware tokenizer, so it correctly prefixes unquoted keys containing `. - $ : /`, lines with an *unquoted* value (e.g. `CFBundleName = WordPress;`), and keys sitting behind an inter-token `.strings` comment (e.g. `CFBundleName /* note */ = WordPress;`) — matching the grammar `plutil` accepts. Previously the line-based matcher left those keys written to the merged file without the prefix while still bookkeeping them *with* it, leaving the output inconsistent with the reported keys (which could resurface the very collisions the prefix avoids and break downstream key extraction). [#741] +- `L10nHelper.merge_strings` no longer crashes on a `:text` `.strings` file whose value is a nested dictionary (`"k" = { … };`) — valid input `plutil` accepts but the flat-`.strings` prefixing tokenizer can't rewrite. It now copies that file's lines through unprefixed with a `UI.important` warning instead of aborting the whole merge, mirroring the fail-soft behavior on the duplicate-key scanner path. [#750] ### Internal Changes diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb index 83168681f..9f082dc74 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -78,6 +78,9 @@ def self.read_utf8_lines(file) # @note The method is able to handle input files which are using different encodings, # guessing the encoding of each input file using the BOM (and defaulting to UTF8). # The generated file will always be in utf-8, by convention. + # @note If a file's keys can't be prefixed (e.g. it holds a nested-dictionary value, `"k" = { … };`, + # which `plutil` accepts but the flat-`.strings` tokenizer can't rewrite), its lines are copied + # through unprefixed with a warning rather than aborting the whole merge. # # @raise [RuntimeError] If one of the paths provided is not in text format (but XML or binary instead), or if any of the files are missing. # @@ -104,7 +107,17 @@ def self.merge_strings(paths:, output_path:) # sit (e.g. `CFBundleName /* note */ = WordPress;`) and `key = value`-looking text inside a comment is # left alone — keeping the written keys consistent with the (`plutil`-derived) keys bookkept above. lines = read_utf8_lines(input_file) - lines = Fastlane::Helper::Ios::StringsFileValidationHelper.prefix_keys(lines: lines, prefix: prefix) + begin + lines = Fastlane::Helper::Ios::StringsFileValidationHelper.prefix_keys(lines: lines, prefix: prefix) + rescue StandardError => e + # `plutil` accepts flat-`.strings` constructs the tokenizer doesn't — most notably a nested-dictionary + # value (`"k" = { … };`), which parses fine (so the file clears the `:text` gate above) yet exposes no + # flat `key = value` for `prefix_keys` to rewrite, so it raises on the `{`. Fail soft: copy this file's + # lines through unprefixed rather than aborting the whole merge — mirroring the scanner path, where + # `scan_for_duplicate_keys` returns `:unscannable` instead of crashing the lane. `lines` is untouched + # by the raise (the assignment above never completes), so it still holds the original file contents. + UI.important("Could not add prefix `#{prefix}` to the keys in `#{input_file}` (#{e.message}); copying its lines through unprefixed.") + end lines.each { |line| tmp_file.write(line) } tmp_file.write("\n") end diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index c0dcddfd2..e92b61d10 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -160,19 +160,25 @@ def file_encoding(path) end end - it 'does not crash merging a text `.strings` file whose value is a nested dictionary' do - # Regression: `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 still `:text` and passes the format gate, so a public `ios_merge_strings_files` - # run that previously copied the line through now aborts. It should degrade gracefully instead of raising. + it 'copies a nested-dictionary value through unprefixed (with a warning) instead of crashing the merge' do + # Regression: `merge_strings` bookkeeps keys via `plutil` (which parses a nested-dictionary value fine) + # but rewrites keys by re-tokenizing the raw lines through `prefix_keys`, whose flat-`.strings` grammar + # has no `{` — so it raised `Invalid character`. Such a file is still `:text` and clears the format gate, + # so a public `ios_merge_strings_files` run that previously copied the line through would abort. It now + # degrades gracefully — copying the un-prefixable line verbatim and warning — like the scanner path. content = %("k" = { a = b; };\n) Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir| input_file = File.join(tmp_dir, 'InfoPlist.strings') File.write(input_file, content) output_file = File.join(tmp_dir, 'output.strings') + + expect(FastlaneCore::UI).to receive(:important).with(a_string_including('Could not add prefix `pfx.`').and(a_string_including('unprefixed'))) expect do described_class.merge_strings(paths: { input_file => 'pfx.' }, output_path: output_file) end.not_to raise_error + + # The nested-dict line survives verbatim — unprefixed, since it has no flat `key = value` to rewrite. + expect(File.read(output_file)).to include('"k" = { a = b; };') end end From 7afb9a58dd0574a7e4d84b12038fc7d9645ce450 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 3 Jul 2026 15:09:15 -0600 Subject: [PATCH 3/4] Fix typo in `ios_lint_localizations` unsupported-format warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 9eabbb06 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. --- .../wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb | 2 +- spec/ios_lint_localizations_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb index edfd1892c..c93481502 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb @@ -62,7 +62,7 @@ def self.find_duplicated_keys(params) duplicate_keys[language] = payload.map { |key, value| "`#{key}` was found at multiple lines: #{value.join(', ')}" } unless payload.empty? when :unsupported_format UI.important <<~WRONG_FORMAT - File `#{path}` is in #{payload} format, while finding duplicate keys can only occurr on files that are in ASCII-plist format. + File `#{path}` is in #{payload} format, while finding duplicate keys can only occur on files that are in ASCII-plist format. Since your files are in #{payload} format, you should probably disable the `check_duplicate_keys` option from this `#{action_name}` call. WRONG_FORMAT when :unscannable diff --git a/spec/ios_lint_localizations_spec.rb b/spec/ios_lint_localizations_spec.rb index 737eaa6a5..5223b293e 100644 --- a/spec/ios_lint_localizations_spec.rb +++ b/spec/ios_lint_localizations_spec.rb @@ -194,7 +194,7 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_strings_ File.write(File.join(fr_lproj, 'Localizable.strings'), File.read(xml_file)) expected_message = <<~EXPECTED_WARNING - File `#{fr_lproj}/Localizable.strings` is in xml format, while finding duplicate keys only make sense on files that are in ASCII-plist format. + File `#{fr_lproj}/Localizable.strings` is in xml format, while finding duplicate keys can only occur on files that are in ASCII-plist format. Since your files are in xml format, you should probably disable the `check_duplicate_keys` option from this `ios_lint_localizations` call. EXPECTED_WARNING expect(FastlaneCore::UI).to receive(:important).with(expected_message) From 0744c8796808965746054c5ccdfbf433f92aae63 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:53:02 -0600 Subject: [PATCH 4/4] Prefix nested dict/array `.strings` values instead of failing soft MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. `` 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. --- CHANGELOG.md | 5 +- .../helper/ios/ios_l10n_helper.rb | 35 ++++++---- .../ios/ios_strings_file_validation_helper.rb | 66 +++++++++++++++++-- spec/ios_l10n_helper_spec.rb | 65 ++++++++++++++---- spec/ios_lint_localizations_spec.rb | 8 +-- ...ios_strings_file_validation_helper_spec.rb | 48 +++++++++++++- 6 files changed, 191 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60759ef77..b19084193 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,10 @@ _None_ - `StringsFileValidationHelper.find_duplicated_keys` now parses *unquoted* keys and values (valid `.strings` syntax, common in `InfoPlist.strings`) instead of raising `Invalid character`, matching the character set `plutil` accepts for unquoted strings (alphanumerics plus `_ . - $ : /`). This lets `ios_lint_localizations`' `check_duplicate_keys` work on `InfoPlist.strings`-style files. [#741] - `StringsFileValidationHelper.find_duplicated_keys` now also accepts comments placed *between* the tokens of a statement (e.g. `"key" /* note */ = "value";`), which `plutil` allows, instead of raising `Invalid character` on the `/`. [#741] -- `ios_lint_localizations`' `check_duplicate_keys` no longer crashes the lane on a file that parses as a property list but isn't a flat `.strings` (e.g. a nested-dictionary value); it now warns via `UI.important` and skips that file. [#741] +- `ios_lint_localizations`' `check_duplicate_keys` no longer crashes the lane on a file that parses as a property list but isn't a flat `.strings` the scanner can tokenize (e.g. a `` value); it now warns via `UI.important` and skips that file. [#741] - `L10nHelper.merge_strings` now applies the key prefix via a comment-aware tokenizer, so it correctly prefixes unquoted keys containing `. - $ : /`, lines with an *unquoted* value (e.g. `CFBundleName = WordPress;`), and keys sitting behind an inter-token `.strings` comment (e.g. `CFBundleName /* note */ = WordPress;`) — matching the grammar `plutil` accepts. Previously the line-based matcher left those keys written to the merged file without the prefix while still bookkeeping them *with* it, leaving the output inconsistent with the reported keys (which could resurface the very collisions the prefix avoids and break downstream key extraction). [#741] -- `L10nHelper.merge_strings` no longer crashes on a `:text` `.strings` file whose value is a nested dictionary (`"k" = { … };`) — valid input `plutil` accepts but the flat-`.strings` prefixing tokenizer can't rewrite. It now copies that file's lines through unprefixed with a `UI.important` warning instead of aborting the whole merge, mirroring the fail-soft behavior on the duplicate-key scanner path. [#750] +- `StringsFileValidationHelper.find_duplicated_keys` (and `scan_for_duplicate_keys`) now tokenize dictionary- and array-valued entries (`"k" = { … };`, `"k" = ( … );`, nesting allowed), skipping the container body — so a `:text` file that uses them is scanned for duplicate *top-level* keys instead of returning `:unscannable`. [#750] +- `L10nHelper.merge_strings` now prefixes the outer key of a dictionary/array-valued entry and copies the value through verbatim, instead of crashing on it — valid input `plutil` accepts that the flat-`.strings` tokenizer previously couldn't rewrite. If a file still holds a construct the tokenizer can't rewrite, its lines are copied through unprefixed with a `UI.important` warning (and its keys are bookkept unprefixed to match, so a genuine cross-file collision is still reported rather than silently collapsed) instead of aborting the whole merge. [#750] ### Internal Changes diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb index 9f082dc74..5f6a63792 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -78,9 +78,11 @@ def self.read_utf8_lines(file) # @note The method is able to handle input files which are using different encodings, # guessing the encoding of each input file using the BOM (and defaulting to UTF8). # The generated file will always be in utf-8, by convention. - # @note If a file's keys can't be prefixed (e.g. it holds a nested-dictionary value, `"k" = { … };`, - # which `plutil` accepts but the flat-`.strings` tokenizer can't rewrite), its lines are copied - # through unprefixed with a warning rather than aborting the whole merge. + # @note Dictionary- and array-valued entries (`"k" = { … };`, `"k" = ( … );`, nesting allowed) are + # prefixed on their outer key with the value preserved verbatim. If a file still holds some + # construct the tokenizer can't rewrite, its lines are copied through unprefixed with a warning + # (and its keys are then bookkept unprefixed too, so the reported duplicates stay accurate) + # rather than aborting the whole merge. # # @raise [RuntimeError] If one of the paths provided is not in text format (but XML or binary instead), or if any of the files are missing. # @@ -97,27 +99,36 @@ def self.merge_strings(paths:, output_path:) raise "The file `#{input_file}` does not exist or is of unknown format." if fmt.nil? raise "The file `#{input_file}` is in #{fmt} format but we currently only support merging `.strings` files in text format." unless fmt == :text - string_keys = read_strings_file_as_hash(path: input_file).keys.map { |k| "#{prefix}#{k}" } - duplicates += (string_keys & all_keys_found) # Find duplicates using Array intersection, and add those to duplicates list - all_keys_found += string_keys + raw_keys = read_strings_file_as_hash(path: input_file).keys tmp_file.write("/* MARK: - #{File.basename(input_file)} */\n\n") # Add the prefix to every key. We tokenize via `StringsFileValidationHelper.prefix_keys` rather than # matching keys with a line-based regex, so that keys are found regardless of where `.strings` comments # sit (e.g. `CFBundleName /* note */ = WordPress;`) and `key = value`-looking text inside a comment is - # left alone — keeping the written keys consistent with the (`plutil`-derived) keys bookkept above. + # left alone. It also handles dictionary/array values (`"k" = { … };`) — prefixing the outer key and + # copying the value verbatim. lines = read_utf8_lines(input_file) + applied_prefix = prefix begin lines = Fastlane::Helper::Ios::StringsFileValidationHelper.prefix_keys(lines: lines, prefix: prefix) rescue StandardError => e - # `plutil` accepts flat-`.strings` constructs the tokenizer doesn't — most notably a nested-dictionary - # value (`"k" = { … };`), which parses fine (so the file clears the `:text` gate above) yet exposes no - # flat `key = value` for `prefix_keys` to rewrite, so it raises on the `{`. Fail soft: copy this file's - # lines through unprefixed rather than aborting the whole merge — mirroring the scanner path, where + # `plutil` may still accept a construct the tokenizer can't rewrite: it parses fine (so the file + # clears the `:text` gate above) yet `prefix_keys` raises on it. Fail soft: copy this file's lines + # through unprefixed rather than aborting the whole merge — mirroring the scanner path, where # `scan_for_duplicate_keys` returns `:unscannable` instead of crashing the lane. `lines` is untouched - # by the raise (the assignment above never completes), so it still holds the original file contents. + # by the raise (the assignment above never completes), so it still holds the original file contents, + # and `applied_prefix` records that the keys went out *unprefixed* so the bookkeeping below matches. + applied_prefix = '' UI.important("Could not add prefix `#{prefix}` to the keys in `#{input_file}` (#{e.message}); copying its lines through unprefixed.") end + + # Bookkeep the keys as they were actually written — prefixed, or unprefixed on the fail-soft path. + # Doing this *after* the rewrite keeps the reported duplicates consistent with the merged file even + # when prefixing fell back, so a genuine collision is still surfaced rather than silently collapsed. + string_keys = raw_keys.map { |k| "#{applied_prefix}#{k}" } + duplicates += (string_keys & all_keys_found) # Find duplicates using Array intersection, and add those to duplicates list + all_keys_found += string_keys + lines.each { |line| tmp_file.write(line) } tmp_file.write("\n") end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_strings_file_validation_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_strings_file_validation_helper.rb index aa95cbdcc..5a0144e28 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_strings_file_validation_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_strings_file_validation_helper.rb @@ -13,7 +13,8 @@ class StringsFileValidationHelper # `resume_context` holds the context to return to once a comment ends. Comments are valid not only at # the top level but also *between* the tokens of a statement (e.g. `"key" /* note */ = "value";`), so a # comment must resume the state it interrupted rather than always dropping back to `:root`. - State = Struct.new(:context, :buffer, :in_escaped_ctx, :found_key, :resume_context) + # `depth` tracks how deeply nested we are inside a container value (`{ … }` / `( … )`); see `:in_container_value`. + State = Struct.new(:context, :buffer, :in_escaped_ctx, :found_key, :resume_context, :depth) # Characters allowed in an *unquoted* string — a key or a value. Unquoted strings are valid # `.strings` syntax (the old-style ASCII property-list format) and are common in `InfoPlist.strings` @@ -46,6 +47,31 @@ class StringsFileValidationHelper resume end + # A value can be a nested container — a dictionary `{ … }` or an array `( … )` — which `plutil` accepts + # (e.g. `"k" = { a = b; };` or `"k" = ( "a", "b" );`). We don't rewrite or record anything *inside* a + # container: its inner keys are not top-level keys, and `prefix_keys` copies the value through verbatim. + # We only need to find the matching close delimiter, so we just count nesting depth. `OPEN_CONTAINER` + # doubles as the entry transition from `:after_quoted_key_and_eq` and as the nested-open transition. + OPEN_CONTAINER = lambda do |state, _c| + state.depth += 1 + :in_container_value + end + + # Close one level of nesting. The value is only finished — and we go back to expecting the terminating + # `;` — once depth returns to 0; otherwise we're still inside an outer container. + CLOSE_CONTAINER = lambda do |state, _c| + state.depth -= 1 + state.depth.zero? ? :after_quoted_value : :in_container_value + end + + # A `/` inside a container may start a comment — whose body can contain `{ } ( ) ; "` that must NOT count + # toward nesting — or just be an ordinary value character (e.g. a path). Defer the decision one char, + # resuming the container in either case. + ENTER_CONTAINER_COMMENT = lambda do |state, _c| + state.resume_context = :in_container_value + :maybe_container_comment + end + TRANSITIONS = { root: { /\s/u => :root, @@ -120,6 +146,8 @@ class StringsFileValidationHelper '/' => ENTER_COMMENT_OR_VALUE, /\s/u => :after_quoted_key_and_eq, '"' => :in_quoted_value, + # A container value — a dictionary `{ … }` or an array `( … )`, which may nest (e.g. `"k" = { a = b; };`). + /[{(]/u => OPEN_CONTAINER, # An unquoted value, e.g. `CFBundleName = WordPress;` as used by `InfoPlist.strings`. UNQUOTED_STRING_CHARACTER => :in_unquoted_value }, @@ -139,6 +167,32 @@ class StringsFileValidationHelper '/' => ENTER_COMMENT, /\s/u => :after_quoted_value, ';' => :root + }, + # Inside a container value (`{ … }` / `( … )`). We ignore the contents — inner keys aren't top-level + # keys and the value is copied verbatim by `prefix_keys` — and only track nesting so we can find the + # matching close. Quoted strings and comments are entered explicitly because their bodies can contain + # `{ } ( ) ;` that must not affect the depth count; everything else (`=`, `;`, `,`, whitespace, unquoted + # text, newlines) is consumed by the catch-all, which must stay LAST so the specific keys win first. + in_container_value: { + /[{(]/u => OPEN_CONTAINER, + /[})]/u => CLOSE_CONTAINER, + '"' => :in_container_quoted_string, + '/' => ENTER_CONTAINER_COMMENT, + /./mu => :in_container_value + }, + # A quoted string inside a container. Skipped wholesale (its `{ } ( ) ; ,` are literal, not structural) + # until the closing quote returns us to the container. Escapes are handled globally (see the escape + # branch in `find_duplicated_keys`, whose allow-list includes this context). + in_container_quoted_string: { + '"' => :in_container_value, + /./mu => :in_container_quoted_string + }, + # One char after a `/` inside a container: `*`/`/` confirm a comment (which resumes the container once + # it ends), anything else means the `/` was just a value character and we stay in the container. + maybe_container_comment: { + /\*/u => :in_block_comment, + '/' => :in_line_comment, + /./mu => :in_container_value } }.freeze @@ -150,7 +204,7 @@ class StringsFileValidationHelper def self.find_duplicated_keys(file:) keys_with_lines = Hash.new { |h, k| h[k] = [] } - state = State.new(context: :root, buffer: StringIO.new, in_escaped_ctx: false, found_key: nil, resume_context: :root) + state = State.new(context: :root, buffer: StringIO.new, in_escaped_ctx: false, found_key: nil, resume_context: :root, depth: 0) # Using our `each_utf8_line` helper instead of `File.readlines` ensures we can also read files that are # encoded in UTF-16, yet process each of their lines as a UTF-8 string, so that `RegExp#match?` don't throw @@ -161,7 +215,7 @@ def self.find_duplicated_keys(file:) # This is more straightforward than having to account for it in the `TRANSITIONS` table. if state.in_escaped_ctx || c == '\\' # Just because we check for escaped characters at the global level, it doesn't mean we allow them in every context. - allowed_contexts_for_escaped_characters = %i[in_quoted_key in_quoted_value in_block_comment in_line_comment] + allowed_contexts_for_escaped_characters = %i[in_quoted_key in_quoted_value in_block_comment in_line_comment in_container_quoted_string] raise "Found escaped character outside of allowed contexts on line #{line_no + 1} (current context: #{state.context})" unless allowed_contexts_for_escaped_characters.include?(state.context) state.buffer.write(c) if state.context == :in_quoted_key @@ -214,7 +268,9 @@ def self.scan_for_duplicate_keys(file:, assume_valid: false) # unquoted key is wrapped in quotes (`key` → `"key"`). Because it tokenizes the file the same way # `find_duplicated_keys` does, it is comment-aware: a key sitting behind an inter-token comment (e.g. # `key /* note */ = value;`) is still prefixed, and `key = value`-looking text *inside* a comment is left - # alone — a distinction a line-based regex can't reliably make. + # alone — a distinction a line-based regex can't reliably make. It is likewise container-aware: a + # dictionary or array value (`"k" = { … };` / `"k" = ( … );`, nesting allowed) has only its outer key + # prefixed, with the value — including any keys *inside* it — copied through verbatim. # # @param [Array] lines The file's lines, already decoded to UTF-8 (e.g. via `L10nHelper.read_utf8_lines`). # @param [String] prefix The prefix to insert before every key. A nil/empty prefix returns `lines` unchanged. @@ -222,7 +278,7 @@ def self.scan_for_duplicate_keys(file:, assume_valid: false) def self.prefix_keys(lines:, prefix:) return lines if prefix.nil? || prefix.empty? - state = State.new(context: :root, buffer: StringIO.new, in_escaped_ctx: false, found_key: nil, resume_context: :root) + state = State.new(context: :root, buffer: StringIO.new, in_escaped_ctx: false, found_key: nil, resume_context: :root, depth: 0) lines.map do |line| rewritten = +'' line.each_char do |c| diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index e92b61d10..e7e492ebb 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -160,25 +160,68 @@ def file_encoding(path) end end - it 'copies a nested-dictionary value through unprefixed (with a warning) instead of crashing the merge' do - # Regression: `merge_strings` bookkeeps keys via `plutil` (which parses a nested-dictionary value fine) - # but rewrites keys by re-tokenizing the raw lines through `prefix_keys`, whose flat-`.strings` grammar - # has no `{` — so it raised `Invalid character`. Such a file is still `:text` and clears the format gate, - # so a public `ios_merge_strings_files` run that previously copied the line through would abort. It now - # degrades gracefully — copying the un-prefixable line verbatim and warning — like the scanner path. + it 'prefixes the outer key of a nested-dictionary value and preserves the value verbatim' do + # A dictionary/array value (`"k" = { … };`) is valid `:text` that `plutil` accepts; the tokenizer now + # prefixes the outer key and copies the container body through unchanged, rather than failing to rewrite it. content = %("k" = { a = b; };\n) Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir| input_file = File.join(tmp_dir, 'InfoPlist.strings') File.write(input_file, content) output_file = File.join(tmp_dir, 'output.strings') + expect(FastlaneCore::UI).not_to receive(:important) + described_class.merge_strings(paths: { input_file => 'pfx.' }, output_path: output_file) + + # Outer key prefixed, nested value untouched — and the result still parses back to the prefixed key. + expect(File.read(output_file)).to include('"pfx.k" = { a = b; };') + expect(described_class.read_strings_file_as_hash(path: output_file).keys).to contain_exactly('pfx.k') + end + end + + it 'keeps a container-valued key distinct from a same-named key in another file (no silent collision)' do + # Before containers were tokenizable, this file was written unprefixed while bookkept *with* the prefix, so + # a same-named key elsewhere collided in the merged output yet went unreported and was silently collapsed by + # `plutil`. Now the key is actually prefixed, so the two stay distinct and neither value is clobbered. + Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir| + flat = File.join(tmp_dir, 'A.strings') + nested = File.join(tmp_dir, 'B.strings') + File.write(flat, %("MyKey" = "original";\n)) + File.write(nested, %("MyKey" = { sub = val; };\n)) + output_file = File.join(tmp_dir, 'output.strings') + + duplicates = described_class.merge_strings(paths: { flat => nil, nested => 'pfx.' }, output_path: output_file) + + expect(duplicates).to be_empty + merged = described_class.read_strings_file_as_hash(path: output_file) + expect(merged.keys).to contain_exactly('MyKey', 'pfx.MyKey') + expect(merged['MyKey']).to eq('original') # the flat file's value is not clobbered + end + end + + it 'falls back to copying a file through unprefixed — and bookkeeps it unprefixed — when prefixing raises' do + # Backstop for any construct the tokenizer still can't rewrite: `merge_strings` warns and copies the file + # through unprefixed rather than aborting. Because it then bookkeeps those keys *unprefixed* (matching what + # was written), a genuine collision with another file is still reported instead of silently collapsing. + Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir| + dest = File.join(tmp_dir, 'A.strings') + weird = File.join(tmp_dir, 'B.strings') + File.write(dest, %("shared" = "one";\n)) + File.write(weird, %("shared" = "two";\n)) + output_file = File.join(tmp_dir, 'output.strings') + + # Force the fail-soft path for the prefixed file only, regardless of its content. + allow(Fastlane::Helper::Ios::StringsFileValidationHelper).to receive(:prefix_keys).and_wrap_original do |orig, **kwargs| + raise 'boom' if kwargs[:prefix] == 'pfx.' + + orig.call(**kwargs) + end expect(FastlaneCore::UI).to receive(:important).with(a_string_including('Could not add prefix `pfx.`').and(a_string_including('unprefixed'))) - expect do - described_class.merge_strings(paths: { input_file => 'pfx.' }, output_path: output_file) - end.not_to raise_error - # The nested-dict line survives verbatim — unprefixed, since it has no flat `key = value` to rewrite. - expect(File.read(output_file)).to include('"k" = { a = b; };') + duplicates = described_class.merge_strings(paths: { dest => nil, weird => 'pfx.' }, output_path: output_file) + + # B was written unprefixed, so its `shared` collides with A's `shared` — and that collision is REPORTED. + expect(duplicates).to eq(['shared']) + expect(File.read(output_file)).to include('"shared" = "two";') end end diff --git a/spec/ios_lint_localizations_spec.rb b/spec/ios_lint_localizations_spec.rb index 5223b293e..9afbab6ca 100644 --- a/spec/ios_lint_localizations_spec.rb +++ b/spec/ios_lint_localizations_spec.rb @@ -267,10 +267,10 @@ def write_localizable(lang, content) end it 'warns and skips (without crashing) a file that parses as a plist but is not a flat `.strings`' do - # A nested-dictionary value is a valid old-style plist that `plutil` accepts but the duplicate-key - # scanner can't tokenize. This used to crash the lane (the scanner raised, uncaught); now it is - # surfaced via `UI.important` and the file is skipped. - write_localizable('en', "\"k\" = { a = b; };\n") + # A `` value is a valid old-style plist that `plutil` accepts but the duplicate-key scanner can't + # tokenize. This used to crash the lane (the scanner raised, uncaught); now it is surfaced via + # `UI.important` and the file is skipped. (Dictionary/array values, by contrast, are now tokenized.) + write_localizable('en', "\"k\" = <48656c6c6f>;\n") expect(FastlaneCore::UI).to receive(:important).with(/Could not check .* for duplicate keys/) result = nil diff --git a/spec/ios_strings_file_validation_helper_spec.rb b/spec/ios_strings_file_validation_helper_spec.rb index c7c2f646c..54f86a317 100644 --- a/spec/ios_strings_file_validation_helper_spec.rb +++ b/spec/ios_strings_file_validation_helper_spec.rb @@ -139,6 +139,44 @@ end end + context 'when a value is a nested dictionary or array' do + # `plutil` accepts container values in the old-style ASCII plist (e.g. `"k" = { a = b; };` or `"k" = ( … );`), + # and they can nest. The tokenizer skips the container body — its inner keys are not top-level keys — while + # still tracking nesting so delimiters hidden inside quoted strings or comments don't end the value early, + # and still detecting duplicate *top-level* keys. + it 'does not treat keys inside a dictionary value as top-level keys' do + content = <<~STRINGS + "k" = { a = b; c = d; }; + "j" = "v"; + STRINGS + with_tmp_file(named: 'InfoPlist.strings', content: content) do |path| + expect(described_class.find_duplicated_keys(file: path)).to be_empty + end + end + + it 'detects duplicate top-level keys whose values are containers' do + content = <<~STRINGS + "k" = { a = b; }; + "k" = ( "x", "y" ); + STRINGS + with_tmp_file(named: 'InfoPlist.strings', content: content) do |path| + expect(described_class.find_duplicated_keys(file: path)).to eq('k' => [1, 2]) + end + end + + it 'tracks nesting through delimiters hidden inside quoted strings and comments' do + # The inner `}` and `;` in the quoted `"c;d}"` and in the `/* } ; */` comment must NOT be read as + # structural — the container ends only at the real closing brace, so the second `"k"` is a duplicate. + content = <<~STRINGS + "k" = { a = { b = "c;d}"; }; /* } ; */ e = f; }; + "k" = 1; + STRINGS + with_tmp_file(named: 'InfoPlist.strings', content: content) do |path| + expect(described_class.find_duplicated_keys(file: path)).to eq('k' => [1, 2]) + end + end + end + describe '.scan_for_duplicate_keys' do it 'returns `[:scanned, duplicates]` for a `:text` file, with the duplicate hash (empty if none)' do with_tmp_file(named: 'dups.strings', content: "\"k\" = \"a\";\n\"k\" = \"b\";\n") do |path| @@ -159,8 +197,14 @@ end end - it 'returns `[:unscannable, message]` for a `:text` plist the tokenizer cannot read' do - with_tmp_file(named: 'nested.strings', content: "\"k\" = { a = b; };\n") do |path| + it 'returns `[:scanned, …]` for a `:text` plist whose values are (now tokenizable) nested dictionaries or arrays' do + with_tmp_file(named: 'nested.strings', content: "\"k\" = { a = b; };\n\"j\" = ( \"x\", \"y\" );\n") do |path| + expect(described_class.scan_for_duplicate_keys(file: path)).to eq([:scanned, {}]) + end + end + + it 'returns `[:unscannable, message]` for a `:text` plist the tokenizer still cannot read (e.g. a `` value)' do + with_tmp_file(named: 'data.strings', content: "\"k\" = <48656c6c6f>;\n") do |path| status, message = described_class.scan_for_duplicate_keys(file: path) expect(status).to eq(:unscannable) expect(message).to match(/Invalid character/)