SyntaxTreeDeserializer: Hold on to omitted nodes across multiple updates #17
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please test |
|
@rintaro I thought it would be better to have the new syntax tree actually use the original RawSyntax reference from the lookup table, but I couldn't work out how to do that... |
|
Perhaps we can use "hackish" factory initializer pattern for now. protocol _RawSyntaxFactory {}
extension _RawSyntaxFactory {
fileprivate init(_instance: Self) { self = _instance }
}
extension RawSyntax: Codable, _RawSyntaxFactory {
convenience init(from decoder: Decoder) throws {
if omitted {
/* ... */
let lookupNode = lookupFunc(id)
self.init(_instance: lookupNode)
return
} else {
/* ... */
}
}
}When https://forums.swift.org/t/allow-self-x-in-class-convenience-initializers/15924 is implemented, we probably can migrate to it (i.e. just |
|
Ah, that's neat! Thanks @rintaro – I'll update shortly. |
nathawes
force-pushed
the
r44877860-hold-on-to-omitted-syntax-nodes-across-multiple-incremental-updates
branch
from
75c5504 to
9f0bb49
Compare
|
@swift-ci please test |
|
Build failed |
|
@rintaro Does this look ok now? |
rintaro
reviewed
Oct 2, 2018
Sources/SwiftSyntax/RawSyntax.swift
Outdated
| // 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. | ||
| protocol _RawSyntaxFactory {} |
There was a problem hiding this comment.
Sorry, this can be private, and we don't need fileprivate in init(_instance:) decl.
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
nathawes
force-pushed
the
r44877860-hold-on-to-omitted-syntax-nodes-across-multiple-incremental-updates
branch
from
32494c7 to
c306a90
Compare
|
Thanks @rintaro! |
|
@swift-ci please test |
|
Build failed |
nathawes
deleted the
r44877860-hold-on-to-omitted-syntax-nodes-across-multiple-incremental-updates
branch
adevress
pushed a commit
to adevress/swift-syntax
that referenced
this pull request
Jan 14, 2024
…ules Allow format rules to be enabled/disabled.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RawSyntax.init(from decoder: Decoder)looks up omitted nodes by id from aWeakLookupTable, 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 theSyntaxTreeDeserializeronly holds a strong reference to the most recent syntax tree, meaning the original node (and table entry) could have been cleaned up, even though further incremental updates could still refer to that id.Resolves rdar://problem/44877860