b8d3fec59a
## The bug
Any property getter reached from `GetProperties` that calls `InvalidateProperties` takes the tooltip build down with it:
```
System.ArgumentNullException: Value cannot be null. (Parameter 'array')
at Server.ObjectPropertyList.AppendStringDirect(String value)
at Server.Mobiles.PlayerMobile.GetProperties(IPropertyList list)
```
`InvalidateProperties` rebuilds **in place** — `Reset()`, then `GetProperties()` again on the same instance — and `Reset()` does two destructive things to a build already in flight:
1. **It returns the pooled interpolation buffer.** The compiler rents it in the handler ctor and returns it in the closing `Add`, so *every hole is evaluated while it is live*:
```csharp
var handler = new InterpolatedStringHandler(1, 2, list); // InitializeInterpolation() RENTS
handler.AppendFormatted(pl.Rank.Title); // <-- getter runs HERE
handler.AppendLiteral("\t");
handler.AppendFormatted(faction.Definition.PropName);
list.Add(1060776, ref handler); // consumes span, RETURNS
```
```
GetProperties(list)
├─ InitializeInterpolation() -> _arrayToReturnToPool = Rent(256) buffer LIVE
├─ « hole 1: pl.Rank.Title »
│ └─ PlayerState.Rank.get (lazy recompute)
│ └─ Invalidate() -> InvalidateProperties() -> m_PropertyList.Reset()
│ └─ Dispose(): Return(buf); _arrayToReturnToPool = null buffer GONE
└─ handler.AppendFormatted("Knight")
└─ _arrayToReturnToPool.AsSpan(_pos..)
└─ ArgumentNullException (Parameter 'array')
```
It surfaces as `ArgumentNullException` rather than `NullReferenceException` because the `Range` overload of `AsSpan` must read `array.Length`, so the BCL null-checks and names the parameter `array`.
2. **It rewinds the packet cursor**, so properties already written are overwritten by the nested pass — a silently corrupted tooltip even where the buffer survives.
## The fix: refuse, don't recover
There is no correct recovery, and retrying the build would only hide the defect. A nested invalidation now logs an error with a stack trace, **throws in `DEBUG`** so it gets found and fixed, and in `RELEASE` returns without touching the list — a possibly stale tooltip, but no crash, no corrupted packet, and nothing leaked back to the pool. Getters that genuinely must invalidate should defer:
```csharp
Timer.DelayCall(InvalidateProperties);
```
The guard flag lives on the `ObjectPropertyList`, not the entity: it is that list's own lifecycle, it costs nothing (both `Item` and `ObjectPropertyList` absorb it in existing padding, and the list is allocated lazily), and it stays correct when builds for different entities nest.
Base instance sizes are unchanged from `main`: Item 128 B, Mobile 792 B, ObjectPropertyList 72 B, PlayerMobile 1216 B.
`PropertyList` also publishes the list into `m_PropertyList` **before** building it rather than assigning through `??=` afterwards, so a nested `InvalidateProperties` sees the build in progress instead of recursing into a second throwaway list whose work is discarded.
`ObjectPropertyList` re-rents its scratch buffer instead of spanning a null array, so a stray `Reset()` from any other caller degrades rather than aborting `GetProperties`.
## Factions `PlayerState`: maintained, not lazily computed
The getter that surfaced this is now a plain field read — the whole `if (m_InvalidateRank)` block and the flag itself are gone:
```csharp
public RankDefinition Rank => m_Rank;
```
`UpdateRank()` recomputes at each point an input actually changes:
| Site | Why |
|---|---|
| `RankIndex` setter | this player's index changed |
| end of `KillPoints` setter | two paths write `m_RankIndex` directly, bypassing the setter; runs once the swap bookkeeping and `ZeroRankOffset` have settled |
| `Faction.AddMember` | *after* the insert — the member count is not settled during the ctor |
| `FactionState` load | once ordering and `ZeroRankOffset` are final |
Supporting fixes this forced out:
- **Both ctors seed the lowest rank.** Nothing recomputes on read any more, so `Rank` has to be usable immediately — including for members that never get a `RankIndex` assigned, which is *every member with no kill points*. Without this, `Rank.Title` NREs.
- **`Rank` always resolves.** Ranks are ordered by `Required` descending ending at `0`, so a *negative* percent (`RankIndex` out of sync with `ZeroRankOffset`) matched nothing and left `m_Rank` null. It no longer divides by a zero `ZeroRankOffset` either.
- **A pre-existing staleness bug.** The `KillPoints` setter writes `m_RankIndex` directly in two places, so the cached rank was never refreshed when a player crossed zero kill points.
All six readers of `Rank` were checked; none relied on the old side effect.
One behaviour change worth flagging: rank refreshes are now **eager** where they used to be lazy, so a `KillPoints` change invalidates each swapped player as it happens. The swap loops break as soon as ordering is satisfied — typically 0–2 swaps — but it is on the path that runs on every faction kill.
## Documentation
The rule is written down so it is enforceable rather than folklore:
- **CLAUDE.md** audit rule 19
- **`dev-docs/property-lists.md`** — new "Never Invalidate From Inside `GetProperties`" section with the failing/passing pattern
- **`dev-docs/claude-skills/modernuo-property-lists.md`** — key rule + anti-pattern
- **`dev-docs/claude-skills/modernuo-code-audit.md`** — rule 19, ERROR severity
## Tests
- `ObjectPropertyListReentrancyTests` — `Reset()` and `Dispose()` re-entered mid-hole (both red against `main` with the exact exception above), nesting behaviour, and the new contract: `DEBUG` throws, `RELEASE` survives, and the build is never retried into a loop.
- `FactionRankTests` — `Rank` is populated before anything reads it, tracks `RankIndex` without a read, is stable across reads, and still resolves when `RankIndex` is out of sync with `ZeroRankOffset`. Red-verified: removing the ctor seed fails the first one.
793/793 `Server.Tests` and 608/608 `UOContent.Tests` pass.
## Noted, not addressed here
`~ObjectPropertyList()` returns the rented array to `STArrayPool<char>.Shared` from the **finalizer thread**, and that pool is single-threaded by design. Left alone as a separate concern.
13 KiB
| name | description |
|---|---|
| modernuo-code-audit | Auto-trigger whenever writing or modifying .cs files under Projects/. Audits code for ModernUO convention violations. Warnings only - flag issues and ask before fixing. |
ModernUO Code Audit
When This Activates
- Any time you write, edit, or modify a
.csfile underProjects/ - After generating code snippets for the user
- During code review
Audit Rules (Warnings Only)
Flag these issues but do NOT auto-fix. Ask the user before making changes.
1. LINQ: Know What's Optimized (.NET 10)
Not all LINQ is banned. .NET 10 JIT/PGO eliminates overhead for specific patterns. Anything not listed below is still forbidden on hot paths.
Tier 1 — Zero-cost (use freely on hot paths):
foreachoverIEnumerable<T>backed byT[],List<T>,Stack<T>,Queue<T>— PGO devirtualizes the enumerator, zero heap allocation.Contains()after a preceding LINQ operator (.Distinct(),.OrderBy(),.Reverse(),.Union(),.Intersect(),.Except(),.Concat(),.SelectMany(),.Where().Select(),.Skip(),.Take(),.OfType(),.Cast(),.Shuffle()) — LINQ has ~30 specialized overrides that skip the intermediate work (no sort, no HashSet, no buffering).Count()on sized collections (ICollection<T>, or afterRange/Repeat/Skip/Take/Append) — O(1) property access, no enumeration.OrderBy().First()/.OrderByDescending().First()/.OrderBy().Last()— O(N) min/max scan, no sort performed.Shuffle().Take(n)— reservoir sampling, single pass, O(n) memoryEnumerable.Range()/Enumerable.Sequence()followed by.Count(),.Contains(),.ToArray(),.ToList(),.ElementAt(),.Last()— arithmetic, not enumeration
Tier 2 — Low overhead (acceptable on warm paths, benchmark if critical):
.Skip(n).Take(m).ToArray()onT[]/List<T>— vectorizedSpan<T>.CopyTo(still allocates output).LeftJoin()/.RightJoin()— ~2x faster than manualGroupJoin+SelectMany+DefaultIfEmpty.Where(predicate)onT[]/List<T>—WhereIteratorstill heap-allocates, but enumeration is PGO-optimized. Manualforeach+ifis still faster for true hot paths.
Tier 3 — Still forbidden on hot paths (write manual code):
.Select(f).Where(p)(this order — each intermediate iterator allocates).GroupBy(),.ToDictionary(),.ToHashSet(),.ToLookup()(always allocate internal structures).Aggregate()(delegate overhead per element).Sum()/.Min()/.Max()onfloat/double(no SIMD in LINQ on ARM).SelectMany()when iterating results (not.Contains()) — multiple enumerator allocations.Zip()when iterating — enumerator allocations- Any LINQ over
IAsyncEnumerable<T>— no PGO/escape analysis - Long chains like
.Where().Select().OrderBy().Take()— each step allocates an iterator
Prerequisites: .NET 10, tiered compilation + Dynamic PGO enabled (default). Tier 1 optimizations require ~30+ calls for JIT warmup.
Quick decision: If the exact pattern is in Tier 1 → use it. If it's in Tier 2 → acceptable unless profiling shows it's a bottleneck. If it's anything else → manual for/foreach + PooledRefList<T>.
2. No Console.WriteLine
Bad: Console.WriteLine(...), Console.Write(...)
Good: private static readonly ILogger logger = LogFactory.GetLogger(typeof(MyClass)); then logger.Information(...), logger.Warning(...), logger.Error(...)
Requires: using Server.Logging;
3. No Concurrency Primitives in Game Code
Bad: ConcurrentDictionary, ConcurrentQueue, ConcurrentBag, volatile, lock(...), Mutex, Semaphore, Monitor, Interlocked, ReaderWriterLock
Why: Server is single-threaded. These add overhead for no benefit.
Instead: Use regular Dictionary<K,V>, List<T>, plain fields.
4. Never Iterate World.Mobiles or World.Items Directly
Bad: foreach (var m in World.Mobiles.Values), World.Items.Values.Where(...)
Good: map.GetMobilesInBounds<T>(bounds), map.GetMobilesInRange<T>(point, range), map.GetItemsInRange<T>(point, range)
Why: Full world iteration is O(n) over all entities. Spatial queries use sector indexing.
5. Clean Up References in OnDelete/OnAfterDelete
Check: Classes with Item or Mobile references should clean them in OnDelete() or OnAfterDelete().
Pattern:
public override void OnAfterDelete()
{
_someReference = null;
base.OnAfterDelete();
}
6. Cancel Timers in OnDelete/OnAfterDelete
Check: Any class with TimerExecutionToken or Timer fields must cancel them on deletion.
Pattern:
public override void OnAfterDelete()
{
_timerToken.Cancel(); // For TimerExecutionToken
_timer?.Stop(); // For Timer references
_timer = null;
base.OnAfterDelete();
}
7. Use STArrayPool, Not ArrayPool
Bad: ArrayPool<T>.Shared.Rent(...) in game logic
Good: STArrayPool<T>.Shared.Rent(...) in game logic
Why: STArrayPool is single-threaded optimized (no locks). Use ArrayPool only in explicitly multi-threaded code.
Also: Always return rented arrays in a finally block.
8. No new List in Hot Paths
Bad: var list = new List<Mobile>(); in frequently-called methods
Good: using var list = PooledRefList<Mobile>.Create();
Why: PooledRefList uses pooled arrays, zero GC pressure. It's a ref struct (stack-allocated).
9. Serialization Class Requirements
Check: Classes with [SerializationGenerator] MUST be partial.
Check: [Constructible] on parameterless constructors for items/mobiles.
Check: TimerExecutionToken fields must NOT have [SerializableField].
Check: Use using ModernUO.Serialization; when using serialization attributes.
10. No Task.Run or new Thread
Bad: Task.Run(...), new Thread(...), ThreadPool.QueueUserWorkItem(...) in game code
Why: Game logic runs on the single-threaded event loop. Background threads cause race conditions.
Exception: Server infrastructure code (Projects/Server/Main.cs, World saves) may use threading.
11. Never Assume Era
Check: If code uses era-conditional logic (Core.AOS, Core.SE, etc.) and the user hasn't specified a target era, ASK which expansion to target.
Why: Different eras have dramatically different mechanics.
12. Naming Conventions
Check: _camelCase for private fields, PascalCase for properties/methods/classes.
Note: Legacy code may use m_ prefix -- don't flag existing m_ fields but use _ for new code.
13. No Empty Gumps
Check: Any gump (legacy Gump constructor, or BuildLayout) must not have a code path that produces zero visual elements (no AddBackground, no AddPage with content, etc.).
Why: The client has no way to close an empty gump — no close button, no right-click dismiss. This leaks a gump slot on both client and server until relog.
Common cause: Early return in a constructor or BuildLayout when prerequisites aren't met.
Fix: Use a static DisplayTo(Mobile from) method that validates prerequisites before constructing the gump. Make the constructor private. See Projects/UOContent/Gumps/Go/GoGump.cs for the canonical pattern.
14. PropertyList String Literals Must Be Holes
Check: In any IPropertyList.Add() interpolated string, string constants must be wrapped as holes {"text"}, not bare literals.
Bad: list.Add(1060658, $"Chances\t{_charges}"); — "Chances" becomes a delimiter, not an argument.
Good: list.Add(1060658, $"{"Chances"}\t{_charges}"); — "Chances" is an argument.
Why: The handler treats bare text as delimiters and {} contents as arguments. The property list system is used beyond the game client (e.g., web rendering) which must distinguish arguments from delimiters. Only \t should be a bare literal.
Also: If you don't know the text for a cliloc number, see Projects/Server/Localization/Localization.cs LoadClilocs() to learn the binary format, and ask the user where their cliloc.enu file is.
15. Braces Required on All Control Flow
Check: ALL if, else, for, foreach, while, do, switch statements must have braces, even for single-line bodies.
Bad:
if (condition)
DoSomething();
Good:
if (condition)
{
DoSomething();
}
Why: Reduces merge conflicts and diff sizes.
16. Prefer Switch Expressions and Switch-When Patterns
Check: Where a chain of if/else if maps inputs to outputs, prefer a switch expression. Where pattern matching with guards improves clarity, prefer switch-when.
Bad:
if (type == GemType.StarSapphire) return "star sapphire";
else if (type == GemType.Emerald) return "emerald";
else return "gem";
Good:
return type switch
{
GemType.StarSapphire => "star sapphire",
GemType.Emerald => "emerald",
_ => "gem"
};
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():
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.
19. No InvalidateProperties From Inside GetProperties
Check: Any property read by a GetProperties override — including through helpers — must be a pure read. Flag getters that call InvalidateProperties() (or a wrapper like Invalidate()) as a side effect.
Bad: a Rank getter that lazily recomputes and then calls Invalidate(); reading it from GetProperties re-enters the build.
Good: invalidate in the setter that actually changes the value, or defer with Timer.DelayCall(InvalidateProperties).
Why: InvalidateProperties() rebuilds the list in place (Reset() + rebuild). Reset() returns the pooled interpolation buffer — which the compiler rents for the whole $"..." expression, so every hole is evaluated while it is live — and rewinds the packet cursor. Re-entering mid-build throws ArgumentNullException (parameter "array") out of GetProperties from a line unrelated to the offending getter, or silently corrupts the tooltip. The engine refuses and logs an error, and DEBUG throws, so this shows up as a crash in development.
Note: Lazy recomputation inside a getter is fine. It is the notification that must not happen there.
See: dev-docs/property-lists.md § "Never Invalidate From Inside GetProperties".
Severity Levels
- ERROR: Rules 3, 9, 10, 13, 19 (will cause bugs, build failures, or client-side leaks)
- 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)
How to Report
When you find violations, report them as:
[AUDIT] {SEVERITY}: {Description}
File: {path}:{line}
Suggestion: {fix}
Do NOT silently fix issues. Always flag and ask.
See Also
dev-docs/code-standards.md- Full coding standards documentationdev-docs/claude-skills/modernuo-serialization.md- Serialization rulesdev-docs/claude-skills/modernuo-timers.md- Timer cleanup rulesdev-docs/claude-skills/modernuo-threading.md- Threading model detailsdev-docs/claude-skills/modernuo-property-lists.md- PropertyList interpolation rules