Skip to content

Sort glob matches so generated output is reproducible - #3

Merged
heiseshandian merged 4 commits into
mainfrom
fix/deterministic-output
Aug 22, 2026
Merged

heiseshandian merged 4 commits into
mainfrom
fix/deterministic-output

Conversation

@heiseshandian

@heiseshandian heiseshandian commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2.

The change

One line of behaviour — sort the glob matches by path before they go into languageMapping:

final allJsonFiles = (await buildStep.findAssets(Glob(globPattern)).toList())
  ..sort((a, b) => a.path.compareTo(b.path));

Plus the comment explaining why, a version bump to 0.0.4, and a CHANGELOG entry.

findAssets yields matches in an unspecified order and nothing sorted them. build_runner
backs that call two different ways — a dart:io directory crawl on a cold asset graph
(raw readdir order, not alphabetical) and the persisted graph's node insertion order on
an incremental build — so identical inputs produced different output depending on how the
build ran. languageMapping is a LinkedHashMap, so that order became the order of the
emitted classes and I18n getters. Full analysis in #2.

This is not only cosmetic — it changed method signatures

Worth calling out, because I did not expect it and it is the strongest argument for landing
this.

languageMapping.entries.first decides which locale defines the generated API. Interpolation
parameters are emitted positionally, in the order the {{...}} placeholders appear in
that locale's translated string:

var translatedMatches = matches.map((e) => e.group(0)).toSet().toList();
...
..requiredParameters.add(Parameter((p) => p..name = translatedMatches.join(",")))

Different languages order placeholders differently within a sentence, so the driving locale
silently determines the public signature. Verified in feedmepos/pos-v8, two methods:

Key en-US string zh-Hans string
order:checkout.split.combineSuccessDescription {{target}} combined with {{source}}. {{source}} 已合并到 {{target}}。
inventory:...partialFailure {{failedCount}} of {{totalCount}} inventory changes could not be completed. {{totalCount}} 项库存变更中有 {{failedCount}} 项未完成。

Committed output before this change:

String combineSuccessDescription(source, target) { ... }
String partialFailure(totalCount, failedCount) { ... }

After:

String combineSuccessDescription(target, source) { ... }
String partialFailure(failedCount, totalCount) { ... }

So pos-v8's generated API was being derived from a non-en-US locale, despite the
// Loops through the files in en-US comment in the source. Since these parameters are
untyped dynamic, a swap like this is invisible to the compiler — a positional call site
would just interpolate the wrong values at runtime.

Nothing in pos-v8 calls either generated method today (both call sites go through
i18next.t(key, variables: {...}) with a named map), so there is no live bug to fix there —
but the exposure was real.

Verification

Tests (added in this PR)

The repo had none, so this branch adds them plus a CI workflow that runs them
(.github/workflows/test.yml, green on this PR).

The earlier claim that the builder is untestable was wrong. It does write with
File(outFile).writeAsStringSync(...) instead of buildStep.writeAsString, so
package:build_test's writer never sees the output — but pointing out_file at an absolute
path in a scratch directory lets testBuilder drive the real builder and the test read the
result back. That harness is test/support/run_generator.dart. (Don't chdir instead:
dart test runs test files in separate isolates in a single process, so Directory.current
is shared and the tests race.)

test asserts before sort after
determinism_test same 4 assets in 4 different input orders → identical output ❌ ✅
determinism_test the alphabetically first locale drives the generated API ❌ ✅
determinism_test namespace getters are emitted alphabetically ❌ ✅
golden_test full generated output for an all-branches fixture ✅ ✅

The golden fixture covers plain keys, nested objects, single/duplicate/multiple
interpolations, count plurals, {{a.b}} object params, {{a, uppercase}} format params,
reserved-word keys (return/void) and a key needing character sanitising. It was captured
from the pre-sort generator with the assets already in sorted order, so the fact that it
passes on both sides is the evidence that ordering is the only behaviour change.

End to end against feedmepos/pos-v8

Via a path dependency_overrides pointing at this branch,
dart run build_runner build --delete-conflicting-outputs:

  1. cold build (rm -rf .dart_tool/build)
  2. warm incremental build
  3. cold build again

All three produced a byte-identical lib/localizations/source.dart
(sha256 316a932072850c5c…). Namespace order is now alphabetical:

calendar, common, home, inventory, lockscreen, order, queue,
register_user, report, restaurant, settings, shell, transaction

Reproduced independently through tool/generate_from_dir.dart, which feeds a real
localizations tree (39 json files) straight into the builder and can shuffle the input
order:

