Never reuse a field number a deletion freed

CHANGELOG.md

@@ -11,9 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add an editor validation engine: implement `IValidationRule` and run it across the whole project a few milliseconds per editor tick instead of freezing for thirty seconds. Only the assets a rule claims are loaded. See [Asset Validation](./docs/features/editor-tools/asset-validation.md) ([#288](https://github.com/Ambiguous-Interactive/unity-helpers/issues/288)).
-- Add `[WProtoSubtype(typeof(Base))]`, so a subtype joins a WallstopProto hierarchy without picking a field number. The editor assigns and commits the number after the reload that first sees it, and **Assign WallstopProto Subtype Tags** retires a removed one so it is never reused. See [Polymorphism](./docs/features/serialization/serialization.md#polymorphism) ([#587](https://github.com/Ambiguous-Interactive/unity-helpers/issues/587), [#601](https://github.com/Ambiguous-Interactive/unity-helpers/issues/601)).
+- Add an editor validation engine: implement `IValidationRule` and run it across the whole project a few milliseconds per tick, not one thirty-second freeze. Only claimed assets load, and one `-executeMethod` runs it in CI with a JSON report and a reviewable suppression file. See [Asset Validation](./docs/features/editor-tools/asset-validation.md) ([#288](https://github.com/Ambiguous-Interactive/unity-helpers/issues/288)).
+- Add `[WProtoSubtype(typeof(Base))]`, so a subtype joins a WallstopProto hierarchy without picking a field number. The editor assigns and commits the number on the next reload, and **Assign WallstopProto Subtype Tags** retires a removed one -- hand-numbered or not -- so it is never reused. See [Polymorphism](./docs/features/serialization/serialization.md#polymorphism) ([#587](https://github.com/Ambiguous-Interactive/unity-helpers/issues/587), [#601](https://github.com/Ambiguous-Interactive/unity-helpers/issues/601), [#606](https://github.com/Ambiguous-Interactive/unity-helpers/issues/606)).
 - Add `Sfc64Random`, the Small Fast Chaotic generator: a published-pedigree 64-bit generator with a very small hot path that answers `NextUlong` in one state advance. See [Random Generators](./docs/features/utilities/random-generators.md) ([#516](https://github.com/Ambiguous-Interactive/unity-helpers/issues/516)).
+- Add `[WProtoReserved]`, which records a field number or name a removed `[WProtoMember]` held. Taking one again is a build error rather than a save that reads back as the wrong thing, and the exported schema carries the matching proto3 `reserved` lines. See [Retiring a member](./docs/features/serialization/serialization.md#retiring-a-member) ([#608](https://github.com/Ambiguous-Interactive/unity-helpers/issues/608)).
 - Add a proto schema exporter: **Tools > Wallstop Studios > Unity Helpers > Proto Schema Exporter** writes `proto3` for your `[WProtoContract]` types, so anything downstream can read your saves. Search and tick the exact types, name a package, and write one file or one per assembly, namespace or type ([#424](https://github.com/Ambiguous-Interactive/unity-helpers/issues/424), [#595](https://github.com/Ambiguous-Interactive/unity-helpers/issues/595)).
 - Add strict UTF-8 validation to WallstopProto strings and Uri: wire bytes that are not valid UTF-8 refuse the payload as malformed instead of decoding to replacement characters, as proto3 requires ([#580](https://github.com/Ambiguous-Interactive/unity-helpers/issues/580)).
 - Add `IntMap<TValue>`, an int-keyed open-addressing map measured at 1.26x–2.19x `Dictionary<int,int>` on hit-heavy lookups, with no comparer indirection on the lookup path. See [Data Structures](./docs/features/utilities/data-structures.md#intmap-int-keyed-open-addressing-map) ([#578](https://github.com/Ambiguous-Interactive/unity-helpers/issues/578)).
@@ -149,6 +150,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Fix two documentation examples that could not work as printed: the enum display-name sample imported `Core.Attribute` rather than `Core.Attributes`, and the `link.xml` sample preserved an assembly name that does not exist, which strips silently ([#441](https://github.com/Ambiguous-Interactive/unity-helpers/issues/441)).
 - Fix fifteen broken documentation links on `AssetDatabaseBatchScope`: its `<see cref>` references to `AssetDatabase.Refresh`, `CreateAsset` and `ImportAsset` named an ambiguous overload or an unresolvable type, so an IDE linked the wrong overload or nothing ([#594](https://github.com/Ambiguous-Interactive/unity-helpers/issues/594)).
 - Fix the IntelliSense tooltip on ten public `ReflectionHelpers` delegate factories, which carried two `<summary>` tags and showed the vaguer one ([#441](https://github.com/Ambiguous-Interactive/unity-helpers/issues/441)).
 - Fix zero-valued `ValueTuple` components and fixed-width map keys being omitted by WallstopProto where protobuf-net writes them explicitly, including enum tuple map keys ([#399](https://github.com/Ambiguous-Interactive/unity-helpers/issues/399)).

Editor/Tools/WProtoSubtypeTagAssigner.cs

@@ -312,6 +312,28 @@ WProtoIncludeAttribute include in baseType.GetCustomAttributes<WProtoIncludeAttr
                     );
                 }
 
+                // A reserved number is spent as surely as a live one -- the generator refuses a
+                // discriminator that takes it -- so a tool that assigned around only the live
+                // numbers would hand out a number the next compile rejects, which is the deadlock
+                // this tool exists to remove.
+                foreach (
+                    WProtoReservedAttribute held in baseType.GetCustomAttributes<WProtoReservedAttribute>(
+                        false
+                    )
+                )
+                {
+                    foreach (int fieldNumber in held.FieldNumbers)
+                    {
+                        inventory.Reserved.Add(
+                            new WProtoSubtypeTagPlan.Entry(
+                                "[WProtoReserved]",
+                                baseName,
+                                fieldNumber
+                            )
+                        );
+                    }
+                }
+
                 const BindingFlags Declared =
                     BindingFlags.Public
                     | BindingFlags.NonPublic

Editor/Tools/WProtoSubtypeTagPlan.cs

@@ -129,6 +129,7 @@
         )
         {
             List<Declaration> tagless = new List<Declaration>();
+            List<Declaration> pinned = new List<Declaration>();
             Dictionary<string, HashSet<int>> taken = new Dictionary<string, HashSet<int>>(
                 StringComparer.Ordinal
             );
@@ -146,9 +147,14 @@
 
                 if (declaration.HasTag)
                 {
+                    string pinnedKey = PairKey(declaration.SubTypeName, declaration.BaseTypeName);
                     Claim(taken, declaration.BaseTypeName, declaration.Tag);
-                    explicitTags[PairKey(declaration.SubTypeName, declaration.BaseTypeName)] =
-                        declaration.Tag;
+                    if (!explicitTags.ContainsKey(pinnedKey))
+                    {
+                        explicitTags[pinnedKey] = declaration.Tag;
+                        pinned.Add(declaration);
+                    }
+
                     continue;
                 }
 
@@ -169,6 +175,9 @@
             Dictionary<string, Entry> retiredByPair = new Dictionary<string, Entry>(
                 StringComparer.Ordinal
             );
+            Dictionary<string, Entry> allRetired = new Dictionary<string, Entry>(
+                StringComparer.Ordinal
+            );
             foreach (Entry entry in Safe(retired))
             {
                 if (!entry.IsUsable)
@@ -182,6 +191,12 @@
                 {
                     retiredByPair[key] = entry;
                 }
+
+                // Keyed by pair AND number. retiredByPair keeps one entry per pair, which is all a
+                // restore needs and is NOT enough to re-emit: a pair that retired two numbers --
+                // what a hand-edited number leaves behind -- lost one of them on the next run, and
+                // a dropped retirement is a number that is free again a run later.
+                allRetired[RetirementKey(entry)] = entry;
             }
 
             List<Entry> assignments = new List<Entry>();
@@ -190,7 +205,11 @@
                 StringComparer.Ordinal
             );
             HashSet<string> keptPairs = new HashSet<string>(StringComparer.Ordinal);
-            HashSet<string> restoredPairs = new HashSet<string>(StringComparer.Ordinal);
+            // Keyed by pair AND number, not by pair. A pair can hold more than one retirement -- a
+            // hand-edited number leaves one and a later deletion leaves another -- and re-adding
+            // the type under the first would otherwise free the second, which is the exact reuse
+            // the record exists to forbid.
+            HashSet<string> restoredRetirements = new HashSet<string>(StringComparer.Ordinal);
 
             foreach (Entry entry in Safe(existing))
             {
@@ -216,15 +235,27 @@
                     continue;
                 }
 
-                // The subtype pinned its own number and pinned the same one, so the manifest entry
-                // is simply redundant. Retiring it would forbid the very declaration that now holds
-                // it, and the next build would refuse a hierarchy that changed in no way at all.
-                if (
-                    explicitTags.TryGetValue(key, out int pinned)
-                    && pinned == entry.Tag
-                    && !retiredByPair.ContainsKey(key)
-                )
+                // A number written by hand is as durable a wire contract as one this tool
+                // assigned, and until it was recorded here the only trace that the number had ever
+                // been spent was the declaration itself -- which is deleted along with the type it
+                // sits on. Keeping the entry is what turns that deletion into a retirement (#606).
+                if (explicitTags.TryGetValue(key, out int pinnedTag))
                 {
+                    Claim(taken, entry.BaseTypeName, pinnedTag);
+                    assignments.Add(
+                        pinnedTag == entry.Tag
+                            ? entry
+                            : new Entry(entry.SubTypeName, entry.BaseTypeName, pinnedTag)
+                    );
+
+                    // Editing a shipped number in place is the one thing the guidance forbids, and
+                    // it used to leave no trace at all. The number it left still means this type to
+                    // every payload written under it.
+                    if (pinnedTag != entry.Tag)
+                    {
+                        retirements[RetirementKey(entry)] = entry;
+                    }
+
                     continue;
                 }
 
@@ -245,6 +276,33 @@
                 retirements[RetirementKey(entry)] = entry;
             }
 
+            // Before the tag-less passes, because an explicit number is stated by the source and
+            // needs neither restoring nor inventing -- it only needs recording.
+            pinned.Sort(CompareDeclarations);
+            foreach (Declaration declaration in pinned)
+            {
+                string key = PairKey(declaration.SubTypeName, declaration.BaseTypeName);
+                if (!keptPairs.Add(key))
+                {
+                    continue;
+                }
+
+                assignments.Add(
+                    new Entry(declaration.SubTypeName, declaration.BaseTypeName, declaration.Tag)
+                );
+
+                // Remove-then-re-add for the explicit form: the type is back under the number it
+                // held, so the retirement that was standing in for it is lifted rather than left to
+                // forbid the very declaration now holding it.
+                if (
+                    retiredByPair.TryGetValue(key, out Entry wasRetired)
+                    && wasRetired.Tag == declaration.Tag
+                )
+                {
+                    restoredRetirements.Add(RetirementKey(wasRetired));
+                }
+            }
+
             foreach (Entry entry in retiredByPair.Values)
             {
                 string key = PairKey(entry.SubTypeName, entry.BaseTypeName);
@@ -256,7 +314,7 @@
                 // Remove-then-re-add, which is the case the whole design exists for: the number the
                 // type had is still held for it, so it comes back rather than being handed out.
                 assignments.Add(entry);
-                restoredPairs.Add(key);
+                restoredRetirements.Add(RetirementKey(entry));
                 keptPairs.Add(key);
             }
 
@@ -284,9 +342,9 @@
                 fresh.Add(assignment);
             }
 
-            foreach (Entry entry in retiredByPair.Values)
+            foreach (Entry entry in allRetired.Values)
             {
-                if (!restoredPairs.Contains(PairKey(entry.SubTypeName, entry.BaseTypeName)))
+                if (!restoredRetirements.Contains(RetirementKey(entry)))
                 {
                     retirements[RetirementKey(entry)] = entry;
                 }
@@ -328,15 +386,21 @@
                 "// Subtype Tags. Commit it: these numbers are the wire contract for every\r\n"
             );
             builder.Append(
-                "// [WProtoSubtype] declared without one, so a payload saved today is read back by\r\n"
+                "// [WProtoSubtype] in this assembly, so a payload saved today is read back by\r\n"
+            );
+            builder.Append(
+                "// this file. A subtype that wrote its own number is recorded here too, because\r\n"
+            );
+            builder.Append(
+                "// deleting the type deletes the only other record that the number was spent.\r\n"
             );
             builder.Append(
-                "// this file. Do not renumber an entry, and do not delete a retired one -- a\r\n"
+                "// Do not renumber an entry, and do not delete a retired one -- a retired number\r\n"
             );
             builder.Append(
-                "// retired number is held so a later subtype cannot be given a number old saves\r\n"
+                "// is held so a later subtype cannot be given a number old saves already mean\r\n"
             );
-            builder.Append("// already mean something else by.\r\n");
+            builder.Append("// something else by.\r\n");
             builder.Append(
                 "//\r\n// The editor rewrites this file automatically after an assembly reload that finds a\r\n"
             );
@@ -473,7 +537,7 @@
         /// <summary>
         /// One <c>[WProtoSubtype]</c> as written, whether or not it stated a field number.
         /// </summary>
         public readonly struct Declaration
         {
             /// <summary>
             /// Initializes the declaration.
@@ -512,7 +576,7 @@
         /// <summary>
         /// One field number belonging to one subtype on one base.
         /// </summary>
         public readonly struct Entry
         {
             /// <summary>
             /// Initializes the entry.