pointfreeco / swift-perception

Observable tools, backported.
MIT License
545 stars 38 forks source link

Runtime warnings #49

Open arnauddorgans opened 6 months ago

arnauddorgans commented 6 months ago

Description

This code snippet produces 3 runtime warnings on appear about Perceptible state was accessed but is not being tracked. Track changes to state by wrapping your view in a 'WithPerceptionTracking' view.

@Perceptible
class FeatureModel {
    var text = ""
}

struct ContentView: View {
    @Perception.Bindable var model: FeatureModel

    var body: some View {
        WithPerceptionTracking {
            TextField("", text: $model.text)
                .background(GeometryReader { proxy in
                    Color.clear
                        .randomModifier(of: proxy.safeAreaInsets)
                })
        }
    }
}

Checklist

Expected behavior

No runtime warning

Actual behavior

Runtime warning

Steps to reproduce

Full app code

@Perceptible
class FeatureModel {
    var text = ""
}

struct ContentView: View {
    @Perception.Bindable var model: FeatureModel

    var body: some View {
        WithPerceptionTracking {
            TextField("", text: $model.text)
                .background(GeometryReader { proxy in
                    Color.clear
                        .randomModifier(of: proxy.safeAreaInsets)
                })
        }
    }
}

extension View {
    @ViewBuilder
    func randomModifier<T>(of value: T) -> some View where T: Equatable {
        if #available(iOS 17.0, *) {
            self
        } else {
            fatalError() // We don't care here
        }
    }
}

@main
struct TCABugApp: App {
    var body: some Scene {
        WindowGroup {
            ContentView(model: .init())
        }
    }
}

Perception version information

1.1.2

Destination operating system

iOS 14.0

Xcode version information

Xcode 15.3

Swift Compiler version information

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0
stephencelis commented 6 months ago

@arnauddorgans I can't seem to reproduce this on 1.1.3 / main. Can you confirm that it's been fixed and close this out, or can you please submit a failing test case?

stephencelis commented 6 months ago

Ah never mind, I couldn't reproduce it in our unit test suite but I was able to reproduce it in an app. Very strange! Will look into it!

m430 commented 4 months ago

Any update? Same problem.

mbrandonw commented 4 months ago

Hi @m430, no update on our end. If someone has time to look into the issue we would happily accept a PR!

arnauddorgans commented 4 months ago

@m430 I used a workaround by using a SwiftUI State

@State private var searchText: String

var body: some View {
  WithPerceptionTracking {
    TextField("", text: $searchText)
      .bind($store.searchText, to: $searchText)    
  }
}
ajkolean commented 2 months ago

Just encountered this issue as well.

The interaction between GeometryReader and WithPerceptionTracking might lead to runtime warnings due to the way GeometryReader integrates into the SwiftUI view hierarchy. It seems that GeometryReader could be introducing a new view context that disrupts the state tracking context established by WithPerceptionTracking. This disruption might cause the Perception library to lose track of state changes, resulting in runtime warnings indicating that state is being accessed without proper tracking.

I examined the Thread.callStackSymbols and can confirm that accessing the GeometryProxy within the GeometryReader is triggering key path reads, causing the perceptionRegistrar.access method to be invoked.

This issue is likely due to the way SwiftUI and the Perception library manage state updates and how views re-render. The GeometryReader might be indirectly causing the model member getter to be called, even if it is not explicitly within the GeometryReader. This can happen due to SwiftUI's internal mechanisms for batching updates and managing state changes.

It seems like the safest course is to re-establish tracking whenever using GeometryReader.

GeometryReader { proxy in
  WithPerceptionTracking {
     //
  }
}
mbrandonw commented 2 months ago

It seems like the safest course is to re-establish tracking whenever using GeometryReader.

This isn't related to this specific issue. What you are referring to is true of any escaping closure, and is mentioned in the docs:

You will also need to use WithPerceptionTracking in any escaping, trailing closures used in SwiftUI’s various navigation APIs, such as the sheet modifier:

tyler927 commented 1 month ago

Hey @mbrandonw , I recently ran into this "issue" myself while using GeometryReader. I was actually unaware that GeometryReader was an escaping closure. I totally see that it is now, but I think what tripped me up is it is not a modifier like the sheet is. Personally, I think it would be nice to explicitly call out GeometryReader in the docs as well.

janczawadzki commented 1 month ago

This is an interesting project but this caveat is bit concerning. Is there a chance this could be addressed in some future version with patching the view context hierarchy? Or is this hitting a more fundamental SwifUI design limitation/issue?

stephencelis commented 1 month ago

@janczawadzki Can you explain why the caveat is concerning in particular? We don't think there's a fundamental issue with the library, and we think the library solves a real world problem for folks that want to adopt the Observation machinery of Swift in <iOS 17.

The issue raised here is one that I hope folks see as understandable:

Note that the runtime warnings provided by the library are completely additive for debugging purposes, and don't exist in release, and the main issue is that it's difficult to suppress warnings for every situation: it requires demangling stack symbols and detecting very specific action closures and otherwise.

An alternative would have been for us to not emit warnings at all and just let folks know they need to test their views thoroughly, but we thought that's solving things in the wrong direction, and not providing the baseline of tools for folks to investigate potential problems and missing WithPerceptionTracking views.

janczawadzki commented 1 month ago

@janczawadzki Can you explain why the caveat is concerning in particular? We don't think there's a fundamental issue with the library, and we think the library solves a real world problem for folks that want to adopt the Observation machinery of Swift in <iOS 17.

It absolutely addresses a real problem, and my question wasn't at all around messages - those are necessary!

Challenge in adopting this in a non-trivial project is just knowing where modifications are needed, since this can't be identified (AFAIK?) via static analysis. How big of a change is this for me.

Hence my question - library would be super useful, but I don't understand the internals of the state stack well enough to know if this issue could be overcome or not. Best case scenario would obviously be "it just works", next best would be "here are the places you need to inject WithExceptionTracking." (... and I'm not sure how that would work exactly, to be clear!)