nalexn / ViewInspector

Runtime introspection and unit testing of SwiftUI views
MIT License
2.09k stars 145 forks source link

Strict Concurrency #298

Closed Tyler-Keith-Thompson closed 1 month ago

Tyler-Keith-Thompson commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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
nh7a commented 2 months ago

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 commented 1 month ago

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