New concurrency warnings when building with Swift 5.10 #1560

allevato · 2024-02-29T14:07:33Z

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.

The text was updated successfully, but these errors were encountered:

thomasvl · 2024-02-29T14:12:06Z

fyi - @tbkka @FranzBusch @Lukasa

thomasvl · 2024-02-29T14:19:57Z

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 · 2024-02-29T16:26:59Z

AnyMessageStorage definitely looks sketchy.

Lukasa · 2024-02-29T16:29:44Z

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 · 2024-02-29T16:42:44Z

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 · 2024-02-29T17:05:11Z

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 · 2024-02-29T17:27:38Z

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 · 2024-02-29T17:32:51Z

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 · 2024-02-29T17:35:39Z

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 · 2024-02-29T17:57:47Z

@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 · 2024-02-29T20:06:58Z

Opened #1563 for some of the issues.

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

thomasvl · 2024-03-15T14:43:52Z

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 · 2024-03-15T14:44:47Z

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

Lukasa · 2024-03-15T15:27:51Z

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

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

I think this is the rest of the issues in #1560 This is the different approach discussed in #1564.

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

thomasvl · 2024-03-19T13:48:08Z

Actually, I might have pulled those off the branch build, where I think some things haven't built, let me try the main branch again.

thomasvl · 2024-03-19T14:16:55Z

Ok, here's a test on main: https://github.com/apple/swift-protobuf/actions/runs/8344464434/job/22837023897?pr=1585

