Skip to content

Commit 9f0bb49

Browse files
author
Nathan Hawes
committed
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
1 parent a997013 commit 9f0bb49

7 files changed

+185
-3
lines changed

‎Sources/SwiftSyntax/RawSyntax.swift

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,17 @@ extension RawSyntax {
341341
}
342342
}
343343

344-
extension RawSyntax: Codable {
344+
// This is needed purely to have a self assignment initializer for
345+
// RawSyntax.init(from:Decoder) so we can reuse omitted nodes, instead of
346+
// copying them.
347+
protocol _RawSyntaxFactory {}
348+
extension _RawSyntaxFactory {
349+
fileprivate init(_instance: Self) {
350+
self = _instance
351+
}
352+
}
353+
354+
extension RawSyntax: Codable, _RawSyntaxFactory {
345355
/// Creates a RawSyntax from the provided Foundation Decoder.
346356
convenience init(from decoder: Decoder) throws {
347357
let container = try decoder.container(keyedBy: CodingKeys.self)
@@ -359,7 +369,7 @@ extension RawSyntax: Codable {
359369
guard let lookupNode = lookupFunc(id) else {
360370
throw IncrementalDecodingError.nodeLookupFailed(id)
361371
}
362-
self.init(lookupNode)
372+
self.init(_instance: lookupNode)
363373
return
364374
}
365375

‎Sources/SwiftSyntax/WeakLookupTable.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,9 @@ class WeakLookupTable<Element: Identifiable & AnyObject> {
188188
return reserveCapacity(estimatedCount &+ 1)
189189
}
190190

191-
/// Inserts the given object into the table.
191+
/// Inserts the given object into the table if the table doesn't already
192+
/// contain an object with the same identifier.
193+
/// - returns: true if the object was inserted
192194
@discardableResult
193195
func insert(_ obj: Element) -> Bool {
194196
var (pos, alreadyExists) = _findHole(obj.id)

‎Tests/SwiftSyntaxTest/DeserializeFile.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,18 @@ public class DecodeSyntaxTestCase: XCTestCase {
1515
XCTAssertEqual("\(parsed)", source)
1616
}())
1717
}
18+
19+
public func testIncrementalDeserialize() {
20+
XCTAssertNoThrow(try {
21+
let deserializer = SyntaxTreeDeserializer()
22+
var tree: SourceFileSyntax? = nil
23+
let responses = try (1...3).map {
24+
try Data(contentsOf: getInput("incremental-deserialize-\($0).json"))
25+
}
26+
for data in responses {
27+
tree = try deserializer.deserialize(data, serializationFormat: .json)
28+
}
29+
XCTAssertEqual(tree?.description, "x\ny")
30+
}())
31+
}
1832
}

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
{
2+
"id":39,
3+
"kind":"SourceFile",
4+
"layout":[
5+
{
6+
"id":38,
7+
"kind":"CodeBlockItemList",
8+
"layout":[
9+
{
10+
"id":35,
11+
"kind":"CodeBlockItem",
12+
"layout":[
13+
{
14+
"id":34,
15+
"kind":"IdentifierExpr",
16+
"layout":[
17+
{
18+
"id":33,
19+
"tokenKind":{
20+
"kind":"identifier",
21+
"text":"x"
22+
},
23+
"leadingTrivia":[],
24+
"trailingTrivia":[],
25+
"presence":"Present"
26+
},
27+
null
28+
],
29+
"presence":"Present"
30+
},
31+
null,
32+
null
33+
],
34+
"presence":"Present"
35+
}
36+
],
37+
"presence":"Present"
38+
},
39+
{
40+
"id":37,
41+
"tokenKind":{
42+
"kind":"eof",
43+
"text":""
44+
},
45+
"leadingTrivia":[],
46+
"trailingTrivia":[],
47+
"presence":"Present"
48+
}
49+
],
50+
"presence":"Present"
51+
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
{
2+
"id":45,
3+
"kind":"SourceFile",
4+
"layout":[
5+
{
6+
"id":44,
7+
"kind":"CodeBlockItemList",
8+
"layout":[
9+
{
10+
"id":35,
11+
"omitted":true
12+
}
13+
],
14+
"presence":"Present"
15+
},
16+
{
17+
"id":43,
18+
"tokenKind":{
19+
"kind":"eof",
20+
"text":""
21+
},
22+
"leadingTrivia":[
23+
{
24+
"kind":"Newline",
25+
"value":1
26+
}
27+
],
28+
"trailingTrivia":[],
29+
"presence":"Present"
30+
}
31+
],
32+
"presence":"Present"
33+
}

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
{
2+
"id":54,
3+
"kind":"SourceFile",
4+
"layout":[
5+
{
6+
"id":53,
7+
"kind":"CodeBlockItemList",
8+
"layout":[
9+
{
10+
"id":35,
11+
"omitted":true
12+
},
13+
{
14+
"id":50,
15+
"kind":"CodeBlockItem",
16+
"layout":[
17+
{
18+
"id":49,
19+
"kind":"IdentifierExpr",
20+
"layout":[
21+
{
22+
"id":48,
23+
"tokenKind":{
24+
"kind":"identifier",
25+
"text":"y"
26+
},
27+
"leadingTrivia":[
28+
{
29+
"kind":"Newline",
30+
"value":1
31+
}
32+
],
33+
"trailingTrivia":[],
34+
"presence":"Present"
35+
},
36+
null
37+
],
38+
"presence":"Present"
39+
},
40+
null,
41+
null
42+
],
43+
"presence":"Present"
44+
}
45+
],
46+
"presence":"Present"
47+
},
48+
{
49+
"id":52,
50+
"tokenKind":{
51+
"kind":"eof",
52+
"text":""
53+
},
54+
"leadingTrivia":[],
55+
"trailingTrivia":[],
56+
"presence":"Present"
57+
}
58+
],
59+
"presence":"Present"
60+
}

‎Tests/SwiftSyntaxTest/WeakLookupTableTest.swift

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,18 @@ public class WeakLookupTableTestCase: XCTestCase {
108108

109109
_ = tree // Prevents 'never read' warning
110110
}
111+
112+
func testSameIdentifierDifferentObject() {
113+
let table = WeakLookupTable<MinimalNode>()
114+
115+
let object1 = MinimalNode(id: "1", children: [])
116+
let object2 = MinimalNode(id: "1", children: [])
117+
118+
XCTAssertTrue(table.insert(object1))
119+
XCTAssert(table["1"] === object1)
120+
XCTAssertFalse(table.insert(object2))
121+
XCTAssert(table["1"] === object1)
122+
}
111123
}
112124

113125
#endif // DEBUG

0 commit comments

Comments
 (0)