Skip to content

loc: replace reflection-based args with tuple-params + IDictionary overloads#354

Merged
codemonkeychris merged 3 commits into
mainfrom
codemonkeychris/loc-tuple-args
May 20, 2026
Merged

loc: replace reflection-based args with tuple-params + IDictionary overloads#354
codemonkeychris merged 3 commits into
mainfrom
codemonkeychris/loc-tuple-args

Conversation

@codemonkeychris
Copy link
Copy Markdown
Collaborator

Why

PR #353 skipped Localization_LocaleSwitching under AOT because
t.Message(key, new { name = "World" }) reflects over the anonymous type
to build the ICU args dictionary, and the trimmer drops the property
getters under NativeAOT (ctor + backing fields are statically reachable
from the new { ... } site, but property getters aren't, since the only
reader was a reflective GetProperties() call). GetProperties()
returned empty, MessageFormat.FormatMessage threw on {name}, the
catch swallowed, and the raw ICU pattern leaked through to the UI.

We don't want a reflection-based dispatcher in this code path anyway —
it forces every [UnconditionalSuppressMessage] we''ve written on the
type, and the security model (refuse arbitrary DTOs via
IsAcceptableArgsContainer) is doing nothing the type system couldn''t
do statically.

What

Replace the object?-typed args parameter with two AOT-safe overloads:

// 1. Explicit dict — for shared/programmatic args
public string Message(MessageKey key, IDictionary<string, object>? args = null);

// 2. Tuple-params — for the compact inline case
public string Message(MessageKey key,
    (string Name, object Value) arg1,
    params (string Name, object Value)[] more);

Callers go from this:

t.Message(Greeting, new { name = "World" })
t.Message(SearchResults, new { count = 0, query = "test" })

to this:

t.Message(Greeting, ("name", "World"))
t.Message(SearchResults, ("count", 0), ("query", "test"))

Same shape for RichMessage. The tagged variant uses the dict overload
because params must be last:

t.RichMessage(Loc.Profile.Welcome,
    args: new Dictionary<string, object> { ["name"] = "Alice" },
    tags: new() { ["bold"] = text => Text(text).Bold() })

ValueTuple is a nominal type with concrete fields — building a
Dictionary<string,object> from tuple values needs zero reflection,
so the whole IL2026/IL2070 surface for this method disappears.

Deleted

  • ToArgsDictionary, IsAcceptableArgsContainer, _propertyCache
  • The [UnconditionalSuppressMessage("Trimming", "IL2026"/"IL2070")] attrs
  • using System.Reflection, System.Collections.Concurrent,
    System.Diagnostics.CodeAnalysis (now unused)
  • The [LocArgs] soft opt-in marker (string-name match in
    IsAcceptableArgsContainer)

Migrated callers

  • Selftest + AppTests localization fixtures (two files)
  • IntlAccessorTests, RichMessageTests
  • Reactor.Cli/Loc/SourceRewriter: emits tuple syntax now. CLI codegen
    for interpolated strings now writes t.Message(key, ("name", expr))
    instead of t.Message(key, new { name = expr }). Updated
    SourceRewriterTests assertions.
  • Docs: _pipeline/templates/localization.md.dt narrative and
    _pipeline/apps/localization/App.cs snippets (the compiled
    docs/guide/localization.md regenerates from these). Plus the
    hand-edited docs/reference/localization-howto.md example.

AOT skip list

  • Localization_LocaleSwitching removed from DefaultAotSkipPatterns
    in SelfTestRunner.cs.
  • "Localization with anonymous-type args" row removed from
    docs/aot-support.md — there''s no anonymous-type path left to break.

Verified

  • dotnet build src/Reactor — clean (AOT warnings remain as errors).
  • dotnet test tests/Reactor.Tests --filter "FullyQualifiedName~Localization" — 282/282 pass.
  • dotnet run --project tests/Reactor.AppTests.Host -- --self-test
    (JIT, full suite) — 0 failures.
  • AOT republish + selftest pending verification on this PR; will report
    back with numbers.

Breaking change

