pointfreeco / swift-perception

Observable tools, backported.
MIT License
575 stars 44 forks source link

Not getting a perception check warning #53

Closed hiltonc closed 8 months ago

hiltonc commented 8 months ago

Description

While migrating some app code to use Perception I ran into a case where I did not get a perception check warning but should have. This is concerning because we're depending on these warnings to ensure that the feature will work on older iOS versions while doing development on iOS 17.

Checklist

Expected behavior

When running on iOS 17 I expect to get a perception check warning for not using WithPerceptionTracking.

Actual behavior

Observation works on iOS 17 (text goes from "loading" to "loaded"), but there is no perception check warning. As expected, on iOS 16 the text stays at "loading" because WithPerceptionTracking was not used.

Steps to reproduce

  1. Open the Xcode project in Demo.zip
  2. Run on iOS 17 simulator

Perception version information

1.1.2

Destination operating system

iOS 17

Xcode version information

Xcode 15.2

Swift Compiler version information

swift-driver version: 1.87.3 Apple Swift version 5.9.2 (swiftlang-5.9.2.2.56 clang-1500.1.0.2.5)
Target: arm64-apple-macosx14.0
mbrandonw commented 8 months ago

Hi @hiltonc, this is happening for a combination of reasons in your code.

It's due to setting the laodingState in the initializer:

init(loadState: LoadState<String?, Error> = .loading) {
  defer {
    self.loadState = loadState
  }
}

and due to the didSet on loadingState:

var loadState: LoadState<String?, Error> = .loading {
  didSet {
    if case let .loaded(loadedModel) = loadState {
      data = loadedModel
    } else if case .failure = loadState {
      data = ""
    }
  }
}

Setting loadingState in the initailizer causes the didSet to fire, which accesses loadingState. However, at that moment the access is not happening inside a SwiftUI body, and so no warning is emitted. But, the result of the perception check is cached so that it doesn't have to be computed again (which is quite expensive).

And so then later in the view when model.loadingState is accessed again it does not emit a warning since the result was cached.

It's not a perfect system, but it's the best we've come up with. Detecting field access in a SwiftUI body without WithPerceptionTracking is imprecise and so we can have false positives, and its expensive so we can have false negatves too.

However, in general it is quite precarious to access observable fields directly in an initailizer or didSet. We demonstrated this in a tweet many months back, and it is still true today.

So, to fix you should access the underscored field in didSet so that you do not accidentally trigger access:

var loadState: LoadState<String?, Error> = .loading {
  didSet {
    if case let .loaded(loadedModel) = _loadState {
      data = loadedModel
    } else if case .failure = _loadState {
      data = ""
    }
  }
}

And same with the initializer:

init(loadState: LoadState<String?, Error> = .loading) {
  defer {
    _loadState = loadState
  }
}

If you have ideas to improve the perception checking to be more precise and/or efficient we'd happily take a PR!

Since this isn't an issue with the library I am going to convert this to a discussion.