SE-0337: Incremental Migration to Concurrency Checking

Evolution Proposal Reviews
1

Hello, Swift community.

The review of SE-0337: Incremental Migration to Concurrency Checking begins now and runs through January 21, 2022.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. If you do email me directly, please put "SE-0337" somewhere in the subject line.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

As always, thank you for contributing to Swift.

Ben Cohen
Review Manager

13 Likes
3

+1. I think this is is a great proposal: it does a lot with a little, deploying a single concept in a clear way. I particularly appreciate the description of concurrency checking "modes", which will be a useful reference to try to understand what the compiler thinks is going on.

Yes. Easing the adoption of Swift Concurrency has been a major design goal of the entire concurrency process, and substantial pre-existing Swift codebases will always take time to migrate. Enabling a slower transition period is very valuable.

I think this proposal is close to Swift at its best: a single, straightforward construction that can be used in a number of places to communicate the same piece of information. I will say that I have a few concerns about the ease with which a developer could forget to add @preconcurrency to a Sendable annotation, but in principle this can be mitigated with tooling (e.g. the ABI/API checker).

A quick reading, combined with some prior discussion on the topic.

4

Small clarification request about the "@preconcurrency attribute on import declarations" section.

When an import is marked @preconcurrency, the following rules are in effect: (A)

  • If an implicitly non-Sendable type is used where a Sendable type is needed:
    • If the type is visible through a @preconcurrency import, the diagnostic is suppressed (prior to Swift 6) or emitted as a warning (in Swift 6 and later).
    • Otherwise, the diagnostic is emitted normally, but a separate diagnostic is provided recommending that @preconcurrency import be used to work around the issue. (B)
  • If an explicitly non-Sendable type is used where a Sendable type is needed:
    • If the type is visible through an @preconcurrency import , a warning is emitted instead of an error, even in Swift 6.
    • Otherwise, the diagnostic is emitted normally.
  • If the @preconcurrency attribute is unused[3], a warning will be emitted recommending that it be removed.

Lines (A) and (B) together seem to say "when an import is marked @preconcurrency, a diagnostic is provided recommending that @preconcurrency import be used".

But obviously if the import is already using @preconcurrency you wouldn't be able to change it to use @preconcurrency, so I think I'm misreading it.

Is this describing a situation where a different import in the same file is marked @preconcurrency?

5

I think the problem is in the introduction of the bulleted list (A), not in the list itself: the introduction should say something like “@preconcurrency can be used on import declarations and here is what it means”. I’ll improve the wording here, thanks!

Doug

1 Like
6
  • What is your evaluation of the proposal?

+1 (with request for consideration of change below)

  • Is the problem being addressed significant enough to warrant a change to Swift?

Definitely.

  • Does this proposal fit well with the feel and direction of Swift?

Seems to, yes.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Haven't.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Read it through once moderately carefully. Thought about it in the context of teams I've been on.

I haven't done much with Swift Concurrency yet, so I'm reading it as someone relatively new to the topic and so potentially in the target audience for those with an older code base moving to Swift 6 and taking advantage of the new Swift Concurrency capabilities.

Feedback and addition/change request

Many teams I've worked on have the warnings-are-errors setting turned on as a policy.
When I read the workflow description in the Proposed Solution section, item (2) says (emphasis mine):

Start solving those problems. If they relate to types from another module, a fix-it will suggest using a special kind of import, @preconcurrency import , which silences these warnings.

However, later in the @preconcurrency attribute on import declarations section it says (emphasis mine):

  • If the type is visible through a @preconcurrency import , the diagnostic is suppressed (prior to Swift 6) or emitted as a warning (in Swift 6 and later).

Seems like this second part is going to be a problem for the errors-as-warnings teams, and also seems counter to the Proposed Solution section.

In particular, teams which dictate errors-as-warnings turned on who want or need to upgrade to Swift 6 but need to use the helpful @preconcurrency annotation this proposal provides to annotate modules that haven't been updated yet will find that their build will fail and their CI will yell at everyone because of the warnings. Seems like that's going to get in the way of using this nice migration path this proposal is creating.

For these teams it would be useful to be able to disable those particular warnings (only the ones that are generated as downgraded errors because of the @preconcurrency annotation) while retaining warnings/errors for anything not shielded by the @preconcurrency annotation.

Thank you to all involved for all the hard work on Swift Concurrency and this effort towards making the transition manageable for those with existing code bases.

1 Like
7

The intent behind Swift 6 is that it will not silently accept the presence of a potential data race (without unsafe code explicitly involved somewhere), so @preconcurrency must not completely silence the diagnostics. Hence, they'll be warnings in Swift 6.

Now, this isn't the only suppression mechanism. One is permitted to write an @unchecked Sendable conformance for a type from another module; that suppresses the warning because the type is now Sendable. Teams that use Swift 6 with warnings-as-errors will need to maintain these @unchecked Sendable conformances, which seems acceptable because it is explicit.

Doug

4 Likes
8

Thanks for the reply.

I'm just imagining the situation for a larger team (40+ engineers) and a large code base (800K+ lines) with many packages, some of which are third party packages of moderate to significant size (packages like GRDB or maybe Apollo-iOS which are non-trivially large).

Even with a team that large, converting the entire app is going to take months. And third party packages may get updated on a slower schedule. While useful for small and medium apps, writing @unchecked Sendable conformances for every type in every module as you're working to convert a large code base like this, or waiting for a large third party package to get updated, seems pretty unworkable. This scenario seems like exactly the kind of situation this proposal is trying to make less painful (or even workable at all).

Such a large project and team size isn't extremely common, but quite a few of the more significant apps in the App Store are at this scale and involve teams this large or larger (where warnings-as-errors is most important).

So, in conclusion:

  • I still think the specific flag to disable just the @preconcurrency warnings would be helpful for these teams (maybe even necessary), and,

  • The text in the Proposed Solution, item (2) should be clarified to indicate that the fix-it will not appear in Swift 6 because adding @precondition import will do nothing in Swift 6.

I would be curious to hear what others on larger projects might think. Maybe they've given up on warnings-as-errors long ago :)

Thank you for your consideration of this feedback and for all involved's hard work on this significant enhancement to Swift.

1 Like
9

During which time, you’d then keep the project in Swift 5.x language mode using the Swift 6 compiler, no?

2 Likes
10

Maybe! I forget we can do this because I've never had reason to use it before. Seems like an excellent point.

11

Review Conclusion

The proposal has been accepted.

2 Likes