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 API to provide varargs #120

Open
akshayjshah opened this issue Jul 6, 2017 · 5 comments
Open

Add API to provide varargs #120

akshayjshah opened this issue Jul 6, 2017 · 5 comments

Comments

@akshayjshah
Copy link
Contributor

Typically, variadic arguments are optional - the function needs to handle len(varargs)==0 already. Making dig automatically treat varargs as optional would make a wider variety of plain-Go functions work out of the box.

@abhinav abhinav assigned abhinav and unassigned glibsm and HelloGrayson Jul 10, 2017
abhinav added a commit that referenced this issue Jul 10, 2017
This adds support for functions with a variadic number of arguments to
be Provided and Invoked. We will declare a dependency on a slice of the
variadic type when a variadic function is encountered.

With #120, this will be changed to an optional depedency so that
variadic constructors Just Work.
@abhinav
Copy link
Collaborator

abhinav commented Jul 10, 2017

This needs further discussion.

Currently, constructors that have variadic arguments cannot be called from
dig. For NewFoo(...Option) *Foo, you cannot provide []Option or Option.

So before we make variadic arguments optional, we have to figure out how, if
at all, users will be able to Provide them to constructors.

After discussions with @alsamylkin and @akshayjshah, there are two solutions
to this:

  1. Disallow providing variadic arguments to constructors. A function
    NewFoo(*Bar, ...Option) *Foo may be Provided into the container but we
    will supply no method of filling in that ...Option. The constructor will
    be called with just the *Bar. We are categorizing supplying variadic
    arguments as a complex use case, the kind for which we expect people to
    use dig.In, dig.Out, or wrapper functions. An advantage of this
    solution is that we can add support for providing variadic arguments later
    if a better solution becomes possible.
@alsamylkin suggested that under this solution, if necessary, `fx` could
provide a function like `fx.Expand` that lets users explicitly opt into
vararg-slice conversion. As in,

    fx.Provide(fx.Expand(NewFoo))

Is equivalent to,

    fx.Provide(func(b *Bar, opts []Option) *Foo { return NewFoo(b, opts...) })
  1. Allow providing variadic arguments as slices by default (optional as per
    this issue). A function NewFoo(*Bar, ...Option) may be Provided.
    Additionally, a []Option may optionally be provided. The function will
    be called with that slice expanded as if with opts.... Arguably, this is
    how a user would expect to provide values for variadic arguments. Another
    advantage is that most existing constructors users write will Just Work
    without any wrapping.

CC @alsamylkin @akshayjshah @breerly @glibsm

@akshayjshah
Copy link
Contributor Author

I'm fine with the first option above - treating all variadic arguments as unmet optional dependencies - in the short term. If we can't come to agreement quickly, let's land that change and continue discussion.

For a 1.0 release, I think that the second option (providing variadic arguments as slices) is better. One of dig's design goals was to let plain-Go constructor functions work correctly without extra ceremony. Functional options are very common, so we shouldn't treat them as an advanced use case - they're regular constructors, and they should just work.

Concretely, "no extra ceremony" means that for any constructor NewFoo, graph.Provide(NewFoo) should work. Furthermore, changing NewFoo from func(*A) to func(*A, ...FooOption) shouldn't immediately require changes to the way it's provided to dig graphs.

While I see @alsamylkin's point about the syntax for passing arguments, I don't think we should choose an implementation that allows only one option. Since varargs and slices are so closely related in Go, providing them as slices seems both simple and obvious to me. I suspect that most experienced Gophers will feel the same.

@ghost
Copy link

ghost commented Jul 11, 2017

I think we see the problem from different sides: callee and the caller. While the callee does receive a slice, the caller doesn't have it, the compiler is going to create it. DIG is the caller in this case, so it needs to find all the arguments with this type, but it doesn't support multiple instances.

So DIG already fails the "no extra ceremony" approach with constructors that expect several arguments of the same type. This is how I see varargs and think it represents a larger portion of issues, that we think is ok to wrap with a struct and named fields.

The reason I prefer the first option is that I can totally be wrong and we can add this feature without breaking changes, while if a second approach will become undesirable, it would be a breaking change. That's why I prefer a more conservative approach.

Some stats about searching for keywords on Github:
func New: 1.1M
type option: 6K

abhinav added a commit that referenced this issue Jul 11, 2017
Dig constructors may now be variadic functions. These functions will be
called with their dependencies as usual and no arguments will be passed
for the variadic arguments.

So a constructor `New(*Foo, *Bar, ...Option) *Baz` will be treated as
`New(*Foo, *Bar) *Baz`.

See #120 for more discussion.
abhinav added a commit that referenced this issue Jul 11, 2017
Dig constructors may now be variadic functions. These functions will be
called with their dependencies as usual and no arguments will be passed
for the variadic arguments.

So a constructor `New(*Foo, *Bar, ...Option) *Baz` will be treated as
`New(*Foo, *Bar) *Baz`.

See #120 for more discussion.
@akshayjshah
Copy link
Contributor Author

Retitling this and tagging it as a post-1.0 feature.

@akshayjshah akshayjshah changed the title Treat varargs as optional? Add API to provide varargs Jul 19, 2017
@akshayjshah
Copy link
Contributor Author

Let's plan to revisit this now that we're post 1.0. Value groups are probably the way to go here.

abhinav added a commit to uber-go/fx that referenced this issue Nov 19, 2021
Add support to fx.Annotate for variadic functions.
The last parameter of variadic functions is now treated as a slice.
With this users can feed value groups into variadic functions.

Ref uber-go/dig#120

Co-authored-by: Abhinav Gupta <abg@uber.com>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
hbdf pushed a commit to hbdf/dig that referenced this issue Jul 14, 2022
hbdf pushed a commit to hbdf/dig that referenced this issue Jul 14, 2022
* Clean up wording and variable naming

* More language fixes
luoboton added a commit to luoboton/fx that referenced this issue Aug 24, 2022
Add support to fx.Annotate for variadic functions.
The last parameter of variadic functions is now treated as a slice.
With this users can feed value groups into variadic functions.

Ref uber-go/dig#120

Co-authored-by: Abhinav Gupta <abg@uber.com>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
@abhinav abhinav removed their assignment Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants