diff --git a/CLAUDE.md b/CLAUDE.md index 1b52b6816..0c41d2087 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,6 +28,7 @@ Apply these when writing or reviewing `.cs` files under `Projects/`. 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 +19. **No `InvalidateProperties()` from inside `GetProperties`** — every property a `GetProperties` override reads must be a pure read. `InvalidateProperties()` rebuilds the list in place (`Reset()` + rebuild), and `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. A getter that invalidates therefore throws `ArgumentNullException` (parameter `"array"`) out of `GetProperties` from an unrelated-looking line, or silently corrupts the tooltip. The engine refuses and logs an error; `DEBUG` throws. Lazy recomputation in a getter is fine — the *notification* is not. Invalidate in the setter that changes the value, or defer with `Timer.DelayCall(InvalidateProperties)` → `dev-docs/property-lists.md` § Never Invalidate From Inside `GetProperties` ## Dev-Docs Reference diff --git a/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs b/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs new file mode 100644 index 000000000..8da5f748a --- /dev/null +++ b/Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs @@ -0,0 +1,142 @@ +using System; +using Xunit; + +namespace Server.Tests; + +/// +/// The interpolation buffer is rented by the handler ctor and returned by the closing Add, so every +/// hole is evaluated while it is live. A Reset()/Dispose() landing in that window used to leave the +/// next Append* spanning a null array: ArgumentNullException, parameter "array". +/// +public class ObjectPropertyListReentrancyTests +{ + // Stands in for a property getter that invalidates while its own tooltip is being built. + private static string ResettingHole(ObjectPropertyList list, string value) + { + list.Reset(); + return value; + } + + private static string DisposingHole(ObjectPropertyList list, string value) + { + list.Dispose(); + return value; + } + + [Fact] + public void InterpolatedAdd_ResetMidHole_DoesNotThrow() + { + var opl = new ObjectPropertyList(null); + + var ex = Record.Exception( + () => opl.Add(1060776, $"{ResettingHole(opl, "Knight")}\t{"Council of Mages"}") + ); + + Assert.Null(ex); + } + + [Fact] + public void InterpolatedAdd_DisposeMidHole_DoesNotThrow() + { + var opl = new ObjectPropertyList(null); + + var ex = Record.Exception( + () => opl.Add(1060776, $"{DisposingHole(opl, "Knight")}\t{"Council of Mages"}") + ); + + Assert.Null(ex); + } +} + +/// +/// The guard is per-list, so nested builds (a GetProperties override that reads another entity's +/// PropertyList) cannot unguard the outer one the way a single shared slot would. +/// +public class ObjectPropertyListNestedBuildTests +{ + [Fact] + public void NestedBuild_DoesNotUnguardTheOuterList() + { + var outer = new ObjectPropertyList(null); + var inner = new ObjectPropertyList(null); + + outer.IsBuilding = true; + inner.IsBuilding = true; // another entity starts building, and finishes + inner.IsBuilding = false; + + Assert.True(outer.IsBuilding); + } + + [Fact] + public void Reset_MidInterpolation_LeavesTheListUsable() + { + var opl = new ObjectPropertyList(null); + + opl.Add(1060776, $"{Reset(opl, "Knight")}\t{"Council of Mages"}"); + opl.Add(1042971, "still working"); + opl.Terminate(); + + Assert.NotNull(opl.Buffer); + } + + private static string Reset(ObjectPropertyList list, string value) + { + list.Reset(); + return value; + } +} + + +/// +/// Invalidating from inside GetProperties is a defect in the getter, not a case to recover from: +/// DEBUG throws, RELEASE keeps a possibly stale tooltip without crashing or leaking. +/// +[Collection("Sequential Server Tests")] +public class PropertyListInvalidationDuringBuildTests +{ + private class SelfInvalidatingMobile : Mobile + { + public int Builds; + + public override void GetProperties(IPropertyList list) + { + Builds++; + base.GetProperties(list); + InvalidateProperties(); + list.Add(1060776, $"{"Knight"}\t{"Council of Mages"}"); + } + } + + private static SelfInvalidatingMobile Place(int x) + { + var m = new SelfInvalidatingMobile(); + m.MoveToWorld(new Point3D(x, 1000, 0), Map.Felucca); + return m; + } + + [Fact] + public void InvalidatingFromGetProperties_FailsLoudlyWithoutTearingDownTheBuild() + { + var wasEnabled = ObjectPropertyList.Enabled; + ObjectPropertyList.Enabled = true; + + try + { + var m = Place(1000); + +#if DEBUG + Assert.Throws(() => _ = m.PropertyList); +#else + Assert.Null(Record.Exception(() => _ = m.PropertyList)); +#endif + + // Refused, not retried. + Assert.Equal(1, m.Builds); + m.Delete(); + } + finally + { + ObjectPropertyList.Enabled = wasEnabled; + } + } +} diff --git a/Projects/Server/Items/Item.cs b/Projects/Server/Items/Item.cs index 4d5ee13b3..186437400 100644 --- a/Projects/Server/Items/Item.cs +++ b/Projects/Server/Items/Item.cs @@ -14,6 +14,7 @@ *************************************************************************/ using System; +using System.Diagnostics; using System.Collections.Generic; using System.Reflection; using System.Runtime.CompilerServices; @@ -798,7 +799,22 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert public virtual int HuedItemID => m_ItemID; - public ObjectPropertyList PropertyList => m_PropertyList ??= InitializePropertyList(new ObjectPropertyList(this)); + public ObjectPropertyList PropertyList + { + get + { + if (m_PropertyList == null) + { + // Publish the list before building it so a nested InvalidateProperties can see the + // build in progress and defer instead of recursing into a second throwaway list. + var list = new ObjectPropertyList(this); + m_PropertyList = list; + InitializePropertyList(list); + } + + return m_PropertyList; + } + } /// /// Overridable. Fills an with everything applicable. By default, this invokes @@ -2429,9 +2445,19 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert private ObjectPropertyList InitializePropertyList(ObjectPropertyList list) { - GetProperties(list); - AppendChildProperties(list); - list.Terminate(); + list.IsBuilding = true; + + try + { + GetProperties(list); + AppendChildProperties(list); + list.Terminate(); + } + finally + { + list.IsBuilding = false; + } + return list; } @@ -2448,6 +2474,26 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert return; } + // Always a bug in the property getter, and there is no correct recovery: refuse rather than + // hide it. RELEASE keeps a possibly stale tooltip, DEBUG throws. + // See dev-docs/property-lists.md "Never Invalidate From Inside GetProperties". + if (m_PropertyList?.IsBuilding == true) + { + logger.Error( + "{Entity} called InvalidateProperties() while its property list was being built. Remove the side effect from the property getter, or defer it with Timer.DelayCall.\n{StackTrace}", + this, + new StackTrace() + ); + +#if DEBUG + throw new InvalidOperationException( + $"{this} invalidated its property list from inside GetProperties. Remove the side effect from the property getter." + ); +#else + return; +#endif + } + if (m_Map != null && m_Map != Map.Internal && !World.Loading) { int? oldHash; diff --git a/Projects/Server/Mobiles/Mobile.cs b/Projects/Server/Mobiles/Mobile.cs index 34daaf796..f22c9a939 100644 --- a/Projects/Server/Mobiles/Mobile.cs +++ b/Projects/Server/Mobiles/Mobile.cs @@ -26,6 +26,7 @@ using Server.Network; using Server.Prompts; using Server.Targeting; using System; +using System.Diagnostics; using System.Collections.Generic; using System.Runtime.CompilerServices; using Server.Buffers; @@ -2291,7 +2292,22 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro public int CompareTo(Mobile other) => other == null ? -1 : Serial.CompareTo(other.Serial); public virtual int HuedItemID => m_Female ? 0x2107 : 0x2106; - public ObjectPropertyList PropertyList => m_PropertyList ??= InitializePropertyList(new ObjectPropertyList(this)); + public ObjectPropertyList PropertyList + { + get + { + if (m_PropertyList == null) + { + // Publish the list before building it so a nested InvalidateProperties can see the + // build in progress and defer instead of recursing into a second throwaway list. + var list = new ObjectPropertyList(this); + m_PropertyList = list; + InitializePropertyList(list); + } + + return m_PropertyList; + } + } public virtual void GetProperties(IPropertyList list) { @@ -7225,8 +7241,18 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro private ObjectPropertyList InitializePropertyList(ObjectPropertyList list) { - GetProperties(list); - list.Terminate(); + list.IsBuilding = true; + + try + { + GetProperties(list); + list.Terminate(); + } + finally + { + list.IsBuilding = false; + } + return list; } @@ -7243,6 +7269,26 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro return; } + // Always a bug in the property getter, and there is no correct recovery: refuse rather than + // hide it. RELEASE keeps a possibly stale tooltip, DEBUG throws. + // See dev-docs/property-lists.md "Never Invalidate From Inside GetProperties". + if (m_PropertyList?.IsBuilding == true) + { + logger.Error( + "{Entity} called InvalidateProperties() while its property list was being built. Remove the side effect from the property getter, or defer it with Timer.DelayCall.\n{StackTrace}", + this, + new StackTrace() + ); + +#if DEBUG + throw new InvalidOperationException( + $"{this} invalidated its property list from inside GetProperties. Remove the side effect from the property getter." + ); +#else + return; +#endif + } + if (m_Map != null && m_Map != Map.Internal && !World.Loading) { int? oldHash; diff --git a/Projects/Server/PropertyList/ObjectPropertyList.cs b/Projects/Server/PropertyList/ObjectPropertyList.cs index 6404eccbd..58b247990 100644 --- a/Projects/Server/PropertyList/ObjectPropertyList.cs +++ b/Projects/Server/PropertyList/ObjectPropertyList.cs @@ -55,6 +55,12 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable private int _pos; private char[]? _arrayToReturnToPool; + /// + /// True while GetProperties is populating this list. Set by the owning entity so a nested + /// InvalidateProperties can be refused instead of Reset()ing a build already in flight. + /// + internal bool IsBuilding { get; set; } + public ObjectPropertyList(IEntity? e) { Entity = e; @@ -319,8 +325,23 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable private static int GetDefaultLength(int literalLength, int formattedCount) => Math.Max(256, literalLength + formattedCount * 11); + // Reset()/Dispose() return the scratch buffer to the pool. If either lands while a `$"..."` + // handler is still appending, re-rent rather than spanning a null array and throwing out of + // GetProperties. Mobile/Item hold the primary guard; this covers any other caller. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void EnsureInterpolationBuffer() + { + if (_arrayToReturnToPool == null) + { + _arrayToReturnToPool = STArrayPool.Shared.Rent(256); + _pos = 0; + } + } + public void AppendLiteral(string value) { + EnsureInterpolationBuffer(); + if (value.Length == 1) { var chars = _arrayToReturnToPool.AsSpan(); @@ -354,6 +375,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(T value) { + EnsureInterpolationBuffer(); + string? s; if (value is IFormattable) { @@ -384,6 +407,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(T value, string? format) { + EnsureInterpolationBuffer(); + // '#' marks an integer argument as a cliloc ("#"). Integers only -- a float/double/decimal // '#' is the standard numeric format, not a cliloc marker. if (format == "#" && value is int or uint or long or ulong or short or ushort or byte or sbyte) @@ -442,6 +467,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(ReadOnlySpan value) { + EnsureInterpolationBuffer(); + if (value.TryCopyTo(_arrayToReturnToPool.AsSpan(_pos..))) { _pos += value.Length; @@ -454,6 +481,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(ReadOnlySpan value, int alignment = 0, string? format = null) { + EnsureInterpolationBuffer(); + var leftAlign = false; if (alignment < 0) { @@ -488,6 +517,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable public void AppendFormatted(string? value) { + EnsureInterpolationBuffer(); + if (value?.TryCopyTo(_arrayToReturnToPool.AsSpan(_pos..)) == true) { _pos += value.Length; diff --git a/Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs b/Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs new file mode 100644 index 000000000..2416055b1 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs @@ -0,0 +1,100 @@ +using System.Collections.Generic; +using Server; +using Server.Factions; +using Xunit; + +namespace UOContent.Tests; + +/// +/// PlayerState.Rank is read from GetProperties, so it must stay a plain field read. These pin what +/// that requires: the rank is never null, and it is correct without anyone having read it first. +/// +[Collection("Sequential UOContent Tests")] +public class FactionRankTests +{ + // The faction ctor builds its own Definition, so no world state is needed. + private static Faction NewFaction() => new CouncilOfMages(); + + private static PlayerState AddMember(Faction faction, List owner) + { + var state = new PlayerState(new Mobile(), faction, owner); + owner.Add(state); + return state; + } + + [Fact] + public void Rank_IsPopulatedBeforeAnythingReadsIt() + { + var faction = NewFaction(); + var state = new PlayerState(new Mobile(), faction, []); + + // Nothing recomputes on read, so the ctor must leave a usable value or Rank.Title NREs. + Assert.NotNull(state.Rank); + Assert.NotNull(state.Rank.Title); + } + + [Fact] + public void Rank_IsTheLowestRank_ForAnUnrankedMember() + { + var faction = NewFaction(); + var owner = new List(); + var a = AddMember(faction, owner); + var b = AddMember(faction, owner); + + a.UpdateRank(); + b.UpdateRank(); + + var lowest = faction.Definition.Ranks[^1]; + + Assert.Equal(lowest.Rank, a.Rank.Rank); + Assert.Equal(lowest.Rank, b.Rank.Rank); + } + + [Fact] + public void SettingRankIndex_UpdatesRankWithoutAnyoneReadingIt() + { + var faction = NewFaction(); + var owner = new List(); + var top = AddMember(faction, owner); + var bottom = AddMember(faction, owner); + + faction.ZeroRankOffset = 2; + + top.RankIndex = 0; + bottom.RankIndex = 1; + + // No read triggered these, yet the ordering is reflected. + Assert.True(top.Rank.Rank > bottom.Rank.Rank); + } + + [Fact] + public void ReadingRank_IsStableAndSideEffectFree() + { + var faction = NewFaction(); + var owner = new List(); + var state = AddMember(faction, owner); + + faction.ZeroRankOffset = 1; + state.RankIndex = 0; + + var first = state.Rank; + var second = state.Rank; + + Assert.Same(first, second); + } + + [Fact] + public void RankIndexOutOfSyncWithZeroRankOffset_StillResolvesARank() + { + var faction = NewFaction(); + var owner = new List(); + var a = AddMember(faction, owner); + AddMember(faction, owner); + + // A negative percent used to match no rank at all, leaving Rank null. + faction.ZeroRankOffset = 1; + a.RankIndex = 5; + + Assert.NotNull(a.Rank); + } +} diff --git a/Projects/UOContent/Engines/Factions/Core/Faction.cs b/Projects/UOContent/Engines/Factions/Core/Faction.cs index 33166f5a0..6c440584b 100644 --- a/Projects/UOContent/Engines/Factions/Core/Faction.cs +++ b/Projects/UOContent/Engines/Factions/Core/Faction.cs @@ -242,7 +242,11 @@ public abstract class Faction : IComparable, ISpanParsable public virtual void AddMember(Mobile mob) { - Members.Insert(ZeroRankOffset, new PlayerState(mob, this, Members)); + var state = new PlayerState(mob, this, Members); + Members.Insert(ZeroRankOffset, state); + + // Ranked after the insert: the ctor ran while Owner was still short a member. + state.UpdateRank(); mob.AddToBackpack(FactionItem.Imbue(new Robe(), this, false, Definition.HuePrimary)); mob.SendLocalizedMessage(1010374); // You have been granted a robe which signifies your faction diff --git a/Projects/UOContent/Engines/Factions/Core/FactionState.cs b/Projects/UOContent/Engines/Factions/Core/FactionState.cs index 0170c718c..b8eba5091 100644 --- a/Projects/UOContent/Engines/Factions/Core/FactionState.cs +++ b/Projects/UOContent/Engines/Factions/Core/FactionState.cs @@ -114,6 +114,13 @@ public class FactionState } } + // The loop above only assigns RankIndex to members with kill points, and nothing + // computes rank on read, so rank everyone now that the ordering has settled. + foreach (var player in Members) + { + player.UpdateRank(); + } + FactionItems = []; if (version >= 2) diff --git a/Projects/UOContent/Engines/Factions/Core/PlayerState.cs b/Projects/UOContent/Engines/Factions/Core/PlayerState.cs index 792d2b4e3..e420836ce 100644 --- a/Projects/UOContent/Engines/Factions/Core/PlayerState.cs +++ b/Projects/UOContent/Engines/Factions/Core/PlayerState.cs @@ -8,7 +8,6 @@ public class PlayerState : IComparable { private Town m_Finance; - private bool m_InvalidateRank = true; private int m_KillPoints; private MerchantTitle m_MerchantTitle; private RankDefinition m_Rank; @@ -22,6 +21,10 @@ public class PlayerState : IComparable Faction = faction; Owner = owner; + // Owner does not contain this state yet, so the count is short by one; the caller ranks it + // after inserting. + SeedLowestRank(); + Attach(); Invalidate(); } @@ -54,6 +57,9 @@ public class PlayerState : IComparable } } + // Members are still being read; FactionState ranks everyone once the ordering settles. + SeedLowestRank(); + Attach(); } @@ -116,6 +122,8 @@ public class PlayerState : IComparable Owner.Remove(this); Owner.Insert(Faction.ZeroRankOffset, this); + // Direct, not through RankIndex: ZeroRankOffset is mid-update. The + // UpdateRank() at the end of this setter covers it. m_RankIndex = Faction.ZeroRankOffset; Faction.ZeroRankOffset++; } @@ -180,6 +188,7 @@ public class PlayerState : IComparable } m_KillPoints = value; + UpdateRank(); Invalidate(); } } @@ -193,49 +202,72 @@ public class PlayerState : IComparable if (m_RankIndex != value) { m_RankIndex = value; - m_InvalidateRank = true; + + UpdateRank(); + Invalidate(); } } } - public RankDefinition Rank + /// + /// Read from PlayerMobile.GetProperties, so it must stay a plain field read -- recomputing or + /// invalidating here re-enters the property list build. Maintained by . + /// + public RankDefinition Rank => m_Rank; + + // Lowest rank (Required 0): correct for an unranked member, and never null, so Rank.Title + // cannot NRE before the first UpdateRank(). + private void SeedLowestRank() { - get + var ranks = Faction.Definition.Ranks; + + if (ranks.Length > 0) { - if (m_InvalidateRank) + m_Rank = ranks[^1]; + } + } + + /// + /// Recomputes the cached rank. Call whenever , the faction's + /// ZeroRankOffset, or the member count changes -- and only once they have settled. + /// + public void UpdateRank() + { + var ranks = Faction.Definition.Ranks; + + if (ranks.Length == 0) + { + return; + } + + int percent; + + if (Owner.Count == 1) + { + percent = 1000; + } + else if (m_RankIndex == -1 || Faction.ZeroRankOffset <= 0) + { + percent = 0; + } + else + { + percent = (Faction.ZeroRankOffset - m_RankIndex) * 1000 / Faction.ZeroRankOffset; + } + + // Ranks run Required-descending ending at 0, so anything >= 0 matches below. A negative + // percent (RankIndex out of sync with ZeroRankOffset) would otherwise leave it null. + m_Rank = ranks[^1]; + + for (var i = 0; i < ranks.Length; i++) + { + var check = ranks[i]; + + if (percent >= check.Required) { - var ranks = Faction.Definition.Ranks; - int percent; - - if (Owner.Count == 1) - { - percent = 1000; - } - else if (m_RankIndex == -1) - { - percent = 0; - } - else - { - percent = (Faction.ZeroRankOffset - m_RankIndex) * 1000 / Faction.ZeroRankOffset; - } - - for (var i = 0; i < ranks.Length; i++) - { - var check = ranks[i]; - - if (percent >= check.Required) - { - m_Rank = check; - m_InvalidateRank = false; - break; - } - } - - Invalidate(); + m_Rank = check; + break; } - - return m_Rank; } } diff --git a/dev-docs/claude-skills/modernuo-code-audit.md b/dev-docs/claude-skills/modernuo-code-audit.md index 39a8d4983..5d5af15bb 100644 --- a/dev-docs/claude-skills/modernuo-code-audit.md +++ b/dev-docs/claude-skills/modernuo-code-audit.md @@ -191,8 +191,17 @@ mob.SendMessage($"You earned a {rank:L} trophy!"); // "gold" not "Gold" **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 (will cause bugs, build failures, or client-side leaks) +- **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) diff --git a/dev-docs/claude-skills/modernuo-property-lists.md b/dev-docs/claude-skills/modernuo-property-lists.md index 4440c63db..51e47ff35 100644 --- a/dev-docs/claude-skills/modernuo-property-lists.md +++ b/dev-docs/claude-skills/modernuo-property-lists.md @@ -20,6 +20,12 @@ description: > 3. **String interpolation** works with `IPropertyList` -- use `$"..."` syntax 4. **`[InvalidateProperties]`** on `[SerializableField]` auto-refreshes tooltip on change 5. **Call `InvalidateProperties()`** manually when non-serialized state changes tooltip +6. **Never invalidate from inside `GetProperties`** -- every property a `GetProperties` override + reads must be a pure read. `InvalidateProperties()` rebuilds in place (`Reset()` + rebuild), so a + getter with that side effect tears down the list mid-build: it returns the pooled interpolation + buffer under an in-flight `$"..."` handler (`ArgumentNullException`, parameter `"array"`) and + rewinds the packet cursor. The engine refuses and logs an error; `DEBUG` throws. Defer instead: + `Timer.DelayCall(InvalidateProperties)` ## IPropertyList Interface @@ -235,6 +241,7 @@ block.Add("Cannot be repaired".AsSpan()); // plain span, no string alloc - **Excessive rebuilds**: Don't call `InvalidateProperties()` in tight loops - **Assuming tooltip support**: Check `ObjectPropertyList.Enabled` if needed - **One giant `Add()` for multi-line text**: A property over ~512 chars crashes the legacy 2D client. Use `AddChunked`/`OplTextBlock` for variable-length free text +- **Side-effecting property getters**: A getter reached from `GetProperties` that calls `InvalidateProperties()` (directly or via a helper like `Invalidate()`) re-enters the build and is refused — error logged, `DEBUG` throws. Lazy recomputation in a getter is fine; the *notification* is not. Invalidate where the value changes, or `Timer.DelayCall(InvalidateProperties)` ## Real Examples - Item properties: `Projects/Server/Items/Item.cs` (AddNameProperties, GetProperties) diff --git a/dev-docs/property-lists.md b/dev-docs/property-lists.md index 98e26efc1..929429bcd 100644 --- a/dev-docs/property-lists.md +++ b/dev-docs/property-lists.md @@ -301,6 +301,66 @@ public void UseCharge() } ``` +### Never Invalidate From Inside `GetProperties` (CRITICAL) + +`InvalidateProperties()` rebuilds the list **in place** — `Reset()`, then `GetProperties()` again on +the same instance. Calling it from a property getter that the build itself reaches is therefore +re-entrant, and `Reset()` does two destructive things to the build in flight: + +1. It returns the pooled interpolation scratch buffer. The compiler rents that buffer in the + interpolated-string handler's constructor and returns it in the closing `Add`, so **every hole is + evaluated while the buffer is live**. Pulling it out mid-append makes the next `Append*` span a + null array — `ArgumentNullException: Value cannot be null. (Parameter 'array')` thrown out of + `GetProperties`, from a line that looks unrelated to the getter that caused it. +2. It rewinds the packet cursor, so properties already written are overwritten by the nested pass. + +This is always a defect in the property getter, so the engine refuses rather than trying to recover: +a nested call logs an error with a stack trace, throws in `DEBUG`, and in `RELEASE` returns without +touching the list — leaving a possibly stale tooltip, but never a crash, a corrupted packet, or a +leaked pool buffer. Retrying the build would only hide the bug. + +```csharp +// BAD -- a getter with a side effect. Reading it from GetProperties re-enters the build. +public RankDefinition Rank +{ + get + { + if (_invalidateRank) + { + _rank = Recompute(); + _invalidateRank = false; + Invalidate(); // -> InvalidateProperties() -> Reset() on the list being built + } + + return _rank; + } +} + +// GOOD -- getters stay side-effect free; invalidate where the value actually changes. +public int RankIndex +{ + get => _rankIndex; + set + { + if (_rankIndex != value) + { + _rankIndex = value; + _invalidateRank = true; + Invalidate(); + } + } +} +``` + +Lazy recomputation inside a getter is fine — it is the *notification* that must not happen there. If +something genuinely must invalidate in response to a read, defer it off the build: + +```csharp +Timer.DelayCall(InvalidateProperties); +``` + +**Check when writing a `GetProperties` override**: every property it reads must be a pure read. + ## ObjectPropertyList Internals Defined in `Projects/Server/PropertyList/ObjectPropertyList.cs`: