Skip to content
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

Enable instantiating @MainActor-bound types in Swift 5.10+ #70

Merged
merged 15 commits into from
Apr 8, 2024

Conversation

dfed
Copy link
Owner

@dfed dfed commented Apr 7, 2024

This PR makes it possible to use SafeDI with Swift 5.10+ and Strict Concurrency checking turned on. To accomplish this, we made the following changes:

  1. Made generated init methods on @Instantiable be isolated to the parent actor context – removed the nonisolated from these generated init methods.
  2. The instantiate(...) method for both Instantiator and ErasedInstantiator become @MainActor-bound
  3. Created two new instantiator objects: NonisolatedInstantiator and NonisolatedErasedInstantiator. As the name implies, these instantiators's instantiate(...) methods run in a nonisolated context.

We made Instantiator and ErasedInstantiator @MainActor-bound because in most applications the root of the dependency tree is @MainActor-bound, and in applications the majority of delayed-built products are @MainActor-bound. In an effort to make the common case simple, we have optimized for simplifying the @MainActor-bound case.

Addresses #65.

This is a breaking change.

To adopt this change locally, consumers will need to follow their build errors after updating to 0.5.0. If something requires instantiation outside of main actor isolation, utilize a Nonisolated-prefixed Instantiator.

@dfed dfed self-assigned this Apr 7, 2024
@dfed dfed force-pushed the dfed--main-instantiators branch from 39fb223 to b61de26 Compare April 7, 2024 18:45
Copy link

codecov bot commented Apr 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.56%. Comparing base (2dc36d0) to head (4966958).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
+ Coverage   99.51%   99.56%   +0.04%     
==========================================
  Files          40       44       +4     
  Lines        9139     9200      +61     
==========================================
+ Hits         9095     9160      +65     
+ Misses         44       40       -4     
Files Coverage Δ
...feDI/DelayedInstantiation/ErasedInstantiator.swift 100.00% <100.00%> (+50.00%) ⬆️
...ces/SafeDI/DelayedInstantiation/Instantiator.swift 100.00% <100.00%> (ø)
...dInstantiation/NonisolatedErasedInstantiator.swift 100.00% <100.00%> (ø)
...DelayedInstantiation/NonisolatedInstantiator.swift 100.00% <100.00%> (ø)
...afeDICore/Generators/DependencyTreeGenerator.swift 97.23% <100.00%> (-0.04%) ⬇️
Sources/SafeDICore/Generators/ScopeGenerator.swift 100.00% <100.00%> (ø)
Sources/SafeDICore/Models/Dependency.swift 100.00% <ø> (ø)
Sources/SafeDICore/Models/Initializer.swift 98.93% <100.00%> (-0.10%) ⬇️
Sources/SafeDICore/Models/Property.swift 98.91% <100.00%> (+0.07%) ⬆️
Sources/SafeDICore/Models/TypeDescription.swift 98.06% <100.00%> (+0.03%) ⬆️
... and 8 more

@dfed dfed force-pushed the dfed--main-instantiators branch from 1ef19b3 to 655fa97 Compare April 7, 2024 19:04
@@ -156,36 +156,6 @@ Property declarations within `@Instantiable` types decorated with [`@Instantiate

`@Instantiated`-decorated properties must be an `@Instantiable` type, or of an `additionalType` listed in an `@Instantiable(fulfillingAdditionalTypes:)`‘s declaration.

#### Utilizing @Instantiated with type erased properties
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this moved down so that we discuss it after we discuss the Instantiator

@@ -19,34 +19,40 @@
// SOFTWARE.

/// A SafeDI dependency designed for the deferred instantiation of a type-erased instance of a
/// type decorated with `@Instantiable`. Instantiation is thread-safe.
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we're explicitly determining threading here it seemed duplicative to say Instantiation is thread-safe.

@dfed dfed requested review from MrAdamBoyd and kierajmumick April 7, 2024 19:19
@dfed dfed marked this pull request as ready for review April 7, 2024 19:19
@@ -1974,7 +1974,7 @@ final class SafeDIToolTests: XCTestCase {

extension Root {
public convenience init() {
func __safeDI_childABuilder(recreated: Recreated) -> ChildA {
nonisolated func __safeDI_childABuilder(recreated: Recreated) -> ChildA {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part of the code-gen has me pretty convinced that Nonisolated was indeed the correct prefix on the Instantiator

@dfed dfed force-pushed the dfed--main-instantiators branch 2 times, most recently from 102cb8c to f53fdb2 Compare April 7, 2024 20:16
@dfed dfed force-pushed the dfed--main-instantiators branch from f53fdb2 to d1e61c1 Compare April 7, 2024 20:18
@@ -0,0 +1,56 @@
// Distributed under the MIT License
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How I wish you could git cp

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if only!

@@ -136,33 +136,48 @@ public struct Property: Codable, Hashable, Comparable, Sendable {
/// An `Instantiator` property.
/// The instantiated product is not forwarded down the dependency tree. This is done intentionally to avoid unexpected retains.
case instantiator
/// A `ErasedInstantiator` property.
/// An `ErasedInstantiator` property.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy paste error? 😛

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most definitely

Copy link
Collaborator

@MrAdamBoyd MrAdamBoyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one grammar question.

@dfed dfed merged commit 44a71a1 into main Apr 8, 2024
11 checks passed
@dfed dfed deleted the dfed--main-instantiators branch April 8, 2024 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants