Reuse wrappers - #2569
Conversation
onevclaw
left a comment
There was a problem hiding this comment.
The synchronous wrapper-reuse direction looks sound, but I found two blocking regressions at the exact PR head (3a9e3c28a885ceab58d84357f39620aa5931deaa):
swift buildfails on macOS with 10 errors inNSButton+Kingfisher.swift.TaskPropertyAccessornow passes aKingfisherWrapper<Object>, while the normal and alternate-image paths still treat the argument as anNSButtonand access.kf. Please complete theNSButtonmigration and make the corresponding associated-object settersnonmutating.swift build -Xswiftc -swift-version -Xswiftc 6fails inHasImageComponent+Kingfisher.swiftbecause the new[weak base]captures carry the non-SendableUI object through@Sendablecallbacks. The same command succeeds on the base commit. Please retainWeakBoxor use an equivalent@unchecked Sendableweak-reference container, then reconstruct the wrapper inside the callbacks.
The nearby comment describing a “weak box” should also be updated if that implementation changes.
As a cross-check, after temporarily completing the NSButton migration, the macOS suite passed with 364 tests and no failures, including an additional released-target/alternative-source lifecycle probe. This supports the core synchronous-accessor change; the remaining blockers are the incomplete AppKit migration and Swift 6 sendability regression.
onevclaw - an assistant to @onevcat
|
Thank you. I've restored the |
onevpaw
left a comment
There was a problem hiding this comment.
I reviewed exact head e33fc8756054ef11dd8dead0a3de7ebd0e113ff3 and found no blocking correctness issues. The wrapper reuse preserves cancellation, stale-result handling, target lifetime, and the AppKit-specific paths.
Local validation passed the full iOS and macOS test suites, SwiftPM builds in the default and Swift 6 modes, all simulator builds, and an additional nil-source/stale-result probe. One macOS cancellation test timed out once and passed on Fastlane retry.
Two non-blocking suggestions:
- Consider removing the unrelated
[weak self]change inKingfisherManager.swiftto keep the scope focused. - Since
KingfisherWrapperis a value type, “wrapper initializations” would be more precise than “allocations” unless the allocation claim is supported by profiling or benchmark data.
I’m leaving this as a comment rather than an approval because the visible CI state is still pending.
onevpaw - an assistant to @onevcat
onevtail
left a comment
There was a problem hiding this comment.
I reviewed the exact head 73c093995909b6cdf9492af5a7c90bd35e99976c and found no blocking code issues.
The refactor preserves task cancellation, weak-reference lifetime, stale-result detection, and the independence of image/background and image/alternate task channels. SwiftPM builds, Swift 6 compilation, and the complete iOS, macOS, and tvOS test suites passed locally; additional Apple-platform builds and a temporary dual-channel concurrency probe also passed.
The only optional follow-up would be to turn that dual-channel probe into a permanent regression test, but I do not consider it necessary for this PR. Since the visible CI state is still pending with no checks reported for the exact head, I am leaving this as a non-approving review until CI is confirmed.
onevtail - an assistant to @onevcat
Since setters on
KingfisherWrapperdo not mutate itself, all setters can be markednonmutating. This enables us to have direct access to getters/setters without accessingbasenor initializing new wrappers. In other words, it eliminates most ofKingfisherWrapperinitializations onsetImage(...)lifecycle and reuse just an instance (we only initialize new instance on escaping closures).