/__w/swift-protobuf/swift-protobuf/main/Sources/SwiftProtobuf/Google_Protobuf_Any+Registry.swift:20:17: error: 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/main/.build/x86_64-unknown-linux-gnu/debug/SwiftProtobuf.build/DerivedSources/resource_bundle_accessor.swift:4:16: error: 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 = {
               ^

Aside: things get printing multiple times which makes me wonder if 5.10 has some other issue going on.

@FranzBusch @Lukasa @tbkka – given the problems with resource_bundle_accessor.swift it seems like we'll have to stop using -Xswiftc -warnings-as-errors, do you see any other way around this?

thomasvl · 2024-03-19T14:27:21Z

Oh no. If I build on my Mac with Xcode 15.3, I don't get either of these. Does that mean Linux's version of DispatchQueue might not be annotated correctly in 5.10?

Any suggestions for that then?

thomasvl · 2024-03-19T14:28:36Z

I guess we could hack the resource one with a #if check for Linux and not add the privacy manifest info?

AppleStore only requires the informatiion on iOS, watchOS, tvOS, visionOS; so exclude the resources for Linux and macOS. As of Swift 5.10 (apple#1560), the generated Linux code when there are resources gets flagged with issues when using `StrictConcurrency=complete`; so this dodges the issue.

Lukasa · 2024-03-19T14:55:00Z

For the dispatch queue issue, use @preconcurrency import to silence it on Linux.

Lukasa · 2024-03-19T14:55:26Z

Did you file an issue for the Module warning?

thomasvl · 2024-03-19T14:56:53Z

For the dispatch queue issue, use @preconcurrency import to silence it on Linux.

Ha, I'm testing that out now, I'll move that to it's own PR shortly.

Did you file an issue for the Module warning?

Haven't gotten there yet. If you'd got the time to do it, it will likely get more attention if filed by one of you.

thomasvl · 2024-03-19T14:59:14Z

@preconcurrency appears to be a nogo:

/__w/swift-protobuf/swift-protobuf/main/Sources/SwiftProtobuf/Google_Protobuf_Any+Registry.swift:20:17: remark: '@preconcurrency' attribute on module 'Dispatch' is unused
@preconcurrency import Dispatch
~~~~~~~~~~~~~~~~^

Lukasa · 2024-03-19T15:31:32Z

This warning is firing on Linux?

thomasvl · 2024-03-19T15:48:14Z

This warning is firing on Linux?

Yup - https://github.com/apple/swift-protobuf/actions/runs/8345218414/job/22839591811?pr=1585

I think this is the rest of the issues in #1560 This is the different approach discussed in #1564.

See apple#1560 for the details, but things currently don't pass on linux, and some of the issues are within the Swift toolchain.

See #1560 for the details, but things currently don't pass on linux, and some of the issues are within the Swift toolchain.

thomasvl · 2024-04-18T14:14:24Z

I don't appear to get the CharacterSet warning when building on macOS either, so I guess that's another issue within Linux Foundation.

So I think we're down to all problems in the Swift Linux distro now. Everything we can control should be warning free.

FranzBusch · 2024-04-24T10:18:53Z

We should be able to fix the warning with CharacterSet by using a @preconcurrency import struct Foundation.CharacterSet. Importantly this import needs to be conditionalised on the platform by using #if canImport(Darwin). Only on Darwin we can avoid the @preconcurrency import since there it is correctly annotated.

thomasvl · 2024-04-26T14:53:43Z

Looking back at that code, we don't really need to use CharacterSet, I'll do a quick PR to drop it.

thomasvl · 2024-04-26T17:00:11Z

Now we're down to just the generated file for resources and then the dispatch related one.

Guess it's safe to close this now since we've done everything we can for the 5.10 specific issues.

thomasvl · 2024-07-08T15:20:27Z

I happened to be looking at swift-nio, and realized they did do the conditional in Package.swift that would break cross comp for the privacy manifest, do we want to do that also? (Our use of Dispatch for the Any registry will still prevent warnings-as-errors)

thomasvl · 2024-09-20T19:54:30Z

I'm going to go ahead and close this since it was specific to 5.10. We may end up revisit some things for Swift 6, but it feels like that should be a new issue.

rebello95 · 2024-10-03T05:16:47Z

Related: #1729

thomasvl mentioned this issue Feb 29, 2024

Revisit _NameMap in light of Swift 5 changing the default String encoding #1561

Open

thomasvl mentioned this issue Feb 29, 2024

For Swift 5.10, mark Storage defaultInstance nonisolated(unsafe) #1563

Merged

thomasvl mentioned this issue Feb 29, 2024

For Swift 5.10, mark Message/Enum _protobuf_nameMap nonisolated(unsafe) #1564

Closed

thomasvl mentioned this issue Mar 14, 2024

Add swift 5.10 to CI to the 1.x branch. #1570

Merged

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 15, 2024

Mark _NameMap and supporting types as Sendable.

5720244

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

thomasvl mentioned this issue Mar 15, 2024

Mark _NameMap and supporting types as Sendable. #1577

Merged

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 18, 2024

Mark _NameMap and supporting types as Sendable.

688b640

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

thomasvl added a commit that referenced this issue Mar 19, 2024

Mark _NameMap and supporting types as Sendable.

8c295ee

I think this is the rest of the issues in #1560 This is the different approach discussed in #1564.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 19, 2024

Mark _NameMap and supporting types as Sendable.

281abb5

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 19, 2024

Mark _NameMap and supporting types as Sendable.

627d77b

I think this is the rest of the issues in apple#1560 This is the different approach discussed in apple#1564.

thomasvl mentioned this issue Mar 19, 2024

Only include PrivacyInfo.xcprivacy on some platforms. #1586

Closed

thomasvl added a commit that referenced this issue Mar 25, 2024

Mark _NameMap and supporting types as Sendable.

2341e0f

I think this is the rest of the issues in #1560 This is the different approach discussed in #1564.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 26, 2024

Don't use -warings-as-errors on 5.10.

b01afe8

See apple#1560 for the details, but things currently don't pass on linux, and some of the issues are within the Swift toolchain.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 26, 2024

Don't use -warings-as-errors on 5.10.

1abb6a8

See apple#1560 for the details, but things currently don't pass on linux, and some of the issues are within the Swift toolchain.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Mar 27, 2024

Don't use -warings-as-errors on 5.10.

db6cf7a

See apple#1560 for the details, but things currently don't pass on linux, and some of the issues are within the Swift toolchain.

thomasvl added a commit that referenced this issue Mar 29, 2024

Don't use -warings-as-errors on 5.10.

10a3202

See #1560 for the details, but things currently don't pass on linux, and some of the issues are within the Swift toolchain.

thomasvl changed the title ~~New concurrency warnings when building with Xcode 15.3 beta (Swift 5.10)~~ New concurrency warnings when building with Swift 5.10 Jul 8, 2024

thomasvl closed this as completed Sep 20, 2024

rebello95 mentioned this issue Oct 3, 2024

Swift 6 concurrency error on _StorageClass.defaultInstance #1729

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New concurrency warnings when building with Swift 5.10 #1560

New concurrency warnings when building with Swift 5.10 #1560

allevato commented Feb 29, 2024 •

edited

Loading

thomasvl commented Feb 29, 2024

thomasvl commented Feb 29, 2024

Lukasa commented Feb 29, 2024

Lukasa commented Feb 29, 2024

thomasvl commented Feb 29, 2024

tbkka commented Feb 29, 2024

FranzBusch commented Feb 29, 2024

thomasvl commented Feb 29, 2024 •

edited

Loading

tbkka commented Feb 29, 2024

tbkka commented Feb 29, 2024

thomasvl commented Feb 29, 2024

thomasvl commented Mar 15, 2024

thomasvl commented Mar 15, 2024

Lukasa commented Mar 15, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

Lukasa commented Mar 19, 2024

Lukasa commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

Lukasa commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Apr 18, 2024

FranzBusch commented Apr 24, 2024

thomasvl commented Apr 26, 2024

thomasvl commented Apr 26, 2024

thomasvl commented Jul 8, 2024

thomasvl commented Sep 20, 2024

rebello95 commented Oct 3, 2024

New concurrency warnings when building with Swift 5.10 #1560

New concurrency warnings when building with Swift 5.10 #1560

Comments

allevato commented Feb 29, 2024 • edited Loading

Storage classes

_NameMap

thomasvl commented Feb 29, 2024

thomasvl commented Feb 29, 2024

Lukasa commented Feb 29, 2024

Lukasa commented Feb 29, 2024

thomasvl commented Feb 29, 2024

tbkka commented Feb 29, 2024

FranzBusch commented Feb 29, 2024

thomasvl commented Feb 29, 2024 • edited Loading

tbkka commented Feb 29, 2024

tbkka commented Feb 29, 2024

thomasvl commented Feb 29, 2024

thomasvl commented Mar 15, 2024

thomasvl commented Mar 15, 2024

Lukasa commented Mar 15, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

Lukasa commented Mar 19, 2024

Lukasa commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Mar 19, 2024

Lukasa commented Mar 19, 2024

thomasvl commented Mar 19, 2024

thomasvl commented Apr 18, 2024

FranzBusch commented Apr 24, 2024

thomasvl commented Apr 26, 2024

thomasvl commented Apr 26, 2024

thomasvl commented Jul 8, 2024

thomasvl commented Sep 20, 2024

rebello95 commented Oct 3, 2024

allevato commented Feb 29, 2024 •

edited

Loading

thomasvl commented Feb 29, 2024 •

edited

Loading