-
Notifications
You must be signed in to change notification settings - Fork 64
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.
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
Enable strict concurrency #213
Conversation
Motivation: To catch potential data races at build time. Modifications: - Bump Swift tools version from 5.8 to 5.9 in Package.swift. - Enable strict concurrency in Package.swift. - Adjust documentation to section on the supported Swift versions. - Implement minor fixes for the surfaced strict concurrency warnings. - Add `-require-explicit-sendable` to the 6.0, nightly 6.0 and nightly main CI checks. Result: Strict concurrency adoption.
Tests/X509Tests/SignatureTests.swift
Outdated
@@ -12,7 +12,7 @@ | |||
// | |||
//===----------------------------------------------------------------------===// | |||
|
|||
import XCTest | |||
@preconcurrency import XCTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is right. What errors did this silence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These:
Static property 'secKeyRSA' is not concurrency-safe because non-'Sendable' type 'SecKey?' may have shared mutable state; this is an error in the Swift 6 language mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we have a missing import. Instead of putting @preconcurrency
here, please add this:
#if canImport(Darwin)
@preconcurrency import Security
#endif
@@ -12,7 +12,7 @@ | |||
// | |||
//===----------------------------------------------------------------------===// | |||
|
|||
import XCTest | |||
@preconcurrency import XCTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly I'm not convinced this is right, what errors did this silence?
.github/workflows/main.yml
Outdated
@@ -13,9 +13,9 @@ jobs: | |||
with: | |||
linux_5_9_arguments_override: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | |||
linux_5_10_arguments_override: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to lose warnings-as-errors on 5.10, as we can no longer maintain it with this change.
.github/workflows/main.yml
Outdated
linux_6_0_arguments_override: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
linux_nightly_6_0_arguments_override: "--explicit-target-dependency-import-check error" | ||
linux_nightly_main_arguments_override: "--explicit-target-dependency-import-check error" | ||
linux_5_10_arguments_override: "-Xswiftc --explicit-target-dependency-import-check error" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed removing the -Xswiftc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thank you!
Motivation:
To catch potential data races at build time.
Modifications:
-require-explicit-sendable
to the 6.0, nightly 6.0 and nightly main CI checks.Result:
Strict concurrency adoption.