Skip to content

Commit 75c5504

Browse files
author
Nathan Hawes
committed
SyntaxTreeDeserializer: Hold on to omitted nodes across multiple updates
RawSyntax.init(from decoder: 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. Resolves rdar://problem/44877860
1 parent a997013 commit 75c5504

7 files changed

+185
-12
lines changed

Sources/SwiftSyntax/RawSyntax.swift

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -360,19 +360,18 @@ extension RawSyntax: Codable {
360360
throw IncrementalDecodingError.nodeLookupFailed(id)
361361
}
362362
self.init(lookupNode)
363-
return
364-
}
365-
366-
let presence = try container.decode(SourcePresence.self, forKey: .presence)
367-
if let kind = try container.decodeIfPresent(SyntaxKind.self, forKey: .kind) {
368-
let layout = try container.decode([RawSyntax?].self, forKey: .layout)
369-
self.init(kind: kind, layout: layout, presence: presence, id: id)
370363
} else {
371-
let kind = try container.decode(TokenKind.self, forKey: .tokenKind)
372-
let leadingTrivia = try container.decode(Trivia.self, forKey: .leadingTrivia)
373-
let trailingTrivia = try container.decode(Trivia.self, forKey: .trailingTrivia)
374-
self.init(kind: kind, leadingTrivia: leadingTrivia,
375-
trailingTrivia: trailingTrivia, presence: presence, id: id)
364+
let presence = try container.decode(SourcePresence.self, forKey: .presence)
365+
if let kind = try container.decodeIfPresent(SyntaxKind.self, forKey: .kind) {
366+
let layout = try container.decode([RawSyntax?].self, forKey: .layout)
367+
self.init(kind: kind, layout: layout, presence: presence, id: id)
368+
} else {
369+
let kind = try container.decode(TokenKind.self, forKey: .tokenKind)
370+
let leadingTrivia = try container.decode(Trivia.self, forKey: .leadingTrivia)
371+
let trailingTrivia = try container.decode(Trivia.self, forKey: .trailingTrivia)
372+
self.init(kind: kind, leadingTrivia: leadingTrivia,
373+
trailingTrivia: trailingTrivia, presence: presence, id: id)
374+
}
376375
}
377376
if let callback = decoder.userInfo[.rawSyntaxDecodedCallback] as?
378377
(RawSyntax) -> Void {

Sources/SwiftSyntax/WeakLookupTable.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,10 +189,14 @@ class WeakLookupTable<Element: Identifiable & AnyObject> {
189189
}
190190

191191
/// Inserts the given object into the table.
192+
/// - returns: true if no existing table object was overwritten
192193
@discardableResult
193194
func insert(_ obj: Element) -> Bool {
194195
var (pos, alreadyExists) = _findHole(obj.id)
195196
if alreadyExists {
197+
// The existing and new object have the same identifier, but aren't
198+
// necessarily the same object, so we still need to update it
199+
buckets[pos].value = obj
196200
return false
197201
}
198202

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
}
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+
}
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+
}
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"] === object2)
122+
}
111123
}
112124

113125
#endif // DEBUG

0 commit comments

Comments
 (0)