Add Sendable annotations to swift-syntax #2425
Merged
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
ahoppen
changed the title
ahoppen
force-pushed
the
ahoppen/sendable-conformances
branch
from
4fb95c4 to
aa91cfc
Compare
|
@swift-ci Please test |
…` across actor boundaries
ahoppen
force-pushed
the
ahoppen/sendable-conformances
branch
from
aa91cfc to
485f166
Compare
2 tasks
grynspan
added a commit
to swiftlang/swift-testing
that referenced
this pull request
Jan 17, 2024
We are getting a new warning in CI:
```
.../swift-testing/Sources/TestingMacros/ConditionMacro.swift:46:13: warning: let '_sourceLocationLabel' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor; this is an error in Swift 6
private let _sourceLocationLabel = TokenSyntax.identifier("sourceLocation")
^
```
Work around it by using `private var ... {}` instead of `private let`. I'm assuming that creating a `TokenSyntax` is not prohibitively expensive, so the change should be safe.
@ahoppen confirms that this operation is reasonably cheap and that syntax nodes are not safely sendable in the 5.9 tag of swift-syntax, so the warning is technically valid. Although our code does not access these constants across concurrency domains, the compiler is (in theory) free to invoke our macro expansion code on multiple tasks concurrently.
Concurrency safety in swift-syntax is tracked with swiftlang/swift-syntax#2425.
ahoppen
force-pushed
the
ahoppen/sendable-conformances
branch
from
485f166 to
1388b09
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
rintaro
approved these changes
Jan 23, 2024
Comment on lines
-207
to
+219
| payload.presence = newValue | ||
| return RawSyntax(arena: arena, payload: .parsedToken(payload)) | ||
| if arena == self.raw.arenaReference { | ||
| payload.presence = newValue | ||
| return RawSyntax(arena: arena, payload: .parsedToken(payload)) | ||
| } | ||
| // If the modified token is allocated in a different arena, it might have | ||
| // a different or no `parseTrivia` function. We thus cannot use a | ||
| // `parsedToken` anymore. | ||
| return .makeMaterializedToken( | ||
| kind: formKind(), | ||
| leadingTrivia: formLeadingTrivia(), | ||
| trailingTrivia: formTrailingTrivia(), | ||
| presence: newValue, | ||
| tokenDiagnostic: tokenDiagnostic, | ||
| arena: arena | ||
| ) |
There was a problem hiding this comment.
Just to make sure I understand correctly, this should be reverted when SwiftParser is merged, right?
There was a problem hiding this comment.
I’d prefer to keep it even if/when we merge SwiftSyntax + SwiftParser. Technically, the old implementation is not correct because the new arena could have a different parseTriviaFunction than the arena the node is allocated in and thus the trivia pieces could change here if we don’t materialize them.
shahmishal
added a commit
that referenced
this pull request
Jan 26, 2024
…ances Revert "Merge pull request #2425 from ahoppen/ahoppen/sendable-conformances"
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.
Audit all modules in swift-syntax, add
Sendableannotations to the types that are sendable.The audit uncovered that
SyntaxArenawas sent across actor boundaries even thoughSyntaxArenais not thread safe. I fixed these issues by:ParsingSyntaxArenaslightly: If you modify a parsed token and the new arena is a different one than the old arena (and thus might not be aParsingSyntaxArena), we materialize it and its trivia. That way we no longer need to search through all child arenas to find aParsingSyntaxArenaand theirparseTriviafunction. It also fixes a potential correctness issue if the modified node is allocated in aParsingSyntaxArenathat has a differentparseTriviafunction than the originalParsingSyntaxArena.RetainedSyntaxArena: In many cases where we were passing around aSyntaxArena, we just needed to ensure that the syntax arena stayed alive but never accessed any properties of it.RetainedSyntaxArenais an opaque wrapper aroundSyntaxArenaand even thoughSyntaxArenais not sendable, we can send this type across actor boundaries because it doesn’t allow access to the internal state ofSyntaxArena.SyntaxArenaAllocated(Buffer)Pointer: This is a wrapper aroundUnsafe(Buffer)Pointerthat indicates that the buffer is allocated in a syntax arena. Because the syntax arena will always outlive any syntax nodes that reference its contents, we know that the buffer's contents won't get deallocated while being accessed and thus we can add an uncheckedSendableconformance to it.Unsafe(Buffer)Pointeris not sendable by default.SyntaxArena.hasParent, which already had aFIXMEassociated with it. This PR won’t make the situation any worse than it is today, so I just moved theFIXMEbut we should fix it in a follow-up PR.rdar://117813694