SyntaxTreeDeserializer: Hold on to omitted nodes across multiple updates

RawSyntax.init(from: Decoder) looks up omitted nodes by id from a
WeakLookupTable, initializing itself as a copy. This copy is then used in the
new tree, but wasn't being written back to the lookup table, leaving it with a
weak reference to the original node. This was an issue because the
SyntaxTreeDeserializer only holds a strong reference to the most recent syntax
tree, meaning the orignal node (and table entry) could have been cleaned up,
even though further incremental updates could still refer to that id.

This patch changes RawSyntax.init(from: Decoder) to reuse the RawSyntax node
from the WeakLookupTable, rather than copying it.

Resolves rdar://problem/44877860

Author: Nathan Hawes

Sources/SwiftSyntax/RawSyntax.swift

@@ -124,13 +124,6 @@ final class RawSyntax {
       }
     }).value
   }
-    
-  /// Creates a copy of `other`.
-  init(_ other: RawSyntax) {
-    self.data = other.data
-    self.presence = other.presence
-    self.id = other.id
-  }
 
   init(kind: SyntaxKind, layout: [RawSyntax?], presence: SourcePresence,
        id: SyntaxNodeId? = nil) {
@@ -341,7 +334,17 @@ extension RawSyntax {
   }
 }
 
-extension RawSyntax: Codable {
+// This is needed purely to have a self assignment initializer for
+// RawSyntax.init(from:Decoder) so we can reuse omitted nodes, instead of
+// copying them.
+private protocol _RawSyntaxFactory {}
+extension _RawSyntaxFactory {
+  init(_instance: Self) {
+    self = _instance
+  }
+}
+
+extension RawSyntax: Codable, _RawSyntaxFactory {
   /// Creates a RawSyntax from the provided Foundation Decoder.
   convenience init(from decoder: Decoder) throws {
     let container = try decoder.container(keyedBy: CodingKeys.self)
@@ -359,7 +362,7 @@ extension RawSyntax: Codable {
       guard let lookupNode = lookupFunc(id) else {
         throw IncrementalDecodingError.nodeLookupFailed(id)
       }
-      self.init(lookupNode)
+      self.init(_instance: lookupNode)
       return
     }
 

Sources/SwiftSyntax/WeakLookupTable.swift

@@ -188,7 +188,9 @@ class WeakLookupTable<Element: Identifiable & AnyObject> {
     return reserveCapacity(estimatedCount &+ 1)
   }
 
-  /// Inserts the given object into the table.
+  /// Inserts the given object into the table if the table doesn't already
+  /// contain an object with the same identifier.
+  /// - returns: true if the object was inserted
   @discardableResult
   func insert(_ obj: Element) -> Bool {
     var (pos, alreadyExists) = _findHole(obj.id)

Tests/SwiftSyntaxTest/DeserializeFile.swift

@@ -15,4 +15,18 @@ public class DecodeSyntaxTestCase: XCTestCase {
       XCTAssertEqual("\(parsed)", source)
     }())
   }
+
+  public func testIncrementalDeserialize() {
+    XCTAssertNoThrow(try {
+      let deserializer = SyntaxTreeDeserializer()
+      var tree: SourceFileSyntax? = nil
+      let responses = try (1...3).map {
+        try Data(contentsOf: getInput("incremental-deserialize-\($0).json"))
+      }
+      for data in responses {
+        tree = try deserializer.deserialize(data, serializationFormat: .json)
+      }
+      XCTAssertEqual(tree?.description, "x\ny")
+    }())
+  }
 }

Tests/SwiftSyntaxTest/Inputs/incremental-deserialize-1.json

@@ -0,0 +1,51 @@
+{
+  "id":39,
+  "kind":"SourceFile",
+  "layout":[
+    {
+      "id":38,
+      "kind":"CodeBlockItemList",
+      "layout":[
+        {
+          "id":35,
+          "kind":"CodeBlockItem",
+          "layout":[
+            {
+              "id":34,
+              "kind":"IdentifierExpr",
+              "layout":[
+                {
+                  "id":33,
+                  "tokenKind":{
+                    "kind":"identifier",
+                    "text":"x"
+                  },
+                  "leadingTrivia":[],
+                  "trailingTrivia":[],
+                  "presence":"Present"
+                },
+                null
+              ],
+              "presence":"Present"
+            },
+            null,
+            null
+          ],
+          "presence":"Present"
+        }
+      ],
+      "presence":"Present"
+    },
+    {
+      "id":37,
+      "tokenKind":{
+        "kind":"eof",
+        "text":""
+      },
+      "leadingTrivia":[],
+      "trailingTrivia":[],
+      "presence":"Present"
+    }
+  ],
+  "presence":"Present"
+}

Tests/SwiftSyntaxTest/Inputs/incremental-deserialize-2.json

@@ -0,0 +1,33 @@
+{
+  "id":45,
+  "kind":"SourceFile",
+  "layout":[
+    {
+      "id":44,
+      "kind":"CodeBlockItemList",
+      "layout":[
+        {
+          "id":35,
+          "omitted":true
+        }
+      ],
+      "presence":"Present"
+    },
+    {
+      "id":43,
+      "tokenKind":{
+        "kind":"eof",
+        "text":""
+      },
+      "leadingTrivia":[
+        {
+          "kind":"Newline",
+          "value":1
+        }
+      ],
+      "trailingTrivia":[],
+      "presence":"Present"
+    }
+  ],
+  "presence":"Present"
+}

Tests/SwiftSyntaxTest/Inputs/incremental-deserialize-3.json

@@ -0,0 +1,60 @@
+{
+  "id":54,
+  "kind":"SourceFile",
+  "layout":[
+    {
+      "id":53,
+      "kind":"CodeBlockItemList",
+      "layout":[
+        {
+          "id":35,
+          "omitted":true
+        },
+        {
+          "id":50,
+          "kind":"CodeBlockItem",
+          "layout":[
+            {
+              "id":49,
+              "kind":"IdentifierExpr",
+              "layout":[
+                {
+                  "id":48,
+                  "tokenKind":{
+                    "kind":"identifier",
+                    "text":"y"
+                  },
+                  "leadingTrivia":[
+                    {
+                      "kind":"Newline",
+                      "value":1
+                    }
+                  ],
+                  "trailingTrivia":[],
+                  "presence":"Present"
+                },
+                null
+              ],
+              "presence":"Present"
+            },
+            null,
+            null
+          ],
+          "presence":"Present"
+        }
+      ],
+      "presence":"Present"
+    },
+    {
+      "id":52,
+      "tokenKind":{
+        "kind":"eof",
+        "text":""
+      },
+      "leadingTrivia":[],
+      "trailingTrivia":[],
+      "presence":"Present"
+    }
+  ],
+  "presence":"Present"
+}

Tests/SwiftSyntaxTest/WeakLookupTableTest.swift

@@ -108,6 +108,18 @@ public class WeakLookupTableTestCase: XCTestCase {
 
     _ = tree // Prevents 'never read' warning
   }
+
+  func testSameIdentifierDifferentObject() {
+    let table = WeakLookupTable<MinimalNode>()
+
+    let object1 = MinimalNode(id: "1", children: [])
+    let object2 = MinimalNode(id: "1", children: [])
+
+    XCTAssertTrue(table.insert(object1))
+    XCTAssert(table["1"] === object1)
+    XCTAssertFalse(table.insert(object2))
+    XCTAssert(table["1"] === object1)
+  }
 }
 
 #endif // DEBUG