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

Strict Concurrency #298

Conversation

Tyler-Keith-Thompson
Copy link
Contributor

#291 took a first crack at support async/await. While this certainly gets us moving in the short term Swift 6 is threatening to cause all kinds of issues around the corner.

This PR adds the strict concurrency flag and takes a crack at fixing most, if not all issues when that's on.

Most of this is somewhat obvious, view protocols (Like KnownViewType) just need to be main actor isolated and then their public declarations need to be marked @preconcurrency.

There's some less obvious changes, like for example when a main actor isolated type conforms to Collection. I'll tackle that in a future comment.

@Tyler-Keith-Thompson
Copy link
Contributor Author

Tyler-Keith-Thompson commented Mar 30, 2024

So there are spots in ViewInspector where a type might conform to Collection but it's doing a bunch of view things and view things reasonably require it to be main actor isolated. This raises the "so what the hell do we do?" question.

Now the reality about Swift's concurrency model is mostly that it brings visibility where there was none before and it forces deliberate choices. Realistically, InspectableView wasn't really thread safe before, it's just that most everything happened on the main thread so it was never really an issue. Now that it's marked deliberately as MainActor there's this natural tension between being a Collection which isn't concurrency aware, and being a View-based lookup which is main actor isolated.

I'm going to propose this solution: https://developer.apple.com/documentation/swift/mainactor/assumeisolated(_:file:line:)-swift.type.method

All this does is take the assumption that was there previously (it was main actor isolated) and codify it. If this gets run from a different thread (it shouldn't, realistically since ViewInspector can control much of that) it'll simply crash. The reality is that's what would happen before, too. Since you could've taken this collection which wasn't thread safe and caused a BAD_ACCESS type crash by monkeying with it on a background thread.

FWIW an alternative option would be to change Collection or Iterator types to AsyncSequence and AsyncIterator but the impact that has on consumers is less than ideal.

@nalexn
Copy link
Owner

nalexn commented Mar 31, 2024

On my first attempt to resolve the emerged strict concurrency warnings I faced a problem that as soon as something is marked as @MainActor, this requirement starts to spread all over, and ends up in the user's tests, where every call to the API from XCTestCase requires running from the main actor. And you cannot annotate the test class as @MainActor because it derives from XCTestCase (this is also a warning).

So I was thinking - maybe we should not explicitly appeal to @MainActor in the APIs at all? Yes, some code, like view hosting, and onTap button callback has to be called from MainActor.run { ... }, but overall, the library is dealing with parsing a bunch of view structs, a static copy of the view hierarchy. Data races can only happen with shared state attached to views, so if we do MainActor.run { ... } internally instead of demanding this publicly, maybe this would make the end experience as smooth as possible, while allowing us to satisfy the concurrency requirements?

@Tyler-Keith-Thompson
Copy link
Contributor Author

Hmm, so far I'm not running into any issues with having to mark tests as @MainActor. That's because of the @preconcurrency attribute working its magic. MainActor.run can work in async contexts, but MainActor.assumeIsolated is the equivalent for synchronous APIs.

@nalexn
Copy link
Owner

nalexn commented Mar 31, 2024

Also, the library doesn't use much of the shared state, we can consider introducing a custom actor to keep that state in sync with minimum efforts. I haven't tried yet, but I suspect attribution with @CustomActor doesn't have the same tendency of spreading.
I'm referring to this warning/error if the future
Screenshot 2024-03-31 at 11 23 45

name: "ViewInspector",
dependencies: [],
swiftSettings: [
.enableExperimentalFeature("StrictConcurrency")
Copy link
Owner

Choose a reason for hiding this comment

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

We may not need to declare it at the package level - if you open the project through ViewInspector.xcodeproj instead of Package.swift, that should be possible to enable this setting internally in the project

@nh7a
Copy link
Contributor

nh7a commented Apr 16, 2024

every call to the API from XCTestCase requires running from the main actor.

This is not true. It can be safely called from any actor context.
For example, there are three ways to use a MainActor isolated function as below, and test3() can run in any global actor context.

@MainActor struct Foo {
    func f() -> Bool { true }
}

final class FooTestCase: XCTestCase {
    @MainActor
    func test1() {
        // isolated to MainActor.
        let foo = Foo()
        XCTAssertTrue(foo.f())
    }

    func test2() {
        MainActor.assumeIsolated {
            // crash if it's not isolated to MainActor.
            let foo = Foo()
            XCTAssertTrue(foo.f())
        }
    }

    func test3() async {
        // not necessarily isolated to MainActor, but fine.
        let foo = Foo()
        let res = await foo.f()
        XCTAssertTrue(res)
    }
}

@Tyler-Keith-Thompson
Copy link
Contributor Author

Closing in favor of #302 which is more fleshed out.

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.

None yet

3 participants