swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.49k stars 10.35k forks source link

[SR-15238] Key paths are not "Sendable" #57560

Closed stephencelis closed 2 months ago

stephencelis commented 3 years ago
Previous ID SR-15238
Radar rdar://problem/83465949
Original Reporter @stephencelis
Type Bug
Environment Swift 5.5+ (with -Xfrontend -warn-concurrency)
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 2 | |Component/s | Compiler | |Labels | Bug, Concurrency, TypeChecker | |Assignee | None | |Priority | Medium | md5: e9831dccbc5222ce92d77b619d62c314

Issue Description:

Key paths cannot be captured inside code that should be concurrently executable. To repro, try compiling the following code with the -Xfrontend -warn-concurrency build flag.

func f(_ f: @Sendable @escaping () -> Void) {}

func g() {
  let kp = \String.count
  f { kp } // Cannot use let 'kp' with a non-sendable type 'KeyPath<String, Int>' from concurrently-executed code
}

While the proposal suggests that key paths should conform to sendable (except for subscripts in which the referenced types are not sendable), I do not see any conformance in the repo.

typesanitizer commented 3 years ago

@swift-ci create

nh7a commented 2 years ago

With Xcode 14.1 beta 3 with SWIFT_STRICT_CONCURRENCY = "targeted", I'm seeing a lot of warnings as below.

Cannot form key path that captures non-sendable type 'KeyPath<AttributeScopes.SwiftUIAttributes, AttributeScopes.SwiftUIAttributes.FontAttribute> Generic class 'KeyPath' does not conform to the 'Sendable' protocol

and I can't even tell where it's happening.

BastianKusserow commented 1 year ago

@nh7a did you by any chance find out how to solve or at least suppress those warnings? Normally I'd think I can just suppress the warning with an @preconcurrency import, but as KeyPath lives in the swift standard library, I think I can't do that?

kokluch commented 1 year ago

I'm facing the same issue. Lot's of warnings for KeyPath non-sendable conformance in various files where there are no KeyPath what so ever...

"Generic class 'KeyPath' does not conform to the 'Sendable' protocol"

jpsim commented 1 year ago

This is a significant pain point when using SWIFT_STRICT_CONCURRENCY=complete with Swift 5.8/5.9 as well.

In particular the AttributedString APIs that use dynamic member lookup completely break:

var string = AttributedString("Hello")
string.accessibilitySpeechSpellsOutCharacters = true

Produces this error:

<unknown>:0: error: cannot form key path that captures non-sendable type 'KeyPath<AttributeScopes.AccessibilityAttributes, AttributeScopes.AccessibilityAttributes.SpellOutAttribute>'
Swift.KeyPath:1:14: note: generic class 'KeyPath' does not conform to the 'Sendable' protocol
public class KeyPath<Root, Value> : PartialKeyPath<Root> {
             ^
error: fatalError

As mentioned by others in this issue, the diagnostics have no line/column information so it can be hard to pinpoint what line is causing the issue.

Changing -strict-concurrency=targeted works around the problem. I've filed FB13085158 to track the bug on the Foundation side of things.

For AttributedString in particular, I've been able to work around it by using more verbose APIs:

var attributes = AttributeContainer()
attributes[AttributeScopes.AccessibilityAttributes.SpellOutAttribute.self] = true
let string = AttributedString("Hello", attributes: attributes)

But that's very non-obvious that you can even do this.

hborla commented 8 months ago

This will be resolved with SE-0418. You can test it out using a main development snapshot and enabling -enable-experimental-feature InferSendableFromCaptures.

jpsim commented 8 months ago

Thanks for the good news @hborla will it be included in 5.10? If not, is it possible for it to be cherry-picked?

StefaniOSApps commented 7 months ago

Hello everyone,

I wanted to share a workaround specifically for suppressing the non-Sendable warning for KeyPaths in concurrent Swift code:

extension KeyPath: @unchecked Sendable {}

This approach is targeted solely at the Sendable conformance warning for KeyPaths and should be used cautiously as a temporary solution. It allows continued development without the immediate need to refactor large codebases, awaiting a more permanent resolution.

Looking forward to any input or better alternatives from the community.

Best, Stefan

McNight commented 5 months ago
extension KeyPath: @unchecked Sendable {}

Thanks for your workaround. Maybe we can improve it by leveraging compilation condition checks ?!

#if compiler(<6.0) || !hasFeature(InferSendableFromCaptures)
#warning("Workaround trading a bunch of Strict Concurrency related warnings. To be removed when Swift 6.0 is available.")
extension KeyPath: @unchecked Sendable {}
#endif

EDIT: Sticked a warning inside.

jhaungs commented 4 months ago

According to the Note in the Type Properties section of the TSPL book, static initialization is guaranteed to be thread-safe:

Stored type properties are lazily initialized on their first access. 
They’re guaranteed to be initialized only once, even when accessed by multiple threads simultaneously...

If Swift 6 no longer upholds this claim, then the book note is out of date. If it still does, then is this compiler warning even correct? Especially given that this is a let, which should definitely be initialized exactly once on first use.

Static property 'Counter' is not concurrency-safe because non-'Sendable' type 'KeyPathComparator' may have shared mutable state

struct Thing {
  var counter: Int
  static let Counter = KeyPathComparator(\Self.counter)
}
3a4oT commented 2 months ago

Xcode 16 beta 4. This still causes problems.

hborla commented 2 months ago

Xcode 16 beta 4. This still causes problems.

You need to enable the InferSendableFromCaptures upcoming feature.

hborla commented 2 months ago

https://github.com/swiftlang/swift-evolution/blob/main/proposals/0418-inferring-sendable-for-methods.md is implemented in 6.0.

3a4oT commented 2 months ago

hey @hborla , thanks for the reply. maybe I hold it wrong but I put it under Package.swift like:

package.targets.filter { $0.type != .system }.forEach { target in
    var settings: [SwiftSetting] = target.swiftSettings ?? []

     // Enable concurrency  checks for all targets
    settings.append(.enableExperimentalFeature("StrictConcurrency=complete"))

    // experimental feature PER
    // https://github.com/apple/swift-evolution/blob/main/proposals/0409-access-level-on-imports.md
      settings.append(.enableExperimentalFeature("AccessLevelOnImport"))
    // https://github.com/swiftlang/swift-evolution/blob/main/proposals/0418-inferring-sendable-for-methods.md
     settings.append(.enableExperimentalFeature("InferSendableFromCaptures"))

    target.swiftSettings = settings
}

It doesn't work for me. At least Xcode shows 15k warnings even after clean build.

3a4oT commented 2 months ago

PS. If I move it directly to the target settings, it seems to work:

    .target(name: "SomeClient",
            dependencies: [.product(name: "ComposableArchitecture",
                                    package: "swift-composable-architecture")],
            path: "Sources/Utils/SomeClient",
            swiftSettings: [
                .enableExperimentalFeature("InferSendableFromCaptures")
            ]),
3a4oT commented 2 months ago

Seems like when I have both at:

            swiftSettings: [
                .enableExperimentalFeature("InferSendableFromCaptures"),
                .enableExperimentalFeature("StrictConcurrency=complete")
            ]),

Xcode produces warnings. Is it expected?

xedin commented 2 months ago

@3a4oT InferSendableFromCaptures is an upcoming feature now so you should use .enableUpcomingFeature("InferSendableFromCaptures")

3a4oT commented 2 months ago

@3a4oT InferSendableFromCaptures is an upcoming feature now, so you should use .enableUpcomingFeature("InferSendableFromCaptures")

Thanks, @xedin. It helped and now works as expected on Xcode 16 beta 4. How would I know that it's time to move from .enableExperimentalFeature to enableUpcomingFeature setting? How is it related to older Swift versions, for instance - our main focus is Swift 5.10 and Xcode 15.x until Swift 6 is officially released. Should I worry about swift version checks and apply different settings based on Swift version?

AnthonyLatsis commented 2 months ago

How would I know that it's time to move from .enableExperimentalFeature to enableUpcomingFeature setting?

For context, experimental features are a grey zone in our features system — little is certain about them. Experimental features that are available in released compilers may be seen as guaranteed to be promoted, but I am not aware of any mention of such a guarantee in the official docs and very much doubt the existence of one. If an experimental feature is expected to be promoted, e.g. by being associated with an accepted Swift evolution proposal, you could enable it both as an experimental and upcoming feature. The compiler will not error out on unrecognized features, and one of the two settings will take effect. Then you could audit the settings to see if the feature is still experimental in swift-tools-version every time you bump the minimum version. If a time comes when you want your package to support both an old version of Swift where the feature is upcoming and a new version of Swift where the feature is enabled by default (and hence enabling it via enableUpcomingFeature is a compilation error), then the corresponding enableUpcomingFeature setting will need to be applied inside a #if compiler(<X) conditional compilation block.

hborla commented 2 months ago

InferSendableFromCaptures was never an experimental feature in a shipping version of Swift. It was only an experimental feature in the nightly toolchains while under development during the Swift Evolution process.

If you're trying out a feature in a Swift.org development snapshot, check the proposal to see whether you need to use enableExperimentalFeature or enableUpcomingFeature (the proposal header will say if it's still in an experimental state or if it's fully implemented). If you're trying out a feature in an Xcode beta or in a shipping version of Swift, you should always use enableUpcomingFeature.

AnthonyLatsis commented 2 months ago

If you're trying out a feature in an Xcode beta or in a shipping version of Swift, you should always use enableUpcomingFeature.

So the purpose of making an experimental feature enablable in production is to allow enabling it in release-asserts dev snapshots? If so, why do we allow enabling them in Swift releases?

hborla commented 2 months ago

So the purpose of making an experimental feature enablable in production is to allow enabling it in release-asserts dev snapshots? If so, why do we allow enabling them in Swift releases?

No, this particular experimental feature was never enable-able in production. Experimental features are always enable-able in release-asserts builds. The argument to EXPERIMENTAL_FEATURE is to make the feature enable-able in noasserts builds. However, people make mistakes on this argument so frequently that I'm considering removing it to make it much more explicit which features are enable-able in production.

AnthonyLatsis commented 2 months ago

The argument to EXPERIMENTAL_FEATURE is to make the feature enable-able in noasserts builds.

Ah, right, thanks for clarifying.

https://github.com/swiftlang/swift/blob/8b7a75c8f3a01063d00e6425ab5ecb8790bda97e/include/swift/Basic/LangOptions.h#L615-L621

OguzYuuksel commented 4 weeks ago

I cannot make it work with Xcode 16. Anyone else?

connorhargus commented 3 weeks ago

For those not using a Package.swift file in Xcode 16 find "Infer Sendable for Methods and Key Path Literals" in your Project's Build Settings