Enable more flags to check for concurrency compatibility

[finestructure]

Discussed in #3442

^{Originally posted by mattmassicotte October 17, 2024}
Hello friends! There are two additional flags that, while technically can be source-incompatible, can also have a meaningfully-positive impact on warnings produced by strict concurrency.

InferSendableFromCaptures: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0418-inferring-sendable-for-methods.md
GlobalActorIsolatedTypesUsability: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0434-global-actor-isolated-types-usability.md

I've had to use both of these to address warnings that I would otherwise not be able to fix. I think it may make sense to consider enabling these as well. Thoughts?

[daveverwer]

I'm not sure how much of an impact these will have on the totals but we should make sure to mark the date when these flags went into effect on the Ready for Swift 6 charts in case their impact is significant.

[finestructure]

I've tried applying these flags with Xcode 16.1 but I'm struggling to find a variant that works with SwiftPM builds:

🖥️ env DEVELOPER_DIR=/Applications/Xcode-16.1.0.app xcrun swift build --arch arm64 -Xswiftc -Xfrontend -Xswiftc -stats-output-dir -Xswiftc -Xfrontend -Xswiftc .stats -Xswiftc -strict-concurrency=complete -Xswiftc -enable-upcoming-feature=InferSendableFromCaptures -Xswiftc -enable-upcoming-feature=GlobalActorIsolatedTypesUsability
Building for debugging...
[0/3] Write sources
[1/3] Copying Documentation.docc
[2/3] Write swift-version--7754E27361AE5C74.txt
error: unknown argument: '-enable-upcoming-feature=InferSendableFromCaptures'

I've tried all manner of variations:

• with/without -Xswiftc
• with both -enable-... and --enable-...
• with both ...-feature=XXX and ...-feature XXX

It does appear to be working when setting OTHER_SWIFT_FLAGS for the xcodebuild builds.

[finestructure]

cc @mattmassicotte

[mattmassicotte]

hmmm. How about this:

-Xswiftc -enable-upcoming-feature -Xswiftc GlobalActorIsolatedTypesUsability`

[finestructure]

Ah, of course, it's a -Xswiftc per flag! That works, thanks a lot for the pointer.

We're currently reprocessing with 16.1 so these new flags won't land straight away (so we don't mix results). It'll go live later this week.

[finestructure]

So I have this in place and passing our builder tests but I'm having second thoughts whether we should run with these flags.

We're already seeing discrepancies between what we report and what users see and these custom flags will make it even harder for authors to compare their local results with what we show. Enabling complete concurrency checking or running in language mode 6 are well enough documented but setting upcoming feature flags is trickier.

While this would presumably help with the error and compatible package counts, shouldn't we be testing as plain a build config as possible?

To you point, @mattmassicotte

I've had to use both of these to address warnings that I would otherwise not be able to fix. I think it may make sense to consider enabling these as well. Thoughts?

On reflection, I don't think we want to achieve the minimum number of warnings, we want to show the number of warnings you're getting when you run the stock compiler with concurrency warnings enabled.

This may guide the decision on defaults but it can't do so if we part with the defaults.

[daveverwer]

While this would presumably help with the error and compatible package counts, shouldn't we be testing as plain a build config as possible?

I agree wholeheartedly with this 👍

[mattmassicotte]

Makes total sense! It's true that the only way to get a valid picture of Swift 6 mode compatibiilty is by running with Swift 6 mode. That's not the same thing as Swift 5 + StrictConcurrency, but that may not be what you want. Totally trust your judgement here!

[finestructure]

the only way to get a valid picture of Swift 6 mode compatibiilty is by running with Swift 6 mode

We run in Swift 5 mode with -strict-concurrency=complete because it's the only way to get stats for concurrency warnings. It's an attempt at running as close as possible to Swift 6 mode with something like "continue build on error".

If we ran in Swift 6 mode we could only really report a boolean "worked" / "didn't work" and no error or warning count, unfortunately.

[mattmassicotte]

This is not as close as possible to Swift 6 mode though! Swift 6 mode is, roughly, all flags on + concurrency warnings become errors.

But, I totally get that these things are in tension. If you turn on the ~ 20 upcoming feature flags, you are almost at Swift 6 mode while preserving warnings. But you also have the most annoying-to-reproduce build set up. I think keeping things simple is very desirable though.

[finestructure]

I actually almost raised that question yesterday, if including these flags is closer to mode 6 than just mode 5 + -strict-concurrency=complete. So you're saying we'd have to pass in 20 additional flags in this awkward -Xswiftc -enable-upcoming-feature -Xswiftc X style...? 😵‍💫

One of the questions is what users are more likely interested in:

• an issue count if the package was compiled with language mode 6
• an issue count if the package was compiled with language mode 5 + complete concurrency checking

When planning the feature we assumed these two to be the same but if that's not the case, we should revisit that assumption.

A few more observations/thoughts:

• We'd be changing a setting while we're gathering data (earlier data points were gathered with different settings). We can highlight this in the plot.
• Unless we add the flags we're unlikely to actually see any changes to the issue count from the compiler side. Language mode 5 + complete concurrency is not going to materially change going forward, so we'd only be measuring package authors' efforts.
• I wish there was an easier way to opt into mode 6 that captures all issues other then running mode 5 + all the flags. The reason we don't want to run with mode 6 is that it will stop at the first error and not give us a complete picture. We lack a mode 6 + errors as warnings.
• If we adopted the flags, how would we ensure to keep up to date if there are more flags to be added?

I think we may want mode 5 + all the flags, awkward as it is. It's about language mode 6 and mode + complete concurrency doesn't seem to cover it well enough.

[daveverwer]

One of the questions is what users are more likely interested in:

• an issue count if the package was compiled with language mode 6
• an issue count if the package was compiled with language mode 5 + complete concurrency checking

The answer to this is clear to me. Swift 5 mode + extra concurrency warnings is no longer a real world situation now that Swift 6 is released.

Developers want the results as if Swift 6 mode were switched on.

It's truly a shame that we can't get the error numbers in that situation, but in my opinion that's clearly what we're aiming for.

[mattmassicotte]

You are correct. The Swift 6 language mode is (pretty much) defined as Swift 5 + ALL upcoming feature flags. So, turning on any more flags gets you closer to 6 mode. I just specifically suggested these particular flags because they can have a meaningful impact on concurrency-specific warnings.

There's no way I know of to go in the opposite direction and only downgrade errors to warnings in 6 mode.

[finestructure]

Just to record a link here as well, there might be a resource to track what flags we should be using in the making here: https://toot.iamkonstantin.eu/@konstantin/113464415244283221

[finestructure]

Also cc-ing @hborla for awareness/review of the idea to include additional build flags.

[finestructure]

Looking through the list of flags here: https://flags.swiftythemes.com/language-features/6.0.html I've come up with the following list of "upcoming" feature flags to include:

• DisableOutwardActorInference
• DynamicActorIsolation
• GlobalActorIsolatedTypesUsability
• GlobalConcurrency
• InferSendableFromCaptures
• IsolatedDefaultValues
• RegionBasedIsolation
• StrictConcurrency

I don't think we want to include any of the "experimental" ones, do we?

cc @mattmassicotte @hborla

[DagAgren]

The answer to this is clear to me. Swift 5 mode + extra concurrency warnings is no longer a real world situation now that Swift 6 is released.

This is not quite true. I work on a large project migrating to Swift 6, and the thing we found is that mixing Swift 5 and Swift 6 mode in different packages in the same codebase is actually really dangerous, so we are new running our "Swift 6" code as Swift 5 + all flags and will keep doing it until we can switch to Swift 6 for every part of the codebase at once.

[mattmassicotte]

@DagAgren This is a well-understood (but terrible) situation. You have Swift 5 code that is incorrectly annotated, combined with the DynamicActorIsolation flag (which is on with Swift 6 mode). You can selectively disable the runtime assertions, if you want to admit data races though. If you have turned on DynamicActorIsolation with Swift 5 code, you should also see these assertions trip.

One thing is StrictConcurrency automatically turns on IsolatedDefaultValues, GlobalConcurrency, and RegionBasedIsolation.

What I think you want is:

• StrictConcurrency
• DisableOutwardActorInference
• GlobalActorIsolatedTypesUsability
• InferSendableFromCaptures

[DagAgren]

@mattmassicotte The problem is less that it is incorrectly annotated, but that it is not annotated at all, and Swift 5 will sometimes eagerly infer @mainactor annotations incorrectly - such as when calling a function that takes an unannotated closure from a function that is @MainActor. In this case, Swift 5 will treat the closure as if it were @MainActor, and allow you to call correctly-annotated @MainActor functions in a Swift 6 module, causing a crash.

I think we have all of those except GlobalActorIsolatedTypesUsability, which I had not heard about. I did notice that Swift 5 mode was less good at inferring things than Swift 6 mode, so rolling back from 6 to 5 + flags introduced new warnings, could that flag affect that?

Also, since you seem to know this stuff: Everywhere seems to document StrictConcurrency as being .enableExperimentalFeature("StrictConcurrency") in Package.swift (rather than .enableUpcomingFeature). Is this correct, or something that has changed?

[mattmassicotte]

@DagAgren you are right, I wasn't being precise enough. I use "incorrect" and "missing" interchangeably sometimes and I should not. You can read more about the phenomenon here: https://www.swift.org/migration/documentation/swift-6-concurrency-migration-guide/incrementaladoption#Unmarked-Sendable-Closures

All of the possible flags that apply to Swift 5 are documented here: https://www.swift.org/migration/documentation/swift-6-concurrency-migration-guide/sourcecompatibility

But the only ones that will have a material impact on concurrency-related warnings when in Swift 5 mode are the ones I included above. The others could still produce warnings/errors/problems though. So turning them all on is the way to get closest to the 6 language mode. Again, this really depends on what you want the output to represent.

The difference between enableUpcomingFeature and enableExperimentalFeature is always confusing and poorly documented. But, you can use .enableExperimentalFeature("StrictConcurrency") for both a Swift 5 and Swift 6 compiler.

[DagAgren]

Do enableUpcomingFeature and enableExperimentalFeature use the same namespace or different ones?

[mattmassicotte]

I'm not sure, I'm afraid.

[finestructure]

This is now live with builder 4.58.0 and the following flags:

    static let defaultSwift6Flags = [
        // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/3443#issuecomment-2485604275
        "StrictConcurrency",  // implies GlobalConcurrency, IsolatedDefaultValues, RegionBasedIsolation
        "DisableOutwardActorInference",
        "GlobalActorIsolatedTypesUsability",
        "InferSendableFromCaptures",
    ]

It'll impact individual builds immediately. The next full rebuild will happen with the release of Xcode 16.2 or on Nov 26, whichever comes sooner.

Thanks again for the help and input, @mattmassicotte !

[finestructure]

I flip-flopped a couple of times on re-opening this but instead opened a new issue here: #3552