Add structured concurrency support

[Tyler-Keith-Thompson]

There are a few places where ViewInspector is falling behind in regards to structured concurrency. This PR is an attempt to work through that.

Backward compatibility might need some discussion. It's pretty easy to get the 90% case, but if we want to ensure no breaking changes we may have to adopt a brand new AsyncInspectionEmissary type protocol (and use similar tricks elsewhere).

Some changes should be totally safe, like synchronizing state on Inspector

[Tyler-Keith-Thompson]

Some notes: I've run the entire test suite 1000 times to look for any flakes and what's there as of the time of writing is solid.

The ComposedGestureExampleTests seemed to make some assumptions that weren't quite valid about synchronous behaviors. The change I made was very minor and unless I misunderstand the tests probably should've been true the whole time. I just made sure the inspection happened after the publisher fired.

[Tyler-Keith-Thompson]

Another note. Previously the view hosting and inspection code really sort of made an assumption that each inspection would be set up before the callbacks would happen. A straight translation to async resulted in a rather unfortunate situation where the view would be expelled after the first inspection.

We could expect consumers to use async let but a better solution seems to be making the scope of view hosting more clear. I decided on an API sort of like this:

func testAsyncViewInspectAfter() async throws {
    let sut = TestView(flag: false)
    try await ViewHosting.host(sut) {
        try await $0.inspection.inspect { view in
            let text = try view.button().labelView().text().string()
            XCTAssertEqual(text, "false")
            sut.publisher.send(true)
        }
        try await $0.inspection.inspect(after: .seconds(0.1)) { view in
            let text = try view.button().labelView().text().string()
            XCTAssertEqual(text, "true")
        }
    }
}

where $0 is the view you hosted. This seemed like a good tradeoff as it allows consumers to be deliberate about what they'd like to do while a view is hosted

[Tyler-Keith-Thompson]

onReceive poses its own challenges. As we move into structured concurrency the issue is that a publisher might fire off before there is a subscription. I've currently "worked around" this by deciding it's a consumer problem and using a task group in tests that rely on publisher behavior similar to what was there before:

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
func testAsyncViewInspectOnReceive() async throws {
    let sut = TestView(flag: false)
    try await ViewHosting.host(sut) { sut in
        try await withThrowingDiscardingTaskGroup { group in
            group.addTask {
                try await sut.inspection.inspect { view in
                    let text = try view.button().labelView().text().string()
                    XCTAssertEqual(text, "false")
                    sut.publisher.send(true)
                }
            }

            group.addTask {
                try await sut.inspection.inspect(onReceive: sut.publisher) { view in
                    let text = try view.button().labelView().text().string()
                    XCTAssertEqual(text, "true")
                    sut.publisher.send(false)
                }
            }

            group.addTask {
                try await sut.inspection.inspect(onReceive: sut.publisher.dropFirst()) { view in
                    let text = try view.button().labelView().text().string()
                    XCTAssertEqual(text, "false")
                }
            }
        }
    }
}

[nalexn]

I appreciate the massive effort here in exploring the structured concurrency support! Understandably we should pursue backward compatibility whenever possible. I think here it makes sense to go all in with the breaking API change for the async inspection, if needed, to address the existing limitations and trade-offs imposed by the published API. I like the new suggested API for View hosting. As an idea, I'd suggest exploring a way to incorporate the task group into the hosting, so that

try await ViewHosting.host(sut) { sut in
        try await withThrowingDiscardingTaskGroup { group in
            group.addTask {
                  try await sut.inspection.inspect ...
            }
            group.addTask {
                  try await sut.inspection.inspect ...
            }
        }
}

would look like this on the consumer side:

try await ViewHosting.host(sut) { sut in
        try await sut.inspection.inspect ...
        try await sut.inspection.inspect ...
}

This would involve always creating a group inside ViewHosting.host and calling group.addTask inside every enclosed sut.inspection.inspect. A simple case with a single inspection should be covered by this API as well.

Aside from that, what are the remaining concerns? I still need to better grasp the scope of the changes, but at the first glace all looks good, tests pass

[Tyler-Keith-Thompson]

Glad you got a chance to look at it! I'll see what progress I can make today. The PR is likely to get a bit more noisy in terms of changes for the rest of my wishlist, here's what I think remains:

