New concurrency warnings when building with Swift 5.10

allevato commented 6 months ago

As a consequence of SE-0412: Strict concurrency for global variables, we're getting new concurrency warnings when building swift-protobuf. They can be narrowed down to two places:

Default instances of storage classes:

swift-protobuf/Sources/SwiftProtobuf/AnyMessageStorage.swift:144:14: error: static property 'defaultInstance'
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
  static let defaultInstance = AnyMessageStorage()
             ^

swift-protobuf/Sources/SwiftProtobuf/descriptor.pb.swift:3111:16: error: static property 'defaultInstance' 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
    static let defaultInstance = _StorageClass()
             ^

The _NameMap of each generated message:

swift-protobuf/Sources/SwiftProtobuf/any.pb.swift:195:21: error: static property '_protobuf_nameMap' 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
  public static let _protobuf_nameMap: _NameMap = [
                    ^

SE-0412 added the requirements that:

Under strict concurrency checking, require every global variable to either be isolated to a global actor or be both:

immutable

of Sendable type

Neither the storage classes nor _NameMap are Sendable. I need some more eyes who are better versed with Swift concurrency than I am 🙂

Storage classes

For the storage classes, we could just drop the defaultInstance property entirely and just allocate a new empty instance. However, it is a nice optimization to defer allocation of storage until the message is actually mutated. It's not super important if you're creating a new message and then immediately parsing or mutating it, but anywhere that we return an empty message as a default value of an unset message-typed field, it's very nice to not have to allocate storage if the user is only going to read from it.

Can we just mark the storage classes as Sendable on @unchecked Sendable since the messages themselves already conform to that, and the storage types can't be accessed from outside the message?

_NameMap

_NameMap looked more challenging because of the string interning logic and storage of Unsafe*Pointers, but upon further reflection, a _NameMap can never be mutated after its been initialized, so I think we could just declare _NameMap to be @unchecked Sendable and call it a day, right?

As a follow-up, now that Swift Strings are UTF-8 backed, we could probably revisit the UTF-8 buffer interning approach and just store Strings directly. The UTF-8 buffer interning dates back to 2017, so it was when Swift native strings weren't guaranteed to be UTF-8 backed, which I believe was the original motivation for the buffer approach.

thomasvl commented 6 months ago

fyi - @tbkka @FranzBusch @Lukasa

thomasvl commented 6 months ago

I went ahead and opened #1561 for topic of revisiting _NameMap. It might make sense do the @unchecked Sendable to get pass the warnings "right now", but doing that revisit to potential remove the issue seems like it might be a good thing in general.

Lukasa commented 6 months ago

AnyMessageStorage definitely looks sketchy.

Lukasa commented 6 months ago

Ok, so I think AnyMessageStorage is probably fine, but the default instance is causing us some pain. This is because it's used to skip an allocation, as any mutation to the storage should be CoW'd away. All well and good, but it means we're hitting this. @tbkka is it possible for us to suppress this warning on a per-attribute basis, on the grounds that we're only using it safely?

thomasvl commented 6 months ago

Why would AnyMessageStorage be any different than an other of the _Storage classes? The outer Message will call _uniqueStorage() before triggering any mutations within the AnyMessageStorage; so doesn't that make the usage the same as the others?

The thing being flagged about AnyMessageStorage is actually the same as the generated ones, it's the static let defaultInstance() = ... initialization. Right?

tbkka commented 6 months ago

Based on my (limited) understanding, we should mark the backing class as @unchecked Sendable. It's not an entirely satisfying answer -- we're basically just saying to the compiler "trust us, we never use this in a bad way" -- but it seems the best we can do for now.

FranzBusch commented 6 months ago

I don't think we should make the backing class @unchecked Sendable but rather the struct holding the class. Since that struct is enforcing the CoW behaviour. If we would ever hold that backing class somewhere differently then that place also needs to make sure it implements the CoW correctly.

thomasvl commented 6 months ago

I don't think we should make the backing class @unchecked Sendable but rather the struct holding the class. Since that struct is enforcing the CoW behaviour. If we would ever hold that backing class somewhere differently then that place also needs to make sure it implements the CoW correctly.

What do you mean by backing class? And holding class?

Right now we mark the message structs at @unchecked Sendable and the storage classes with nothing, but the problem is the static let defaultInstance within the storage class.

[Edited]

tbkka commented 6 months ago

What do you mean by backing class?

The _Storage classes.

I don't think we should make the backing class @unchecked Sendable ...

@FranzBusch So how do we deal with the static global that holds an empty copy of that class? Those are the errors that @thomasvl is referring to above.

tbkka commented 6 months ago

@FranzBusch suggested to me offline that

We use nonisolated(unsafe) on the global/static vars that hold the default instances
The _Storage classes remain unmarked
The CoW structs be @unchecked Sendable

thomasvl commented 6 months ago

Opened #1563 for some of the issues.

thomasvl commented 6 months ago

With the repeated ones out of the way, here are some of the additional ones I'm seeing:

/__w/swift-protobuf/swift-protobuf/Sources/SwiftProtobuf/Google_Protobuf_Any+Registry.swift:20:17: warning: let 'knownTypesQueue' 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
fileprivate let knownTypesQueue =
                ^
/__w/swift-protobuf/swift-protobuf/.build/x86_64-unknown-linux-gnu/release/SwiftProtobuf.build/DerivedSources/resource_bundle_accessor.swift:4:16: warning: static property 'module' 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
    static let module: Bundle = {
               ^
[16/29] Compiling SwiftProtobufTestHelpers Descriptor+TestHelpers.swift
[17/29] Compiling SwiftProtobufPluginLibrary CodeGenerator.swift
/__w/swift-protobuf/swift-protobuf/Sources/SwiftProtobufPluginLibrary/NamingUtils.swift:364:17: warning: let 'backtickCharacterSet' 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
fileprivate let backtickCharacterSet = CharacterSet(charactersIn: "`")
                ^
/__w/swift-protobuf/swift-protobuf/.build/x86_[64](https://github.com/apple/swift-protobuf/actions/runs/8297904866/job/22710035443?pr=1577#step:4:65)-unknown-linux-gnu/release/SwiftProtobufPluginLibrary.build/DerivedSources/resource_bundle_accessor.swift:4:16: warning: static property 'module' 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
    static let module: Bundle = {
               ^

thomasvl commented 6 months ago

The module: Bundle ones are generate code by SwiftPM, anyone know if that's already tracked by for Swift in general?

Lukasa commented 6 months ago

I couldn't find an immediate issue, can't hurt to file a new one.