Skip to content

Add support for IntoSet/Map(multiple = true) #467

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

scottjasso
Copy link

@scottjasso scottjasso commented Feb 19, 2025

Fixes #249

Initial stab at implementing. Haven't added significant tests or updated any docs -- wanted to check this is the right direction first.

Using @IntoSet(multiple = true) requires returning a Set. Using @IntoMap(multiple = true) requires returning a Map. In either case, all providers will be merged together the final collection. This also makes it possible to bind an emptySet or emptyMap to act as the default.

A few notes:

IntoSet impl

This changes the IntoSet implementation from setOf() to something like:

buildSet(3) {
  add(itemOne())
  add(itemTwo)
  addAll(itemThreeAndFour())
}
  • 3 here is just the number of providers -- we can't know how many items a multiple = true could provide, so our guess is just 1 item per provider. This is the same as what dagger generates for @ElementsIntoSet, e.g. ImmutableSet.builderWithExpectedSize(3)....
  • We have to use add and addAll to properly distinguish between a multiple = true value and a multiple = false value (+= could use the wrong overload for a Set<Iterable<T>>).

IntoMap impl

Similarly, this changes the IntoMap implementation from mapOf() to something like:

buildMap(3) {
  this += pairOne()
  this += pairTwo()
  this += mapOfThreeAndFour()
}
  • In this case, we can use the += operator since we know the values are either Pair or Map -- it's unambiguous. Using put isn't directly possible -- there's no put(Pair).

IntoSet w/ Lazy or Functions

The existing behavior is a little tricky. Let's say you have a Set<Lazy<T>> that you want to bind (same thing goes for () -> T aka Function0<T>)

  • If you have only IntoSet providers of type T, the generated code will automatically wrap your provider(s) with lazy { }.
  • If you have any IntoSet providers of type Lazy<T>, that will cause all of the IntoSet providers of type T to be ignored. Only the lazy providers will be used, and no wrapping happens.

This is a bit surprising, IMO. However, I'm not sure how this should interact with multiple = true. If you want a binding of Set<Lazy<T>>, but you have a multiple = true provider of Set<T>, then the provider would have to be evaluated in order to find all the items, defeating the point of Lazy.

Example:

val binding: Set<Lazy<T>>

@Provides @IntoSet fun provideT(): T = ... 

// problematic line:
@Provides @IntoSet(multiple = true) fun provideSetT(): Set<T> = ...

// if either is uncommented, the above two are _always_ ignored.
// @Provides @IntoSet fun provideLazyT(): Lazy<T> = ...
// @Provides @IntoSet(multiple = true) fun provideSetLazyT(): Set<Lazy<T>> = ...

Some options:

  1. (I chose this) Fail fast, saying multiple = true is not allowed for a binding of Set<Lazy<T>>
  2. Just ignore those providers, so provideSetT() is not used in the generated code (but provideT() is).
  3. Evaluate the provider anyway, and wrap each item with lazy { }, something like addAll(provideSetT().map { lazy { it } })

@evant
Copy link
Owner

evant commented Feb 24, 2025

Just want to say thank you for this! Will get to a code review when I get a chance but at a quick glance it looks good!

IntoSet w/ Lazy or Functions

yeah that's tricky, will have to think about this. Option 1 is the most conservative so I agree that's a good starting point. Curious what dagger does? I believe it has an equivalent feature set.

@scottjasso
Copy link
Author

Curious what dagger does? I believe it has an equivalent feature set.

it actually doesn't allow this behavior: https://dagger.dev/dev-guide/multibindings

As with any other binding, in addition to depending on a multibound Set<Foo>, you can also depend on Provider<Set<Foo>> or Lazy<Set<Foo>>. You cannot, however, depend on Set<Provider<Foo>>.

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.

Better handle empty multiple bindings.
2 participants