Handle inter-token comments and unquoted merge keys in .strings helpers

Two sibling cases of the same "stricter than `plutil`" class this PR targets:

- `StringsFileValidationHelper.find_duplicated_keys` now accepts comments placed
  *between* the tokens of a statement (e.g. `"key" /* note */ = "value";`), which
  `plutil` allows, instead of raising `Invalid character`. A `resume_context` lets a
  comment return to the state it interrupted, and a `:maybe_comment_or_value` state
  disambiguates the one position where `/` could start either a comment or a
  `/usr/bin`-style unquoted value.

- `L10nHelper.merge_strings` now prefixes unquoted keys containing `. - $ : /` and
  lines with an *unquoted* value (e.g. `CFBundleName = WordPress;`), matching the
  unquoted-string grammar `plutil` accepts. The unquoted-value case keys on the fact
  that an unquoted string can't contain spaces, so it won't mistake comment prose
  (`and = a red herring ...`) for a key/value pair.

Author: jkmassel

CHANGELOG.md

@@ -15,7 +15,9 @@ _None_
 ### Bug Fixes
 
 - `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]
+- `L10nHelper.merge_strings` now applies the key prefix to unquoted keys containing `. - $ : /` and to lines with an *unquoted* value (e.g. `CFBundleName = WordPress;`), matching the unquoted-string grammar `plutil` accepts. Previously those keys were written to the merged file without the prefix while still being bookkept *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]
 
 ### Internal Changes
 

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb

@@ -102,9 +102,17 @@ def self.merge_strings(paths:, output_path:)
               # Read line-by-line to reduce memory footprint during content copy
               read_utf8_lines(input_file).each do |line|
                 unless prefix.nil? || prefix.empty?
-                  # The `/u` modifier on the RegExps is to make them UTF-8
+                  # The `/u` modifier on the RegExps is to make them UTF-8.
+                  # The unquoted-key character set below matches what `plutil` accepts (alphanumerics plus
+                  # `_ . - $ : /`, as in `StringsFileValidationHelper::UNQUOTED_STRING_CHARACTER`) so that keys
+                  # containing `. - $ : /` (e.g. reverse-DNS keys) are prefixed like any other.
                   line.gsub!(/^(\s*")/u, "\\1#{prefix}") # Lines starting with a quote are considered to be start of a key; add prefix right after the quote
-                  line.gsub!(/^(\s*)([A-Z0-9_]+)(\s*=\s*")/ui, "\\1\"#{prefix}\\2\"\\3") # Lines starting with an identifier followed by a '=' are considered to be an unquoted key (typical in InfoPlist.strings files for example)
+                  # Lines starting with an unquoted key followed by a `=` and a *quoted* value (typical in `InfoPlist.strings`).
+                  line.gsub!(%r{^(\s*)([a-zA-Z0-9_.$:/-]+)(\s*=\s*")}u, "\\1\"#{prefix}\\2\"\\3")
+                  # Lines starting with an unquoted key followed by a `=` and an *unquoted* value (e.g. `CFBundleName = WordPress;`).
+                  # The value is required to be a single unquoted token ending in `;` — an unquoted string can't contain spaces, so
+                  # this won't mistake comment prose like `and = a red herring for when…` (spaces in the "value") for a key/value pair.
+                  line.gsub!(%r{^(\s*)([a-zA-Z0-9_.$:/-]+)(\s*=\s*)([a-zA-Z0-9_.$:/-]+\s*;)}u, "\\1\"#{prefix}\\2\"\\3\\4")
                 end
                 tmp_file.write(line)
               end

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_strings_file_validation_helper.rb

@@ -5,11 +5,15 @@ module Helper
     module Ios
       class StringsFileValidationHelper
         # context can be one of:
-        #   :root, :maybe_comment_start, :in_line_comment, :in_block_comment,
+        #   :root, :maybe_comment_start, :maybe_comment_or_value, :in_line_comment, :in_block_comment,
         #   :maybe_block_comment_end, :in_quoted_key, :in_unquoted_key,
         #   :after_quoted_key_before_eq, :after_quoted_key_and_eq,
         #   :in_quoted_value, :in_unquoted_value, :after_quoted_value
-        State = Struct.new(:context, :buffer, :in_escaped_ctx, :found_key)
+        #
+        # `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)
 
         # 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`
@@ -19,6 +23,29 @@ class StringsFileValidationHelper
         # An unquoted key runs until the first whitespace or `=`; an unquoted value until whitespace or `;`.
         UNQUOTED_STRING_CHARACTER = %r{[a-zA-Z0-9_.$:/-]}u
 
