Macro bonanza

[stephencelis]

This branch introduces a few new tools and changes made possible by Swift 5.9 macros.

Discussion forthcoming here: #2555

[Matejkob]

Hi there! 👋

I've just taken a look at the macro implementation and have left a few inline comments. While I hope these observations will be helpful, please feel free to disregard any that you don't find useful. I understand that different perspectives can lead to varying approaches!

Great use case of Swift Macro! I love it!

Sending much love,
Mateusz 🫶

[Matejkob]

Instead of using a large conditional, why not simply compare the FunctionDeclSyntax.description string with the expected description of the reduce method? I suppose one could add additional trivia to the function, like this:

func    reduce(into state: inout State, action: Action) -> Effect<Action>

However, I believe we would still be able to handle that. We are comparing only the argument names, not their types, but am I right in thinking it's safe to assume they will be State and Action, respectively?

[Matejkob]

The question remains how to handle access modifiers. The best solution would be to ensure that it is the same as the type declaration, but I'm not sure if this will be possible. Maybe it can be implemented based on the body property access modifier? If the two are the same then emit a warning.

[Matejkob]

Also, I'm thinking about an edge case where someone might have code like this:

var body: some Reducer<State, Action> {
  // somewhere here private reduce is called
}

private func reduce(into state: inout State, action: Action) -> Action {
  // ...
}

In this scenario, wouldn't the private reduce method, be detected instead of the one intended from the protocol?

[stephencelis]

While the current might be more complex, I'm comfortable with the checks, and I think it's checking just what needs to be checked and nothing more. Folks can technically define a reduce that matches the protocol conformance but uses a custom type for State or Action (since those are just associated types), and one could also define custom secondary names. An extreme example:

func reduce(
  into myState: MyState,
  action myAction: MyAction<Something>
) -> EffectOf<MyFeature>

[stephencelis]

In this scenario, wouldn't the private reduce method, be detected instead of the one intended from the protocol?

That's not valid Swift, so the compiler would complain at the conformance level:

🛑 Method 'reduce(into:action:)' must be as accessible as its enclosing type because it matches a requirement in protocol 'Reducer'

[Matejkob]

In this scenario, wouldn't the private reduce method, be detected instead of the one intended from the protocol?

That's not valid Swift, so the compiler would complain at the conformance level:

🛑 Method 'reduce(into:action:)' must be as accessible as its enclosing type because it matches a requirement in protocol 'Reducer'

OK, I think I understand what happened here. In theory, the following code should indeed result in an error, but... it seems that, thanks to the implementation of the Reduce protocol being handled by a macro, the code works just fine 🤯

@Reducer
struct MyTest {
  struct State {}

  enum Action {
    case none
  }

  var body: some Reducer<State, Action> { Reduce { _, _ in .none } }

  private func reduce(into state: inout State, action: Action) -> Effect<Action> {
    .none
  }

[Matejkob]

While the current might be more complex, I'm comfortable with the checks, and I think it's checking just what needs to be checked and nothing more. Folks can technically define a reduce that matches the protocol conformance but uses a custom type for State or Action (since those are just associated types), and one could also define custom secondary names. An extreme example:

func reduce(
  into myState: MyState,
  action myAction: MyAction<Something>
) -> EffectOf<MyFeature>

Sorry, I'm a bit confused now. So, is your intention that every method like this should produce an error, right? Even ones with different types, like this example:

@Reducer
struct MyTest {
  struct State {}

  enum Action {
    case none
  }

  enum LocalAction {
    case some
  }

  var body: some Reducer<State, Action> { Reduce { _, _ in .none } }

  private func reduce(into state: inout State, action: LocalAction) -> Effect<Action> {
    .none
  }
}

Is that correct?

[stephencelis]

@Matejkob I think there's only so much we can do from the macro to detect that a method matches the conformance. Someone could type alias the Effect return type so we can't do any checking there, and while we could traverse the reducer to see if there are State or Action types/aliases and use that to silence the warning for your above example, I'm not sure it's worth the extra work when at the end of the day, defining a method named reduce is confusing enough that we think a rename is warranted for folks to silence the warning. In fact, I did just add a syntax visitor that tries to fix-it a rename.

[Matejkob]

@stephencelis Thanks for your explanation. I understand the challenges with the macro approach and agree that a rename is a more straightforward solution. Appreciate your work on this and the helpful response!