Skip to content

fix: align numeric placeholder generation with JS implementation#239

Open
mogelbrod wants to merge 2 commits into
lingui:mainfrom
mogelbrod:correct-numeric-indices
Open

fix: align numeric placeholder generation with JS implementation#239
mogelbrod wants to merge 2 commits into
lingui:mainfrom
mogelbrod:correct-numeric-indices

Conversation

@mogelbrod

@mogelbrod mogelbrod commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

The SWC plugin assigned auto-generated numeric placeholder indices ({0}, {1}, …) differently from the reference Babel/JS macro (@lingui/babel-plugin-lingui-macro). Because message IDs and values keys are derived from these indices, the two toolchains could extract different catalogs for identical source, and thus break translation lookups in projects that mix the JS and SWC tooling.

This PR fixes two distinct divergences in how those indices are generated.

Background: how the JS macro numbers placeholders

In the JS macro, a per-message counter (getExpressionIndex, a simple () => index++) is advanced only when an expression has no name — i.e. in expressionToArgument when the expression is not an identifier. Two consequences follow, and the SWC plugin violated both:

  1. Named placeholders must not consume a numeric index.
  2. Indices are allocated in source order, as expressions are encountered
    during traversal.

Bug 1 — named placeholders consumed a numeric index

ph({ name }), {label: value} and bare identifiers carry an explicit name and, in the reference macro, never advance the counter. The SWC plugin derived the next numeric index from values_indexed.len(), which also counted named placeholders, so any unnamed expression following a named one was numbered one too high.

<Trans>
  {ph({ available: 1 })} of <Plural value={zones.length} one="# zone" other="# zones" /> available
</Trans>
message
Before {available} of {1, plural, one {# zone} other {# zones}} available
After / in JS {available} of {0, plural, one {# zone} other {# zones}} available

Fix: 5ffbfd5

Replaced the values_indexed.len()-based index with a dedicated numeric_index counter (next_numeric_index) that is advanced only when an auto-numeric placeholder is actually assigned — never for named placeholders. This mirrors
getExpressionIndex.

Bug 2 — choice-component value index was not allocated in source order

For <Plural> / <Select> / <SelectOrdinal>, the SWC plugin always allocated the value expression's index first, before processing the option attributes (one, other, …). While this is probably more intuitive, the JS implementation instead iterates attributes in source order and allocates the value's index at the position where the value attribute actually appears.

This only diverges when a non-identifier value is not the first attribute and an earlier option contains a numeric-indexed expression — but when it does, the numbers differ:

<Plural one={`${a.b} glass`} value={items.length} other="many" />
message
Before {0, plural, one {{1} glass} other {many}}
After / in JS {1, plural, one {{0} glass} other {many}}

Would it be worthwhile to update the JS implementation to match the SWC version instead?

With the conventional value-first ordering both implementations already agreed (value{0}), so existing snapshots were unaffected.

Fix: e08f777

  • The token now records value_pos: the position of the value attribute among the cases in source order (0 for the plural(value, { … }) call form, where the value is always first).
  • push_icu allocates the value's index at value_pos while iterating the cases, so indices are consumed in source order. The value placeholder is still emitted first in the ICU string by rendering each case body into a buffer (via String::split_off) and assembling {value, format, …cases} at the end.
    Index allocation (numeric_index / values_indexed) persists across the split, so only the output position is decoupled from the allocation order.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.99%. Comparing base (300cf86) to head (e08f777).

Files with missing lines Patch % Lines
crates/lingui_macro/src/builder.rs 97.29% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   94.95%   94.99%   +0.03%     
==========================================
  Files          10       10              
  Lines        2597     2637      +40     
==========================================
+ Hits         2466     2505      +39     
- Misses        131      132       +1     
Flag Coverage Δ
unittests 94.99% <98.14%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crates/lingui_macro/src/jsx_visitor.rs 89.09% <100.00%> (+0.65%) ⬆️
crates/lingui_macro/src/macro_utils.rs 89.26% <100.00%> (+0.05%) ⬆️
crates/lingui_macro/src/builder.rs 98.73% <97.29%> (-0.23%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant