Isolate OTPMapProvider to the main actor - #160
Conversation
Every call into the protocol originates from MapCoordinator, which is already @mainactor, and conformances drive UI — MKMapViewAdapter sets an MKMapView delegate and mutates overlays. The isolation was real but unstated, which left hosts building in the Swift 6 language mode with main-actor default isolation unable to conform: their members come out @mainactor and cannot satisfy nonisolated requirements. MKMapViewAdapter and MockMapProvider infer the isolation through their conformances, so only the mock's test needed an explicit @mainactor.
|
Warning Review limit reached
Next review available in: 53 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe public ChangesMap provider isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change documents main-actor isolation for the map provider without altering runtime behavior, and the reported checks pass; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The isolation rationale claimed every call originates from MapCoordinator, but the demo calls the provider directly (OTPDemoViewController clears routes and annotations from its clear-trip action). The conclusion held — that view controller is main-actor isolated too — but the sentence justifying a public API change should not be contradicted by a file in this repo. Say what is actually true, and add the part a host needs: conformances inherit the isolation. README told integrators to implement OTPMapProvider without mentioning that conformances are now main-actor isolated, which is the one thing they have to react to.
e57755f to
12c16b7
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
This is the right change, and the description does a good job of explaining why it isn't just an annotation.
I verified the premise independently rather than taking it on faith: MapCoordinator is already @MainActor and holds all 24 calls into the protocol, TripPlanner is @MainActor, MKMapViewRepresentable's makeUIView/updateUIView are main-actor through UIViewRepresentable, and the demo touches mapProvider only from viewDidLoad, an @objc action, and presentTripPlanner. So the isolation you're declaring is genuinely the one the package already relies on. Both conformances declare it on the primary declaration and pick up whole-type isolation, and MKMapViewDelegate/UIGestureRecognizerDelegate are both NS_SWIFT_UI_ACTOR in the SDK, so MKMapViewAdapter has no isolated-member-vs-nonisolated-requirement conflict.
Documenting it as source-breaking rather than slipping it in is the correct handling pre-1.0, and putting it in the protocol doc comment and the README's Customizing section is where implementers will actually hit it.
One small thing, not worth holding the merge for: the doc comment says "Conforming types inherit this isolation, so the members of a custom provider are main-actor isolated too." That's true for a conformance on the primary declaration, but a host that conforms in an extension only gets isolation on the members satisfying requirements. The README sentence steers people right either way, so take it or leave it.
Approving on the merits. Holding the actual merge only until CI is green — the failing build here is the VehicleRentalSource cancellation flake, which reproduces on unmodified main and has nothing to do with this change. #161 carries the fix for it.
2 participants
Summary
OTPMapProviderwith@MainActor, documenting isolation that was alreadytrue in practice but unstated.
MapCoordinatorinside the package, host UI code outside it — and conformances drive UI: the shipped
MKMapViewAdapterassigns anMKMapViewdelegate, installs a gesture recognizer andmutates overlays.
own doc comment and the README's "Customizing" section.
Why
Hosts that build in the Swift 6 language mode with main-actor default isolation cannot
conform to this protocol today. Their members come out
@MainActorautomatically, and amain-actor-isolated method cannot satisfy a nonisolated protocol requirement, so the host
build fails outright.
The workaround available to a host — declaring the conformance
nonisolatedand wrappingevery body in
MainActor.assumeIsolated— is boilerplate across all sixteen requirementsand asserts an invariant the package already guarantees. Stating the isolation here is
both more honest and less code.
This came out of work to render OTPKit trip plans on a SwiftUI
Mapin OneBusAway iOS,which needs a second
OTPMapProviderimplementation backed by published state rather thanan
MKMapView. That host builds in the Swift 6 language mode with the concurrencydiagnostic groups escalated to errors.
Scope
Deliberately small.
MKMapViewAdapterand the testMockMapProviderinfer the isolationthrough their conformances and needed no changes; only the mock's test required an
explicit
@MainActor.This is a source-breaking change for any host that conforms off the main actor, which is
why it is documented rather than slipped in — we are pre-1.0, and the README now tells
implementers what the protocol expects of them. Existing callers are unaffected: every
call site in this repo, the demo app included, is already main-actor isolated.
Test plan
xcodebuild build -scheme OTPKit— BUILD SUCCEEDED, no new warningsxcodebuild test -scheme OTPKit— 229 tests in 20 suites, TEST SUCCEEDEDswiftlint --strict— 0 violations in 143 filesOBAKit, Swift 6 language mode, main-actor default isolation,concurrency diagnostics as errors) against this branch via a local package path:
BUILD SUCCEEDED, zero warnings in the five escalated concurrency groups
OBAKitTestsrun against this branch: 2277 tests, the only 2 failuresreproduce identically on unmodified
mainNote that this package builds with
swiftLanguageModes: [.v5], so CI here cannot exercisethe strict-isolation scenario this change exists to fix — the OneBusAway build above is the
only thing that does.
Summary by CodeRabbit