Remove leftover planning notes and the phaseS spike script

Assisted-by: Claude:claude-opus-4-8:Claude Code
3 weeks ago · f669a2cdd2
3 changed files with 0 additions and 1194 deletions
--- a/PHASE_3C_TODO.md
+++ b/PHASE_3C_TODO.md
@ -1,103 +0,0 @@
 # Phase 3c -- storage flip (linked list -> slot fields). Fresh-session execution checklist.
 > **UPDATE 2026-06-14: Phases 4 (token drop) + 5 (comments->trivia) are now DONE (20 commits on
 > `ast-source-generator-rebased`, byte-identical, unpushed) -- they were reordered AHEAD of 3c.
 > So this checklist is now SIMPLER than written: CSharpTokenNode/CSharpModifierToken are deleted
 > (no token children), InsertSpecialsDecorator is deleted, and there are NO positional comment
 > children or `Roles.Comma` trailing-comma markers any more -- IGNORE the "Risks/gotchas" notes
 > about comments/commas/token positional children below; they no longer exist. The only
 > comment-as-node forms are ErrorExpression + comment-bearing EmptyStatement (normal nodes).
 > StartLocation/EndLocation are ALREADY stored fields set at print (not computed) -- the location
 > part of 3c is done. The GetNodeAt-family nav methods are dead -> just DELETE them. Launch from
 > current clean HEAD (not `20a04825e`). The ILSpy-tests submodule is now fully checked out in the
 > worktree (net40 configs run; no nuget-symlink workaround needed).**
 Atomic change (corner C1): the build does NOT compile again until ALL of A+B+C land together.
 Spec: `ROLES_FREE_SLOT_AST_DESIGN.md` Part I.1 + Phase 3c.
 Build/test (every run): prefix `OPENSSL_ENABLE_SHA1_SIGNATURES=1`; build with
 `-p:RestoreEnablePackagePruning=false`; suite is
 `dotnet test ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj --report-trx`
 (needs the `ILSpy-tests/nuget` symlink -> `/home/siegfried/Projects/ILSpy/ILSpy-tests/nuget`,
 recreate if absent or every test Assert.Fails at fixture setup). After each commit discard any
 pruned `*packages.lock.json`. Let the pre-commit format hook run (it touches DecompilerVersionInfo.cs
 harmlessly). Use `Assisted-by:` trailer, never Co-Authored-By.
 ## 0. Optional pre-flip cleanups (cheap, do first or skip)
 - [ ] Squash `f914d57b3` (tried slot-order reorder) + `7fea03be7` (reverted it) -- they cancel; branch
      is local-only so an interactive rebase is safe. Optional, cosmetic.
 - [ ] `UsingAliasDeclaration` alias: `AliasRole` holds an Identifier child but is exposed only as a
      string wrapper (no Identifier property). Add `[Slot("AliasRole")] public partial Identifier
      AliasToken { get; set; }` so the schema is complete (or handle inside the flip).
 ## A. AstNodeCollection<T> -> list-backed  (CSharp/Syntax/AstNodeCollection.cs)
 Currently a *view* over the linked list (iterates parent.firstChild filtering by role). Rewrite:
 - [ ] Back with `List<T>`; hold `AstNode parent` + `int slotIndex` (the collection's slot).
 - [ ] Keep the used surface (already enumerated in design Part I.1): Add, AddRange, Insert,
      InsertBefore(item)/InsertAfter(item), Remove, Clear, ReplaceWith(IEnumerable), MoveTo, Detach,
      AcceptVisitor, indexer/Count/Contains/CopyTo/GetEnumerator, FirstOrNullObject/LastOrNullObject.
 - [ ] On every mutation, set added child's parent + role + childIndex, detach old, and **renumber
      following children's childIndex** (insert/remove shift). Strict-tree invariant.
 - [ ] Implement IReadOnlyList<T> AND explicit IReadOnlyList<INode> (for the matcher, Phase 6).
 ## B. AstNode base rewrite  (CSharp/Syntax/AstNode.cs)
 - [ ] Remove `firstChild/lastChild/prevSibling/nextSibling`. Add `int childIndex = -1`. Keep `parent`,
      keep `flags` (role index stays until Phase 7 -- a child's role is set when placed in a slot).
 - [ ] Make abstract (generator implements per leaf): `int GetChildCount()`, `AstNode? GetChild(int)`,
      `void SetChild(int, AstNode?)`, `CSharpSlotInfo GetChildSlot(int)`. (Replaces the bridge's
      SlotCount/GetSlotRole/IsCollectionSlot/GetChild/SetChild.)
 - [ ] `SetChildNode<T>(ref T field, T value, int slotIndex)`: frozen-free; validate not-parented /
      not-in-another-tree / type-compatible; detach old; write field; set value.parent + childIndex.
 - [ ] Re-express the linked-list API over slots (flattened index, Part I.4 -- single slot = width 1
      even when empty; collection slot = contiguous run of its count; offsets after a collection shift
      by its length):
      - FirstChild/LastChild/NextSibling/PrevSibling: computed from (parent, childIndex), skipping
        empty optional single slots.
      - Children / HasChildren: iterate flattened slots in order.
      - Descendants/GetDescendantsImpl, GetNodeAt/GetNodesBetween/Contains: now go through Children
        (already do) -- verify they work once Children is slot-based. (These location-nav methods are
        NRefactory-inherited and currently have no in-tree callers, but the API is kept; location
        correctness itself is covered by LocationsInAstTests' text-slice assertion, not via GetNodeAt.)
 - [ ] Re-express the **role API** (still used by consumers until Phase 7) over slots:
      - GetChildByRole<T>(role): scan slots for GetChildSlot(i).Role == role; single -> (T)GetChild(i)
        (role null object if empty); collection role -> first element.
      - GetChildrenByRole<T>(role): return the collection for that role. NB the base needs to reach the
        collection object generically -- emit a generator hook (e.g. virtual AstNodeCollection
        GetCollection(int slot)) or have GetChildrenByRole map role->slot and return the field.
      - SetChildByRole, AddChild(child, role), AddChildWithExistingRole, InsertChildBefore/After(sibling),
        Remove(), Detach, ReplaceWith(node) = parent.SetChild(childIndex, node).
 - [ ] StartLocation/EndLocation: still compute from first/last child (over slots) for now -- the
      stored-field-at-print version is coupled to the Phase-4 token drop, NOT this phase.
 - [ ] Clone: MemberwiseClone; reset parent/childIndex; per-slot deep clone (loop GetChildCount/GetChild
      or generator-emitted Clone) + CloneAnnotations (+ trivia, once that lands in Phase 5).
 ## C. Generator  (ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs)
 - [ ] From the existing [Slot] declarations, emit: backing fields (single `T f;` / collection
      `readonly AstNodeCollection<T> f`); property bodies single `get => f; set => SetChildNode(ref f,
      value, <slot>)`, collection `get => f`; the flat GetChildCount/GetChild/SetChild/GetChildSlot
      with collection range arithmetic; CSharpSlotInfo statics; per-slot Clone.
 - [ ] Inherited (override) slots already declared on leaves (flatten) -- emit their field+body on the
      leaf (the field lives on the leaf, the base contract member becomes abstract or stays virtual).
 - [ ] Drop/replace the transitional SlotCount/GetSlotRole/IsCollectionSlot bridge emission.
 ## D. Iterate to green
 - [ ] Build; fix the cascade of compile errors (internal users of firstChild/nextSibling, any direct
      linked-list pokes, AstNodeCollection ctor signature changes). Expect many.
 - [ ] Full suite -> must return to 1195/984 baseline, byte-identical, 0 diffs.
 - [ ] ilspycmd smoke-decompile a couple assemblies; perf re-check vs the ~33s CoreLib baseline (E2).
 ## E. Owed at the flip (tracked tasks #15, #16)
 - [ ] #16: re-introduce the slot-order invariant (now holds by construction -- children stored by slot)
      as a DEBUG CheckInvariant; wire into the IAstTransform pipeline (design Part I.13).
 - [ ] #15: revert the one bridge-era decompiler change -- `IntroduceExtensionMethods` ReplaceWith
      (commit 6869ef7a7) -- now unnecessary; audit for any other decompiler-transform changes.
 ## Risks / gotchas
 - The role API reimplementation (single vs collection, role->slot lookup) is the fiddly core -- get
  GetChildByRole/GetChildrenByRole exactly right; lots of consumers depend on them.
 - Pattern matcher: `AstNodeCollection<T>` is used by the matcher; keep IReadOnlyList<INode> working
  (full matcher port is Phase 6, but don't break it now).
 - Comments/trivia are still ordinary `Roles.Comment` children in the linked-list model; after the flip
  they're collection/role children too -- verify comment output unchanged (Phase 5 moves them to the
  trivia channel later).
 - This delivers the O(1) access + is where perf finally arrives (the bridge had neither).
--- a/ROLES_FREE_SLOT_AST_DESIGN.md
+++ b/ROLES_FREE_SLOT_AST_DESIGN.md
@ -1,925 +0,0 @@
 # Roles-free slot-based C# AST -- complete design + implementation plan
 Branch: `ast-source-generator-rebased`. Authoritative, self-contained design + step list for
 moving the C# AST from roles + doubly-linked-list children to a slot-based model. Companion:
 line-by-line pattern-matcher equivalence proofs in
 `~/.claude/plans/rustling-bubbling-oasis-agent-ae8f0184ec5b19d68.md`.
 ## Context (why)
 ILSpy's C# AST (`ICSharpCode.Decompiler/CSharp/Syntax/`) is NRefactory-derived: children live
 in a per-node doubly-linked list, each tagged with a `Role` (9-bit index packed in `flags`).
 Consequences: `GetChildByRole` is a linear scan; child order is insertion order, not schema
 order (forcing hacks like the `InsertChildAfter` splice in
 `TransformFieldAndConstructorInitializers.RemoveImplicitConstructor`); every property
 read/write walks/relinks the list. The ILAst (`IL/Instructions/*`) already uses the better
 model -- fixed *slots* with O(1) access and schema-ordered children -- generated from a
 template. We are porting that model onto the C# AST, and using the move to shed other
 NRefactory full-fidelity baggage the decompiler does not need.
 Foundation already in place on this branch: a Roslyn incremental generator
 (`ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs`) that, from a
 `[DecompilerAstNode]` attribute + reflection over each node's properties, generates the
 mechanical members -- `AcceptVisitor` x3, the three `IAstVisitor` interfaces, `DoMatch`, null
 nodes, and pattern placeholders. Storage is still role + linked-list (handwritten
 `GetChildByRole` properties). This plan replaces that storage with slots and removes roles.
 ## Decisions locked with the user
 - Roslyn incremental source generator (in place).
 - Node declaration = **partial properties, source-order slots** (no explicit indices).
 - **Delete `Role` entirely** (and `Role<T>`, `Roles`, `TokenRole`, `GetChildByRole`,
  `SetChildByRole`, `GetChildrenByRole`, `node.Role`); rewrite all consumers; accepted public-API
  break. **Keep `AstNode` and `AstNodeCollection<T>` names** -- reimplement the collection over slots
  rather than rename it (avoids a gratuitous API break and keeps the `AstNode`/`AstNodeCollection`/
  `AstType` name family; the IL parallel is implementation, not naming).
 - **Drop token nodes** (`CSharpTokenNode`/`CSharpModifierToken` deleted); their meaning -> scalars.
 - **Comments and preprocessor directives = node trivia** over a `Trivia` base
  (`Comment`/`PreProcessorDirective` derive from it). Stored in the existing annotation channel as a
  lazily-created `NodeTrivia` holder -- **no per-node fields**, so the ~99.99% no-trivia nodes cost
  nothing (`CloneAnnotations` copies it for free). `AddLeadingTrivia`/`AddTrailingTrivia`, printed
  before/after the node. `#define` (the only directive emitted; no bracketing) becomes leading trivia
  on the `SyntaxTree`. Doc comments = a leading `Documentation` slot (structural, separate).
 - **The AST models structure + content, never formatting/whitespace.** Whitespace, indentation,
  spacing, blank lines, brace style are print-time, driven by `CSharpFormattingOptions` (independent
  of the AST); `Trivia` is content (comments+directives), not whitespace.
 - **Drop `IFreezable`/`Freeze`/`IsFrozen`** -- dead NRefactory baggage (no consumer ever freezes;
  not even the null singletons).
 - **Locations** = stored `StartLocation`/`EndLocation` (`TextLocation` each), assigned at print time;
  no `Span` tuple, no compute-from-children recursion.
 - **Inheritance** = abstract contract on bases + per-leaf ordered slots, flat `sealed` dispatch.
 - **Pattern matcher** = faithful port to list-based collection matching; slim `INode`; tests first.
 - **Null-object -> NRT**: backing field uniformly nullable; *syntactically required* slot =
  non-null accessor (the static contract), optional slot = nullable accessor; delete the null
  objects. `CheckInvariant` enforces required-non-null at between-transform checkpoints.
 - `Identifier` stays a node (single fixed slot). `CSharpSlotInfo` is positional. Annotations
  unchanged.
 ==========================================================================================
 # PART I -- THE COMPLETE DESIGN
 ==========================================================================================
 ## 1. Native model
 `AstNode` base (handwritten):
 ```csharp
 public abstract class AstNode : AbstractAnnotatable, ICloneable, PatternMatching.INode {
    AstNode? parent;
    int childIndex = -1;            // flattened index within parent
    TextLocation startLocation, endLocation;   // set at print time
    public AstNode? Parent { get; }
    public int ChildIndex { get; }
    public CSharpSlotInfo? Slot => parent?.GetChildSlot(childIndex);     // replaces node.Role
    public TextLocation StartLocation { get; }   // returns startLocation (no recursion)
    public TextLocation EndLocation { get; }     // returns endLocation
    internal void SetLocations(TextLocation start, TextLocation end);    // print-time writer
    // generated per concrete leaf (flat, sealed):
    protected abstract int GetChildCount();
    protected abstract AstNode? GetChild(int index);   // null = empty optional slot
    protected abstract void SetChild(int index, AstNode? value);
    protected abstract CSharpSlotInfo GetChildSlot(int index);
    public ChildrenCollection Children { get; }        // document order (struct view)
    public void ReplaceWith(AstNode n) => parent!.SetChild(childIndex, n);
    // NextSibling/PrevSibling/FirstChild/LastChild computed from (parent, childIndex),
    // skipping empty optional slots.
    protected void SetChildNode<T>(ref T field, T value, int index) where T : AstNode?; // see below
    // Trivia (comments + preprocessor directives), Trivia = base of Comment/PreProcessorDirective.
    // NO per-node trivia fields: stored in the existing AbstractAnnotatable annotation channel as a
    // lazily-created NodeTrivia holder {leading, trailing}, so a node WITHOUT trivia (the ~99.99%
    // common case) pays nothing extra. CloneAnnotations copies it for free (Part 6/8).
    public void AddLeadingTrivia(Trivia t);   // get-or-create the NodeTrivia annotation, append
    public void AddTrailingTrivia(Trivia t);
    public IEnumerable<Trivia> LeadingTrivia { get; }   // empty (no alloc) when the annotation is absent
    public IEnumerable<Trivia> TrailingTrivia { get; }
 }
 ```
 `SetChildNode` (analog of IL's `SetChildInstruction`): validate value not parented / not in another
 tree / type-compatible with slot; write field; set value.parent/childIndex; detach old. Strict-tree
 invariant (old node detached immediately); no IL "stale positions". (No frozen check -- `IFreezable`
 is dropped.)
 `CSharpSlotInfo` (the `SlotInfo` analog; positional):
 ```csharp
 public sealed class CSharpSlotInfo {
    public string Name { get; }        // "Left", "Body", "Parameters"
    public Type ChildType { get; }     // declared child type
    public bool IsOptional { get; }    // syntactically optional => nullable accessor (Part 11);
                                        // required => non-null accessor. Drives CheckInvariant.
    public bool IsCollection { get; }
 }
 ```
 Per-slot static instances generated per class (`BinaryOperatorExpression.LeftSlot`).
 `node.Slot == X.YSlot` replaces `node.Role == X.YRole` (~43 consumer sites).
 `AstNodeCollection<T>` (kept; reimplemented over slots, analog of `InstructionCollection<T>`):
 List-backed, knows parent + slot, implements `IReadOnlyList<T>` and explicitly
 `IReadOnlyList<INode>` (for the matcher). Insert/remove renumbers following children's `childIndex`.
 Must preserve the actually-used surface: `Add`, `AddRange`, `Insert`, `InsertBefore(item,...)`/
 `InsertAfter(item,...)` (by reference node), `Remove`, `Clear`, `ReplaceWith(IEnumerable<T>)`,
 `MoveTo(targetCollection)` (cross-collection move -- corner C6), `Detach`, `AcceptVisitor`,
 indexer/`Count`/`Contains`/`CopyTo`/`GetEnumerator`. `FirstOrNullObject`/`LastOrNullObject` become
 `FirstOrDefault`/`LastOrDefault` (returning `null`) once null objects are gone (Part 11).
 ## 2. Node declaration (author writes)
 ```csharp
 [DecompilerAstNode]
 public partial class BinaryOperatorExpression : Expression {
    public partial Expression Left { get; set; }
    public partial Expression Right { get; set; }
    public BinaryOperatorType Operator { get; set; }   // operator text derives from this
 }
 ```
 The generator emits the other half: backing fields, property bodies
 (`get => left; set => SetChildNode(ref left, value, <n>)`), the `CSharpSlotInfo` statics,
 `GetChildCount/GetChild/SetChild/GetChildSlot`, `Clone`, and (as today) AcceptVisitor / DoMatch
 / null node (until Part 11) / pattern placeholder. Slot order = source declaration order
 (Roslyn preserves member order).
 Declaration mechanism (how the generator reads each slot):
 - Single child slot: `public partial T Name { get; set; }` (T : AstNode). Read-only `{ get; }`
  for a printer-derived child (e.g. a computed token replacement) -- rare after the token drop.
 - Collection slot: `public partial AstNodeCollection<T> Name { get; }` (get-only; generator owns the
  instance).
 - **Optionality (Part 11 NRT): the declared nullability IS the slot's `IsOptional`** --
  `T Name` => *syntactically required* (non-null accessor = the static contract; getter returns
  `field!`), `T? Name` => optional (nullable accessor, `null` when empty). The **backing field is
  uniformly nullable either way** (so transforms may transiently empty any slot); only the accessor
  signature and `IsOptional` differ. Pre-NRT phases keep non-null accessors with null-object
  substitution; the nullability annotations are applied in Phase 8.
 - Inherited contract members are re-declared `override partial` in the leaf at their ordered
  position (Part 3).
 ## 3. Inheritance composition
 Base classes are the shared **contract** (for polymorphic consumption); each concrete node owns
 its **implementation and order**. Base declares shared *child-slot* members `abstract`
 (`EntityDeclaration.Attributes`); shared *scalars* (`Modifiers`, `SymbolKind`) stay concrete on
 the base. Each leaf declares its full slot set as partial properties in document order,
 `override`-implementing the contract members where they belong; the generator emits one flat
 `sealed` dispatch per leaf. No base-delegation, no offset arithmetic (this is the divergence
 from ILAst, which forbids split children via `sealed`; C# AST splits, so we use the
 abstract-contract approach instead).
 Confirmed by spike E4 (prototype `352d1eb3c`): base-delegation is not merely heavier but
 *structurally incorrect* here -- for TypeMembers the inherited shared slots (`ReturnType`/
 `NameToken`) are NON-CONTIGUOUS in source order (`NameToken` is emitted after the leaf-only
 `PrivateImplementationType`, `CSharpOutputVisitor.cs:2577-2582`), so base-delegation, which
 front-pins the inherited block to indices `0..BaseSlotCount-1`, cannot produce correct document
 order. Flatten is required, not merely preferred; it is also smaller in total generated LOC (family
 158 vs 171) and avoids a base virtual call + offset math on the hot child-access path. The one
 flatten risk (a leaf silently mis-ordering/omitting a re-declared contract slot) is
 generator-controllable: the generator owns ordering and emits a per-leaf contract-coverage
 self-check.
 ## 4. Collections & flattened indexing
 A node's flattened child index space = slots in declaration order. A **single slot is always
 width-1** -- it occupies its flattened index even when empty (`GetChild` returns `null` at that
 fixed position); only **collection** slots vary their width, occupying a contiguous run of the
 collection's current length. So the index space is stable across the Phase 11 null->nullable
 transition (an emptied single slot does not shift the offsets of later slots). Generated
 `GetChild/SetChild/GetChildSlot` use range arithmetic (single slots at fixed offsets; offsets after
 a collection shift by its count). `AstNodeCollection<T>` insert/remove renumbers following children.
 This removes the `TransformFieldAndConstructorInitializers` reordering hack (a param added to
 `TypeDeclaration.Parameters` lands at its schema position regardless of insertion time).
 **Renumber scheme (decided by spike E2, prototype `dd58586b8`): plain eager renumber-on-mutation.**
 It beats the linked list on every realistic op mix (positional Get/IndexOf on a list are O(1) vs the
 list's O(n) scans); the O(n^2) front-insert-no-read worst case is not a decompiler workload and is
 negligible in absolute terms (14 ms at 5000 elements vs a ~33 s CoreLib decompile). A lazy
 dirty-watermark scheme (childIndex recomputed on first ordinal read, ~20 LOC, no API change) is the
 documented fallback, applied only if profiling ever flags a specific oversized collection.
 Open sub-point (from E4): when a leaf does not store a contract slot (e.g. `FieldDeclaration` has no
 `PrivateImplementationType`/`NameToken` storage), decide whether that slot occupies a flattened index
 or is contract-only-not-a-slot -- nail this down with the width-1 rule above.
 ## 5. Drop token nodes
 The ~85 `CSharpTokenNode` child slots are removed (the printer's `WriteToken(Role, text)` is
 handed the text; it never reads a token child). `CSharpTokenNode` and `CSharpModifierToken`
 types are deleted. Meaning -> scalars (most already exist): operators ->
 `BinaryOperatorType`/`AssignmentOperatorType`/`UnaryOperatorType`; accessor keyword ->
 `AccessorKind`; member modifiers -> `Modifiers` flags (flip storage from `CSharpModifierToken`
 children to a field); `async`/`await` presence -> `bool IsAsync` on `UsingStatement`/
 `ForeachStatement`/`LambdaExpression`/`AnonymousMethodExpression`; attribute target -> an
 `AttributeTarget` enum (complete value set: `None, Assembly, Module, Type, Method, Property, Field,
 Event, Param, Return` -- note `Property` is used today, `TransformFieldAndConstructorInitializers`
 sets `"property"`/`"field"`); punctuation -> pure output.
 Verified by spike E3 (prototype `6e29b23ec`): the decompile-to-text printer never reads these token
 children -- dropping `OperatorToken` and `ReturnToken`/`SemicolonToken` produced byte-identical
 Pretty output (667/667). `VisitBinaryOperatorExpression` derives operator text from the `Operator`
 enum; `ComposedType` reads `HasRefSpecifier`/`HasNullableSpecifier`/`PointerRank`, not its token
 children; `InsertRequiredSpacesDecorator` branches on token TEXT, never the Role; and
 `InsertParenthesesVisitor` is type/precedence-driven (zero paren drift). The genuine coupling is
 `InsertMissingTokensDecorator` (rebuilds tokens + assigns Roles on the `CreateWriterThatSetsLocationsInAST`
 location path, NOT in the Pretty path) and `InsertSpecialsDecorator`'s role-keyed trivia placement --
 see Phase 4.
 ## 6. Comments, doc comments, directives
 - Free comments = **node trivia**. Today a `Comment` attaches via `Roles.Comment` to almost any
  node (the `InsertSpecialsDecorator` interleaves at print) -- six receiver categories in use:
  block statement-list, an `Expression`, a non-block `Statement`, between `TypeDeclaration.Members`,
  an `AstType`, a `VariableInitializer`, an `Attribute`, an `Accessor`. Two comment-node types
  cannot reproduce that. Instead, every comment is re-homed onto **the node it annotates** as
  leading or trailing trivia:
  - Storage (memory-efficient -- comments/directives are the absolute outlier in decompiler output,
    so the no-trivia case must cost ZERO): NO per-node trivia fields. Trivia lives in the existing
    `AbstractAnnotatable` annotation channel (the one `object annotations` field every node already
    has) as a lazily-created `NodeTrivia` holder `{leading, trailing}`, added only when a node first
    gets trivia. A node without trivia (the ~99.99%) pays nothing -- no new field, no allocation. This
    keeps trivia outside the flattened child-index space (no matcher interaction, `CheckInvariant`'s
    child-range arithmetic stays clean).
  - API: `AddLeadingTrivia(Trivia)` / `AddTrailingTrivia(Trivia)` (the distinction is the method
    called, not a caller-set flag) get-or-create the `NodeTrivia` annotation and append; read-back via
    `LeadingTrivia`/`TrailingTrivia` (empty, no allocation, when absent).
  - Print: output visitor emits leading trivia at `StartNode` (before the node's children) and
    trailing at `EndNode` (after), insertion order per group -- replacing the `InsertSpecialsDecorator`
    `Roles.Comment` path. The "between members" case becomes a *leading* comment on the following
    member (re-homed at the call site).
  - `Clone` is automatic: trivia is an annotation, so the existing `CloneAnnotations()` copies it --
    no special trivia-copy code.
  - Guard: assert no site needs an *interior* comment between two real children where neither is the
    annotated node (the enumeration shows none -- all are first/last/leading-on-member).
 - **Preprocessor directives are trivia too**, on the SAME channel. `Comment` and
  `PreProcessorDirective` both derive from a `Trivia` base, so leading/trailing trivia is a list of
  `Trivia`. `Trivia` is **content the printer must emit verbatim (comments + directives), NOT
  whitespace** -- whitespace/blank-lines are never modeled in the AST (Part 7). This unifies what
  were two mechanisms (trivia channel + "marker nodes in the collection") into one, and takes
  directives off the child-index space (a `#define` is file-level trivia, not a semantic child).
  `#define` becomes **leading trivia on the `SyntaxTree`** (top-of-file); `IntroduceUsingDeclarations`
  reads `syntaxTree.LeadingTrivia` instead of `Children`. `PreProcessorDirective` carries a
  `DirectiveKind` + payload. Grounded: only `#define` is emitted today (`CSharpDecompiler.cs:2263`)
  and there is no bracketing (region/if/endif/pragma never constructed); if any are added later they
  attach as leading/trailing trivia on the relevant node (the natural model).
 - Doc comments: a leading `Documentation` slot on `EntityDeclaration` (a `DocumentationComment`
  node, multi-line text + span); rides with its member. Replaces today's fragile preceding
  `Comment(Documentation)` siblings (`AddXmlDocumentationTransform`). Kept separate from the trivia
  channel (it is structural -- a slot that rides with the member -- not free-floating trivia).
 ## 7. Source locations & the AST/formatting boundary
 **Principle: the AST models structure + content, never formatting or whitespace.** Whitespace,
 indentation, spacing, blank lines, and brace style are *print-time* concerns -- the output visitor
 produces them when turning the AST into text, driven by `CSharpFormattingOptions`, which stays an
 **independent** object passed to the printer, not state on the nodes. The AST carries no whitespace
 nodes and no formatting flags. (This is why locations are assigned at print time, and why `Trivia`
 is comments+directives only, not whitespace.) Consequence for the rewrite: do not migrate any
 formatting concept into a node or slot; if a current node carries an incidental formatting hint, it
 belongs in the printer + options, not the AST.
 Locations: keep the existing `StartLocation`/`EndLocation` properties (`TextLocation` each), the primary
 consumer being PDB generation (`DebugInfo.SequencePoint` is `StartLine/StartColumn/EndLine/EndColumn`;
 `SequencePointBuilder` reads `node.StartLocation`/`EndLocation`) plus `AstNode`'s own tree-nav
 (`GetNodeAt`/`GetNodesBetween`/`Contains`). **No `Span` tuple** -- keeping the two property names is
 the facade that spares the ~33 readers any churn. Today these are *virtual, computed by recursion*
 to the token leaves (`AstNode.cs:183-199`); the token drop removes those leaves, so the new model
 **stores** the values in two fields and **drops the recursion**. Assigned by the existing mechanism:
 the output visitor brackets every node with `StartNode`/`EndNode`, and `WrapInWriterThatSetsLocationsInAST`
 reads the writer's tracked `Location` and calls the internal `SetLocations`. No computed fallback is
 needed: locations are only meaningful post-format today too (token-leaf locations are `Empty` until
 `InsertMissingTokensDecorator` runs), and all ~33 readers run post-format. Token-drop consequence:
 `SequencePointBuilder`'s `{`/`}` breakpoints move from brace-token locations to the
 `BlockStatement` start/end.
 ## 8. Annotations
 Unchanged. `AstNode : AbstractAnnotatable` stays; annotations are orthogonal to the child model
 (keyed by node identity, not slots), and core to the decompiler (124 `.Annotation<...>` reads,
 incl. the `ILInstruction`-per-node mapping `SequencePointBuilder` uses). Generated `Clone` must
 keep calling `CloneAnnotations()` -- which also copies comment/directive trivia for free, since
 Part 6 stores the `NodeTrivia` holder as an annotation (same node-identity-keyed channel).
 ## 9. Role deletion & node.Slot
 `Role`/`Role<T>`/`Roles`/`TokenRole`/`GetChildByRole`/`SetChildByRole`/`GetChildrenByRole` deleted.
 `AstNodeCollection<T>` is **kept** (reimplemented over slots, Part 1) -- not deleted/renamed.
 `node.Role` -> `node.Slot` (`CSharpSlotInfo`). Consumer rewrites: `GetChildByRole(R)` -> typed
 property or `GetChild(i)` (~14 external; ~439 internal vanish into generated bodies);
 `SetChildByRole(R, v)` -> typed property set; `node.Role == X.YRole` -> `node.Slot == X.YSlot` (~43);
 `GetChildrenByRole` -> the typed `AstNodeCollection<T>` property; `AddChild(c, R)`/
 `InsertChildBefore/After` -> `collection.Add`/`Insert`; `node.Role.IsValid(...)` -> slot/type check
 on `CSharpSlotInfo.ChildType`.
 ## 10. Pattern-matching engine (faithful list-based port)
 Role-based collection matching becomes list-vs-list (each `AstNodeCollection<T>` already *is* the
 per-role sub-list). See Part II Phase 6 for the full design (slim `INode`; central matcher over
 `IReadOnlyList<INode>`; `Repeat`/`OptionalNode` over index cursor; `AstNodeCollection<T>.DoMatch`;
 generator `PatternPlaceholder` signature; `IdentifierExpressionBackreference` rewrite). Semantics
 preserved 1:1; unit tests added first.
 ## 11. Null-object -> NRT
 **Backing field is uniformly nullable**; the accessor encodes the contract. Optional slot ->
 nullable accessor (`Expression? Body`, returns the field, `null` when empty); *syntactically
 required* slot -> non-null accessor (`Expression Left`, getter returns `field!`). The nullable field
 lets a transform transiently empty any slot ("optional during a transform"); the non-null accessor
 is the static contract that a finished tree has it filled, enforced by `CheckInvariant` at
 between-transform checkpoints (Part 13). Reading a transiently-empty required child throws at the
 bug site; an unrefilled one is caught at the next checkpoint -- so no special `Detach`/`Remove`
 rule is needed.
 Classification = *syntactically required* (the C# grammar is the oracle; mechanical proxy: a slot is
 required iff `CSharpOutputVisitor` dereferences it unconditionally). `default-to-optional` only as a
 tie-breaker. Drop the 18 generated null-node classes, `AstNode.IsNull`, and the `.Null` singletons.
 Consumer sweep: ~91 `.IsNull` -> `== null` (dead on required slots), ~99 `.Null` -> `null`; output
 visitor + `DepthFirstAstVisitor` drop `VisitNullNode` and null-check before recursing into optional
 children. Simplifies the matcher: `INode.IsNull` is then removed (empty = `null`).
 Verify before locking: no path emits a finished tree with a required slot empty (the decompiler
 emits an `ErrorExpression`/comment instead) -- a false-required there would throw where today it
 printed nothing.
 ## 12. What the generator emits (end state)
 Per `[DecompilerAstNode]` node, from its partial-property declarations: backing fields; slot
 property bodies; `CSharpSlotInfo` statics; `GetChildCount/GetChild/SetChild/GetChildSlot` (flat,
 sealed, range arithmetic for collections); `Clone` (per-slot + `CloneAnnotations`); `AcceptVisitor`
 x3; the `IAstVisitor` interfaces; `DoMatch`; pattern placeholder (`DoMatchCollection` list-based).
 For required slots the getter returns `field!` (the static non-null contract over a nullable field);
 optional slots return the nullable field. No null-node classes after Part 11. Optionally generate
 `SymbolKind` from the model (replacing 14 handwritten overrides) -- deferred nicety.
 ## 13. AST invariants (CheckInvariant)
 The slot model makes structural invariants *checkable* (the role + linked-list model could not
 validate a `childIndex` because there was none). Add a DEBUG-only recursive `CheckInvariant` on
 `AstNode`, the analog of `ILInstruction.CheckInvariant`, asserting per child:
 - `child.Parent == this` AND `this.GetChild(child.ChildIndex) == child` (parent/index round-trip);
 - child's runtime type is assignable to `GetChildSlot(child.ChildIndex).ChildType` (slot typing);
 - collection slots: elements occupy the slot's contiguous index range, in order, with matching
  `childIndex`;
 - strict tree: every node reachable exactly once (no shared subtrees);
 - (post-NRT, Part 11) every slot with `CSharpSlotInfo.IsOptional == false` is non-null -- this is
  the enforcement of "syntactically required" at the checkpoint (transforms may transiently empty it
  between checkpoints; here it must be filled).
 Recurse into all children. Wire it into the C# transform pipeline to run after every
 `IAstTransform` in DEBUG (mirroring the IL pipeline's per-transform `CheckInvariant`). This is
 both a permanent correctness guard for transforms ("we want every transform to behave") and a
 migration accelerator -- a transform or a mis-generated slot that corrupts the tree fails at the
 exact transform, not as a downstream decompilation diff. Directly guards corners C1/C6.
 ==========================================================================================
 # PART II -- IMPLEMENTATION STEPS
 ==========================================================================================
 Guiding constraints: every phase ends green on the decompiler suites (`dotnet test --solution
 ILSpy.sln --report-trx`, prefixed `OPENSSL_ENABLE_SHA1_SIGNATURES=1`); the round-trip + Pretty
 suites are the real behavioral gate for the transforms/printer. The base-storage flip is the one
 unavoidably coordinated change; everything else is staged.
 **EXECUTION ORDER (revised 2026-06-13): Phase 4 -> Phase 5 -> Phase 3c.** The storage flip (3c) was
 originally sequenced before token-drop (4) and comments-to-trivia (5), but at the flip the still-live
 positional non-slot children (`Roles.Comment` / `Roles.Comma` / `Roles.PreProcessorDirective`,
 interleaved in document order by `InsertSpecialsDecorator` and attached to many no-collection nodes:
 CastExpression, ExpressionStatement, VariableInitializer, Attribute, AstType, ErrorExpression) have
 nowhere to live in a pure-slot tree. Landing Phase 4 + Phase 5 first removes every non-slot child, so
 the 3c flip becomes a clean pure-slot conversion with no interim sidecar. Trade-off accepted: the
 O(1)/perf payoff is deferred to 3c, and Phase 4's `InsertMissingTokensDecorator` location-path gating
 work is front-loaded. Phases 6-9 follow 3c as before.
 ## Phase 0 -- Prerequisite (DONE)
 Reconciled `ast-source-generator` onto master; generator builds and runs; visitor/DoMatch/null/
 pattern generation in place. (Branch `ast-source-generator-rebased`, pushed.)
 ## Phase S -- De-risking spikes (DONE)
 Ran E1-E4 in isolated worktrees (off the warmup commit `76a965ae2`); all four returned
 high-confidence verdicts. Prototype commits kept for reference (not landed):
 - **E1 migration mechanic -> BRIDGE** (commit `3050aee76`): slot accessors over the linked list,
  family-by-family conversion, single later storage flip. Pretty output byte-identical to baseline
  (667/667); DEBUG slot-order==document-order invariant held across all 667 fixtures. Prerequisites
  surfaced: `Role.NullObjectUntyped`; collection slots need a separate design (ride to the flip on
  `AstNodeCollection`); bridge gives the API but not O(1) perf (that arrives at the flip).
 - **E2 collection perf -> PLAIN EAGER renumber** (commit `dd58586b8`): the O(n^2) trap is
  pathological (front-insert with no ordinal reads) only; on realistic op mixes eager List-backed
  beats the linked list (whose positional Get/IndexOf are O(n)). Lazy dirty-watermark kept as a
  documented ~20-LOC fallback. No smarter scheme up front.
 - **E3 token-drop -> LOW blast radius** (commit `6e29b23ec`): dropping token slots is byte-neutral
  for the decompile-to-text printer (already scalar-driven). Real risk re-pointed to
  `InsertMissingTokensDecorator` on the AST-with-locations (GUI) path, which Pretty does NOT cover.
 - **E4 inheritance -> KEEP FLATTEN** (commit `352d1eb3c`): base-delegation is structurally
  incorrect -- inherited slots are non-contiguous in source order (`NameToken` after
  `PrivateImplementationType`), which a front-pinned base block cannot represent. Flatten is also
  smaller overall (family 158 vs 171 LOC).
 CheckInvariant (Phase 1) is brought up first so the spikes ran under it (the E1 invariant IS the
 Phase 3a slot-order guard).
 ## Phase 1 -- Slot base infrastructure + CheckInvariant (handwritten)
 Reimplement `AstNodeCollection<T>` over slots (List-backed, parent+slot, renumbering,
 `IReadOnlyList<T>`+explicit `IReadOnlyList<INode>`; preserve the used surface per Part I.1); add
 `CSharpSlotInfo`. Add to `AstNode`: `childIndex`, `Slot`, stored `StartLocation`/`EndLocation` +
 internal `SetLocations`, abstract `GetChildCount/GetChild/SetChild/GetChildSlot`, `Children` struct
 view, `SetChildNode<T>`, computed sibling nav, slot-based `Clone`, the `AddLeading/TrailingTrivia`
 side-channel. Drop `IFreezable`/`Freeze`/`IsFrozen`. Implement `CheckInvariant`
 (Part I.13) and wire it into the C# `IAstTransform` pipeline (the transform runner in
 `CSharpDecompiler`) under DEBUG, so it guards every later phase and the spikes. Keep the
 linked-list API temporarily for unconverted nodes (bridge), or build on a sub-branch -- see
 Phase 3 / spike E1.
 Gate: compiles; CheckInvariant runs green on current (pre-migration) output; no node migrated yet.
 ## Phase 2 -- Generator: slot storage emission
 Extend the generator to emit, from partial-property declarations: backing fields, property
 bodies, `CSharpSlotInfo` statics, and `GetChildCount/GetChild/SetChild/GetChildSlot` with the
 flat per-leaf dispatch + collection range arithmetic; adapt `Clone` to per-slot. Add a
 `SLOTAST` diagnostic for malformed declarations (mixed ordering, unsupported slot type).
 Gate: generator unit-builds; emits correct storage for a single pilot node.
 ## Phase 3 -- Family-by-family node migration to slots
 Convert node declarations from handwritten `GetChildByRole` properties to partial properties,
 one family per step, regenerating storage. Order (smallest/most-isolated first):
 1. Expressions (leaf-heavy, few inter-family deps).
 2. Statements.
 3. TypeMembers (EntityDeclaration contract + leaves; exercises inheritance composition).
 4. GeneralScope + types (AstType hierarchy, NamespaceDeclaration, etc.).
 5. Remaining (SyntaxTree, VariableDesignation, etc.).
 Mechanic: **BRIDGE** (decided by spike E1, prototype `3050aee76`; the coordinated flip is a
 non-reviewable big-bang because all nodes share `AstNode`'s firstChild/lastChild traversal and
 `AstNodeCollection` is hardwired to it). Three sub-steps:
 - **3a.** Land the `AstNode` slot layer (`SlotCount`/`GetSlotRole(i)`/`IsCollectionSlot(i)`,
  `GetChild(i)`/`SetChild(i)` over the linked list) PLUS the DEBUG slot-order==document-order
  invariant FIRST, as its own commit, wired into `CSharpOutputVisitor.StartNode` so it runs the
  whole suite on every PR. Prerequisite: add `Role.NullObjectUntyped` (`Role<T>` invariance blocks
  `(Role<AstNode>)role`).
 - **3b.** Convert node declarations to slot accessors family-by-family (Expressions -> Statements ->
  TypeMembers -> GeneralScope+types -> remaining). Each commit is small, mechanical, individually
  revertible, and gated by the assert + byte-identical Pretty suite. Extend the slot-order assert to
  optional/reordered-child families (modifiers, attributes, the EntityDeclaration interleave from E4)
  as they convert -- it is proven only for simple Expressions so far.
 - **3c.** Flip storage to real backing fields as ONE coordinated change once all families route
  through `GetChild`/`SetChild` -- linked list, `AstNodeCollection`, `Clone`, `StartLocation`/
  `EndLocation`, and sibling nav flip together. **Collection slots** are not covered by the
  single-slot bridge accessors (they stay on `GetChildrenByRole`/`AstNodeCollection` and move here) --
  they need their own design step. The bridge yields the slot API but NOT O(1) access; perf only
  arrives at 3c.
 Bridge is output-neutral in practice: E1 proved byte-identical Pretty output across all 667
 decompiled fixtures with the slot-order assert never tripping. The one known insertion-vs-schema
 divergence is the `TransformFieldAndConstructorInitializers` reordering hack; the assert is the tool
 that catches any other.
 Gate per family: full decompiler suite green; the slot-order==document-order assert holds for the
 converted family.
 ## Phase 4 -- Drop tokens
 Convert token-carried meaning to scalars (modifiers field, `AccessorKind`, `IsAsync`,
 `AttributeTarget`, operator enums already present); remove token child slots from declarations;
 delete `CSharpTokenNode`/`CSharpModifierToken`; move `SequencePointBuilder` brace points to
 `BlockStatement` start/end.
 **Effort re-pointed by spike E3** (the decompile-to-text printer is already scalar-driven, so the
 spacing/parens rework the design feared is near-zero -- E3 dropped `OperatorToken` +
 `ReturnToken`/`SemicolonToken` with byte-identical Pretty output, and `InsertRequiredSpacesDecorator`
 branches on token TEXT not Role). The real, Pretty-INVISIBLE coupling is the gating work:
 - **(1, gating)** `InsertMissingTokensDecorator` reconstructs `CSharpTokenNode`/`CSharpModifierToken`
  from the print stream and assigns Roles on the `CreateWriterThatSetsLocationsInAST` path (GUI
  navigation / source maps), NOT exercised by Pretty. Build regression coverage for that path and run
  it against the token-slot drop BEFORE committing the drop.
 - **(2)** `InsertSpecialsDecorator` role-keyed comment/preprocessor placement (low-risk: missing role
  just doesn't advance) -- add comment/directive-bearing fixtures.
 - `InsertParenthesesVisitor` / `InsertRequiredSpacesDecorator` / spacing: verified inert by E3, no
  change expected.
 Gate: Pretty + round-trip suites green; PDB sequence-point tests green; AND the
 AST-with-locations / token-navigation path green (Pretty alone does not exercise the at-risk
 component).
 ## Phase 5 -- Comments + directives (trivia), doc comments
 Add the `Trivia` base (`Comment : Trivia`, `PreProcessorDirective : Trivia`) and the
 `AddLeading/TrailingTrivia` side-channel on `AstNode` (Part I.6); add `DocumentationComment` + the
 `Documentation` slot on `EntityDeclaration`. Migrate the 6 comment-receiver categories to
 `AddLeadingTrivia`/`AddTrailingTrivia` on the annotated node (re-home the between-members comments
 onto the following member); add the interior-comment assert. Move `#define` to leading trivia on the
 `SyntaxTree` and rewrite `IntroduceUsingDeclarations` to read `syntaxTree.LeadingTrivia`. Rewrite
 `AddXmlDocumentationTransform` to set the `Documentation` slot; update the printer to emit leading
 trivia at `StartNode` / trailing at `EndNode` + the doc node; remove the old `Comment`/
 `Roles.Comment` interleaving + `InsertSpecialsDecorator` trivia path.
 Gate: Pretty suite green (comment/doc/define output identical).
 ## Phase 6 -- Pattern-matching engine port (TESTS FIRST)
 6.1 Add `ICSharpCode.Decompiler.Tests/PatternMatching/PatternMatchingTests.cs` (~18 NUnit cases,
    substrate-agnostic; green on the current engine first). Cases: single-node + captures,
    AnyNode/OrNull, NamedNode retrieval, Choice checkpoint-restore, Repeat min/max/greedy/
    backtrack-to-satisfy-trailing-fixed, Optional present/absent/at-end, Backreference,
    IdentifierExpressionBackreference (+ TypeArguments rejection), empty collections, nesting, and
    real `forPattern` + trimmed `forOnArrayPattern` from PatternStatementTransform.
 6.2 Slim `INode`: keep `IsNull`+`DoMatch`; drop `Role`/`FirstChild`/`NextSibling`; new hook
    `DoMatchCollection(IReadOnlyList<INode> other, int pos, Match, BacktrackingInfo)`.
 6.3 `Pattern.cs`: `PossibleMatch.NextOther` -> `int NextOtherIndex`; port the static core to
    list+index (delete role-skip loops; `null` -> `pos>=Count`; `NextSibling` -> `+1`; preserve
    the `stack.Count==patternStack.Count` invariant); new default instance hook
    `DoMatch(pos<Count? other[pos] : null, match)`; drop dead INode explicit members.
 6.4 `Repeat`/`OptionalNode` `DoMatchCollection` over index cursor (`DoMatch` overloads unchanged).
 6.5 `AstNodeCollection<T>.DoMatch(other, match) => Pattern.DoMatchCollection(this, other, match)`
    via the explicit `IReadOnlyList<INode>` view.
 6.6 `AstNode` INode impl: new list hook; remove `NextSibling`/`FirstChild`/old `DoMatchCollection`.
 6.7 Generator: `PatternPlaceholder.DoMatchCollection` new signature (member-type detection stays
    `"AstNodeCollection`1"` -- the type name is unchanged).
 6.8 `IdentifierExpressionBackreference`: replace `referenced.GetChildByRole(Roles.Identifier).Name`
    with `((IdentifierExpression)referenced).Identifier` guarded by `is`.
 Gate: 6.1 suite green after each sub-step; full decompiler suite green (transforms unchanged at
 the API level).
 ## Phase 7 -- Delete Role + consumer rewrites
 Delete `Role`/`Role<T>`/`Roles`/`TokenRole`/`GetChildByRole`/`SetChildByRole`/
 `GetChildrenByRole`/`node.Role`/`AddChild(role)`/`InsertChildBefore/After` (KEEP
 `AstNodeCollection<T>` -- reimplemented over slots, not deleted). Rewrite the ~14
 external `GetChildByRole`, ~43 `.Role` reads (-> `.Slot`), and the `AddChild`/`InsertChild*`/
 `MoveTo` sites (-> collection ops; delete the reordering hack). Remove the role bridge from
 Phase 1/3.
 Gate: compiles with no role references; full suite green; public-API diff reviewed (this is the
 intended break).
 ## Phase 8 -- Null-object -> NRT
 **Required/optional classification is NEW information** (the uniformly-null-object model records no
 such distinction), but the rule is objective: a slot is *syntactically required* iff the C# grammar
 cannot omit it -- mechanical proxy: `CSharpOutputVisitor` dereferences it unconditionally (no
 `if (x != null)` guard / alternate-token branch). Read the printer rather than judging per slot;
 `default-to-optional` only as a tie-breaker. Model: **backing field uniformly nullable**; required
 slot -> non-null accessor (`field!`, the static contract), optional -> nullable accessor; transforms
 may transiently empty any slot, and `CheckInvariant` enforces required-non-null at the
 between-transform checkpoints (so no special `Detach`/`Remove` rule).
 Generator: emit required vs optional accessors from the declared nullability (= `IsOptional`); stop
 emitting null-node classes + `hasNullNode`; `DoMatch`/matcher use `== null`. Delete the 18 `.Null`
 singletons + `AstNode.IsNull` + `INode.IsNull` + `VisitNullNode`. Consumer sweep: ~91 `.IsNull` ->
 `== null` (dead on required slots), ~99 `.Null` -> `null`/empty; output visitor +
 `DepthFirstAstVisitor` null-guards.
 **Classification rule, refined (verified):** "printer dereferences the slot unconditionally" is
 *necessary but not sufficient* for required -- the null-object's silent `VisitNullNode` no-op can
 mask a legitimately-empty slot. **Also check construction sites.** Confirmed landmine:
 `IndexerExpression.Target` is visited unconditionally (`CSharpOutputVisitor.cs:1012`) yet is
 intentionally empty in finished trees -- object initializers `new C { [i] = v }`
 (`CallBuilder.cs:1690`) and property-pattern index paths (`ExpressionBuilder.cs:3585`). So
 `Target` is **optional** (`Expression? Target`), and `VisitIndexerExpression` must guard it (or
 `Target?.AcceptVisitor`) once the null-object no-op is gone. Verified safe: `ErrorExpression` is
 always a complete top-level node (never a stand-in child, so error paths never empty a required
 slot); the truly-required slots (binary/assignment/cast/unary/condition/body) are always filled;
 the printer's explicit `IsNull` guards already mark the known optionals (`ReturnStatement.Expression`,
 `CaseLabel.Expression`, `ThrowStatement.Expression`, `IfElseStatement.FalseStatement`).
 `IndexerExpression.Target` is the **sole** null-`Target` case (verified: of ~17 construction sites
 only `CallBuilder.cs:1690` and `ExpressionBuilder.cs:3585` pass null; `MemberReferenceExpression`/
 `InvocationExpression` `Target` are never null-constructed -- `this.A` is an MRE with a
 `ThisReferenceExpression` target, bare `A` is an `IdentifierExpression`, so there is no implicit-`this`
 null-target). It exists only because an implicit-target indexer (dictionary/object initializer, and
 the property-pattern path) is *modeled* as `IndexerExpression` with a null target. Phase-8 choice:
 (A) classify `IndexerExpression.Target` optional and guard it in `VisitIndexerExpression` (cheap,
 honest); (B) give the implicit-target indexer its own node and keep `Target` required (removes the
 wart, adds a node type). Recommend (A).
 Gate: builds under `#nullable enable` with no new AST nullable warnings; full suite green;
 spot-check optional-child output (abstract methods, expression-bodied members, non-await
 using/foreach).
 ## Phase 9 -- Cleanup & niceties
 **Measured and dropped: bit-packing declared scalars into `flags` is not worth it; the `flags` field
 itself was removed instead.** The plan was to back each declared `bool` (1 bit) and *narrow
 non-`[Flags]` enum* (`BinaryOperatorType`/`UnaryOperatorType`/`AssignmentOperatorType` ~5-6 bits,
 `AccessorKind`, `VarianceModifier`, ...) with a mask+shift over `flags` instead of a field. Measuring
 it (per-thread allocation counting on representative nodes) showed ~zero benefit on the
 high-frequency nodes: the CLR already packs a node's bools and narrow enums into the object's existing
 alignment padding, so hand-packing reclaims slots that do not exist. `ParameterDeclaration` (3 bools +
 `ReferenceKind`) stayed 96 B and `BinaryOperatorExpression` (`BinaryOperatorType`) stayed 80 B after
 packing; only a node whose scalars happened to tip an 8-byte boundary with nothing to share padding
 with (`ComposedType`, 3 bools) dropped 8 B. With packing off the table, the `flags` word's only user
 was `Identifier.IsVerbatim`; that became a plain `bool` (free -- it lands in padding) and **the `flags`
 field and `AstNodeFlagsUsedBits` were removed**, which shrank *every* `AstNode` by 8 bytes (the field
 occupied its own aligned slot rather than pairing with `childIndex` as assumed; a minimal node
 measured 64 -> 56 B) -- a universal win the per-node packing never delivered. `[Flags]` `Modifiers`
 keeps its own field regardless. Remaining cleanup:
 delete dead decorators/visitor methods; optionally generate `SymbolKind` from the model; re-add
 the `Generators` project to `ILSpy.sln` (already done); regenerate lock files; final
 public-API-diff pass; update docs.
 ## Files (representative, not exhaustive)
 - Base/infra: `CSharp/Syntax/AstNode.cs`, new `CSharpSlotInfo.cs`, `AstNodeCollection.cs`
  (reimplemented over slots, not deleted), delete `Role.cs`/`Roles.cs`/`TokenRole.cs`/
  `CSharpTokenNode.cs`/`CSharpModifierToken.cs`.
 - Nodes: every file under `CSharp/Syntax/Expressions|Statements|TypeMembers|GeneralScope` +
  the type/`*AstType` files (partial-property conversion, generator-assisted).
 - Generator: `ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs`.
 - Matcher: `CSharp/Syntax/PatternMatching/*` (per Phase 6).
 - Printer/debug: `CSharp/OutputVisitor/CSharpOutputVisitor.cs`, `TextWriterTokenWriter.cs`,
  `InsertMissingTokensDecorator.cs`, `InsertParenthesesVisitor.cs`, `InsertSpecialsDecorator.cs`,
  `SequencePointBuilder.cs`, `DepthFirstAstVisitor.cs`.
 - Transforms: `AddXmlDocumentationTransform.cs`, `IntroduceUsingDeclarations.cs`,
  `TransformFieldAndConstructorInitializers.cs` (drop reordering hack), `PatternStatementTransform.cs`
  (no API change), plus the ~57 role/`GetChildByRole` consumer sites across `CSharp/`.
 - Tests: new `ICSharpCode.Decompiler.Tests/PatternMatching/PatternMatchingTests.cs`.
 ## Verification (end to end)
 - Per phase: full decompiler suite green (`dotnet test --solution ILSpy.sln --report-trx`,
  `OPENSSL_ENABLE_SHA1_SIGNATURES=1`); round-trip + Pretty are the behavioral gate; PDB
  sequence-point tests guard locations after the token drop.
 - Pattern engine: the new unit suite (Phase 6.1) green throughout.
 - Final: public-API-diff review (intended break: roles/tokens/null-objects removed); build clean
  under `#nullable enable`; ilspycmd smoke decompile of a few representative assemblies.
 ## Weird corners (honest risk list) & de-risking spikes
 C1. **The base-storage flip is not cleanly incremental** (highest risk). All ~110 nodes share
   `AstNode`, so storage cannot be half-slots/half-linked-list at runtime. The "bridge" (slot
   accessors implemented over the linked list, then flip to fields) is awkward precisely where
   the models differ most: `childIndex` is not naturally stored in a linked list, and sibling
   nav is computed differently. The "coordinated flip" avoids the awkward bridge but is a big-
   bang change. This single choice shapes the entire migration. -> SPIKE E1.
   RESOLVED (E1, `3050aee76`): BRIDGE. Byte-neutral on the tested families (667/667 Pretty
   identical, slot-order assert never tripped). Sibling nav stays on the linked list during the
   bridge; `childIndex`/O(1) access + `AstNodeCollection`/`Clone`/sibling-nav all flip together at
   3c. Prerequisite `Role.NullObjectUntyped`.
 C2. **Collection insert/remove renumbering is potentially O(n^2)** where the linked list was
   O(1). `AstNodeCollection<T>` renumbers following children's `childIndex` on every insert/remove.
   Large collections (a `SyntaxTree`'s members, a big `BlockStatement`) under insert-heavy
   transforms could regress decompile time. IL's `InstructionCollection` has the same model and
   is fine, but C# AST collections can be larger. -> SPIKE E2 (benchmark).
   RESOLVED (E2, `dd58586b8`) -- and the framing was inverted: the linked list is the *slower*
   model for realistic transforms (O(n) positional Get/IndexOf); plain eager renumber wins on every
   real op mix. The O(n^2) trap is front-insert-with-no-reads only and negligible in absolute terms.
   Decision: plain eager renumber, lazy dirty-watermark as a documented fallback.
 C3. **Printer fidelity after the token drop.** The spacing/parenthesis machinery
   (`InsertRequiredSpacesDecorator`, `InsertParenthesesVisitor`, `InsertMissingTokensDecorator`)
   may branch on token roles/presence; dropping token nodes could shift output whitespace or
   parens subtly. Pretty tests are exact-text, so any drift fails loudly -- good, but the blast
   radius is unknown until tried. -> SPIKE E3 (drop tokens on a slice, diff Pretty).
   RESOLVED (E3, `6e29b23ec`): the spacing/parens concern is inert -- the printer is already
   scalar-driven; dropping tokens was byte-neutral. The residual risk MOVED to
   `InsertMissingTokensDecorator` on the AST-with-locations (GUI) path, which Pretty does not
   exercise -- that is now the Phase 4 gating item.
 C4. **Inheritance re-declaration.** The chosen flatten model makes each of ~30 declarations
   re-declare inherited contract slots (`override partial ... Attributes`). Repetitive, and a
   mistake (wrong order/omission) is a silent document-order bug. The base-delegation
   alternative trades this for offset arithmetic + non-sealed dispatch. -> SPIKE E4 (compare on
   TypeMembers).
   RESOLVED (E4, `352d1eb3c`): KEEP FLATTEN. Base-delegation is structurally incorrect here
   (inherited slots are non-contiguous in source order -- `NameToken` after
   `PrivateImplementationType` -- which a front-pinned base block cannot represent). The flatten
   mis-order risk is generator-controlled (owns ordering + per-leaf contract-coverage self-check).
 C5. **Pattern placeholders in collection-initializer syntax** (`Statements = { new Repeat(...) }`)
   must implicitly convert `Pattern -> Statement -> PatternPlaceholder` and `Add` to a
   `AstNodeCollection<T>`. Lower risk -- the Phase 6 tests-first approach exercises it directly.
 C6. **Transform idioms**: `Detach`/`MoveTo`/`ReplaceWith`/cross-collection moves get subtle
   slot-model rewrites; the renumbering must stay correct under move-then-readd. Covered by the
   suite, but a source of fiddly bugs.
 C7. **NRT required/optional classification** (Phase 8): the current null-object model records no
   such distinction. RESOLVED via R7 -- the rule is *syntactically required* (the C# grammar is the
   objective oracle; the mechanical proxy is "`CSharpOutputVisitor` dereferences the slot
   unconditionally" -- necessary but not sufficient: also check construction sites, since the
   null-object no-op masks intentionally-empty slots, e.g. `IndexerExpression.Target`). Required ->
   non-null accessor (the static contract); optional -> `T?`. Backing
   fields are uniformly nullable (transform-friendly); `CheckInvariant` enforces required-non-null at
   between-transform checkpoints. default-to-optional only as tie-breaker. Lowered from "main NRT
   risk" to mechanical work.
 De-risking spikes (each isolated, parallel-worktree-friendly):
 - **E1 -- migration mechanic**: implement the Expressions family both ways (bridge over the
  linked list vs coordinated flip to fields) and compare pain + reviewability. Decides Phase 3.
 - **E2 -- collection perf**: micro-benchmark `AstNodeCollection<T>` insert/remove vs the linked list
  on a large method/file; decompile a big assembly and compare wall-clock. Decides whether
  renumbering needs a smarter scheme (e.g. lazy/index-on-demand).
 - **E3 -- token-drop printer probe**: drop tokens on one expression + one statement, run the
  Pretty suite, inspect the diff to size the spacing/parens rework.
 - **E4 -- inheritance shape**: flatten vs base-delegation on TypeMembers; compare generated-code
  volume + ergonomics; confirm or revisit the flatten decision.
 ## Open items
 - Phase 3 mechanic: RESOLVED to BRIDGE by spike E1 (see Phase 3 / Phase S).
 - Collection-renumber scheme: RESOLVED to plain eager + documented lazy fallback by spike E2.
 - `SymbolKind` generation (Phase 9) is optional; flags bit-packing (Phase 9) measured and dropped --
  the `flags` field was removed instead (-8 B/node), see Phase 9.
 - Remaining within-plan probes (not decisions): extend the slot-order assert to optional/reordered-
  child families before the 3c flip (E1/N2); add regression coverage for the
  `InsertMissingTokensDecorator` location path before the token drop (E3/N1); re-time a CoreLib
  decompile after 3c against the ~33 s baseline (E2).
 - See PART III for the full review-findings backlog (the items below were folded in there).
 - **Post-rewrite (future): audit the AST against Roslyn's model and remove warts.** The slot model
  + trivia channel + required/optional nullability already converge on Roslyn's shape, so a parity
  pass becomes tractable once the rewrite lands. Known seed: `IndexerExpression` with null `Target`
  for implicit-target indexers (Roslyn: a distinct `ImplicitElementAccess`). Other candidates to
  check: object-initializer member names (bare `IdentifierExpression` vs an assignment shape),
  node-kind granularity where one node + a flag collapses distinct C# constructs. Gated on the
  rewrite being done; not a near-term task.
 ==========================================================================================
 # PART III -- REVIEW FINDINGS & RESOLUTIONS (added 2026-06-13)
 ==========================================================================================
 A code-grounded review of PARTS I/II surfaced the following holes, gaps, and decisions.
 Each is a tracked task; the "Amends" line names the section to update when the task is handled.
 Ordered as we will handle them.
 ## R0 (task #4) -- Delete `IFreezable`/`Freeze`/`IsFrozen` [LEAD, decided]
 **Finding (verified):** `Freeze`/`IsFrozen`/`IFreezable` is entirely confined to `AstNode.cs`.
 No transform, builder, or consumer ever calls `Freeze()` (the only `.Freeze()` in the tree is an
 unrelated Avalonia brush in `CSharpHighlightingTokenWriter.cs`). The generated null-node singletons
 do not freeze themselves either, so every `IsFrozen` guard in `SetChild`/`AddChild` only ever
 evaluates false. The decompiler is single-threaded; nothing shares or freezes nodes.
 **Decision:** Drop the whole `IFreezable` apparatus in this rewrite (it is exactly the NRefactory
 full-fidelity baggage the Context section says to shed). Consequences to fold in:
 - `AstNode` no longer implements `IFreezable`; remove `Freeze`/`IsFrozen`/`frozenBit`.
 - `SetChildNode` (Part I.1) drops its frozen check.
 - `CheckInvariant` (Part I.13) drops the "frozen consistency" clause.
 - The freed flag bit is moot -- the `flags` word loses the role index anyway.
 **Amends:** Part I.1, Part I.13.
 ## R1 (task #1) -- Comment carriers beyond statement/expression [base-model; before Phase 1/5]
 **Finding (verified, full enumeration):** today a `Comment` attaches via `Roles.Comment` to almost
 any node (the `InsertSpecialsDecorator` interleaves them at print time). `CommentStatement` +
 `CommentedExpression` (Part I.6) cover only 2 of the **6** receiver categories in use. (Doc
 comments -- `CommentType.Documentation` from `AddXmlDocumentationTransform` -- are separate and go
 to the `Documentation` slot.)
 Covered: block statement-list (-> `CommentStatement`: `StatementBuilder.cs:494,1362,1415`,
 `CSharpDecompiler.cs:1436,2049,2103`, `ExpressionBuilder.cs:2445,2623`); on an `Expression`
 (-> `CommentedExpression`: `CallBuilder.cs:567`, `ErrorExpression.cs:57`,
 `ExpressionBuilder.cs:2065,4967`).
 **Not covered -- six gaps:**
 - A. trailing comment on a **non-block `Statement`** -- `StatementBuilder.cs:140,1526,1544`.
 - B. **`TypeDeclaration.Members`** (sibling of `EntityDeclaration`) -- `CSharpDecompiler.cs:1698,
  1727` ("error: enumerator has no value", "nested types are not permitted"). A `CommentStatement`
  is a `Statement`, so it cannot live in a `Members` collection -- the one position that cannot be
  faked with the two proposed nodes. (Corrects an earlier mistaken "never between members" claim.)
 - C. on an **`AstType`** -- `DeclareVariables.cs:616` ("pinned"), `TypeSystemAstBuilder.cs:414`.
 - D. on a **`VariableInitializer`** -- `CSharpDecompiler.cs:2322` (the "could not decompile" field
  message).
 - E. on an **`Attribute`** -- `TypeSystemAstBuilder.cs:813` ("Could not decode attribute
  arguments.").
 - F. on an **`Accessor`** (an `EntityDeclaration`, printed inline) -- `TypeSystemAstBuilder.cs:2123`
  ("init").
 **RESOLVED -- comments become node trivia (leading/trailing), off the child model.**
 (LATER REFINED -- see authoritative Part I.6: the element type is a `Trivia` base covering BOTH
 `Comment` AND `PreProcessorDirective` (directives folded in, not separate marker nodes); storage is
 the existing annotation channel via a lazy `NodeTrivia` holder -- NO per-node fields -- so `Clone`
 is automatic via `CloneAnnotations`. The original sketch below predates those two refinements.)
 - **Storage:** a single per-node trivia collection on `AstNode`, a side-channel like annotations --
  *outside* the flattened child-index space (so no `Comments` slot bloats every leaf's
  `GetChild/SetChild` dispatch, no pattern-matcher interaction, `CheckInvariant`'s child-range
  arithmetic stays clean). Each entry is tagged leading or trailing.
 - **Public API:** `AddLeadingTrivia(Trivia)` / `AddTrailingTrivia(Trivia)` -- the
  leading/trailing distinction is encoded by which method is called; callers never set a flag.
  Read-back via filtered `LeadingTrivia` / `TrailingTrivia` views.
 - **Print mechanic:** the output visitor emits leading trivia at `StartNode` (before the node's
  children) and trailing at `EndNode` (after), preserving insertion order within each group. No
  interleaving into the sibling sequence, no anchor index. Replaces `InsertSpecialsDecorator`'s
  `Roles.Comment` interleaving + the `AstNode.cs` comment-skip walks.
 - **`Clone`** copies trivia alongside `CloneAnnotations()`; `CheckInvariant` treats trivia as
  outside the child range (parent set, no `ChildIndex`).
 - **Doc comments** keep the dedicated `Documentation` slot (structural, rides with the member);
  they are not folded into this trivia channel.
 - **Migration mapping** (the six categories -> the node each comment annotates):
  A `StatementBuilder.cs:140` -> `AddTrailingTrivia`; `:1526,1544` -> `AddLeadingTrivia`.
  B `CSharpDecompiler.cs:1698,1727` -> re-home from `typeDecl.InsertChildBefore(member,...)` to
  `member.AddLeadingTrivia(...)`.
  C `DeclareVariables.cs:616`, `TypeSystemAstBuilder.cs:414` -> `AddLeadingTrivia` on the `AstType`.
  D `CSharpDecompiler.cs:2322` -> `AddTrailingTrivia` on the `VariableInitializer`.
  E `TypeSystemAstBuilder.cs:813` -> `AddTrailingTrivia` on the `Attribute`.
  F `TypeSystemAstBuilder.cs:2123` -> `AddTrailingTrivia` on the `Accessor`.
 - **Implementation guard:** assert no migrated site needs an *interior* comment (between two real
  children of the same node where neither is the annotated node); the enumeration shows all are
  first/last/leading-on-member, so the model is sufficient -- the assert catches a regression.
 **Amends:** Part I.6 (rewrite "comments as slot children" -> trivia channel), Phase 5 (drop
 `InsertSpecialsDecorator` trivia path; add the `AddLeading/TrailingTrivia` migration).
 ## R2 (task #2) -- Locations: keep `StartLocation`/`EndLocation`, drop the `Span` tuple [RESOLVED]
 **Finding (verified, `AstNode.cs:183-199`):** today `StartLocation`/`EndLocation` are *virtual,
 computed by recursion* -- a non-leaf returns `firstChild.StartLocation` / `lastChild.EndLocation`,
 bottoming out at the **token leaves** (`CSharpTokenNode` etc.), which store the real location
 written at print time by `InsertMissingTokensDecorator` (`node.Location = ...`). The Phase-4 token
 drop removes those leaves, so the recursion loses its base case -- *that* is the only reason
 Part I.7 proposed explicit per-node storage. The stored values are still exactly
 `(TextLocation start, TextLocation end)`.
 **Decision -- no `Span` tuple; keep the two existing properties:**
 - **Public API:** keep `StartLocation` / `EndLocation` (each `TextLocation`). This *is* the facade
  resolution -- zero churn across the ~33 readers (`SequencePointBuilder` + the `GetNodeAt`/
  `GetNodesBetween`/`Contains` tree-nav in `AstNode` itself), and consistent with #11. A `Span`
  tuple would force rewriting all 33 sites for no semantic gain. `Span` is removed from the design.
 - **Storage:** two backing `TextLocation` fields (a private struct is an impl detail, not exposed)
  with an `internal` setter / `SetLocations(start, end)`, written by the existing `StartNode`/
  `EndNode` + `WrapInWriterThatSetsLocationsInAST` path Part I.7 already keeps.
 - **Drop the `virtual` compute-from-children recursion:** every printed node is bracketed by
  `StartNode`/`EndNode` and stores its own span directly; nothing needs to derive from children
  once leaves no longer carry locations.
 - **Timing -- no computed fallback needed:** locations are only meaningful after the format pass
  *today too* (token-leaf locations are `Empty` until `InsertMissingTokensDecorator` runs). All
  ~33 readers are post-format (`SequencePointBuilder` on formatted output; UI nav on the printed
  tree); no mid-pipeline reader exists.
 - Token-drop consequence (already in Part I.7): `SequencePointBuilder`'s brace breakpoints move
  from `LBraceToken`/`RBraceToken` locations to `BlockStatement` start/end.
 **Amends:** Part I.1 (replace the `Span` property with stored `StartLocation`/`EndLocation` +
 internal setter; drop the `virtual` recursion), Part I.7 (s/`Span`/`StartLocation`+`EndLocation`/).
 ## R3 (task #3) -- Full `NodeCollection<T>` API surface [before Phase 1]
 **Finding (verified):** Parts I.1/I.4 mention only Add/Insert/remove. The actually-used
 `AstNodeCollection<T>` surface a replacement must cover: `MoveTo` (cross-collection move -- the C6
 case), `ReplaceWith(IEnumerable)`, `AddRange`, `InsertBefore`/`InsertAfter(item,...)` (by reference
 node, **not** index), `Detach`, `AcceptVisitor`, indexer/`Count`/`Contains`/`CopyTo`, and
 `FirstOrNullObject`/`LastOrNullObject`. The last two are entangled with Phase 8 -- they become
 `FirstOrDefault`-returning-`null` once null objects die.
 **Amends:** Part I.1.
 ## R4 (task #5) -- `AttributeTarget` enum completeness [before Phase 4]
 **Finding (verified):** Part I.5 converts the string target to an enum, but the old commented-out
 enum `{None,Assembly,Module,Type,Param,Field,Return,Method}` omits `property`, which
 `TransformFieldAndConstructorInitializers.cs` actually sets (`= "property"`).
 **Action:** enumerate the complete set (assembly, module, type, param, field, return, method,
 property, event -- verify `event`) before generating.
 **Amends:** Part I.5.
 ## R5 (task #6) -- Add `SetChildByRole` to the deletion list [Phase 7]
 **Finding (verified):** Part I.9 lists `GetChildByRole`/`GetChildrenByRole`/`AddChild`/
 `InsertChild*` but omits `SetChildByRole` (used in `AttributeSection.AttributeTarget` setter, etc.).
 **Amends:** Part I.9, Phase 7.
 ## R6 (task #7) -- Empty single-slot index occupancy [doc clarity; Phase 1/8]
 **Finding:** For O(1) fixed-slot access plus the Part I.13 round-trip invariant
 (`GetChild(child.ChildIndex) == child`), an empty optional **single** slot must still occupy its
 flattened index (return `null` at a fixed position); only **collection** slots vary the offset.
 Part I.4 only describes collections shifting offsets, leaving two readings.
 **Action:** state that single slots are always width-1, so the flattened index space is stable
 across the Phase 8 null->nullable transition.
 **Amends:** Part I.4.
 ## R7 (task #8) -- Required/optional model: nullable field, required accessor, checkpoint-enforced [RESOLVED]
 **Question raised:** are any slots actually "required"? Yes -- binary operands, `IfStatement.Condition`/
 `TrueStatement`, loop condition/body, `CastExpression.Type`/`Expression`, assignment sides,
 `MemberReferenceExpression`/`InvocationExpression.Target`, etc. are always filled in a finished tree
 and an empty one is a bug. (Genuinely optional: `ReturnStatement.Expression`, `MethodDeclaration.Body`,
 `IfStatement.FalseStatement`, property accessors.) But every node has a parameterless ctor + settable
 slots and the decompiler builds incrementally (`BinaryOperatorExpression.cs:63-91`), so a required
 field is transiently empty during construction -- "required" cannot mean non-null for the node's whole
 lifetime.
 **Principle (from the user):** during a transform every slot is effectively optional (may be empty);
 `CheckInvariant` -- which runs *between* transforms -- guarantees the required ones exist at those
 checkpoints.
 **Resolution -- split storage from contract:**
 - **Backing field is always nullable.** This is "optional during a transform": any slot may be
  detached/left empty mid-pipeline; transforms never fight the type system; construction stays
  incremental; no `= null!` lie. (Note: this means single optional/required slots both store a
  nullable field -- the field is uniformly nullable; only the *accessor* differs.)
 - **Accessor nullability IS the required marker** (single source of truth, per Part I.2: declared
  nullability = `IsOptional`). Required -> non-null accessor `Expression Left` (getter returns
  `field!` -- throws if read while empty); optional -> `Expression? Body`.
 - **`CheckInvariant` reads `CSharpSlotInfo.IsOptional`** and asserts required slots non-null at each
  post-transform checkpoint -- "make sure the nodes exist," at the stable boundaries, not every instant.
 - **Contract:** between transforms the tree is whole (required filled); within a transform anything
  goes (field tolerates null). Reading a transiently-empty required child throws at the bug site;
  forgetting to refill is caught by `CheckInvariant` at the boundary.
 **This closes #8:** no special `Detach()`/`Remove()` rule -- transient-null is allowed by the field,
 existence is enforced at checkpoints, misuse throws at read.
 **Payoff preserved:** ergonomic non-null reads at the hundreds of `node.Left.AcceptVisitor(...)`
 sites (no `T?` warning flood -- the only reason null objects existed); still deletes the 18 null-node
 classes + `IsNull`/`VisitNullNode` plumbing.
 **Classification rule -- "syntactically required" (objective oracle, replaces per-slot judgment):**
 a slot is required (non-null accessor = the static contract) iff the **C# grammar** cannot omit it
 (`BinaryOperatorExpression.Left`/`Right`, `IfStatement.Condition`/`TrueStatement`, loop condition+
 body, `CastExpression.Type`/`Expression`, assignment sides, `...Target`); optional iff the grammar
 allows it absent (`ReturnStatement.Expression`, `MethodDeclaration.Body`, `IfStatement.FalseStatement`,
 `VariableInitializer` initializer, property accessors, attribute args). **Mechanical proxy:** a slot
 is required iff `CSharpOutputVisitor` dereferences it *unconditionally* (no `if (x != null)` guard /
 no alternate-token branch). The printer is ground truth -- it is the consumer that would NRE, and its
 access pattern necessarily matches the grammar -- so Phase 8 reads the output visitor rather than
 guessing. This de-risks C7. `default-to-optional` survives only as the tie-breaker for a genuinely
 ambiguous slot.
 **Residual:** classification is still per-slot work, but now objective and single-homed (accessor
 nullability feeds both the type and `CheckInvariant`).
 **Verified (and rule refined):** the "unconditional-deref" proxy is necessary but not sufficient --
 the null-object no-op can mask a legitimately-empty slot, so construction sites must also be checked.
 Landmine found: `IndexerExpression.Target` is visited unconditionally yet intentionally empty in
 object initializers (`CallBuilder.cs:1690`) and property-pattern index paths
 (`ExpressionBuilder.cs:3585`) -> classify **optional**, and guard it in `VisitIndexerExpression`.
 This is the **sole** null-`Target` case: `MemberReferenceExpression`/`InvocationExpression` `Target`
 are never null-constructed (`this.A` is an MRE with a `ThisReferenceExpression` target, bare `A` is
 an `IdentifierExpression` -- no implicit-`this` null-target), so both stay required. `ErrorExpression`
 is always a complete top-level node (never a stand-in child), so error paths never empty a required
 slot. The IndexerExpression null-target is a modeling wart (implicit-target indexer as
 `IndexerExpression`); Phase-8 may either make `Target` optional (recommended) or give the
 implicit-target indexer its own node.
 **Amends:** Part I.11 (nullable field + required-accessor model; drop "required slot = non-null
 field"), C7, Part I.13 (the "required slots non-null" clause sources `IsOptional`), Phase 8.
 ## R8 (task #9) -- The bridge is not output-neutral [Phase 3]
 **Finding:** The Phase 3a bridge maps `GetChild(i)` to "the i-th role-ordered child," but
 linked-list children are in **insertion** order while slot order is **schema** order. The moment a
 family converts, the printer (walking `Children`) sees schema order. Today these coincide *except*
 the `TransformFieldAndConstructorInitializers` reordering hack -- so the bridge should be neutral,
 but any other latent insertion-vs-schema divergence surfaces as a Pretty diff at **bridge** time,
 not flip time. The per-family gate ("slot accessors agree with old role accessors") checks per-role
 identity, not document order.
 **Action:** lean on the Pretty suite at bridge time, or add a document-order assertion to the gate.
 **CONFIRMED by E1 (`3050aee76`):** across all 667 decompiled fixtures the bridge produced
 byte-identical Pretty output and the document-order assert never tripped for the tested families --
 so the gate IS the slot-order==document-order invariant of Phase 3a, prototyped and proven for
 simple Expressions. Still to do: extend the assert to optional/reordered-child families
 (modifiers/attributes, the EntityDeclaration interleave from E4) before the 3c flip.
 **Amends:** C1, Phase 3.
 ## R10 (task #11) -- Keep original type names; drop the `NodeCollection` rename [decided]
 **Finding:** The design is asymmetric -- it keeps `AstNode` but renames `AstNodeCollection<T>` ->
 `NodeCollection<T>` to mirror IL's `InstructionCollection<T>` (Parts I.1, I.9, I.10, Phase
 6.5/6.7, Phase 7, Files, R3).
 **Decision:** Keep **both** original names. Reimplement `AstNodeCollection<T>` over slots
 (list-backed, parent+slot, renumbering) rather than delete+rename it. Rationale: (1) renaming a
 heavily-referenced public type is a gratuitous API break on top of the necessary roles/tokens/
 nulls removal, against `reduce-api-diff` and noisier in the Phase 7/9 API-diff gate; (2) `AstNode`/
 `AstNodeCollection<T>`/`AstType` are a name family -- keeping `AstNode` while renaming the
 collection breaks the symmetry; (3) the IL parallel is an implementation detail, not a name.
 **New public names that DO stay:** `CSharpSlotInfo` (no prior type) and `node.Slot` (replaces
 `node.Role`).
 **Sweep:** replace every `NodeCollection<T>` with `AstNodeCollection<T>` and change "delete
 `AstNodeCollection<T>`" to "reimplement over slots" in Parts I.1, I.9, I.10, Phase 6.5/6.7,
 Phase 7, the Files list, and R3 above.
 **Amends:** Parts I.1, I.9, I.10, Phase 6, Phase 7, Files.
 ## R9 (task #10) -- Resolved open items (recorded)
 - **Directives: CONFIRMED no bracketing.** Only `#define` is emitted today; region/if/endif/pragma
  are never constructed (the printer's generic `WritePreProcessorDirective` *can* emit them, but no
  site does). (LATER REFINED -- see Part I.6: directives are NOT separate marker nodes; they are
  folded into the leading/trailing `Trivia` channel alongside comments. `#define` -> leading trivia
  on the `SyntaxTree`. The no-bracketing finding is what makes trivia sufficient.)
 - **Partial properties / LangVersion: non-issue.** `ICSharpCode.Decompiler` is single-target
  `netstandard2.0` with `<LangVersion>14</LangVersion>`; partial properties (C# 13+) are
  compile-time and available. (The lib is *not* multi-targeted, contrary to the CLAUDE.md framing;
  the `init`-breakage constraint is a `netstandard2.0`-runtime `IsExternalInit` concern, orthogonal
  to this.)
 - **Companion proof file** (`~/.claude/plans/rustling-bubbling-oasis-...md`) is load-bearing for
  Phase 6 -- confirm it still exists and matches this revision before starting Phase 6.
--- a/Spikes/phaseS-workflow.js
+++ b/Spikes/phaseS-workflow.js
@ -1,166 +0,0 @@
 export const meta = {
  name: 'phase-s-spikes',
  description: 'Phase S de-risking spikes (E1-E4) for the slot-based C# AST rewrite, in isolated worktrees, then synthesis',
  phases: [
    { title: 'Spikes', detail: 'E1 migration mechanic, E2 collection perf, E3 token-drop printer, E4 inheritance shape' },
    { title: 'Synthesis', detail: 'fold spike findings into the Phase 1/3 decisions' },
  ],
 }
 // Shared context handed to every spike: build discipline + branch state.
 // (The design doc ROLES_FREE_SLOT_AST_DESIGN.md is untracked, so worktrees won't have it;
 //  each prompt carries the spike's relevant slice inline.)
 const COMMON = `
 Repo: ICSharpCode ILSpy decompiler. You are on a fresh git worktree branched off commit 76a965ae2
 ("Drop dead IFreezable/Freeze apparatus from the C# AST"). The C# AST lives in
 ICSharpCode.Decompiler/CSharp/Syntax/ and currently uses the NRefactory role + doubly-linked-list
 child model (per-node firstChild/nextSibling, each child tagged with a Role; GetChildByRole is a
 linear scan). The goal of the larger rewrite is a slot-based model (fixed source-ordered slots,
 O(1) access) generated by the Roslyn incremental generator in
 ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs.
 BUILD DISCIPLINE (critical on this box):
 - Prefix every dotnet build/test with  OPENSSL_ENABLE_SHA1_SIGNATURES=1
 - Pass  -p:RestoreEnablePackagePruning=false  on restore/build to avoid pruning packages.lock.json.
 - Do NOT commit packages.lock.json changes. Keep your prototype commits to source only.
 - The decompiler lib is single-target netstandard2.0, LangVersion 14.
 TEST SETUP (required before running ANY decompiler test in your worktree):
 - The ILSpy-tests submodule is uninitialized; the test harness needs <repo>/ILSpy-tests/nuget to
  exist (matrix compiler / reference-assembly cache) or every test Assert.Fails at fixture setup.
  Run once in your worktree root:  ln -s /home/siegfried/Projects/ILSpy/ILSpy-tests/nuget ILSpy-tests/nuget
 - That cache is INCOMPLETE and there is NO network: matrix configs needing absent packages
  (UseRoslyn4_14_0, UseRoslynLatest, TargetNet40, ...) fail at COMPILE setup with a Path.Exists
  assert in RefAssembliesToolset.GetPath. Those failures are ENVIRONMENTAL, not decompiler bugs --
  ignore them and focus on the toolset configs that actually compile (the default/cached compiler;
  ~1195 cases run today). Filter your test runs accordingly and never attribute a Path.Exists /
  RefAssembliesToolset failure to a code change.
 This is a SPIKE: prototype only enough to answer the question with evidence. Commit your prototype
 in your worktree (so the diff is inspectable) but it will NOT be merged. Be concrete and quantitative.
 `
 const FINDINGS = {
  type: 'object',
  additionalProperties: false,
  required: ['spike', 'question', 'whatIDid', 'findings', 'recommendation', 'confidence'],
  properties: {
    spike: { type: 'string' },
    question: { type: 'string', description: 'the decision this spike informs' },
    whatIDid: { type: 'string' },
    findings: { type: 'string', description: 'key observations, with file refs and numbers where relevant' },
    recommendation: { type: 'string' },
    confidence: { type: 'string', enum: ['low', 'medium', 'high'] },
    metrics: { type: 'string', description: 'concrete numbers: LOC, diff size, perf timings, generated-code volume; "n/a" if none' },
    prototypeCommit: { type: 'string', description: 'sha of the prototype commit in this worktree, or "none"' },
    risksFound: { type: 'string', description: 'anything that surprised you or threatens the design' },
  },
 }
 const SPIKES = [
  {
    key: 'E1', label: 'E1-migration-mechanic',
    prompt: `${COMMON}
 SPIKE E1 -- migration mechanic (gates the Phase 3 plan; HIGHEST VALUE).
 Because all ~110 nodes share AstNode, the base storage cannot be half-slots/half-linked-list at
 runtime. Two candidate mechanics:
 (a) BRIDGE: implement slot accessors (GetChild(i)/SetChild/GetChildCount) OVER the existing linked
     list first -- GetChild(i) returns the i-th child in slot/schema order (map slot index -> Role,
     then find that role's child/children). Convert node *declarations* to partial properties
     family-by-family while storage stays linked-list, then a single later "flip to fields" step.
 (b) COORDINATED FLIP: convert storage to real backing fields in one coordinated change on a
     sub-branch, generator-assisted, per-family commits.
 Take the Expressions family as the testbed. Actually PROTOTYPE the bridge (a) for 2-3 representative
 nodes: BinaryOperatorExpression (two single slots), InvocationExpression (single slot + a collection
 slot), and IdentifierExpression (leaf). Implement the slot accessors over the linked list, add a
 transitional debug assert that slot order == document order, build green. Then assess what the
 coordinated-flip (b) would require for the same nodes (sketch, don't fully build). Report: which is
 more reviewable, where the bridge is awkward (childIndex over a linked list, sibling nav), whether
 document-order diverges from insertion order anywhere you touched, and a clear recommendation for
 the Phase 3 mechanic with confidence.`,
  },
  {
    key: 'E2', label: 'E2-collection-perf',
    prompt: `${COMMON}
 SPIKE E2 -- collection insert/remove renumbering perf (decides whether AstNodeCollection<T> needs a
 smarter index scheme). The slot model gives each node a flattened childIndex; a List-backed
 AstNodeCollection<T> renumbers following children's childIndex on every insert/remove -> potentially
 O(n^2) on large collections (a SyntaxTree's members, a big BlockStatement) under insert-heavy
 transforms, where the linked list was O(1). Write a standalone micro-benchmark (a small console
 project or a BenchmarkDotNet harness under a temp folder) comparing: (1) List-backed with
 renumber-on-every-mutation, (2) a linked-list baseline, across realistic sizes (e.g. 100, 1000,
 5000 elements) and realistic op mixes (append-heavy build, mid-insert-heavy transform, detach/move).
 Run it, collect timings. Then, as a sanity check on real workloads, decompile a large assembly with
 the existing ilspycmd (build it: ICSharpCode.ILSpyCmd) and note wall-clock (this is the linked-list
 baseline today). Report concrete numbers and whether renumbering is a real risk or a non-issue, with
 a recommendation (e.g. plain renumber vs lazy/index-on-demand vs gap-buffer).`,
  },
  {
    key: 'E3', label: 'E3-token-drop-printer',
    prompt: `${COMMON}
 SPIKE E3 -- token-drop printer probe (sizes the Phase 4 spacing/parens rework). The rewrite drops
 CSharpTokenNode/CSharpModifierToken child slots; their meaning moves to scalars and the printer is
 handed text directly. Risk: the spacing/paren machinery (InsertRequiredSpacesDecorator,
 InsertParenthesesVisitor, InsertMissingTokensDecorator, InsertSpecialsDecorator) may branch on token
 roles/presence, so dropping tokens could shift whitespace/parens. PROBE: pick ONE expression node
 (e.g. BinaryOperatorExpression) and ONE statement, remove their token child slots / token-role
 plumbing in the printer path for just those, and run the Pretty test suite
 (ICSharpCode.Decompiler.Tests, the Pretty/exact-text cases) to see what breaks. Inspect the diffs.
 Report: how many Pretty cases drift, the categories of drift (spacing vs parens vs missing
 punctuation), which decorators branch on token roles, and an estimate of the Phase 4 blast radius
 with confidence. You do NOT need the full suite green; you need the size and shape of the fallout.`,
  },
  {
    key: 'E4', label: 'E4-inheritance-shape',
    prompt: `${COMMON}
 SPIKE E4 -- inheritance shape (confirm or revisit the "flatten" decision). The design flattens:
 base classes declare shared child-slot members abstract (the contract), each concrete leaf
 re-declares its FULL slot set as partial properties in document order (override-implementing the
 contract), and the generator emits one flat sealed dispatch per leaf -- no base-delegation, no
 offset arithmetic. The alternative is base-delegation (base owns its slots, derived adds an offset).
 Use the TypeMembers family (EntityDeclaration contract + leaves like MethodDeclaration,
 PropertyDeclaration, FieldDeclaration) as the testbed. Hand-write (or generator-sketch) BOTH shapes
 for 2-3 of those leaves: (1) flatten -- leaf re-declares Attributes/Modifiers/Name + its own slots,
 flat sealed GetChild/SetChild; (2) base-delegation -- base implements its slots, leaf delegates +
 offsets. Compare generated-code volume (lines per leaf), ergonomics, and error-proneness (the
 flatten model risks a silent document-order bug if a leaf mis-orders/omits a re-declared contract
 slot). Report which is better for the C# AST and whether to keep the flatten decision, with
 confidence.`,
  },
 ]
 phase('Spikes')
 const results = await parallel(SPIKES.map(s => () =>
  agent(s.prompt, { label: s.label, phase: 'Spikes', schema: FINDINGS, isolation: 'worktree' })
 ))
 const findings = results.filter(Boolean)
 log(`${findings.length}/4 spikes returned`)
 phase('Synthesis')
 const synthesis = await agent(
  `You are synthesizing the Phase S de-risking spikes for the slot-based C# AST rewrite. Here are the
 ${findings.length} spike findings as JSON:
 ${JSON.stringify(findings, null, 2)}
 Produce a synthesis that resolves the open Phase-1/Phase-3 decisions the spikes were meant to settle:
 - Phase 3 mechanic: bridge vs coordinated-flip (from E1).
 - AstNodeCollection<T> renumbering scheme: plain vs smarter (from E2).
 - Phase 4 token-drop blast radius and any reordering of work (from E3).
 - Inheritance shape: keep flatten vs switch to base-delegation (from E4).
 For each, give the decision, the evidence that drove it, and the confidence. Then list any NEW risks
 the spikes surfaced and any proposed edits to ROLES_FREE_SLOT_AST_DESIGN.md (by section). Be decisive
 where the evidence allows; flag where a spike was inconclusive and what further probe would settle it.`,
  { label: 'synthesis', phase: 'Synthesis', schema: {
    type: 'object', additionalProperties: false,
    required: ['decisions', 'newRisks', 'docEdits'],
    properties: {
      decisions: { type: 'string' },
      newRisks: { type: 'string' },
      docEdits: { type: 'string', description: 'proposed edits to the design doc, by section' },
      inconclusive: { type: 'string', description: 'spikes that did not settle their question + the follow-up probe' },
    },
  } }
 )
 return { findings, synthesis }