Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for excludeFunctions #182

Closed
wants to merge 2 commits into from

Conversation

ZacSweers
Copy link

This is a proposal PR for supporting extra signaling/metadata in poko annotations for excludeFunctions (and leaving a toe-hold for future such configurations).

The intention behind this is that some types, such as Kotlin function types, are known not to have true equality and should be ignored. This especially comes to a head in compose, where capturing lambdas for objects with state become a major headache (https://issuetracker.google.com/issues/256100927).

This is a proposal for how this could look, where both the first-party @Poko annotation and any other custom ones could implement this. The idea is that a custom annotation could offer this with its own default. We could also even remove this from @Poko and only leave it as an optional control for custom annotations.

If this looks acceptable, I'll proceed with adding tests too.

ZacSweers added 2 commits July 9, 2023 17:10
This is a proposal PR for supporting extra signaling/metadata in poko annotations for `excludeFunctions` (and leaving a toe-hold for future such configurations).

The intention behind this is that some types, such as Kotlin function types, are known not to have true equality and should be ignored. This especially comes to a head in compose, where capturing lambdas for objects with state become a major headache (https://issuetracker.google.com/issues/256100927).

This is a proposal for how this could look, where both the first-party `@Poko` annotation and any other custom ones could implement this. The idea is that a custom annotation could offer this with its own default. We could also even remove this from `@Poko` and only leave it as an optional control for custom annotations.

If this looks acceptable, I'll proceed with adding tests too.
@ZacSweers ZacSweers changed the title Add support for excludeFunctions. Add support for excludeFunctions Jul 9, 2023
@JakeWharton
Copy link
Collaborator

The name is a bit misleading since it's functional types that are being ignored. It also only works with Kotlin's functional types and not if you carry over something from the JDK like a Runnable.

What about an annotation on a property which omits that property from inclusion in equals/hashCode and mayyyybe even toString?

@JakeWharton
Copy link
Collaborator

Also wondering if you might want equality for these references to be presence/absence only. That is, two instances with non-null references (assuming a nullable property type) would be equal but two instance with one non-null reference and one null would not be equal.

@ZacSweers
Copy link
Author

What about an annotation on a property which omits that property from inclusion in equals/hashCode and mayyyybe even toString?

That could work but would be a little verbose in practice I think. Another option I messaged drew about is possibly just putting the utilities in a separate artifact that we could borrow and use in our own plugin since this is admittedly kind of a specific case.

Good point about absence 🤔

@JakeWharton
Copy link
Collaborator

It would be verbose, but it's also very explicit. It means you don't need to know about the Runnable vs. () -> Unit thing, you don't need to guess to which properties it applies, and it allows potentially using it on non-lambda types for whatever reason you would want to.

@drewhamilton
Copy link
Owner

drewhamilton commented Jul 10, 2023

I'm more inclined to do a per-property annotation like Jake suggests. It's more flexible and it's nicely parallel in application to @ArrayContentBased. I also don't want the main annotation to become a kitchen sink for niche configurations.

Also, would it actually be more verbose? Yours would look like this:

@Poko(excludeFunctions = true)
class SomeState(
  val label: String,
  val sink: (Event) -> Unit,
) : CircuitUiState

Property annotation would look something like this:

@Poko class SomeState(
  val label: String,
  @IgnoreForEquals val sink: (Event) -> Unit,
) : CircuitUiState

In either case, you add some new text to one line to enable the feature. Unless your use case often has multiple function properties? Edit: I missed "a custom annotation could offer this with its own default."

Also wondering if you might want equality for these references to be presence/absence only. That is, two instances with non-null references (assuming a nullable property type) would be equal but two instance with one non-null reference and one null would not be equal.

Maybe. Is this useful? In Circuit it seems like the event sink is not semantically part of the state, so you still wouldn't want recompositions if the callback is null for some reason.

possibly just putting the utilities in a separate artifact that we could borrow and use in our own plugin since this is admittedly kind of a specific case.

This would be cool but a bigger can of worms. Not sure I want to call these currently-internal APIs stable, especially not before I've supported K2 or analyzed which parts of this can/should be moved to FIR.

@drewhamilton
Copy link
Owner

By the way, it's currently possible to achieve this behavior!

@Poko class SomeState(
  val label: String,
  sink: (Event) -> Unit,
) : CircuitUiState {
  val sink: (Event) -> Unit = sink
}

This omits sink from all three generated functions. Obviously much more verbose, but if you're blocked by this it can be a workaround for now.

@drewhamilton
Copy link
Owner

Closing this in favor of #446.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants