- 
                Notifications
    
You must be signed in to change notification settings  - Fork 199
 
Strict Concurrency #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Strict Concurrency #109
Conversation
Turned-off BITCODE Moved minimum deployment to iOS 15, tvOS 15, macOS 10.15 Set strict concurrency checking to Complete.
This is entirely UI-bound thus it's all in main thread.
| } | ||
| 
               | 
          ||
| public protocol Constrainable { | ||
| @MainActor public protocol Constrainable { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strongly recommend the following instead:
| @MainActor public protocol Constrainable { | |
| @preconcurrency @MainActor | |
| public protocol Constrainable { | 
This makes it so that existing code that doesn't yet have strict concurrency enabled can use this library version without having to make changes to their code.
| } | ||
| 
               | 
          ||
| extension LayoutGuide: Constrainable { | ||
| @MainActor extension LayoutGuide: Constrainable { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as the compiler infers @MainActor isolation for all members declared on this extension since Constrainable is isolated to the @MainActor.
| @MainActor extension LayoutGuide: Constrainable { | |
| extension LayoutGuide: Constrainable { | 
| #endif | ||
| 
               | 
          ||
| extension TinyView: Constrainable { | ||
| @MainActor extension TinyView: Constrainable { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as the compiler infers @MainActor isolation for all members declared on this extension since Constrainable is isolated to the @MainActor.
| @MainActor extension TinyView: Constrainable { | |
| extension LayoutGuide: Constrainable { | 
| } | ||
| 
               | 
          ||
| public extension Collection where Iterator.Element == Constraint { | ||
| @MainActor public extension Collection where Iterator.Element == Constraint { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strongly recommend the following instead:
| @MainActor public extension Collection where Iterator.Element == Constraint { | |
| @preconcurrency @MainActor | |
| public extension Collection where Iterator.Element == Constraint { | 
This makes it so that existing code that doesn't yet have strict concurrency enabled can use this library version without having to make changes to their code.
| 
               | 
          ||
| #if os(OSX) | ||
| public extension Constraint { | ||
| @MainActor public extension Constraint { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since Constraint is already isolated to the @MainActor
| @MainActor public extension Constraint { | |
| public extension Constraint { | 
| } | ||
| #else | ||
| public extension Constraint { | ||
| @MainActor public extension Constraint { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since Constraint is already isolated to the @MainActor
| @MainActor public extension Constraint { | |
| public extension Constraint { | 
| #endif | ||
| 
               | 
          ||
| public extension TinyView { | ||
| @MainActor public extension TinyView { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since TinyView is already isolated to the @MainActor
| @MainActor public extension TinyView { | |
| public extension TinyView { | 
| import AppKit | ||
| 
               | 
          ||
| public extension TinyView { | ||
| @MainActor public extension TinyView { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since TinyView is already isolated to the @MainActor
| @MainActor public extension TinyView { | |
| public extension TinyView { | 
| import UIKit | ||
| 
               | 
          ||
| public extension TinyView { | ||
| @MainActor public extension TinyView { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since TinyView is already isolated to the @MainActor
| @MainActor public extension TinyView { | |
| public extension TinyView { | 
| } | ||
| 
               | 
          ||
| public extension TinyView { | ||
| @MainActor public extension TinyView { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since TinyView is already isolated to the @MainActor
| @MainActor public extension TinyView { | |
| public extension TinyView { | 
| #endif | ||
| 
               | 
          ||
| public extension Constrainable { | ||
| @MainActor public extension Constrainable { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since Constrainable is already isolated to the @MainActor
| @MainActor public extension Constrainable { | |
| public extension Constrainable { | 
| } | ||
| 
               | 
          ||
| public extension TinyView { | ||
| @MainActor public extension TinyView { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since TinyView is already isolated to the @MainActor
| @MainActor public extension TinyView { | |
| public extension TinyView { | 
| #endif | ||
| 
               | 
          ||
| extension TinyEdgeInsets { | ||
| @MainActor extension TinyEdgeInsets { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably not recommended to add this since TinyEdgeInsets itself isn't isolated to the @MainActor and there's no code in any of these members that requires @MainActor isolation
| 
           Any news on this or should someone else take over? 😁  | 
    
| 
           I made all of the changes locally and was about to put up a PR myself before noticing this one. I don’t have an issue pushing my changes and opening a PR myself 🤷🏻♂️  | 
    
          
 Feel free and let me know, I'll close this. I have no time at the moment to work this out further.  | 
    
| 
           Opened a PR here: #112  | 
    
Sprinkled
Sendableand@MainActoras needed.Converted few
vars intolet.Set
strict-concurrency-checkingtoComplete.Moved minimum deployment to iOS 15, tvOS 15, macOS 10.15 (concurrency aware versions).
Updated version info to 5.0 — I recommend to tag as such.