pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.07k stars 1.41k forks source link

Add ignore list parameter to ObservableStateMacro #3080

Closed dev4dev closed 3 months ago

dev4dev commented 3 months ago

It allows to specify a list of macro names that should be ignored inside of the state struct. Without that it is impossible to use custom macroses, as they will interfere with the @ObservationStateTracked macro.

We have some custom property wrapper that will not work with the @ObservableState. Rewriting it into macro turned out as not an easy task. I tried several approaches and nothing worked. I also checked how you did implement @Presented part, and you had to add exceptions into @ObservableState code itself.

This PR extends @ObservableState with an ability to specify a list of custom macroses that one could have in their project. It would be the final user's responsibility to implement a custom macro properly to work with the state changes tracking stuff. But it would make the following code work.

@Reducer
struct ChildFeature {

}

@Reducer
struct Feature {
    @ObservableState(ignore: "Heapify")
    struct State {
        @Heapify
        var childState: ChildFeature.State?
    }

    enum Action {
        case childStaet(ChildFeature.Action)
    }
}

Obviously, I just might not know other way it could be achieved with, would be thankful for any insites or help =)

mbrandonw commented 3 months ago

Hi @dev4dev, I believe the @ObservationStateIgnored macro will take care of this for you. Can you give it a spin and let us know if it suites your needs?

dev4dev commented 3 months ago

@mbrandonw I tried that, the code was like this

@Reducer
struct ChildFeature {

}

@Reducer
struct Feature {
    @ObservableState(ignore: "Heapify")
    struct State {
        @ObservationStateIgnored
        @Heapify
        var childState: ChildFeature.State?
    }

    enum Action {
        case childStaet(ChildFeature.Action)
    }
}

In result it complains that @ObservationStateIgnored produces accessors that it is not supposed to produce. Those are the ones @Heapify adds to work with registrar. And I didn't find the way to add it inside of my macro, it just doesn't provide that flexibility. I tried to use standalone macro that would produce

@ObservationStateIgnored
@Heapify

I hoped that it would just do 2 passes and expand @Heapify, but it added @ObservationStateIgnored only, my macro was ignored altogether.

dev4dev commented 3 months ago

I tried everything I could've come up with, before doing that. Usually changing the 3rd party is the last resort. But it is kinda the blocker for us to adopt that feature. I wish we could have another way for doing that =)

mbrandonw commented 3 months ago

@dev4dev What is @Heapify? A macro? If so, is there a reason it can't be a property wrapper? That would then work very easily.

dev4dev commented 3 months ago

@mbrandonw Yes, it is a property wrapper atm, and they do not play well with Observable stuff. There is an error Property wrapper cannot be applied to a computed property. Or is there a trick to overcome that?

dev4dev commented 3 months ago

I'm even wondering how @Observable would work with all that, and should it? ๐Ÿค”

To clarify, our property wrapper/macro is a workaround to move huge state from the stack into heap, to resolve stack overflow crashes =(

mbrandonw commented 3 months ago

Hi @dev4dev, it is true that @Observable also has problems with property wrappers, and really just any macro that trades a stored property for an underscored property + computed property.

However, @ObservationStateIgnored (and Swift's @ObservationIgnored) should remedy this problem entirely. I'm not sure why you are having problems. Can you provide a project that demonstrates the problem?

dev4dev commented 3 months ago

@mbrandonw https://github.com/dev4dev/MacroPain main.swift contains several examples of what I've tried

dev4dev commented 3 months ago

LOOOOL, I just tried to swap those macroses, @ObservationStateIgnored should go last ๐Ÿคฆ๐Ÿป ok, looks like the issue is resolved

@Reducer
struct ChildFeature {

}

@Reducer
struct Feature {
    @ObservableState
    struct State {
        @Heapify
        @ObservationStateIgnored
        var childState: ChildFeature.State?
    }

    enum Action {
        case childStaet(ChildFeature.Action)
    }
}
dev4dev commented 3 months ago

@mbrandonw sorry for the noise =)

mbrandonw commented 3 months ago

I don't think the order should matter either. Both of these compile just fine:

@ObservableState
struct State {
  @ObservationStateIgnored
  @Binding
  var x: Int
}

@ObservableState
struct State {
  @Binding
  @ObservationStateIgnored
  var x: Int
}

Are you 100% certain it was not working before? Maybe you had a phantom error in Xcode.

Matejkob commented 3 months ago

It could also be a bug related to Swift Macros impl on the compiler side (as we know there is still a few bug there ๐Ÿ˜…). If you are able to reproduce the bug @dev4dev, feel free to share it with me and we can investigate it further.

dev4dev commented 3 months ago

@mbrandonw nope, for me it works when ignore is the last. Don't know what's going on, but at least I have simple solution. Even though it would be verbose, as it is used many times. With this change it would be a matter of one parameter ๐Ÿ˜„

dev4dev commented 3 months ago

@Matejkob i can reopen repo, maybe you could check on your side. It's none zero chance that I might messed up something else ๐Ÿ˜

mbrandonw commented 3 months ago

@dev4dev The repo you shared 404's (either private or wrong URL), but perhaps you can just copy-paste the code for the @Heapify property wrapper.

Matejkob commented 3 months ago

@dev4dev That would be great, you can reopen it or add me as a collaborator.

dev4dev commented 3 months ago

Should be visible now ๐Ÿ‘Œ๐Ÿป

Matejkob commented 3 months ago

I can confirm that this code:

@Reducer
struct ChildFeature {

}

@Reducer
struct Feature {
  @ObservableState
  struct State {
    @ObservationStateIgnored
    @Test
    var childState: ChildFeature.State?
  }

  enum Action {
    case childStaet(ChildFeature.Action)
  }
}

doesn't work on my machine as well. Gonna investigate it tomorrow.

mbrandonw commented 3 months ago

@Test is a macro in this repo, but @dev4dev mentioned that @Heapify was a property wrapper. Is there an example of the order being significant when using @ObservationStateIgnored with a property wrapper?

Further, @Test seems to only serve the purpose of applying a property wrapper, so I do think the macro should just be abandoned and a simple property wrapper should be used in its place.

Matejkob commented 3 months ago

As long as the @Test is a macro wouldn't it be the same bug as this one: BUG - @Observable Macro with @attached(accessor, names: named(didSet)) ยท Issue #68025 ยท apple/swift?

dev4dev commented 3 months ago

@mbrandonw my bad, i think I explained this not well and misrepresented in the code example. Let's work with naming from the example. There is a SomePropertyWrapper that it's used without ObservableState now. Property wrappers don't work with macroses, as far as I know. That's why I was tinkering with the macro @Test.

Further, @Test seems to only serve the purpose of applying a property wrapper, so I do think the macro should just be abandoned and a simple property wrapper should be used in its place.

Not sure how the property wrapper will work in this case, if I add @ObservationStateIgnored then we will lost observability.