+        # Enter a comment from an inter-token position, remembering where to resume once it ends.
+        # (`state.context` is still the originating state when a transition lambda runs — it's only reassigned
+        # to the lambda's return value afterwards — so this captures the state the comment interrupts.)
+        ENTER_COMMENT = lambda do |state, _c|
+          state.resume_context = state.context
+          :maybe_comment_start
+        end
+
+        # A `/` between `=` and the value is ambiguous: it can start a comment (`/* … */` or `// …`) or be the
+        # first character of an unquoted value (e.g. a path like `/usr/bin`). Defer the decision by one character
+        # via `:maybe_comment_or_value`.
+        ENTER_COMMENT_OR_VALUE = lambda do |state, _c|
+          state.resume_context = state.context
+          :maybe_comment_or_value
+        end
+
+        # Restore the context a comment interrupted (defaults to `:root`), then clear the saved context.
+        RESUME_AFTER_COMMENT = lambda do |state, _c|
+          resume = state.resume_context || :root
+          state.resume_context = :root
+          resume
+        end
+
         TRANSITIONS = {
           root: {
             /\s/u => :root,
@@ -34,16 +61,27 @@ class StringsFileValidationHelper
             '/' => :in_line_comment,
             /\*/u => :in_block_comment
           },
+          # Reached only from `:after_quoted_key_and_eq`, where a leading `/` might begin a comment or an
+          # unquoted value. A `*` or `/` confirms a comment; anything else means the `/` was the value's first
+          # character (values aren't buffered, so we just continue scanning the value).
+          maybe_comment_or_value: {
+            /\*/u => :in_block_comment,
+            '/' => :in_line_comment,
+            /./mu => lambda do |state, _c|
+              state.resume_context = :root
+              :in_unquoted_value
+            end
+          },
           in_line_comment: {
-            "\n" => :root,
+            "\n" => RESUME_AFTER_COMMENT,
             /./u => :in_line_comment
           },
           in_block_comment: {
             /\*/ => :maybe_block_comment_end,
             /./mu => :in_block_comment
           },
           maybe_block_comment_end: {
-            '/' => :root,
+            '/' => RESUME_AFTER_COMMENT,
             /./mu => :in_block_comment
           },
           in_quoted_key: {
@@ -71,10 +109,15 @@ class StringsFileValidationHelper
             end
           },
           after_quoted_key_before_eq: {
+            # A comment may sit between the key and the `=` (e.g. `"key" /* note */ = "value";`).
+            '/' => ENTER_COMMENT,
             /\s/u => :after_quoted_key_before_eq,
             '=' => :after_quoted_key_and_eq
           },
           after_quoted_key_and_eq: {
+            # A `/` here may start a comment or an unquoted value (which can contain `/`); disambiguate one char
+            # later. This entry must precede `UNQUOTED_STRING_CHARACTER` below, which also matches `/`.
+            '/' => ENTER_COMMENT_OR_VALUE,
             /\s/u => :after_quoted_key_and_eq,
             '"' => :in_quoted_value,
             # An unquoted value, e.g. `CFBundleName = WordPress;` as used by `InfoPlist.strings`.
@@ -92,6 +135,8 @@ class StringsFileValidationHelper
             UNQUOTED_STRING_CHARACTER => :in_unquoted_value
           },
           after_quoted_value: {
+            # A comment may sit between the value and the terminating `;` (e.g. `"key" = "value" /* note */;`).
+            '/' => ENTER_COMMENT,
             /\s/u => :after_quoted_value,
             ';' => :root
           }
@@ -105,7 +150,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)
+          state = State.new(context: :root, buffer: StringIO.new, in_escaped_ctx: false, found_key: nil, resume_context: :root)
 
           # 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

spec/ios_l10n_helper_spec.rb

@@ -100,6 +100,28 @@ def file_encoding(path)
       end
     end
 
+    it 'prefixes unquoted keys with unquoted values and keys containing `. - $ : /`' do
+      # Regression: prefixing must cover the full unquoted-string grammar `plutil` accepts — unquoted *values*
+      # (e.g. `CFBundleName = WordPress;`) and keys containing `. - $ : /` — so the written file stays consistent
+      # with the prefixed keys we report. (Previously only `[A-Z0-9_]` keys with a *quoted* value were prefixed,
+      # leaving these written without the prefix and resurfacing the very collisions the prefix avoids.)
+      content = <<~STRINGS
+        CFBundleName = WordPress;
+        com.automattic.app-id = "X";
+        "QuotedKey" = "Y";
+      STRINGS
+      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')
+        described_class.merge_strings(paths: { input_file => 'pfx.' }, output_path: output_file)
+        merged = File.read(output_file)
+        expect(merged).to include('"pfx.CFBundleName" = WordPress;')
+        expect(merged).to include('"pfx.com.automattic.app-id" = "X";')
+        expect(merged).to include('"pfx.QuotedKey" = "Y";')
+      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|

spec/ios_strings_file_validation_helper_spec.rb

@@ -98,6 +98,47 @@
     end
   end
 
+  context 'when comments appear between the tokens of a statement' do
+    # Comments are valid `.strings` syntax not only on their own line but also *between* the tokens of a
+    # statement — after a key, around the `=`, or before the terminating `;`. `plutil` accepts all of these,
+    # so the scanner must tokenize them rather than raising `Invalid character` on the `/`.
+    it 'parses comments after a key, around the `=`, and before the `;` without raising' do
+      content = <<~STRINGS
+        "afterKey" /* note */ = "1";
+        "aroundEq" = /* note */ "2";
+        "beforeSemicolon" = "3" /* note */;
+        unquotedKey /* note */ = unquotedValue;
+      STRINGS
+      with_tmp_file(named: 'Localizable.strings', content: content) do |path|
+        expect(described_class.find_duplicated_keys(file: path)).to be_empty
+      end
+    end
+
+    it 'still detects duplicate keys in a file that also contains inline comments' do
+      content = <<~STRINGS
+        "dup" /* first */ = "1";
+        "unique" = "x";
+        "dup" = "2" /* second */;
+      STRINGS
+      with_tmp_file(named: 'Localizable.strings', content: content) do |path|
+        expect(described_class.find_duplicated_keys(file: path)).to eq('dup' => [1, 3])
+      end
+    end
+
+    it 'does not mistake a `/`-leading unquoted value for the start of a comment' do
+      # A `/` right after `=` may begin a comment OR an unquoted value (e.g. a path or URL); the latter,
+      # which `plutil` accepts, must still parse rather than be swallowed as a comment.
+      content = <<~STRINGS
+        "path" = /usr/bin/tool;
+        "url" = https://example.com/x;
+        "path" = /opt;
+      STRINGS
+      with_tmp_file(named: 'Localizable.strings', content: content) do |path|
+        expect(described_class.find_duplicated_keys(file: path)).to eq('path' => [1, 3])
+      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|