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.
224 lines
13 KiB
Markdown
224 lines
13 KiB
Markdown
---
|
|
name: modernuo-code-audit
|
|
description: >
|
|
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 `.cs` file under `Projects/`
|
|
- 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):**
|
|
- `foreach` over `IEnumerable<T>` backed by `T[]`, `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 after `Range`/`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) memory
|
|
- `Enumerable.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()` on `T[]`/`List<T>` — vectorized `Span<T>.CopyTo` (still allocates output)
|
|
- `.LeftJoin()` / `.RightJoin()` — ~2x faster than manual `GroupJoin`+`SelectMany`+`DefaultIfEmpty`
|
|
- `.Where(predicate)` on `T[]`/`List<T>` — `WhereIterator` still heap-allocates, but enumeration is PGO-optimized. Manual `foreach`+`if` is 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()` on `float`/`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**:
|
|
```csharp
|
|
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**:
|
|
```csharp
|
|
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**:
|
|
```csharp
|
|
if (condition)
|
|
DoSomething();
|
|
```
|
|
**Good**:
|
|
```csharp
|
|
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**:
|
|
```csharp
|
|
if (type == GemType.StarSapphire) return "star sapphire";
|
|
else if (type == GemType.Emerald) return "emerald";
|
|
else return "gem";
|
|
```
|
|
**Good**:
|
|
```csharp
|
|
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()`:
|
|
```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.
|
|
|
|
### 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 documentation
|
|
- `dev-docs/claude-skills/modernuo-serialization.md` - Serialization rules
|
|
- `dev-docs/claude-skills/modernuo-timers.md` - Timer cleanup rules
|
|
- `dev-docs/claude-skills/modernuo-threading.md` - Threading model details
|
|
- `dev-docs/claude-skills/modernuo-property-lists.md` - PropertyList interpolation rules
|