pre-sort,  5 shuffle seeds  → 5 different sha256
post-sort, 6 input orders   → all 316a9320… (matches the build_runner run)
pre-sort,  already-sorted   → 316a9320…  ← identical to the post-sort output

That last line is the point: given sorted inputs the old code produces this PR's output
byte for byte, so the change adds no behaviour beyond the ordering.

Structural diff of the generated file

PR output vs. the source.dart pos-v8 has committed today:

451 classes on both sides, class-name sets identical
members present in only one side:                    0
same signature, different i18next.t(...) body:       0
signature differences:                               2  ← the two documented above

dart analyze lib/ in pos-v8 is line-for-line identical with the reordered file swapped
in (178 findings both ways).

Neither flipped method has a call site in pos-v8. Four other multi-parameter generated
methods are called positionally there — completedCount, costPair, quantityPair,
rejectedDetails — and their signatures are unchanged, so this upgrade is safe; but it
shows the exposure is not hypothetical, which is the argument for the base-locale follow-up
below.

Consumer impact

  • Not auto-picked-up. Dart caret semantics below 1.0.0 mean ^0.0.3 resolves to
    >=0.0.3 <0.0.4, so consumers stay on 0.0.3 until they bump the constraint explicitly.
    pos-v8 pins i18next_class_generator: ^0.0.3.
  • Needs a pub.dev publish — pos-v8 resolves this from pub.dev, not from git.
  • The first regeneration after upgrading produces one large reordering diff (7412 lines each
    way for pos-v8), and every regeneration after that is stable. Worth landing as its own
    commit on the consumer side so the churn is isolated and reviewable.
  • Consumers should check for positional calls to generated interpolation methods before
    upgrading, per the signature change above.

Dev-dependency notes

Getting the tests to run needed two unrelated papercuts handled; both are commented at
their call sites.

  • build_web_compilers moves ^3.0.0 → ^4.8.0. The old constraint is scaffold residue —
    nothing here compiles for web — and it blocked build_test from resolving at all.
  • pub get must pass --no-example: example/'s intl: ^0.17.0 conflicts with the
    flutter_localizations any current Flutter SDK pins. flutter test re-runs pub get
    without that flag and hits the same wall, so CI drives dart test directly.
  • build_test stays on ^2.2.3 on purpose. 3.x wants build ^4 and dart_style ^3, and
    dart_style 3 reflows the generated code — that would rewrite the golden for reasons
    unrelated to this change.
  • dart analyze is not in CI: lib/ has 10 pre-existing findings on lines this branch does
    not touch. test/ and tool/ are clean.

Follow-ups not in this PR

  • Route the output write through buildStep.writeAsString so the builder plays by build
    system rules — still worth doing for correctness, though it is no longer a prerequisite
    for testing.
  • Make the base locale an explicit builder option (defaulting to en-US) instead of
    positional entries.first, so it is a stated contract rather than a sorting side effect.
    Today a new locale directory sorting before en-US would silently redefine the generated
    API.
  • Fill in the empty // validation loop (for (var ns in languageMapping.entries) {}) to
    fail the build when locales disagree on key sets, instead of silently generating from one
    of them.

`findAssets` yields the glob matches in an unspecified order, and nothing
sorted them. build_runner backs that call two different ways -- a `dart:io`
directory crawl on a cold asset graph (raw `readdir` order, not
alphabetical) and the persisted graph's node insertion order on an
incremental build -- so identical inputs produced different output
depending on how the build ran.

That order fed `languageMapping`, a LinkedHashMap, and from there the order
of the emitted classes and `I18n` getters. In feedmepos/pos-v8 a
regeneration with no localization change at all moved 1633 lines.

It also decided which locale defines the generated API, since that is
`languageMapping.entries.first`. That is not cosmetic: interpolation
parameters are emitted positionally in the order the `{{...}}` placeholders
appear in the chosen locale's string, so the driving locale silently
determines method signatures. pos-v8 had
`combineSuccessDescription(source, target)` generated from a non-en-US
string while en-US reads "{{target}} combined with {{source}}".

Sorting by path fixes both: class order is alphabetical and stable across
cold and warm builds, and the driving locale is deterministically the
alphabetically first one.
heiseshandian and others added 2 commits August 22, 2026 07:41
The builder writes its output with `File(outFile).writeAsStringSync()`
rather than `buildStep.writeAsString`, so `package:build_test`'s writer
never sees it. That does not make the builder untestable: pointing
`out_file` at an absolute path inside a scratch directory lets
`testBuilder` drive the real builder and the test read the result back.
`test/support/run_generator.dart` is that harness. (An earlier version
chdir'd instead, which races -- `dart test` runs test files in separate
isolates in one process, so `Directory.current` is shared.)

