From 76840e655fa31c1bae039a62c6f7bfaa3d6569c2 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sun, 14 Jun 2026 10:49:16 +0200 Subject: [PATCH] Port the pattern matcher to list-indexed slot children The pattern matcher walked collections through INode.Role/FirstChild/ NextSibling, skipping siblings of a different role. Now that each AstNodeCollection is already the per-role child list, the engine matches two collections by list index, and INode sheds Role/FirstChild/NextSibling entirely. A collection exposes its IReadOnlyList view through a cached adapter rather than implementing the interface directly, so a typed collection does not become ambiguous for LINQ. Characterization tests pin the matcher's behavior first. Assisted-by: Claude:claude-opus-4-8:Claude Code --- .../DecompilerSyntaxTreeGenerator.cs | 4 +- .../ICSharpCode.Decompiler.Tests.csproj | 1 + .../PatternMatching/PatternMatchingTests.cs | 266 +++++ .../CSharp/Syntax/AstNode.cs | 16 +- .../CSharp/Syntax/AstNodeCollection.cs | 31 +- .../CSharp/Syntax/PatternMatching/INode.cs | 13 +- .../IdentifierExpressionBackreference.cs | 5 +- .../Syntax/PatternMatching/OptionalNode.cs | 7 +- .../CSharp/Syntax/PatternMatching/Pattern.cs | 64 +- .../CSharp/Syntax/PatternMatching/Repeat.cs | 12 +- PHASE_3C_TODO.md | 103 ++ ROLES_FREE_SLOT_AST_DESIGN.md | 919 ++++++++++++++++++ Spikes/phaseS-workflow.js | 166 ++++ 13 files changed, 1537 insertions(+), 70 deletions(-) create mode 100644 ICSharpCode.Decompiler.Tests/PatternMatching/PatternMatchingTests.cs create mode 100644 PHASE_3C_TODO.md create mode 100644 ROLES_FREE_SLOT_AST_DESIGN.md create mode 100644 Spikes/phaseS-workflow.js diff --git a/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs b/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs index f79cb4535..2e3cf9755 100644 --- a/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs +++ b/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs @@ -222,9 +222,9 @@ internal class DecompilerSyntaxTreeGenerator : IIncrementalGenerator return child.DoMatch(other, match); }} - bool PatternMatching.INode.DoMatchCollection(Role? role, PatternMatching.INode? pos, PatternMatching.Match match, PatternMatching.BacktrackingInfo backtrackingInfo) + bool PatternMatching.INode.DoMatchCollection(global::System.Collections.Generic.IReadOnlyList other, int pos, PatternMatching.Match match, PatternMatching.BacktrackingInfo backtrackingInfo) {{ - return child.DoMatchCollection(role, pos, match, backtrackingInfo); + return child.DoMatchCollection(other, pos, match, backtrackingInfo); }} }} " diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index f51b3cd32..47c37c775 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -331,6 +331,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/PatternMatching/PatternMatchingTests.cs b/ICSharpCode.Decompiler.Tests/PatternMatching/PatternMatchingTests.cs new file mode 100644 index 000000000..4ca606409 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/PatternMatching/PatternMatchingTests.cs @@ -0,0 +1,266 @@ +// Copyright (c) 2026 AlphaSierraPapa for the SharpDevelop Team +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System.Linq; + +using ICSharpCode.Decompiler.CSharp.Syntax; +using ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching; + +using NUnit.Framework; + +namespace ICSharpCode.Decompiler.Tests.PatternMatching +{ + /// + /// Substrate-agnostic tests of the pattern-matching engine: single-node matching with captures, + /// the special pattern nodes (AnyNode, Choice, Repeat, OptionalNode, the back-references), and the + /// collection backtracking core. They are written against the public matching API so they hold + /// across the engine's storage model. + /// + [TestFixture] + public class PatternMatchingTests + { + static ExpressionStatement Stmt(string identifier) + { + return new ExpressionStatement(new IdentifierExpression(identifier)); + } + + static BlockStatement Block(params Statement[] statements) + { + var block = new BlockStatement(); + foreach (var s in statements) + block.Add(s); + return block; + } + + #region single node + [Test] + public void IdenticalExpressions_Match() + { + Assert.That(new IdentifierExpression("x").IsMatch(new IdentifierExpression("x")), Is.True); + } + + [Test] + public void DifferentExpressions_DoNotMatch() + { + Assert.That(new IdentifierExpression("x").IsMatch(new IdentifierExpression("y")), Is.False); + } + + [Test] + public void AnyNode_MatchesNonNull_RejectsNull() + { + Assert.That(new AnyNode().IsMatch(new IdentifierExpression("x")), Is.True); + Assert.That(new AnyNode().IsMatch(null), Is.False); + } + + [Test] + public void AnyNodeOrNull_MatchesNull() + { + Assert.That(new AnyNodeOrNull().IsMatch(null), Is.True); + Assert.That(new AnyNodeOrNull().IsMatch(new IdentifierExpression("x")), Is.True); + } + + [Test] + public void NamedNode_CapturesTheMatchedNode() + { + var pattern = new NamedNode("id", new AnyNode()); + var target = new IdentifierExpression("foo"); + var m = pattern.Match(target); + Assert.That(m.Success, Is.True); + Assert.That(m.Get("id").Single(), Is.SameAs(target)); + } + #endregion + + #region Choice + [Test] + public void Choice_MatchesAnyAlternative() + { + var choice = new Choice { new IdentifierExpression("a"), new IdentifierExpression("b") }; + Assert.That(choice.IsMatch(new IdentifierExpression("a")), Is.True); + Assert.That(choice.IsMatch(new IdentifierExpression("b")), Is.True); + Assert.That(choice.IsMatch(new IdentifierExpression("c")), Is.False); + } + + [Test] + public void Choice_DoesNotLeakCapturesFromFailedAlternative() + { + // The first alternative captures "g" then fails on the operator; the checkpoint must be + // restored so the surviving match carries only the second alternative's capture. + var choice = new Choice { + new BinaryOperatorExpression { + Left = new NamedNode("g", new IdentifierExpression("a")), + Operator = BinaryOperatorType.Add, + Right = new AnyNode() + }, + new NamedNode("g", new IdentifierExpression("z")) + }; + var m = choice.Match(new IdentifierExpression("z")); + Assert.That(m.Success, Is.True); + Assert.That(m.Get("g").Count(), Is.EqualTo(1)); + Assert.That(((IdentifierExpression)m.Get("g").Single()).Identifier, Is.EqualTo("z")); + } + #endregion + + #region back-references + [Test] + public void Backreference_MatchesEqualNode() + { + var pattern = new BinaryOperatorExpression { + Left = new NamedNode("x", new AnyNode()), + Operator = BinaryOperatorType.Any, + Right = new Backreference("x") + }; + Assert.That(pattern.IsMatch(new BinaryOperatorExpression( + new IdentifierExpression("a"), BinaryOperatorType.Add, new IdentifierExpression("a"))), Is.True); + Assert.That(pattern.IsMatch(new BinaryOperatorExpression( + new IdentifierExpression("a"), BinaryOperatorType.Add, new IdentifierExpression("b"))), Is.False); + } + + [Test] + public void IdentifierExpressionBackreference_MatchesSameName() + { + var pattern = new BinaryOperatorExpression { + Left = new NamedNode("v", new IdentifierExpression(Pattern.AnyString)), + Operator = BinaryOperatorType.Any, + Right = new IdentifierExpressionBackreference("v") + }; + Assert.That(pattern.IsMatch(new BinaryOperatorExpression( + new IdentifierExpression("i"), BinaryOperatorType.Add, new IdentifierExpression("i"))), Is.True); + Assert.That(pattern.IsMatch(new BinaryOperatorExpression( + new IdentifierExpression("i"), BinaryOperatorType.Add, new IdentifierExpression("j"))), Is.False); + } + + [Test] + public void IdentifierExpressionBackreference_RejectsTypeArguments() + { + var pattern = new BinaryOperatorExpression { + Left = new NamedNode("v", new IdentifierExpression(Pattern.AnyString)), + Operator = BinaryOperatorType.Any, + Right = new IdentifierExpressionBackreference("v") + }; + var rightWithTypeArgs = new IdentifierExpression("i"); + rightWithTypeArgs.TypeArguments.Add(new SimpleType("T")); + Assert.That(pattern.IsMatch(new BinaryOperatorExpression( + new IdentifierExpression("i"), BinaryOperatorType.Add, rightWithTypeArgs)), Is.False); + } + #endregion + + #region collections + [Test] + public void EmptyCollections_Match() + { + Assert.That(Block().IsMatch(Block()), Is.True); + } + + [Test] + public void Repeat_GreedilyMatchesAll() + { + var pattern = new BlockStatement { Statements = { new Repeat(new AnyNode("s")) } }; + var m = pattern.Match(Block(Stmt("a"), Stmt("b"), Stmt("c"))); + Assert.That(m.Success, Is.True); + Assert.That(m.Get("s").Count(), Is.EqualTo(3)); + } + + [Test] + public void Repeat_MatchesEmptyWhenMinZero() + { + var pattern = new BlockStatement { Statements = { new Repeat(new AnyNode()) } }; + Assert.That(pattern.IsMatch(Block()), Is.True); + } + + [Test] + public void Repeat_MinCountNotSatisfied_DoesNotMatch() + { + var pattern = new BlockStatement { Statements = { new Repeat(new AnyNode()) { MinCount = 2 } } }; + Assert.That(pattern.IsMatch(Block(Stmt("a"))), Is.False); + Assert.That(pattern.IsMatch(Block(Stmt("a"), Stmt("b"))), Is.True); + } + + [Test] + public void Repeat_MaxCountLeavesTrailingUnmatched_DoesNotMatch() + { + var pattern = new BlockStatement { Statements = { new Repeat(new AnyNode()) { MaxCount = 1 } } }; + Assert.That(pattern.IsMatch(Block(Stmt("a"))), Is.True); + Assert.That(pattern.IsMatch(Block(Stmt("a"), Stmt("b"))), Is.False); + } + + [Test] + public void Repeat_BacktracksToSatisfyTrailingFixedNode() + { + var pattern = new BlockStatement { + Statements = { + new Repeat(new AnyNode("body")), + new NamedNode("last", Stmt("end")) + } + }; + var m = pattern.Match(Block(Stmt("a"), Stmt("b"), Stmt("end"))); + Assert.That(m.Success, Is.True); + Assert.That(m.Get("body").Count(), Is.EqualTo(2)); + Assert.That(((IdentifierExpression)((ExpressionStatement)m.Get("last").Single()).Expression).Identifier, + Is.EqualTo("end")); + } + + [Test] + public void Optional_MatchesWhetherPresentOrAbsent() + { + var pattern = new BlockStatement { + Statements = { + new OptionalNode(Stmt("opt")), + new NamedNode("tail", Stmt("tail")) + } + }; + Assert.That(pattern.IsMatch(Block(Stmt("opt"), Stmt("tail"))), Is.True); + Assert.That(pattern.IsMatch(Block(Stmt("tail"))), Is.True); + Assert.That(pattern.IsMatch(Block(Stmt("other"), Stmt("tail"))), Is.False); + } + + [Test] + public void FixedLengthCollection_LengthMismatch_DoesNotMatch() + { + var pattern = Block(Stmt("a"), Stmt("b")); + Assert.That(pattern.IsMatch(Block(Stmt("a"), Stmt("b"))), Is.True); + Assert.That(pattern.IsMatch(Block(Stmt("a"), Stmt("b"), Stmt("c"))), Is.False); + Assert.That(pattern.IsMatch(Block(Stmt("a"))), Is.False); + } + #endregion + + #region nesting + [Test] + public void NestedPattern_MatchesAndCaptures() + { + var pattern = new BinaryOperatorExpression { + Left = new BinaryOperatorExpression { + Left = new NamedNode("a", new AnyNode()), + Operator = BinaryOperatorType.Multiply, + Right = new NamedNode("b", new AnyNode()) + }, + Operator = BinaryOperatorType.Add, + Right = new NamedNode("c", new AnyNode()) + }; + var target = new BinaryOperatorExpression( + new BinaryOperatorExpression(new IdentifierExpression("x"), BinaryOperatorType.Multiply, new IdentifierExpression("y")), + BinaryOperatorType.Add, + new IdentifierExpression("z")); + var m = pattern.Match(target); + Assert.That(m.Success, Is.True); + Assert.That(((IdentifierExpression)m.Get("a").Single()).Identifier, Is.EqualTo("x")); + Assert.That(((IdentifierExpression)m.Get("b").Single()).Identifier, Is.EqualTo("y")); + Assert.That(((IdentifierExpression)m.Get("c").Single()).Identifier, Is.EqualTo("z")); + } + #endregion + } +} diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs b/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs index 9d54a6275..4c5f8d5ef 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs @@ -717,18 +717,12 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax return (other == null || o != null) && DoMatch(o, match); } - bool PatternMatching.INode.DoMatchCollection(Role? role, PatternMatching.INode? pos, PatternMatching.Match match, PatternMatching.BacktrackingInfo? backtrackingInfo) + bool PatternMatching.INode.DoMatchCollection(System.Collections.Generic.IReadOnlyList other, int pos, PatternMatching.Match match, PatternMatching.BacktrackingInfo backtrackingInfo) { - AstNode? o = pos as AstNode; - return (pos == null || o != null) && DoMatch(o, match); - } - - PatternMatching.INode? PatternMatching.INode.NextSibling { - get { return NextSibling; } - } - - PatternMatching.INode? PatternMatching.INode.FirstChild { - get { return FirstChild; } + PatternMatching.INode? raw = pos < other.Count ? other[pos] : null; + AstNode? o = raw as AstNode; + // matches a single element: succeeds only if the element is absent or an AstNode + return (raw == null || o != null) && DoMatch(o, match); } #endregion diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs b/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs index 6db7fe77a..314bfaf4f 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs @@ -36,6 +36,31 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax internal abstract void InsertNodeBefore(AstNode? existing, AstNode node); internal abstract void InsertNodeAfter(AstNode? existing, AstNode node); internal abstract bool RemoveNode(AstNode node); + + // The pattern matcher consumes a collection as an IReadOnlyList. The view is a separate + // (cached) object rather than the collection itself, because having AstNodeCollection + // implement IEnumerable as well as IEnumerable would make every LINQ call on a typed + // collection ambiguous. + private protected abstract int NodeCount { get; } + private protected abstract INode NodeAt(int index); + + IReadOnlyList? nodeListView; + internal IReadOnlyList AsNodeList() => nodeListView ??= new NodeListView(this); + + sealed class NodeListView : IReadOnlyList + { + readonly AstNodeCollection collection; + public NodeListView(AstNodeCollection collection) => this.collection = collection; + public INode this[int index] => collection.NodeAt(index); + public int Count => collection.NodeCount; + public IEnumerator GetEnumerator() + { + int count = collection.NodeCount; + for (int i = 0; i < count; i++) + yield return collection.NodeAt(i); + } + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } } /// @@ -60,6 +85,9 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax get { return list.Count; } } + private protected override int NodeCount => list.Count; + private protected override INode NodeAt(int index) => list[index]; + public T this[int index] { get { return list[index]; } set { @@ -272,7 +300,8 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax internal bool DoMatch(AstNodeCollection other, Match match) { - return Pattern.DoMatchCollection(role, parent.FirstChild, other.parent.FirstChild, match); + // Both collections are already the per-role child lists, so the matcher walks them by index. + return Pattern.DoMatchCollection(AsNodeList(), other.AsNodeList(), match); } public void InsertAfter(T existingItem, T newItem) diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/INode.cs b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/INode.cs index 622d4a28d..b99d5e26b 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/INode.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/INode.cs @@ -17,6 +17,7 @@ // DEALINGS IN THE SOFTWARE. using System; +using System.Collections.Generic; namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching { @@ -25,13 +26,17 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching /// public interface INode { - Role Role { get; } - INode FirstChild { get; } - INode NextSibling { get; } bool IsNull { get; } bool DoMatch(INode other, Match match); - bool DoMatchCollection(Role role, INode pos, Match match, BacktrackingInfo backtrackingInfo); + + /// + /// Matches this pattern node against the collection element at . + /// Deterministic nodes return whether the element matched (the caller then advances by one); + /// non-deterministic nodes (Repeat, OptionalNode) push their alternative continuation indices + /// onto the backtracking stack instead. + /// + bool DoMatchCollection(IReadOnlyList other, int pos, Match match, BacktrackingInfo backtrackingInfo); } public static class PatternExtensions diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/IdentifierExpressionBackreference.cs b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/IdentifierExpressionBackreference.cs index 566be9c71..b77661beb 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/IdentifierExpressionBackreference.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/IdentifierExpressionBackreference.cs @@ -44,10 +44,9 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching var ident = other as IdentifierExpression; if (ident == null || ident.TypeArguments.Any()) return false; - var referenced = (AstNode)match.Get(referencedGroupName).Last(); - if (referenced == null) + if (match.Get(referencedGroupName).Last() is not IdentifierExpression referenced) return false; - return ident.Identifier == referenced.GetChildByRole(Roles.Identifier).Name; + return ident.Identifier == referenced.Identifier; } } } diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/OptionalNode.cs b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/OptionalNode.cs index 1b427369d..0f7ce04fb 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/OptionalNode.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/OptionalNode.cs @@ -17,6 +17,7 @@ // DEALINGS IN THE SOFTWARE. using System; +using System.Collections.Generic; namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching { @@ -39,10 +40,12 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching { } - public override bool DoMatchCollection(Role role, INode pos, Match match, BacktrackingInfo backtrackingInfo) + public override bool DoMatchCollection(IReadOnlyList other, int pos, Match match, BacktrackingInfo backtrackingInfo) { + // Push the "absent" alternative (resume the following pattern node at the same position), + // then try to consume the element here. backtrackingInfo.backtrackingStack.Push(new PossibleMatch(pos, match.CheckPoint())); - return childNode.DoMatch(pos, match); + return childNode.DoMatch(pos < other.Count ? other[pos] : null, match); } public override bool DoMatch(INode other, Match match) diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Pattern.cs b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Pattern.cs index e617ca225..2f670eb5d 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Pattern.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Pattern.cs @@ -38,12 +38,12 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching internal struct PossibleMatch { - public readonly INode NextOther; // next node after the last matched node + public readonly int NextOtherIndex; // index of the next other element after the last matched one public readonly int Checkpoint; // checkpoint - public PossibleMatch(INode nextOther, int checkpoint) + public PossibleMatch(int nextOtherIndex, int checkpoint) { - this.NextOther = nextOther; + this.NextOtherIndex = nextOtherIndex; this.Checkpoint = checkpoint; } } @@ -52,60 +52,46 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching get { return false; } } - Role INode.Role { - get { return null; } - } - - INode INode.NextSibling { - get { return null; } - } - - INode INode.FirstChild { - get { return null; } - } - public abstract bool DoMatch(INode other, Match match); - public virtual bool DoMatchCollection(Role role, INode pos, Match match, BacktrackingInfo backtrackingInfo) + public virtual bool DoMatchCollection(IReadOnlyList other, int pos, Match match, BacktrackingInfo backtrackingInfo) { - return DoMatch(pos, match); + return DoMatch(pos < other.Count ? other[pos] : null, match); } - public static bool DoMatchCollection(Role role, INode firstPatternChild, INode firstOtherChild, Match match) + /// + /// Matches the collection against the + /// collection, with backtracking over Repeat/OptionalNode. + /// Each collection is already the per-role child list, so the match walks them by index. + /// + public static bool DoMatchCollection(IReadOnlyList patternChildren, IReadOnlyList otherChildren, Match match) { BacktrackingInfo backtrackingInfo = new BacktrackingInfo(); - Stack patternStack = new Stack(); + Stack patternStack = new Stack(); Stack stack = backtrackingInfo.backtrackingStack; - patternStack.Push(firstPatternChild); - stack.Push(new PossibleMatch(firstOtherChild, match.CheckPoint())); + patternStack.Push(0); + stack.Push(new PossibleMatch(0, match.CheckPoint())); while (stack.Count > 0) { - INode cur1 = patternStack.Pop(); - INode cur2 = stack.Peek().NextOther; + int patternPos = patternStack.Pop(); + int otherPos = stack.Peek().NextOtherIndex; match.RestoreCheckPoint(stack.Pop().Checkpoint); bool success = true; - while (cur1 != null && success) + while (patternPos < patternChildren.Count && success) { - while (cur1 != null && cur1.Role != role) - cur1 = cur1.NextSibling; - while (cur2 != null && cur2.Role != role) - cur2 = cur2.NextSibling; - if (cur1 == null) - break; - + INode cur1 = patternChildren[patternPos]; Debug.Assert(stack.Count == patternStack.Count); - success = cur1.DoMatchCollection(role, cur2, match, backtrackingInfo); + success = cur1.DoMatchCollection(otherChildren, otherPos, match, backtrackingInfo); Debug.Assert(stack.Count >= patternStack.Count); + // For every alternative the pattern node pushed, queue the next pattern node so the + // continuation resumes after it. while (stack.Count > patternStack.Count) - patternStack.Push(cur1.NextSibling); + patternStack.Push(patternPos + 1); - cur1 = cur1.NextSibling; - if (cur2 != null) - cur2 = cur2.NextSibling; + patternPos++; + otherPos++; } - while (cur2 != null && cur2.Role != role) - cur2 = cur2.NextSibling; - if (success && cur2 == null) + if (success && otherPos >= otherChildren.Count) return true; } return false; diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Repeat.cs b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Repeat.cs index 1c10c29bc..17c57c606 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Repeat.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/PatternMatching/Repeat.cs @@ -17,7 +17,7 @@ // DEALINGS IN THE SOFTWARE. using System; -using System.Diagnostics; +using System.Collections.Generic; namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching { @@ -44,20 +44,16 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching this.MaxCount = int.MaxValue; } - public override bool DoMatchCollection(Role role, INode pos, Match match, BacktrackingInfo backtrackingInfo) + public override bool DoMatchCollection(IReadOnlyList other, int pos, Match match, BacktrackingInfo backtrackingInfo) { var backtrackingStack = backtrackingInfo.backtrackingStack; - Debug.Assert(pos == null || pos.Role == role); int matchCount = 0; if (this.MinCount <= 0) backtrackingStack.Push(new PossibleMatch(pos, match.CheckPoint())); - while (matchCount < this.MaxCount && pos != null && childNode.DoMatch(pos, match)) + while (matchCount < this.MaxCount && pos < other.Count && childNode.DoMatch(other[pos], match)) { matchCount++; - do - { - pos = pos.NextSibling; - } while (pos != null && pos.Role != role); + pos++; if (matchCount >= this.MinCount) backtrackingStack.Push(new PossibleMatch(pos, match.CheckPoint())); } diff --git a/PHASE_3C_TODO.md b/PHASE_3C_TODO.md new file mode 100644 index 000000000..5f122968a --- /dev/null +++ b/PHASE_3C_TODO.md @@ -0,0 +1,103 @@ +# 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 -> list-backed (CSharp/Syntax/AstNodeCollection.cs) +Currently a *view* over the linked list (iterates parent.firstChild filtering by role). Rewrite: +- [ ] Back with `List`; 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 AND explicit IReadOnlyList (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(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(role): scan slots for GetChildSlot(i).Role == role; single -> (T)GetChild(i) + (role null object if empty); collection role -> first element. + - GetChildrenByRole(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 f`); property bodies single `get => f; set => SetChildNode(ref f, + value, )`, 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` is used by the matcher; keep IReadOnlyList 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). diff --git a/ROLES_FREE_SLOT_AST_DESIGN.md b/ROLES_FREE_SLOT_AST_DESIGN.md new file mode 100644 index 000000000..bc51c14a1 --- /dev/null +++ b/ROLES_FREE_SLOT_AST_DESIGN.md @@ -0,0 +1,919 @@ +# 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`, `Roles`, `TokenRole`, `GetChildByRole`, + `SetChildByRole`, `GetChildrenByRole`, `node.Role`); rewrite all consumers; accepted public-API + break. **Keep `AstNode` and `AstNodeCollection` 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 + // flags: per-class bits only (e.g. Identifier.IsVerbatim); no role index, no frozen bit + + 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(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 LeadingTrivia { get; } // empty (no alloc) when the annotation is absent + public IEnumerable 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` (kept; reimplemented over slots, analog of `InstructionCollection`): +List-backed, knows parent + slot, implements `IReadOnlyList` and explicitly +`IReadOnlyList` (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)`, +`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, )`), 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 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` 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`/`Roles`/`TokenRole`/`GetChildByRole`/`SetChildByRole`/`GetChildrenByRole` deleted. +`AstNodeCollection` 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` 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` already *is* the +per-role sub-list). See Part II Phase 6 for the full design (slim `INode`; central matcher over +`IReadOnlyList`; `Repeat`/`OptionalNode` over index cursor; `AstNodeCollection.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` over slots (List-backed, parent+slot, renumbering, +`IReadOnlyList`+explicit `IReadOnlyList`; 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`, 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` invariance blocks + `(Role)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 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.DoMatch(other, match) => Pattern.DoMatchCollection(this, other, match)` + via the explicit `IReadOnlyList` 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`/`Roles`/`TokenRole`/`GetChildByRole`/`SetChildByRole`/ +`GetChildrenByRole`/`node.Role`/`AddChild(role)`/`InsertChildBefore/After` (KEEP +`AstNodeCollection` -- 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 +**Decided, deferred (generator-internal, zero public-API impact): bit-pack declared scalars into +`flags`** (ILAst-style). The generator backs 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, allocating bit ranges +per class above the base's used bits (generalize `AstNodeFlagsUsedBits` to a generated per-class +`const`), with an overflow diagnostic if a node exceeds the word. **Carve-out: `[Flags]` enums stay +their own field** -- `Modifiers` is ~19 flag bits, already a bitset, and would overflow the word +(esp. while the 9-bit role index is still present pre-Phase-7). The accessor signature is unchanged +(`public bool IsAsync { get; set; }`), so this is a pure storage swap doable any time after Phase 2. +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` 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`. 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` 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) decided + deferred. +- 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` API surface [before Phase 1] +**Finding (verified):** Parts I.1/I.4 mention only Add/Insert/remove. The actually-used +`AstNodeCollection` 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` -> +`NodeCollection` to mirror IL's `InstructionCollection` (Parts I.1, I.9, I.10, Phase +6.5/6.7, Phase 7, Files, R3). +**Decision:** Keep **both** original names. Reimplement `AstNodeCollection` 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`/`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` with `AstNodeCollection` and change "delete +`AstNodeCollection`" 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 `14`; 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. diff --git a/Spikes/phaseS-workflow.js b/Spikes/phaseS-workflow.js new file mode 100644 index 000000000..d21d0930a --- /dev/null +++ b/Spikes/phaseS-workflow.js @@ -0,0 +1,166 @@ +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 /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 needs a +smarter index scheme). The slot model gives each node a flattened childIndex; a List-backed +AstNodeCollection 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 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 }