From 9ea1b547586979d2630a0e37eeb27601468520f5 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sun, 3 May 2026 18:23:50 -0700 Subject: [PATCH] docs(messages): document interpolation anti-patterns and :L format spec (#2441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Captures the durable learnings from the message-interpolation work (PRs #2434, #2436, #2437, #2438, #2440) as reference documentation. **Doc-only PR — no code changes.** The original Phase 2 audit (PR #2435) was development scaffolding and was closed unmerged once Phase 3 consumed it. This PR replaces it with proper reference docs that future authors can consult. ## What's added ### `dev-docs/string-handling.md` - Promote `RawInterpolatedStringHandler` from a one-line note to a proper section listing all APIs that accept it (messages, OPL, gumps, packets). - Document the `:L` lowercase format specifier. - New comprehensive **"Interpolation Anti-Patterns"** section covering 8 patterns with before/after examples — applies to any handler-aware API: 1. Ternary with interpolated branches 2. Switch expression with interpolated arms 3. Pre-built local typed as `string` 4. `.ToString()` (or any string-returning method) inside a hole 5. String concatenation inside a hole 6. `string.Format` feeding a handler-aware API 7. LINQ-built strings inside a hole 8. Pre-built concat var ### `dev-docs/networking-packets.md` - Add **"Player-Facing Message APIs"** section listing `Mobile` / `Item` / `NetState` message methods with their handler overloads. - Note the `IBroadcastFilter` pattern for new spatial-broadcast helpers. ### `dev-docs/property-lists.md`, `dev-docs/gump-system.md` - Cross-reference the new anti-patterns section. - Add explicit `.ToString()` inside holes warning to property-lists (it had no such guidance before). ### `dev-docs/claude-skills/` - Mirror the same content (condensed) in `modernuo-string-handling.md`, `modernuo-networking.md`, `modernuo-property-lists.md`, `modernuo-gump-system.md`. - Add audit rule #17 to `modernuo-code-audit.md` covering all 8 anti-patterns with severity WARNING, plus the `:L` format spec. ### `CLAUDE.md` - Add audit rule #18 summarizing the interpolation anti-patterns + `:L`, pointing to `dev-docs/string-handling.md` for details. ## Why this matters Before this PR there was no documentation explaining when an interpolated string call site silently allocates a string despite the receiving API providing a handler overload. The Phase 3 cleanup (PRs #2436/#2437/#2438) discovered ~28 such sites in the codebase; without these docs the same patterns would re-emerge. The new audit rule + CLAUDE.md entry will catch them at write time. --- CLAUDE.md | 1 + dev-docs/claude-skills/modernuo-code-audit.md | 30 ++- .../claude-skills/modernuo-gump-system.md | 9 + dev-docs/claude-skills/modernuo-networking.md | 57 ++++++ .../claude-skills/modernuo-property-lists.md | 14 ++ .../claude-skills/modernuo-string-handling.md | 26 ++- dev-docs/gump-system.md | 12 ++ dev-docs/networking-packets.md | 82 ++++++++ dev-docs/property-lists.md | 16 ++ dev-docs/string-handling.md | 190 +++++++++++++++++- 10 files changed, 431 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index eca813754..d9ac0246f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,7 @@ Apply these when writing or reviewing `.cs` files under `Projects/`. 15. **Braces required on all control flow** — `if`, `else`, `for`, `foreach`, `while`, `do`, `switch` must always have braces, even for single-line bodies → `dev-docs/code-standards.md` 16. **Prefer switch expressions and switch-when** — use switch expressions for value mapping and switch-when for pattern matching where they improve readability. Exception: skip if unreadable or cold path → `dev-docs/code-standards.md` 17. **No `System.Text.StringBuilder`** — use `ValueStringBuilder` with `stackalloc` (bounded output) or `ValueStringBuilder.Create()` (unbounded). Supports `$"..."` interpolation directly. Always use `using var` for disposal. Use `Reset()` instead of reassigning → `dev-docs/string-handling.md` +18. **Interpolation anti-patterns on handler-aware APIs** — `Send*`/`Say`/`Emote`/`PublicOverhead*`/`IPropertyList.Add`/gump `AddLabel`/`AddHtml`/`Html.Center`/`SpanWriter.Write*` all have `ref RawInterpolatedStringHandler` overloads that allocate zero strings, but only when the call-site argument is a `$"..."` literal directly. Avoid: ternaries with interpolated branches (`Send(c ? $"a" : $"b")`), switch expressions with interpolated arms, pre-built `var s = $"..."` locals (single-use), `.ToString()` / `.String()` / `string.Format` inside holes, string concat (`{a + b}`), LINQ string ops in holes. Use `:L` format spec for lowercase (`{rank:L}` not `rank.ToString().ToLowerInvariant()`) → `dev-docs/string-handling.md` § Interpolation Anti-Patterns ## Dev-Docs Reference diff --git a/dev-docs/claude-skills/modernuo-code-audit.md b/dev-docs/claude-skills/modernuo-code-audit.md index 4c8151d7d..39a8d4983 100644 --- a/dev-docs/claude-skills/modernuo-code-audit.md +++ b/dev-docs/claude-skills/modernuo-code-audit.md @@ -163,9 +163,37 @@ return type switch **Why**: Switch expressions enable JIT/PGO optimization and improve readability. **Exception**: Skip if the switch would be unreadable or the code is on a cold path. +### 17. Interpolation Anti-Patterns (handler-aware APIs) + +**Context**: Many ModernUO APIs accept `ref RawInterpolatedStringHandler` (`Mobile.SendMessage`/`Say`/`Emote`/etc., `Item.Public/Local/NonlocalOverheadMessage`/`SendLocalizedMessageTo`/`SendMessageTo`, `IPropertyList.Add`, `SpanWriter.WriteAscii`/`WriteLatin1`, gump `AddLabel`/`AddHtml`/`AddHtmlLocalized`, `Html.Center`/`Color`/`Right`). The handler overload renders the interpolation directly into a pooled buffer with **zero `string` allocation** — but only when the call-site argument is a `$"..."` literal directly in the parameter slot. + +**Check**: Flag any of the following patterns when the call target is one of those handler-aware APIs. The handler overload is silently bypassed and a `string` is allocated per call. + +| Pattern | Fix | +|---|---| +| `Send(cond ? $"a" : $"b")` | `if/else` with two calls | +| `Send(thing switch { 1 => $"a", _ => $"b" })` | `switch` statement, call per arm | +| `var s = $"foo {x}"; Send(s);` (single-use) | Inline at call site | +| `Send($"x {value.ToString()}")` | Drop `.ToString()` — handler formats directly | +| `Send($"x {td.String()}")` | Drop `.String()` — pass `td` directly | +| `Send($"x {a + b}")` (string concat) | Multiple holes: `Send($"x {a}{b}")` | +| `Send(string.Format("x {0}", v))` | `Send($"x {v}")` | +| `Send($"x {items.Aggregate(...)}")` | Build via `ValueStringBuilder`, pass span | + +**For lowercase output**, use the `:L` format specifier instead of `value.ToString().ToLowerInvariant()`: +```csharp +mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" not "Gold" +``` + +**Why**: These methods are called constantly during gameplay (every chat line, every system message, every gump label, every tooltip). The handler overload exists specifically to eliminate per-call `string` allocation. Each anti-pattern leaks one or more strings per call. + +**Severity**: WARNING. Flag and ask before fixing — some patterns (e.g., reused locals across multiple call sites) are intentional and shouldn't be inlined. + +**See**: `dev-docs/string-handling.md` § "Interpolation Anti-Patterns" for the full reference with detailed before/after examples. + ## Severity Levels - **ERROR**: Rules 3, 9, 10, 13 (will cause bugs, build failures, or client-side leaks) -- **WARNING**: Rules 1 (Tier 3 LINQ), 2, 4, 5, 6, 7, 8, 12, 14, 15 (performance/convention issues) +- **WARNING**: Rules 1 (Tier 3 LINQ), 2, 4, 5, 6, 7, 8, 12, 14, 15, 17 (performance/convention issues) - **INFO**: Rules 1 (Tier 2 LINQ on warm paths — note it but don't flag as violation), 16 (switch patterns — suggest but don't flag) - **ASK**: Rule 11 (need user input) diff --git a/dev-docs/claude-skills/modernuo-gump-system.md b/dev-docs/claude-skills/modernuo-gump-system.md index 75952163d..ad00db114 100644 --- a/dev-docs/claude-skills/modernuo-gump-system.md +++ b/dev-docs/claude-skills/modernuo-gump-system.md @@ -170,6 +170,15 @@ builder.AddLabelPlaceholder(int x, int y, int hue, ReadOnlySpan slotKey); builder.AddHtmlPlaceholder(int x, int y, int w, int h, ReadOnlySpan slotKey, ...); ``` +Text-accepting builders take `ROS` and have `ref RawInterpolatedStringHandler` overloads — `$"..."` literals at the call site are zero-allocation. Same for `Html.Center`/`Html.Color`/`Html.Right` helpers used for HTML markup wrapping. + +```csharp +builder.AddHtml(20, 20, 200, 100, $"
{Title}: {Score:N0}
"); +builder.AddLabel(20, 40, hue, $"You have {gold} gold"); +``` + +Avoid the interpolation anti-patterns (ternaries with `$"..."` branches, `.ToString()` inside holes, pre-built `var msg = $"..."` locals, `string.Format`, etc.) — they silently fall back to string-allocating overloads. See `dev-docs/claude-skills/modernuo-string-handling.md` § "Interpolation Anti-Patterns" for the full list. + ### Interactive ```csharp builder.AddButton(int x, int y, int normalID, int pressedID, int buttonID, ...); diff --git a/dev-docs/claude-skills/modernuo-networking.md b/dev-docs/claude-skills/modernuo-networking.md index 6f8c56ee5..9b049bc89 100644 --- a/dev-docs/claude-skills/modernuo-networking.md +++ b/dev-docs/claude-skills/modernuo-networking.md @@ -220,6 +220,63 @@ reader.Length; // Total data length reader.Remaining; // Bytes remaining ``` +## Player-Facing Message APIs + +For chat / system messages / overhead text, use the convenience methods on `Mobile` and `Item` rather than building packets manually. They handle stackalloc sizing, spatial queries, and visibility filtering, and each has a `ref RawInterpolatedStringHandler` overload for zero-allocation interpolation. + +### Mobile + +```csharp +// Self-message +mob.SendMessage(int hue, ROS text); +mob.SendLocalizedMessage(int number, ROS args = default, int hue = 0x3B2); +mob.SendAsciiMessage(int hue, ROS text); + +// Speech (broadcast in range) +mob.Say(ROS text); // SpeechHue +mob.Emote(ROS text); // EmoteHue +mob.Whisper(ROS text); // WhisperHue, short range +mob.Yell(ROS text); // YellHue, long range + +// Overhead (3 visibility variants × text/localized × affix) +mob.PublicOverheadMessage(MessageType, int hue, bool ascii, ROS text, ...); +mob.PrivateOverheadMessage(MessageType, int hue, int number, ROS args, NetState); +mob.LocalOverheadMessage(MessageType, int hue, bool ascii, ROS text); +mob.NonlocalOverheadMessage(MessageType, int hue, int number, ROS args = default); +``` + +### Item + +```csharp +item.PublicOverheadMessage(MessageType, int hue, bool ascii, ROS text); +item.PublicOverheadMessage(MessageType, int hue, int number, ROS args = default); +item.SendLocalizedMessageTo(Mobile to, int number, ROS args = default); +item.SendLocalizedMessageTo(Mobile to, int number, int hue, ROS args = default); +item.SendMessageTo(Mobile to, ROS text, int hue = 0x3B2); +``` + +### Zero-allocation interpolation + +```csharp +// Compiler picks the handler overload — no string allocated +mob.SendMessage($"You have {gold:N0} gold"); +mob.Say($"Hello, {target.Name}!"); +item.SendLocalizedMessageTo(player, cliloc, $"{a}\t{b}"); +``` + +Several call-site shapes (ternaries with interpolated branches, `.ToString()` inside holes, pre-built `var msg = $"..."` locals, etc.) silently defeat handler binding and allocate a `string`. See `dev-docs/string-handling.md` § "Interpolation Anti-Patterns" for the list and fixes. + +For lowercase output, use `:L`: +```csharp +mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" not "Gold" +``` + +### Implementation note + +The 7 spatial-broadcast variants (`Public/NonlocalOverheadMessage` × text/localized/affix) route through generic `OutgoingMessagePackets.Broadcast*` helpers parameterized over a `private readonly struct` filter that encapsulates the visibility predicate. The `where TFilter : struct, IBroadcastFilter` constraint specializes per filter type and keeps dispatch zero-alloc. If you add a new spatial-message API, add a filter struct and call the existing helper rather than duplicating the loop. + +--- + ## Common Packet Patterns ### Sound Effect diff --git a/dev-docs/claude-skills/modernuo-property-lists.md b/dev-docs/claude-skills/modernuo-property-lists.md index 9eeb5eabc..b9e018cdf 100644 --- a/dev-docs/claude-skills/modernuo-property-lists.md +++ b/dev-docs/claude-skills/modernuo-property-lists.md @@ -90,6 +90,20 @@ list.Add(1060661, $"{"Range"}\t{_range}"); **Rule**: Only `\t` (argument separator) should be bare literal text. Everything else — including string constants — must be inside `{}` holes. +### No `.ToString()` Inside Holes + +`IPropertyList`'s handler formats values directly via `ISpanFormattable.TryFormat` — no intermediate `string` allocation per hole. An explicit `.ToString()` defeats this: + +```csharp +// BAD — .ToString() allocates a string the handler then re-buffers +list.Add(1060658, $"{"Charges"}\t{_charges.ToString()}"); + +// GOOD — handler formats _charges directly with no intermediate string +list.Add(1060658, $"{"Charges"}\t{_charges}"); +``` + +Same applies to `.String()` (TextDefinition), `.GetValue()`, etc. The full list of interpolation anti-patterns (ternaries, switch expressions, pre-built locals, `string.Format`, concat in hole, LINQ in hole) applies equally to `IPropertyList.Add($"...")`. See `dev-docs/string-handling.md` § "Interpolation Anti-Patterns" or `dev-docs/claude-skills/modernuo-string-handling.md`. + ### Cliloc as Argument (Use `:#` Format Specifier) When an argument is itself a cliloc number, use the `:#` format specifier — **not** a `"#number"` string: diff --git a/dev-docs/claude-skills/modernuo-string-handling.md b/dev-docs/claude-skills/modernuo-string-handling.md index 0a9371895..76e00454b 100644 --- a/dev-docs/claude-skills/modernuo-string-handling.md +++ b/dev-docs/claude-skills/modernuo-string-handling.md @@ -116,6 +116,29 @@ sb.AppendFormat("{0:N0} points, {1:N0} kills", score, kills); sb.Append($"{score:N0} points, {kills:N0} kills"); ``` +## Interpolation Anti-Patterns (handler-aware APIs) + +Many APIs accept `ref RawInterpolatedStringHandler` (messages on `Mobile`/`Item`, `IPropertyList.Add`, `SpanWriter.WriteAscii`/`WriteLatin1`, gump `AddLabel`/`AddHtml`, `Html.Center`/`Color`/`Right`, etc.). The handler overload renders the interpolation directly into a pooled buffer with zero `string` allocation — but **only when the call-site argument is a `$"..."` literal directly in position**. These patterns silently defeat that selection. Flag any of them in messaging/gump/OPL code. + +| Pattern | Why bad | Fix | +|---|---|---| +| `Send(cond ? $"a" : $"b")` | Ternary unifies branches as `string` | `if/else` with two calls | +| `Send(thing switch { 1 => $"a", _ => $"b" })` | Switch expr unifies as `string` | `switch` statement, call per arm | +| `var s = $"foo {x}"; Send(s);` | Local typed `string`; ROS overload picked | Inline at call site | +| `Send($"x {value.ToString()}")` | `.ToString()` allocates a `string` per call | Drop `.ToString()` — handler formats directly | +| `Send($"x {td.String()}")` | `TextDefinition.String` allocates | Drop `.String()` (or pass `td` directly if API supports it) | +| `Send($"x {a + b}")` | `string + string` allocates | Multiple holes: `Send($"x {a}{b}")` | +| `Send(string.Format("x {0}", v))` | Format allocates | `Send($"x {v}")` | +| `Send($"x {items.Aggregate(...)}")` | LINQ string ops allocate | `ValueStringBuilder` + pass span | + +For lowercase output, use the `:L` format specifier instead of `value.ToString().ToLowerInvariant()`: + +```csharp +mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" not "Gold" +``` + +`:L` is recognized by `RawInterpolatedStringHandler.AppendFormatted(T, string?)` and the `(ROS, int, string?)` overload. Case-sensitive — use uppercase `:L`. + ## Capacity Sizing Guide | Content | Recommended | @@ -128,6 +151,7 @@ sb.Append($"{score:N0} points, {kills:N0} kills"); | Unbounded (logs, file paths) | `Create()` | ## Related Docs -- `dev-docs/string-handling.md` — full reference +- `dev-docs/string-handling.md` — full reference (incl. interpolation anti-patterns + `:L` spec) - `dev-docs/code-standards.md` — memory management rules - `dev-docs/property-lists.md` — IPropertyList string interpolation (different handler) +- `dev-docs/networking-packets.md` — player-facing message APIs and their handler overloads diff --git a/dev-docs/gump-system.md b/dev-docs/gump-system.md index 2601889a9..90b1f7622 100644 --- a/dev-docs/gump-system.md +++ b/dev-docs/gump-system.md @@ -200,6 +200,18 @@ builder.AddHtmlLocalized(x, y, w, h, clilocNumber); // Localized text builder.AddHtmlLocalized(x, y, w, h, clilocNumber, color); ``` +#### Interpolation in text + +Most text-accepting builders take a `ReadOnlySpan` and have a `ref RawInterpolatedStringHandler` overload, so `$"..."` literals at the call site are zero-allocation. The same applies to `Html.Center`, `Html.Color`, `Html.Right` helpers used when wrapping text in HTML markup: + +```csharp +// Zero allocation — interpolation handler renders directly into a pooled buffer +builder.AddHtml(20, 20, 200, 100, $"
{Title}: {Score:N0}
"); +builder.AddLabel(20, 40, hue, $"You have {gold} gold"); +``` + +Several call-site shapes silently defeat the handler overload selection (ternaries with interpolated branches, `.ToString()` inside holes, pre-built `var msg = $"..."` locals, etc.). See [`dev-docs/string-handling.md`](string-handling.md#interpolation-anti-patterns) for the full list and fixes — they apply equally inside `BuildLayout`. + ### Interactive Elements ```csharp builder.AddButton(x, y, normalID, pressedID, buttonID); diff --git a/dev-docs/networking-packets.md b/dev-docs/networking-packets.md index 61bc127ea..43d4aec15 100644 --- a/dev-docs/networking-packets.md +++ b/dev-docs/networking-packets.md @@ -338,6 +338,88 @@ reader.Buffer; // ReadOnlySpan of full data --- +## Player-Facing Message APIs + +For chat, system messages, and overhead text, prefer the high-level convenience methods on `Mobile` and `Item` — they handle stackalloc sizing, packet buffer initialization, spatial queries, and visibility filtering for you. The underlying packets all live in `OutgoingMessagePackets`. + +### On `Mobile` + +```csharp +// Self-message (sent only to this mobile's NetState) +mob.SendMessage(int hue, ReadOnlySpan text); +mob.SendAsciiMessage(int hue, ReadOnlySpan text); +mob.SendLocalizedMessage(int number, ReadOnlySpan args = default, int hue = 0x3B2); +mob.SendLocalizedMessage(int number, bool append, ReadOnlySpan affix, ReadOnlySpan args = default, int hue = 0x3B2); + +// Speech variants (overhead text from this mobile, broadcast in range) +mob.Say(ReadOnlySpan text); // SpeechHue +mob.Say(int number, ReadOnlySpan args = default); +mob.Emote(ReadOnlySpan text); // EmoteHue +mob.Whisper(ReadOnlySpan text); // WhisperHue, short range +mob.Yell(ReadOnlySpan text); // YellHue, long range + +// Targeted overhead messages +mob.PublicOverheadMessage(MessageType type, int hue, bool ascii, ReadOnlySpan text, bool noLineOfSight = true, AccessLevel accessLevel = AccessLevel.Player); +mob.PublicOverheadMessage(MessageType type, int hue, int number, ReadOnlySpan args = default, bool noLineOfSight = true); +mob.PrivateOverheadMessage(MessageType type, int hue, int number, ReadOnlySpan args, NetState state); +mob.LocalOverheadMessage(MessageType type, int hue, bool ascii, ReadOnlySpan text); +mob.NonlocalOverheadMessage(MessageType type, int hue, int number, ReadOnlySpan args = default); +``` + +### On `Item` + +```csharp +item.PublicOverheadMessage(MessageType type, int hue, bool ascii, ReadOnlySpan text); +item.PublicOverheadMessage(MessageType type, int hue, int number, ReadOnlySpan args = default); +item.SendLocalizedMessageTo(Mobile to, int number, ReadOnlySpan args = default); +item.SendLocalizedMessageTo(Mobile to, int number, int hue, ReadOnlySpan args = default); +item.SendMessageTo(Mobile to, ReadOnlySpan text, int hue = 0x3B2); +``` + +### Direct `NetState` extensions + +When you have a `NetState` and need full control (custom serial, body, font, language): + +```csharp +ns.SendMessage(Serial serial, int graphic, MessageType type, int hue, int font, bool ascii, string lang, ReadOnlySpan name, ReadOnlySpan text); +ns.SendMessageLocalized(Serial serial, int graphic, MessageType type, int hue, int font, int number, ReadOnlySpan name = default, ReadOnlySpan args = default); +ns.SendMessageLocalizedAffix(Serial serial, int graphic, MessageType type, int hue, int font, int number, ReadOnlySpan name, AffixType affixType, ReadOnlySpan affix = default, ReadOnlySpan args = default); +``` + +### Zero-allocation interpolation overloads + +Every method above has a `ref RawInterpolatedStringHandler` overload for the text/args parameter. When the call-site argument is a `$"..."` literal, the compiler picks the handler overload and the message text is rendered directly into a pooled `char[]` — no `string` allocation: + +```csharp +mob.SendMessage($"You have {gold:N0} gold and {bounty:N0} bounty"); +mob.Say($"Hello, {target.Name}!"); +item.SendLocalizedMessageTo(player, cliloc, $"{a}\t{b}"); +mob.PublicOverheadMessage(MessageType.Regular, hue, false, $"I am {mob.Name}"); +``` + +When the argument is a pre-built `string` or `ReadOnlySpan` variable, the `ROS` overload is selected via implicit conversion — also fine, just doesn't get the zero-alloc benefit. + +For methods with two text parameters (`SendLocalizedMessageTo` with affix, `SendLocalizedMessage` with append), only `args` has a handler overload — `affix` stays `ROS` because it's typically a short literal. + +**Critical caveat:** call-site shapes like ternaries, switch expressions, pre-built locals, and `.ToString()` inside the hole silently defeat the handler overload selection. See the [Interpolation Anti-Patterns](string-handling.md#interpolation-anti-patterns) section in the string-handling doc for the full list and the fixes. + +### Lowercase format specifier + +`RawInterpolatedStringHandler` recognizes `:L` to lowercase a value's output (using `MemoryExtensions.ToLowerInvariant`). Useful for enum names in player-facing text: + +```csharp +mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" not "Gold" +``` + +See [`dev-docs/string-handling.md`](string-handling.md#rawinterpolatedstringhandler) for full coverage. + +### Implementation notes + +- `Mobile.PublicOverheadMessage` and `Item.PublicOverheadMessage` route through generic `OutgoingMessagePackets.BroadcastMessage*` helpers (in `OutgoingMessagePackets.Broadcast.cs`) parameterized over a `private readonly struct` filter that encapsulates the per-method visibility predicate (`CanSee`, `InLOS`, `AccessLevel`, `!= self`). The `where TFilter : struct, IBroadcastFilter` constraint specializes per filter type and keeps the dispatch zero-allocation (no boxing, no virtual call — JIT inlines the predicate). +- The convenience methods themselves live in `Mobile.Messages.cs` and `Item.Messages.cs` partial files for organization. + +--- + ## Common Existing Send Methods ### Effects and Sounds diff --git a/dev-docs/property-lists.md b/dev-docs/property-lists.md index a5c543fd9..da0ed40e8 100644 --- a/dev-docs/property-lists.md +++ b/dev-docs/property-lists.md @@ -95,6 +95,22 @@ The compiler generates different calls for each: **Rule of thumb**: The only text that should appear as bare literals in the interpolated string is `\t` (the argument separator). Everything else — including string constants — must be inside `{}` holes. +### No `.ToString()` Inside Holes + +`IPropertyList`'s interpolated string handler formats values directly into a pooled buffer via `ISpanFormattable.TryFormat` — no intermediate `string` allocation per hole. An explicit `.ToString()` defeats this: + +```csharp +// BAD — .ToString() allocates a string, then the handler copies its chars +list.Add(1060658, $"{"Charges"}\t{_charges.ToString()}"); + +// GOOD — the handler formats _charges directly with no intermediate string +list.Add(1060658, $"{"Charges"}\t{_charges}"); +``` + +Same applies to `.String()` on `TextDefinition`, `.GetValue()`, and any method that returns a freshly allocated `string` — drop the call and let the handler format the underlying value directly. + +The full list of interpolation anti-patterns (ternaries, switch expressions, pre-built locals, `string.Format`, concat, LINQ in holes) applies equally to `IPropertyList.Add($"...")`. See [`dev-docs/string-handling.md`](string-handling.md#interpolation-anti-patterns) for the full reference. + ### Cliloc as Argument (Use `:#` Format Specifier) When a cliloc argument is itself another cliloc number (i.e., the argument should resolve to localized text), use the `:#` format specifier on the integer — **not** a `"#number"` string: diff --git a/dev-docs/string-handling.md b/dev-docs/string-handling.md index 930d8820c..6c915c6c9 100644 --- a/dev-docs/string-handling.md +++ b/dev-docs/string-handling.md @@ -5,9 +5,10 @@ This document covers the string building utilities in `Projects/Server/Text/` an ## Table of Contents 1. [ValueStringBuilder](#valuestringbuilder) 2. [RawInterpolatedStringHandler](#rawinterpolatedstringhandler) -3. [StringHelpers](#stringhelpers) -4. [TextEncoding](#textencoding) -5. [Decision Guide](#decision-guide) +3. [Interpolation Anti-Patterns](#interpolation-anti-patterns) +4. [StringHelpers](#stringhelpers) +5. [TextEncoding](#textencoding) +6. [Decision Guide](#decision-guide) --- @@ -122,7 +123,188 @@ For stackalloc-only builders that never grow, `Dispose()` is a no-op. But always **Location**: `Projects/Server/Buffers/RawInterpolatedStringHandler.cs` **Namespace**: `Server.Buffers` -A `[InterpolatedStringHandler]` ref struct used internally by `ValueStringBuilder`'s interpolation support. You should not need to use this directly — use `sb.Append($"...")` instead. +A `[InterpolatedStringHandler]` ref struct that renders an interpolated string directly into a `char[]` rented from `STArrayPool.Shared`. Used as a parameter type to make zero-allocation interpolation overloads possible — when the caller writes `$"..."`, the compiler synthesizes the handler, fills it with the formatted chars, and the receiving method passes `handler.Text` to its underlying span path. + +**You normally don't construct this directly**: it's used as a parameter type. Most ModernUO APIs that accept formatted text already provide a `ref RawInterpolatedStringHandler` overload alongside the `string` / `ReadOnlySpan` overload. The compiler picks the handler overload automatically when the argument is a `$"..."` literal. + +### APIs that accept `ref RawInterpolatedStringHandler` + +- `SpanWriter.WriteAscii`, `WriteLatin1`, `Write(Encoding, …)` — packet building +- `Mobile.SendMessage`, `SendLocalizedMessage`, `SendAsciiMessage`, `Public/Local/Nonlocal/PrivateOverheadMessage`, `Say`, `Emote`, `Whisper`, `Yell` — player-facing chat +- `Item.PublicOverheadMessage`, `SendLocalizedMessageTo`, `SendMessageTo` — item-attributed messages +- `OutgoingMessagePackets.SendMessageLocalized`, `SendMessageLocalizedAffix`, `SendMessage` — direct NetState extensions +- `Html.Center`, `Html.Color`, `Html.Right` — gump HTML helpers + +When in doubt, just write `$"..."` at the call site — if a handler overload exists, the compiler picks it. + +### `:L` Lowercase Format Specifier + +`RawInterpolatedStringHandler` recognizes `:L` as a custom format specifier that lowercases the value's output in-place using `char.ToLowerInvariant`. Handles surrogate pairs correctly via the BCL's vectorized `MemoryExtensions.ToLowerInvariant`. + +```csharp +mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" instead of "Gold" +mob.SendMessage($"Welcome, {playerName:L}"); // lowercased name +mob.SendMessage($"{count:L} kills"); // ints unchanged ("42") +``` + +This eliminates the `value.ToString().ToLowerInvariant()` two-allocation idiom. Works for any type that goes through the handler (enums, strings, anything `ISpanFormattable`). + +The format string is case-sensitive — `:l` is not recognized. Match the convention of standard format specifiers (`:N0`, `:F2`, etc.) and use uppercase `:L`. + +### Pooled buffer lifecycle + +`RawInterpolatedStringHandler` rents a `char[]` from `STArrayPool.Shared` on construction (sized by the literal length + an estimate of formatted chars per hole). Methods that take `ref RawInterpolatedStringHandler` are responsible for calling `handler.Clear()` after consuming `handler.Text`, which returns the buffer to the pool. The rent is single-threaded and lock-free (~tens of nanoseconds), so the cost is negligible compared to the `string` allocation it replaces. + +--- + +## Interpolation Anti-Patterns + +ModernUO has many APIs with `ref RawInterpolatedStringHandler` overloads (messages, gumps, packets, OPL — see the list above). The handler overload is **only selected when the call-site argument is a `$"..."` literal directly in position**. Several patterns silently defeat handler binding and fall back to a `string`-allocating path. Each pattern below has a "before" and "after" — apply the "after" form when writing or reviewing code that interpolates into any handler-aware API. + +### 1. Ternary with interpolated branches + +```csharp +// BAD — the ternary unifies branches as `string`; handler overload not selected +mob.SendMessage(cond ? $"a {x}" : $"b {y}"); +``` +```csharp +// GOOD — each branch is a separate call, each binds to the handler overload +if (cond) +{ + mob.SendMessage($"a {x}"); +} +else +{ + mob.SendMessage($"b {y}"); +} +``` + +`RawInterpolatedStringHandler` is a `ref struct` and cannot appear in a conditional expression result type — the C# compiler unifies the ternary branches to `string`, and the call binds to the `string` / `ROS` overload, allocating the message text per call. + +### 2. Switch expression with interpolated arms + +```csharp +// BAD — switch expression branches unify as `string` +mob.SendMessage(thing switch +{ + 1 => $"a {x}", + _ => $"b" +}); +``` +```csharp +// GOOD — switch statement, each arm calls the handler-aware API directly +switch (thing) +{ + case 1: + mob.SendMessage($"a {x}"); + break; + default: + mob.SendMessage($"b"); + break; +} +``` + +Same root cause as the ternary case. + +### 3. Pre-built local typed as `string` + +```csharp +// BAD — `msg` is a `string`; ROS overload picked, not the handler +var msg = $"foo {x}"; +mob.SendMessage(msg); +``` +```csharp +// GOOD — inline at the call site so the compiler sees the literal +mob.SendMessage($"foo {x}"); +``` + +If the local is reused (multiple calls, multiple branches), keep the local — pre-building avoids re-interpolating per send. Inline only when the local is single-use. + +### 4. `.ToString()` (or any string-returning method) inside a hole + +```csharp +// BAD — .ToString() allocates a string before the handler copies the chars +mob.SendMessage($"You are now {accessLevel.ToString()}."); +mob.SendMessage($"Your guild is {td.String()}."); +``` +```csharp +// GOOD — drop the .ToString() and let the handler format the value directly +mob.SendMessage($"You are now {accessLevel}."); +mob.SendMessage($"Your guild is {td}."); +``` + +The handler's `AppendFormatted` calls `ISpanFormattable.TryFormat` on the value directly, with zero intermediate `string`. An explicit `.ToString()` defeats this. The same applies to `.String()` (TextDefinition), `.GetValue()`, `.AsHexString()`, and any other method that returns a freshly allocated `string`. + +For values that don't implement `ISpanFormattable`, the handler falls back to `value.ToString()` internally — same allocation as the explicit call, but at least the call site is consistent. + +For lowercase output, use the `:L` format specifier instead of `.ToString().ToLowerInvariant()` (see [RawInterpolatedStringHandler](#rawinterpolatedstringhandler)). + +### 5. String concatenation inside a hole + +```csharp +// BAD — `+` on strings allocates an intermediate string +mob.SendMessage($"Total: {a + b}"); +mob.SendMessage($"Title: {string.Concat(prefix, name)}"); +``` +```csharp +// GOOD — multiple holes, each formatted directly into the buffer +mob.SendMessage($"Total: {a}{b}"); +mob.SendMessage($"Title: {prefix}{name}"); +``` + +Note: `int + int` inside a hole is arithmetic, not concatenation — that's fine. The anti-pattern is `string + string` or `string + value`. + +### 6. `string.Format` feeding a handler-aware API + +```csharp +// BAD — string.Format allocates a string the handler then re-buffers +mob.SendMessage(string.Format("You earned {0:N0} gold", amount)); +``` +```csharp +// GOOD — the handler formats `amount` directly into its buffer +mob.SendMessage($"You earned {amount:N0} gold"); +``` + +### 7. LINQ-built strings inside a hole + +```csharp +// BAD — Select/Aggregate/Join on strings allocates a chain of intermediates +mob.SendMessage($"Allies: {names.Aggregate((a, b) => $"{a}, {b}")}"); +``` +```csharp +// GOOD — build via ValueStringBuilder, pass the span +using var sb = new ValueStringBuilder(stackalloc char[256]); +sb.Append("Allies: "); +for (var i = 0; i < names.Count; i++) +{ + if (i > 0) + { + sb.Append(", "); + } + sb.Append(names[i]); +} +mob.SendMessage(sb.AsSpan()); +``` + +For unbounded inputs, use `ValueStringBuilder.Create()` and call `mob.SendMessage(sb.ToString())` if the consumer needs a `string` (one allocation, vs LINQ's many). + +### 8. Pre-built concat var + +```csharp +// BAD — concatenation allocates, then the local picks the ROS overload +var s = obj.Name + " says hi"; +mob.SendMessage(s); +``` +```csharp +// GOOD — interpolation literal at the call site +mob.SendMessage($"{obj.Name} says hi"); +``` + +### Why these matter + +These patterns aren't bugs — they produce correct output. But they each leak a `string` per call, and message/gump/OPL APIs are called constantly during gameplay. The handler overload exists specifically to eliminate that allocation, but only when the call-site argument is a direct `$"..."` literal. + +When in doubt, ask: "is the handler overload selected here?" — and if the argument is anything other than a top-level `$"..."` literal in the parameter slot, the answer is no. ---