Skip to content

nonisolated(unsafe) to opt out of strict concurrency static checking for global variables #2306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 26, 2023
Merged

nonisolated(unsafe) to opt out of strict concurrency static checking for global variables #2306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Sources/SwiftParser/Declarations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ extension DeclarationModifier {
switch self {
case .__consuming, .__setter_access, ._const, ._local, .async,
.borrowing, .class, .consuming, .convenience, .distributed, .dynamic,
.final, .indirect, .infix, .isolated, .lazy, .mutating, .nonisolated,
.nonmutating, .optional, .override, .postfix, .prefix, .reasync,
.required, .rethrows, .static, .weak:
.final, .indirect, .infix, .isolated, .lazy, .mutating, .nonmutating,
.optional, .override, .postfix, .prefix, .reasync, .required,
.rethrows, .static, .weak:
return false
case .fileprivate, .internal, .package, .open, .private,
case .fileprivate, .internal, .nonisolated, .package, .open, .private,
.public, .unowned:
return true
}
Expand Down
27 changes: 23 additions & 4 deletions Sources/SwiftParser/Modifiers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ extension Parser {
}
case (.declarationModifier(.unowned), let handle)?:
elements.append(self.parseUnownedModifier(handle))
case (.declarationModifier(.nonisolated), let handle)?:
elements.append(parseNonisolatedModifier(handle))
case (.declarationModifier(.final), let handle)?,
(.declarationModifier(.required), let handle)?,
(.declarationModifier(.optional), let handle)?,
Expand All @@ -79,7 +81,6 @@ extension Parser {
(.declarationModifier(.indirect), let handle)?,
(.declarationModifier(.isolated), let handle)?,
(.declarationModifier(.async), let handle)?,
(.declarationModifier(.nonisolated), let handle)?,
(.declarationModifier(.distributed), let handle)?,
(.declarationModifier(._const), let handle)?,
(.declarationModifier(._local), let handle)?,
Expand All @@ -98,9 +99,9 @@ extension Parser {
}

extension Parser {
mutating func parseModifierDetail() -> RawDeclModifierDetailSyntax {
mutating func parseModifierDetail(_ keyword: Keyword) -> RawDeclModifierDetailSyntax {
let (unexpectedBeforeLeftParen, leftParen) = self.expect(.leftParen)
let (unexpectedBeforeDetailToken, detailToken) = self.expect(.identifier, TokenSpec(.set, remapping: .identifier), default: .identifier)
let (unexpectedBeforeDetailToken, detailToken) = self.expect(.identifier, TokenSpec(keyword, remapping: .identifier), default: .identifier)
Comment on lines -103 to +104
Copy link
Member

@ahoppen ahoppen Oct 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a good point, and unfortunately any argument is accepted with this current implementation. Which is particularly interesting to me because I noticed that in Modifiers.swift, the unowned attribute is implemented to expect set as a parenthetical argument even though, as far as I can tell from language documentation, unsafe is its potential optional argument. Given that I refactored to use the same optional argument parsing logic that unowned is using, it would seem there is an existing problem there.

Oh, I missed that. In that case you don’t need to change parseModifierDetail at all. We only need to check for set specifically because it’s a lexer-classified keyword and thus not an identifier. unsafe is an identifier at this point still and will only be changed to be a keyword once we expect it.

https://github.com/apple/swift-syntax/blob/f8be751eaefe0d7c72c021f12024a8328a5f3568/Sources/SwiftParser/generated/IsLexerClassified.swift#L18-L20

let (unexpectedBeforeRightParen, rightParen) = self.expect(.rightParen)
return RawDeclModifierDetailSyntax(
unexpectedBeforeLeftParen,
Expand All @@ -118,7 +119,7 @@ extension Parser {

let detail: RawDeclModifierDetailSyntax?
if self.at(.leftParen) {
detail = self.parseModifierDetail()
detail = self.parseModifierDetail(.set)
} else {
detail = nil
}
Expand Down Expand Up @@ -217,4 +218,22 @@ extension Parser {
arena: self.arena
)
}

mutating func parseNonisolatedModifier(_ handle: RecoveryConsumptionHandle) -> RawDeclModifierSyntax {
let (unexpectedBeforeKeyword, keyword) = self.eat(handle)

let detail: RawDeclModifierDetailSyntax?
if self.at(.leftParen) {
detail = self.parseModifierDetail(.unsafe)
} else {
detail = nil
}

return RawDeclModifierSyntax(
unexpectedBeforeKeyword,
name: keyword,
detail: detail,
arena: self.arena
)
}
}
13 changes: 13 additions & 0 deletions Tests/SwiftParserTest/DeclarationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,19 @@ final class DeclarationTests: ParserTestCase {
)
}

func testNonisolatedUnsafeParsing() {
assertParse(
"""
nonisolated(unsafe) let a = 0

struct A {
nonisolated(unsafe) let b = 0
nonisolated(unsafe) var c: Int { 0 }
}
"""
)
}

func testProtocolParsing() {
assertParse("protocol Foo {}")

Expand Down