Remove no-longer-needed `@preconcurrency`s

[MahdiBM]

Motivation

The closed-source Darwin Foundation had marked Date, Data marked as Sendable for a long time. Now both swift-foundation and the swift-corelibs-foundation have them as Sendable as well on Swift 5.9 (5.9.1+ ?): The PRs for swift corelibs-foundation can be found here. We no longer need those @preconcurrencys.

Modifications

Remove @preconcurrencys for Foundation.Date, .Data.

Result

Less warnings in the logs.

Test Plan

Tested on Swift 5.9.1 on Linux.

[MahdiBM]

Should i also remove the @preconcurrencys from the GreetingService examples too?

[MahdiBM]

apparently URL is still not Sendable. I thought it was. I'll modify the PR.

[czechboy0]

Oh cool, thank you @MahdiBM! I wonder if we should do the same in the runtime repo, and the URLSession and AHC transports?

[MahdiBM]

I already have the runtime one ready, was waiting on your response 😅 I'm not sure if the AHC one will need it, but will check.

Update: Somehow AHC is already "updated" since 4 months ago, by you.

[czechboy0]

@swift-server-bot test this please

[czechboy0]

@swift-server-bot test this please

[czechboy0]

Same here, failing on 5.9.0 due to Foundation.Date:1:15: note: struct 'Date' does not conform to the 'Sendable' protocol.

[czechboy0]

@MahdiBM This one will be tricky, as the generated code doesn't know which version it'll run in, so it needs to work on 5.9.0, even if it produces remarks/warnings on newer Linux toolchains.

[simonjbeaumont]

@MahdiBM This one will be tricky, as the generated code doesn't know which version it'll run in, so it needs to work on 5.9.0, even if it produces remarks/warnings on newer Linux toolchains.

I'm not sure I understand this comment. The generated code will still be compiled, just not by us. At the time of compilation we can still check #if swift(...) and if we don't produce warnings on any of our CIs then it shouldn't produce warnings for adopters.

Not counting the case where they fix more issues in Linux toolchains that aren't yet released; for prerelease toolchains we shouldn't expect to be warning-free 100% of the time.

Maybe you were saying we cannot generate code that expects a certain version, the, yes, I agree: the generated code needs to handle all supported versions, but that doesn't mean we can't make it warning free for both Swift 5.9.0 and 5.9.1, or am I misunderstanding something?

[czechboy0]

You're right, if we're willing to generate the conditional compilation code, which I guess there isn't a reason not to.

[simonjbeaumont]

You're right, if we're willing to generate the conditional compilation code, which I guess there isn't a reason not to.

Not only is there no reason not to, this is the only place where it matters. If adopters are compiling with warnings-as-errors then warnings in dependency libraries do not cause errors in the downstream build, but warnings in the generated code will.

Appreciate in this case that these may be remarks or notes (cf. warnings), but in principle the generated code needs to be as clean as we can make it.

[MahdiBM]

Tests Please 🙂

[czechboy0]

@swift-server-bot test this please

[MahdiBM]

@czechboy0 ok that had some obvious problems. Another try please. Hopefully this one or next one should be good.

[simonjbeaumont]

@swift-server-bot test this please

[simonjbeaumont]

@MahdiBM you should be able to test this locally using:

docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.2204.590.yaml run test

[MahdiBM]

@simonjbeaumont thanks, couldn't get that working earlier.

[MahdiBM]

I'm kind of lost why the 5.9.0 tests aren't passing:

/// Types.swift:
#if canImport(Darwin) || swift(>=5.9.1)
import struct Foundation.Date
#else
@preconcurrency import struct Foundation.Date
#endif

This results in this error:

/code/Tests/PetstoreConsumerTests/Generated/Types.swift:1519:36: error: stored property 'createdAt' of 'Sendable'-conforming struct 'bodyPayload' has non-sendable type 'Date'
                        public var createdAt: Foundation.Date
                                   ^

Event removing the #ifs and just using @preconcurrency import struct Foundation.Date doesn't help.

[MahdiBM]

I think one reasonable way to go around this is to use the warnings-as-errors flag for building of the main targets, but not for the test targets. This might also help incase sometime in the future the package needs to live with some warnings in tests just so a part of the code doesn't go untested (e.g. for deprecations, although i saw you had a workaround for silencing those warnings, IIRC)

The solution is simple. Changing the test command:

- command: /bin/bash -xcl "swift $${SWIFT_TEST_VERB-test} $${WARN_AS_ERROR_ARG-} $${SANITIZER_ARG-} $${IMPORT_CHECK_ARG-} $${STRICT_CONCURRENCY_ARG-}"
+ command: /bin/bash -xcl "swift build $${WARN_AS_ERROR_ARG-} $${STRICT_CONCURRENCY_ARG-} $${IMPORT_CHECK_ARG-} && swift $${SWIFT_TEST_VERB-test} $${SANITIZER_ARG-} $${IMPORT_CHECK_ARG-} $${STRICT_CONCURRENCY_ARG-}"

(Replacing swift build with swift $${SWIFT_TEST_VERB-build} if needed)

This makes it so the project is built using the warnings-as-errors flag, but is not tested using that. When using swift build, it does not build the test targets (unless you specify the --build-tests flag).

[czechboy0]

@MahdiBM Are you saying this is a compiler bug? Can you file an issue on the Swift repo with a minimal repro case?

I'd like to get a confirmation from them that this is really a bug that enough #if's can't solve before we turn off warnings-as-errors.

[MahdiBM]

To be honest it could be one of those cases where I'm getting something obvious wrong, but as far as I can tell, yes, I'm saying this is a compiler bug.

I'm not sure how easy it'll be to catch it In a vaccum, but I can try.

I'll also be happy if you could checkout this branch and run the Swift 5.9.0 tests locally and confirm it does seem suspicious to you. It should be as simple as you verifying that the compiler is emitting Date-not-Sendable warnings when @preconcurrency is already there for the Date import 🙂

[czechboy0]

We still have a long list of things we need to do for 1.0, so I won't be able to dig into this before then. Feel free to leave this PR open and move it to draft until post-1.0, then we can investigate further.

[czechboy0]

Marking this as draft for now.

[czechboy0]

Briefly tried to play with this today, but it's just too many changes at once to be able to test this between different versions.

I wonder if you could actually break this up into individual PRs, each PR fixing a single type's remarks/warnings? That should allow us to take better advantage of CI instead of having to spend the time rerunning things locally.

[MahdiBM]

@czechboy0 Sure. I can break it down to 2 PRs: 1- Use canImport(Darwin) instead of the current os(Linux). Will perhaps be slightly more correct and also inline with other OpenAPI libraries. 2- Try to resolve the warnings.

[MahdiBM]

I'll close this then.