• [x] See what I can tweak for multiple inspections where people want to use an async/await API that only inspects after Combine publisher changes (the task group example)
• [x] Add the @MainActor global actor attribute to most of the library since the bulk of what is there would fail if executed on a different actor (this change will be prolific, but simple and importantly it'll help the compiler stop future maintainers from making concurrency mistakes)
• [x] Add docs (side note, I might open an issue to describe some documentation changes I think the library could benefit from, that'll be a separate thing)
• [x] Create a writeup of backward compatibility/migration to utilize this API. This'll hopefully help people who are interested know where to start but also help inform the choice for what semver choices you want to make. (It's not likely to break for many people, actually. Only if they have existing closures which aren't sendable)
• [x] Double check non-iOS platforms to make sure that all works as expected

[Tyler-Keith-Thompson]

So running tests on watchOS had some build issues which I fixed and some runtime fatalErrors which I haven't. Thing is, the same is true in the trunk branch. I'm thinking the work to get watchOS 7 functioning with Xcode 15 is probably best saved for a separate PR.

That said, given how the other platform tests behaved I have no cause to believe that this PR introduces any regressions for watchOS

[nalexn]

@Tyler-Keith-Thompson are you using the .watchOS/watchOS.xcodeproj for watchOS tests?

[Tyler-Keith-Thompson]

@Tyler-Keith-Thompson are you using the .watchOS/watchOS.xcodeproj for watchOS tests?

You're right! I missed that. watchOS works fine.

[Tyler-Keith-Thompson]

@nalexn So I spent quite a bit of time looking at this: https://github.com/nalexn/ViewInspector/pull/291#issuecomment-1962897099

Your proposed API has a couple problems. The first is it'd require a very clever custom ResultBuilder that is likely not worth the effort. The second problem is that it tends to violate what I can find Apple is going for with structured concurrency. In that it hides how that concurrency behaves behind an API rather than make it explicit from a consumer perspective.

My recommendation here is to stick with the task group and I've got 2 reasons for it:

• First off, the only time this matters is when you've got this weird mix of async/await and Combine. It's unclear to me how often this happens out in the wild but it's a bit of a strange mix since they don't really interoperate well. I think this is an edge case.
• The second, as I mentioned, is that the task group (or even async let) approach makes it unwaveringly clear exactly how concurrent code is executing right at the call site.

[Tyler-Keith-Thompson]

My note on backward compatibility. So the biggest "breaking" change we've made here is that inspection closures must be Sendable now. Practically speaking if consumers follow the normative examples you have in the repo and docs they won't run into problems (as evidenced by the fact I didn't have to go change all the tests everywhere).

Similarly, everything is @preconcurrency and @MainActor. This isn't super breaking, because practically speaking ViewInspector absolutely required code to be executed on the main thread, now it's just more explicit. The biggest issue is for future maintenance. A well written test suite will perfectly point out what needs to be marked @preconcurrency but it's basically any public symbol for something that is @MainActor isolated.

It's absolutely possible both of these changes could be breaking in that they'd refuse to compile if somebody did something in an inspection closure that wasn't thread safe or if somebody was using async/await tests with ViewInspector and trying to manage that themselves.

ViewInspector is v0 software, so not sure if you want to bump the major version for these breaks or not, but that should give you a reasonably good picture of how it could break for people. In the majority of cases the Xcode compiler should provide fixits or helpful enough errors for people to resolve any compilation issues.

[Tyler-Keith-Thompson]

Alright, I've addressed all comments (thank you everybody for the reviews!) If it all makes sense I think we're ready to rock on this PR.

I'm happy to tweak it more if necessary or if some of my above reasoning (especially on task groups) doesn't stand up.

[nalexn]

I've merged the PR, thank you, Tyler!

While this is a huge step forward towards supporting structured concurrency, we still have some cleanup to do as part of preparing for Swift 6. This includes a more deliberate choice of attributes, including actors, preconcurrency, sendable, etc. Enabling the strict concurrency checking in build settings reveals the scope of work yet to be done:

[Tyler-Keith-Thompson]

we still have some cleanup to do as part of preparing for Swift 6

Great callout! I'll fiddle with a strict concurrency PR. I suspect this is mostly just lots more main actor isolation but there could be some goofiness beyond that.