Fix 1091: NRE in TyCreateWalkingGraph (#1105)

Author: AlexanderSher

src/Analysis/Ast/Impl/Dependencies/DependencyResolver.cs

@@ -68,10 +68,33 @@ public int Remove(TKey key) {
                     return _version;
                 }
 
-                Interlocked.Increment(ref _version);
+                var version = Interlocked.Increment(ref _version);
 
+                var vertex = _vertices[index];
                 _vertices[index] = default;
-                return _version;
+                if (vertex == null) {
+                    return version;
+                }
+                
+                foreach (var incomingIndex in vertex.Incoming) {
+                    var incoming = _vertices[incomingIndex];
+                    if (incoming != null && incoming.IsSealed) {
+                        _vertices[incomingIndex] = new DependencyVertex<TKey, TValue>(incoming, version, false);
+                    }
+                }
+
+                if (!vertex.IsSealed) {
+                    return version;
+                }
+                
+                foreach (var outgoingIndex in vertex.Outgoing) {
+                    var outgoing = _vertices[outgoingIndex];
+                    if (outgoing != null && !outgoing.IsNew) {
+                        _vertices[outgoingIndex] = new DependencyVertex<TKey, TValue>(outgoing, version, true);
+                    }
+                }
+
+                return version;
             }
         }
 
@@ -135,7 +158,7 @@ private ImmutableArray<int> EnsureKeys(int index, ImmutableArray<TKey> keys, int
                 } else {
                     var vertex = _vertices[keyIndex];
                     if (vertex != default && vertex.IsSealed && !vertex.ContainsOutgoing(index)) {
-                        _vertices[keyIndex] = new DependencyVertex<TKey, TValue>(vertex, version);
+                        _vertices[keyIndex] = new DependencyVertex<TKey, TValue>(vertex, version, false);
                     }
                 }
 