t.Message(key, new { name = "..." }) no longer compiles. The compiler
error is reasonably clear ("cannot convert anonymous type to
IDictionary<string, object>"); no analyzer needed. Migration is a
mechanical new { name = X }("name", X).

Base

This PR is based on codemonkeychris/aot-selftest-skips-round3
(PR #353). When that merges, GitHub will retarget this PR to main.

codemonkeychris added a commit that referenced this pull request May 20, 2026
Localization_LocaleSwitching now runs under AOT (PR #354), so:
  193 skipped -> 192 skipped
  ~542 passed -> ~543 passed
Total fixture count stays at 735.

Also drops "anonymous-type localization args" from the documented
skip-cluster list since that subsystem is now AOT-clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codemonkeychris
Copy link
Copy Markdown
Collaborator Author

AOT verification complete.

Full AOT selftest run against the published binary on this branch:

Metric Before (PR #353) After this PR
ok sub-assertions 2184 2224
not ok 0 0
# SKIP 193 192
Localization_LocaleSwitching skipped runs, 40/40 pass

The 40 newly-passing sub-assertions cover Greeting / Plural_Zero / Plural_Five / Search_Zero / Search_One / Search_Many / Direction across en-US, ko-KR, ar-SA, ja-JP, de-DE, plus en-US round-trip. Tuple-params overload behaves identically under AOT and JIT.

Pushed CONTRIBUTING.md count refresh to match (192 skipped, ~543 passed, 0 failed).

@codemonkeychris codemonkeychris requested a review from Copilot May 20, 2026 15:47
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR removes reflection-based localization argument handling (which breaks under NativeAOT trimming) and replaces it with AOT-safe argument passing via IDictionary<string, object> and tuple-params overloads for Message/RichMessage.

Changes:

  • Updated IntlAccessor to accept explicit dictionaries and added tuple-params overloads that build args without reflection.
  • Migrated call sites (tests, fixtures, CLI source rewriter) from anonymous objects to tuple args / dictionaries.
  • Updated documentation and AOT skip list to reflect the new, AOT-safe localization args model.
Show a summary per file
File Description
tests/Reactor.Tests/Localization/SourceRewriterTests.cs Updates assertions to match tuple-arg codegen output.
tests/Reactor.Tests/Localization/RichMessageTests.cs Switches rich-message args to explicit dictionary.
tests/Reactor.Tests/Localization/IntlAccessorTests.cs Migrates tests from anonymous args objects to tuple args.
tests/Reactor.AppTests.Host/SelfTest/SelfTestRunner.cs Removes localization fixture from AOT skip list and updates rationale.
tests/Reactor.AppTests.Host/SelfTest/Fixtures/LocalizationFixtures.cs Migrates localization fixture calls to tuple args.
tests/Reactor.AppTests.Host/Fixtures/LocalizationFixtures.cs Migrates app-test fixture calls to tuple args.
src/Reactor/Core/Localization/IntlAccessor.cs Core API change: dictionary-based args + tuple overloads; removes reflection helpers.
src/Reactor.Cli/Loc/SourceRewriter.cs Emits tuple-arg syntax instead of anonymous objects.
docs/reference/localization-howto.md Updates example to use tuple args.
docs/aot-support.md Removes anonymous-type localization args as an AOT limitation.
docs/_pipeline/templates/localization.md.dt Updates narrative to describe tuple args as the primary interpolation shape.
docs/_pipeline/apps/localization/App.cs Updates code snippets to use tuple args.
CONTRIBUTING.md Updates expected AOT skip/pass counts after removing the localization skip.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 13/13 changed files
  • Comments generated: 5

Comment thread src/Reactor/Core/Localization/IntlAccessor.cs
Comment thread src/Reactor/Core/Localization/IntlAccessor.cs
Comment thread src/Reactor.Cli/Loc/SourceRewriter.cs Outdated
Comment thread src/Reactor.Cli/Loc/SourceRewriter.cs
Comment thread docs/_pipeline/templates/localization.md.dt Outdated
codemonkeychris added a commit that referenced this pull request May 20, 2026
Five inline findings from copilot-pull-request-reviewer:

1. BuildArgs null-skip semantics. Behavior parity with the removed
   ToArgsDictionary, which dropped null-valued properties so the formatter
   saw the placeholder as missing rather than substituting "null". Tuple
   value type is now object?, nulls are skipped before insertion, and
   a unit test pins the behavior.

2. cref XML doc warnings. The RichMessage <see cref=...> referenced a
   method signature with C# keyword generics and nullable annotations,
   which can fail to resolve. Replaced with a prose reference and a
   <c>RichMessage</c> code element.

3. SourceRewriter param-name escaping (x2). The generator emitted
   $"(\"{param}\", ...)" without escaping the placeholder name. ICU spec
   doesn't forbid " or \ in param names — if one slipped through,
   the emitted source became invalid C#. Now uses
   SymbolDisplay.FormatLiteral to produce a valid C# string literal.

4. Docs template phrasing. (name, value) reads like a positional
   tuple with an identifier first item; readers might try
   	.Message(key, (name, value)). Tightened to ("name", value) and
   added a sentence explaining the first item is the placeholder name
   as a string literal.

Also: ran mur docs compile --skip-screenshots, which regenerated
docs/guide/localization.md (template change) and docs/guide/navigation.md
(picked up an unrelated drift from the navigation doc app that an
earlier commit forgot to regen). Both checked in so docs/guide stays
authoritative.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codemonkeychris
Copy link
Copy Markdown
Collaborator Author

Addressed all 5 reviewer findings in 615b10f:

  1. BuildArgs null semantics — switched the tuple value type to object?, drop null-valued tuples before they hit the dict (matches the removed ToArgsDictionary''s skip-nulls behavior), added an IntlAccessorTests case pinning it.
  2. cref XML warnings on RichMessage doc — the cref referenced a signature with C# keyword generics + nullability, which doesn''t resolve. Replaced with a prose reference + <c>RichMessage</c>.
  3. SourceRewriter ICU param-name escaping (two comments, same finding) — now uses SymbolDisplay.FormatLiteral(param, quote: true) to produce a valid C# string literal so a placeholder with \" or \\ can''t corrupt the emitted source.
  4. Template phrasing — tightened (name, value) to (\"name\", value) with an explicit sentence that the first item is the placeholder name as a string literal. Regenerated docs/guide/localization.md from the template.

JIT localization tests: 283/283 (the +1 is the new null-skip case).

Base automatically changed from codemonkeychris/aot-selftest-skips-round3 to main May 20, 2026 16:23
codemonkeychris added a commit that referenced this pull request May 20, 2026
Localization_LocaleSwitching now runs under AOT (PR #354), so:
  193 skipped -> 192 skipped
  ~542 passed -> ~543 passed
Total fixture count stays at 735.

Also drops "anonymous-type localization args" from the documented
skip-cluster list since that subsystem is now AOT-clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
codemonkeychris added a commit that referenced this pull request May 20, 2026
Five inline findings from copilot-pull-request-reviewer:

1. BuildArgs null-skip semantics. Behavior parity with the removed
   ToArgsDictionary, which dropped null-valued properties so the formatter
   saw the placeholder as missing rather than substituting "null". Tuple
   value type is now object?, nulls are skipped before insertion, and
   a unit test pins the behavior.

2. cref XML doc warnings. The RichMessage <see cref=...> referenced a
   method signature with C# keyword generics and nullable annotations,
   which can fail to resolve. Replaced with a prose reference and a
   <c>RichMessage</c> code element.

3. SourceRewriter param-name escaping (x2). The generator emitted
   $"(\"{param}\", ...)" without escaping the placeholder name. ICU spec
   doesn't forbid " or \ in param names — if one slipped through,
   the emitted source became invalid C#. Now uses
   SymbolDisplay.FormatLiteral to produce a valid C# string literal.

4. Docs template phrasing. (name, value) reads like a positional
   tuple with an identifier first item; readers might try
   	.Message(key, (name, value)). Tightened to ("name", value) and
   added a sentence explaining the first item is the placeholder name
   as a string literal.

Also: ran mur docs compile --skip-screenshots, which regenerated
docs/guide/localization.md (template change) and docs/guide/navigation.md
(picked up an unrelated drift from the navigation doc app that an
earlier commit forgot to regen). Both checked in so docs/guide stays
authoritative.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codemonkeychris codemonkeychris force-pushed the codemonkeychris/loc-tuple-args branch from 615b10f to 3cb6a07 Compare May 20, 2026 16:25
codemonkeychris and others added 3 commits May 20, 2026 09:39
…erloads

The Message(MessageKey, object?) and RichMessage(MessageKey, object?, ...)
overloads built an args dictionary by reflecting over public properties of
the passed object — anonymous types or [LocArgs]-marked records. Under
NativeAOT the trimmer keeps the ctor + backing fields of the anonymous type
(both reachable from the
ew { … } call site) but drops the property
getters, because nothing statically reads them. GetProperties() then
returned an empty array, MessageFormat threw on {name}, the catch
swallowed the exception, and the raw ICU pattern leaked through to the UI.

This change removes the reflection path entirely and gives callers two
AOT-safe shapes:

  - Message(MessageKey, IDictionary<string, object>?) — explicit dict for
    cases that already have one (or want to share/build one programmatically).
  - Message(MessageKey, (string Name, object Value) arg1,
              params (string Name, object Value)[] more) — compact tuple
    syntax for the common inline case. ValueTuple is a nominal type with
    concrete fields; building a Dictionary<string,object> from tuple
    values needs zero reflection.

Same shape for RichMessage. (RichMessage with tags uses the dict
overload — params must come last, so the tuple variant can't carry
the optional 	ags argument.)

Deleted:
  - ToArgsDictionary + IsAcceptableArgsContainer + _propertyCache
  - The IL2026/IL2070 unconditional-suppression attributes those needed
  - System.Reflection + System.Collections.Concurrent +
    System.Diagnostics.CodeAnalysis usings (now unused)

Migrated callers:
  - Selftest + AppTests localization fixtures (two files)
  - IntlAccessorTests, RichMessageTests
  - SourceRewriter CLI codegen: 	.Message(key, new { name = expr })
    → 	.Message(key, ("name", expr)). Updated SourceRewriterTests.
  - Docs: _pipeline/templates/localization.md.dt narrative and
    _pipeline/apps/localization/App.cs snippets, plus the
    hand-edited docs/reference/localization-howto.md example.

Removed from AOT skip list / docs:
  - Localization_LocaleSwitching no longer needs to be skipped under AOT
    (	ests/Reactor.AppTests.Host/SelfTest/SelfTestRunner.cs).
  - The "Localization with anonymous-type args" row in docs/aot-support.md
    is gone — there's no anonymous-type path left.

Verified:
  - dotnet build src/Reactor clean (AOT warnings as errors).
  - 282 localization unit tests pass.
  - Full JIT selftest: 0 failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Localization_LocaleSwitching now runs under AOT (PR #354), so:
  193 skipped -> 192 skipped
  ~542 passed -> ~543 passed
Total fixture count stays at 735.

Also drops "anonymous-type localization args" from the documented
skip-cluster list since that subsystem is now AOT-clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Five inline findings from copilot-pull-request-reviewer:

1. BuildArgs null-skip semantics. Behavior parity with the removed
   ToArgsDictionary, which dropped null-valued properties so the formatter
   saw the placeholder as missing rather than substituting "null". Tuple
   value type is now object?, nulls are skipped before insertion, and
   a unit test pins the behavior.

2. cref XML doc warnings. The RichMessage <see cref=...> referenced a
   method signature with C# keyword generics and nullable annotations,
   which can fail to resolve. Replaced with a prose reference and a
   <c>RichMessage</c> code element.

3. SourceRewriter param-name escaping (x2). The generator emitted
   $"(\"{param}\", ...)" without escaping the placeholder name. ICU spec
   doesn't forbid " or \ in param names — if one slipped through,
   the emitted source became invalid C#. Now uses
   SymbolDisplay.FormatLiteral to produce a valid C# string literal.

4. Docs template phrasing. (name, value) reads like a positional
   tuple with an identifier first item; readers might try
   	.Message(key, (name, value)). Tightened to ("name", value) and
   added a sentence explaining the first item is the placeholder name
   as a string literal.

Also: ran mur docs compile --skip-screenshots, which regenerated
docs/guide/localization.md (template change) and docs/guide/navigation.md
(picked up an unrelated drift from the navigation doc app that an
earlier commit forgot to regen). Both checked in so docs/guide stays
authoritative.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codemonkeychris codemonkeychris force-pushed the codemonkeychris/loc-tuple-args branch from 3cb6a07 to 290eca6 Compare May 20, 2026 16:43
@codemonkeychris codemonkeychris merged commit 3c3bed9 into main May 20, 2026
9 checks passed
@codemonkeychris codemonkeychris deleted the codemonkeychris/loc-tuple-args branch May 20, 2026 17:46
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.

2 participants