## 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.
`ObjectPropertyList.AppendFormatted<T>(value, format)` treated **any** `{value:#}` as the cliloc marker (emitting `#<value>`). But cliloc numbers are integers — a `float`/`double`/`decimal` formatted with `#` is the standard custom-numeric (`#` = digit placeholder) format, not a cliloc reference, so those were being mis-marked.
Gate the marker on an integer value type:
```csharp
if (format == "#" && value is int or uint or long or ulong or short or ushort or byte or sbyte)
```
Now `{someFloat:#}` formats normally (passes `#` through to `TryFormat`); the marker/standard-format ambiguity narrows to the harmless `{0:#}` **integer** case (`#0`). Existing `AddLocalized(int)` / `{value:#}` (all `int`) are unaffected.
Adds `ObjectPropertyListSpanAddTests.HashFormat_OnlyMarksIntegers`: `int {value:#}` → `#<value>`; `double {value:#}` → `42.0.ToString("#")` (`"42"`, no `#`).
## At a glance
```csharp
public override void GetProperties(IPropertyList list)
{
base.GetProperties(list);
// Accumulate any number of free-text lines. On dispose the block flushes via
// AddChunked, splitting across as many OPL properties as needed so none can
// overflow the legacy 2D client's per-property buffer (which would crash it).
using var block = list.TextBlock();
if (luck > 0)
{
block.Add($"Luck Bonus: +{luck}%"); // zero-alloc interpolation
}
block.Add("Cannot be repaired".AsSpan()); // plain text, no string allocation
// Already holding a '\n'-joined string? Skip the builder and chunk directly:
// list.AddChunked(description);
}
```
## What
Adds a safe path for emitting **variable-length, free-form (non-cliloc) tooltip text**:
- **`ObjectPropertyList.Add(ReadOnlySpan<char>)`** overloads — append raw text with no string allocation. Makes the old single-arg `Add(string)` redundant (a `string` binds to the span overload implicitly), so it's dropped.
- **`AddChunked(ReadOnlySpan<char>)`** on `IPropertyList` — splits newline-joined text at `\n` boundaries across as many passthrough-cliloc properties as needed, so no single property exceeds the cap.
- **`OplTextBlock`** (`ref struct`) + **`IPropertyList.TextBlock()`** — an ergonomic builder that accumulates `\n`-joined lines (with a zero-alloc interpolated `Add($"...")` overload) and flushes via `AddChunked` on dispose. Usage: `using var block = list.TextBlock();`.
- **`MaxArgumentLength` (504)** — per-property cap with a hard backstop that clamps + logs anything that slips through.
## Why
The legacy 2D client copies each OPL property's text into a fixed ~512-char (1024-byte) buffer. A single property longer than that smashes an adjacent world object's vtable on the client heap and crashes the client. `AddChunked`/`OplTextBlock` keep multi-line content safely under the cap instead of risking one oversized `Add`.
## Docs
- `dev-docs/property-lists.md` — new "Multi-Line Free Text" deep-dive section; corrected the stale `IPropertyList` listing.
- `dev-docs/claude-skills/modernuo-property-lists.md` — condensed pattern + anti-pattern.
## Tests
9 tests pass (`OplTextBlockTests`, `ObjectPropertyListSpanAddTests`): line joining, empty-line skipping, no-line no-op, zero-alloc interpolation, and long-content chunking staying under `MaxArgumentLength`. Full `UOContent` build is green, confirming dropping `Add(string)` breaks no call sites.
- Fixes double return issue with object property list that is causing corruption.
- Adds DEBUG_ARRAYPOOL define constant which will crash on double return or invalid return scenarios.
> [!IMPORTANT]
> **Developer Notes**
> STArrayPool rented arrays **MUST NOT** be returned **ONLY ONCE** otherwise there will be corruption from double-use.
> Use `DEBUG_ARRAYPOOL` to test potential broken STArrayPool use cases.
> [!NOTE]
> **Why can't I enable the debug all the time?**
> Other than the fact that it will crash due to bad code, the actual tracking system is highly detrimental/problematic for performance and memory consumption by creating objects that have a stack trace.
## Changes
- [X] Adds OPL convenience methods
- `opl.Add(cliloc, value)` and `opl.Add(value)` - value as an integer or string works just like `opl.Add(cliloc, $"{value}")`
- `opl.AddLocalized(cliloc, clilocValue)` - works the same as `opl.Add(cliloc, $"#{clilocValue}");`
- [X] Simplifies basic `list.Add()` situations
- [X] Changes cliloc as an argument so it works with custom IPropertyList implementations (HTML)
- [X] Fixes plants so they support the old localization and new (changed in 7.0.12.0+)
- [X] Exposes more methods to override for Item to make creating custom OPL possible.
## Important Notes
* Using a ternary as an argument, like this `opl.Add(number, showType ? $"{type}\t{value}" : $"{value}");` _will not use the correct string interpolation_. This means if you use a custom PropertyList (for HTML or some other purpose), the property list won't be localized properly.
* All localization values must be interpolated, even if they are literal strings, or integers. Example: `opl.Add(number, $"{"Charges"}\t{m_Charges}");` is correct. Using the following: `$"Charges\t{m_Charges}"` will not work for custom PropertyList implementations!
## Breaking Changes (New API)
ObjectPropertyList supports the following API:
```cs
list.Add(500000);
list.Add(500001, stringArgument);
list.Add("Some text");
list.Add($"Some text with {argument}");
list.Add(500002, $"{arg1}\t{arg2}");
```
## Notes
1. All API uses that require a formatter like this:
```cs
list.Add(500002, "{0}\t{1}", arg1, arg2);
```
Should be changed to use string interpolation, for example:
```cs
list.Add(500002, $"{arg1}\t{arg2}");
```
2. The following paradigm should no longer be used:
```cs
list.Add(1061170, prop.ToString()); // strength requirement ~1_val~
```
The new string interpolation API will avoid having to convert the argument to a string before writing it to the packet. Instead use the following:
```cs
list.Add(1061170, $"{prop}"); // strength requirement ~1_val~
```
### Benchmarks
```cs
| Method | Mean | Error | StdDev | Gen 0 | Allocated |
|------------------------------- |---------:|--------:|--------:|-------:|----------:|
| BenchmarkOldOPL | 241.0 ns | 0.56 ns | 0.47 ns | 0.0105 | 88 B |
| BenchmarkStringInterpolatedOPL | 199.9 ns | 2.44 ns | 2.39 ns | - | - |
```
### Changes
- [X] Removes crash in STArray.Return when array is null.
- [X] Fixes NPE in OPL when entity is null. Serial in packet will be 0 when entity is null.
- [X] Fixes NPE in AosAttributes when Parent is null.
- [X] Changes OPL to use string interpolation.
- [X] Introduces `IPropertyList` to allow extending PropertyList for other uses.