@@ -263,13 +286,14 @@ private bool TryBuildReverseGraph(ImmutableArray<DependencyVertex<TKey, TValue>>
         private bool TryCreateWalkingGraph(in ImmutableArray<DependencyVertex<TKey, TValue>> vertices, int version, out ImmutableArray<WalkingVertex<TKey, TValue>> analysisGraph) {
             var nodesByVertexIndex = new Dictionary<int, WalkingVertex<TKey, TValue>>();
 
-            foreach (var vertex in vertices) {
+            for (var index = 0; index < vertices.Count; index++) {
+                var vertex = vertices[index];
                 if (vertex == null || vertex.IsWalked) {
                     continue;
                 }
 
-                var node = new WalkingVertex<TKey, TValue>(vertices[vertex.Index]);
-                nodesByVertexIndex[vertex.Index] = node;
+                var node = new WalkingVertex<TKey, TValue>(vertices[index]);
+                nodesByVertexIndex[index] = node;
             }
 
             if (nodesByVertexIndex.Count == 0) {
@@ -288,9 +312,12 @@ private bool TryCreateWalkingGraph(in ImmutableArray<DependencyVertex<TKey, TVal
                 foreach (var outgoingIndex in node.DependencyVertex.Outgoing) {
                     if (!nodesByVertexIndex.TryGetValue(outgoingIndex, out var outgoingNode)) {
                         var vertex = vertices[outgoingIndex];
+                        if (vertex == null) {
+                            continue;
+                        }
+
                         outgoingNode = new WalkingVertex<TKey, TValue>(vertex);
                         nodesByVertexIndex[outgoingIndex] = outgoingNode;
-
                         queue.Enqueue(outgoingNode);
                     }
 

src/Analysis/Ast/Impl/Dependencies/DependencyVertex.cs

@@ -29,6 +29,7 @@ internal sealed class DependencyVertex<TKey, TValue> {
         public int Index { get; }
         public string DebuggerDisplay => $"{Key}:{Value}";
 
+        public bool IsNew => _state == (int)State.New;
         public bool IsSealed => _state >= (int)State.Sealed;
         public bool IsWalked => _state == (int)State.Walked;
 
@@ -38,7 +39,7 @@ internal sealed class DependencyVertex<TKey, TValue> {
         private HashSet<int> _outgoing;
         private static HashSet<int> _empty = new HashSet<int>();
 
-        public DependencyVertex(DependencyVertex<TKey, TValue> oldVertex, int version) {
+        public DependencyVertex(DependencyVertex<TKey, TValue> oldVertex, int version, bool isNew) {
             Key = oldVertex.Key;
             Value = oldVertex.Value;
             IsRoot = oldVertex.IsRoot;
@@ -48,7 +49,7 @@ public DependencyVertex(DependencyVertex<TKey, TValue> oldVertex, int version) {
             Version = version;
 
             _outgoing = oldVertex.Outgoing;
-            _state = oldVertex.IsWalked ? (int)State.ChangedOutgoing : (int)State.New;
+            _state = !isNew && oldVertex.IsWalked ? (int)State.ChangedOutgoing : (int)State.New;
         }
 
         public DependencyVertex(TKey key, TValue value, bool isRoot, ImmutableArray<int> incoming, int version, int index) {

src/Analysis/Ast/Test/DependencyResolverTests.cs

@@ -177,6 +177,84 @@ public async Task ChangeValue_MissingKeys() {
             result.ToString().Should().Be("AD");
         }
 
+        [TestMethod]
+        public async Task ChangeValue_Add() {
+            var resolver = new DependencyResolver<string, string>();
+            resolver.ChangeValue("A", "A:BD", true, "B", "D");
+            resolver.ChangeValue("C", "C", false);
+
+            var walker = resolver.CreateWalker();
+            walker.MissingKeys.Should().Equal("B", "D");
+            var node1 = await walker.GetNextAsync(default);
+            var node2 = await walker.GetNextAsync(default);
+            node1.Value.Should().Be("A:BD");
+            node2.Value.Should().Be("C");
+            node1.Commit();
+            node2.Commit();
+
+            walker.Remaining.Should().Be(0);
+            
+            resolver.ChangeValue("B", "B", false);
+            walker = resolver.CreateWalker();       
+            walker.MissingKeys.Should().Equal("D");
+            
+            var node = await walker.GetNextAsync(default); 
+            node.Value.Should().Be("B");             
+            node.Commit();                             
+                                                       
+            node = await walker.GetNextAsync(default); 
+            node.Value.Should().Be("A:BD");            
+            node.Commit();  
+            
+            walker.Remaining.Should().Be(0); 
+            
+            resolver.ChangeValue("D", "D:C", false);
+            walker = resolver.CreateWalker();       
+            walker.MissingKeys.Should().BeEmpty();
+            
+            node = await walker.GetNextAsync(default); 
+            node.Value.Should().Be("D:C");             
+            node.Commit();                             
+                                                       
+            node = await walker.GetNextAsync(default); 
+            node.Value.Should().Be("A:BD");            
+            node.Commit();  
+            
+            walker.Remaining.Should().Be(0);
+        }
+
+        [TestMethod]
+        public async Task ChangeValue_Remove() {
+            var resolver = new DependencyResolver<string, string>();
+            resolver.ChangeValue("A", "A:BC", true, "B", "C");
+            resolver.ChangeValue("B", "B:C", false, "C");
+            resolver.ChangeValue("C", "C", false);
+
+            var walker = resolver.CreateWalker();
+            walker.MissingKeys.Should().BeEmpty();
+            var node = await walker.GetNextAsync(default);
+            node.Value.Should().Be("C");
+            node.Commit();
+
+            node = await walker.GetNextAsync(default);
+            node.Value.Should().Be("B:C");
+            node.Commit();
+            
+            node = await walker.GetNextAsync(default);
+            node.Value.Should().Be("A:BC");
+            node.Commit();
+
+            resolver.Remove("B");
+            walker = resolver.CreateWalker();
+            walker.MissingKeys.Should().Equal("B");
+
+            node = await walker.GetNextAsync(default);
+            node.Value.Should().Be("A:BC");
+            node.Commit();
+
+            walker.Remaining.Should().Be(0);
+        }
+
         [TestMethod]
         public async Task ChangeValue_RemoveKeys() {
             var resolver = new DependencyResolver<string, string>();