On top of it:

- `determinism_test.dart` feeds the same four assets in four different
  orders and asserts the output is identical, that the alphabetically
  first locale drives the generated API, and that namespace getters come
  out alphabetical. All three fail before the sort and pass after.

- `golden_test.dart` pins the full generated output for a fixture that
  covers every branch of the generator: plain keys, nested objects,
  single/duplicate/multiple interpolations, `count` plurals, `{{a.b}}`
  object params, `{{a, uppercase}}` format params, reserved-word keys and
  a key needing character sanitising. The golden was captured from the
  pre-sort generator with the assets already sorted, so it passing on
  both sides is the evidence that sorting is the only behaviour change.

- `tool/update_golden.dart` regenerates the golden.

- `tool/generate_from_dir.dart` runs the generator over a real consumer's
  localizations tree, optionally shuffling the inputs. This is how the
  bug was demonstrated end to end against feedmepos/pos-v8 (39 json
  files): five shuffle seeds produced five different outputs before the
  fix and one identical output after, matching the sha256 from a
  build_runner run. Feeding the pre-sort generator already-sorted inputs
  reproduces the post-fix output byte for byte.

CI runs `dart test` on a pinned Flutter SDK. Two workarounds are needed
and are commented in the workflow: `pub get` has to pass `--no-example`
because example/'s `intl: ^0.17.0` conflicts with the
flutter_localizations any current SDK pins, and `flutter test` re-runs
pub get without that flag, so `dart test` drives the suite instead.
`build_web_compilers` moves to ^4.8.0 -- the ^3.0.0 constraint is
scaffold residue (nothing here compiles for web) and it blocked
build_test from resolving. build_test stays on ^2.2.3 deliberately: 3.x
wants `build ^4` and `dart_style ^3`, and dart_style 3 reflows the
generated code, which would rewrite the golden for unrelated reasons.

`dart analyze` is not in CI: lib/ has 10 pre-existing findings on lines
this branch does not touch. test/ and tool/ are clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes the generator’s output reproducible by ensuring globbed localization assets are processed in a deterministic order, and adds a test/CI harness to prevent regressions. It also bumps the package version and documents the change.

Changes:

  • Sort findAssets(Glob(...)) results before building languageMapping to make generation order deterministic.
  • Add determinism + golden characterization tests, plus helper scripts to run the generator under build_test.
  • Add a CI workflow to run tests, bump version to 0.0.4, and document the behavior change in the changelog.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
lib/i18next_class_generator.dart Sort glob results before generation; add explanatory comments about determinism and driving locale.
pubspec.yaml Version bump + dev dependency additions/updates to support testing/CI.
CHANGELOG.md Document determinism fix and locale selection impact.
.github/workflows/test.yml Add CI job to run dart test under Flutter.
test/determinism_test.dart New tests asserting output stability and deterministic driving-locale behavior.
test/golden_test.dart Golden characterization test to detect non-ordering behavioral changes.
test/goldens/expected_output.dart.txt Golden expected output for characterization test.
test/support/fixtures.dart Test fixtures covering multiple generator branches (interpolations, plurals, sanitization, etc.).
test/support/run_generator.dart Helper to run the builder and capture output via a temp out_file.
tool/update_golden.dart Utility to regenerate/update the golden output file.
tool/generate_from_dir.dart Utility to run the generator against a real localization tree (optionally shuffled input).
test/i18next_class_generator_test.dart Remove unused scaffolded test stub.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/i18next_class_generator.dart Outdated
Comment thread pubspec.yaml
Comment thread CHANGELOG.md
Comment thread tool/generate_from_dir.dart Outdated
…ths in tool

- sort assets by (package, path) so the comparator is total even in
  hypothetical multi-package matches (List.sort is not stable)
- changelog: state the driving locale as the alphabetically first locale
  directory, with en-US only as an example
- tool/generate_from_dir.dart: normalize relative paths to POSIX so the
  constructed asset ids are valid on Windows

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@heiseshandian
heiseshandian merged commit ac0ce1f into main Aug 22, 2026
1 check passed
@heiseshandian
heiseshandian deleted the fix/deterministic-output branch August 22, 2026 06:49
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.

Generated output is not reproducible: glob results are never sorted, so class order (and the driving locale) depend on filesystem / asset-graph order

3 participants