add WithAnnotated for easily populate annotated values

[joesonw]

func NewReadOnlyConnection(...) (*Connection, error)
fx.Provide(fx.Annotated{
  Name: "ro",
  Target: NewReadOnlyConnection,
})
fx.Supply(&Server{})

fx.Invoke(fx.WithAnnotated("ro")(func(roConn *Connection, s *Server) error {
})

This can resolve #715 #691

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #718 (1e22741) into master (4adc2bf) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #718      +/-   ##
==========================================
+ Coverage   97.66%   97.83%   +0.17%     
==========================================
  Files          19       20       +1     
  Lines         513      554      +41     
==========================================
+ Hits          501      542      +41     
  Misses         10       10              
  Partials        2        2              

Impacted Files	Coverage Δ
with_annotated.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4adc2bf...1e22741. Read the comment docs.

[joesonw]

Can I request a review from either of you @shirchen @abhinav please?
If this feature will not be part of plan, it would be really nice to clarify that.
Thank you!

[shirchen]

@joesonw Thanks for the PR! Sorry, we are a little busy this week, give us a few days to get back to you with feedback.

[joesonw]

no problem. take your time.

On Thu, Jan 21, 2021 at 3:32 AM Dmitriy Shirchenko ***@***.***> wrote: @joesonw <https:/joesonw> Thanks for the PR! Sorry, we are a little busy this week, give us a few days to get back to you with feedback. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#718 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AAMPI4JDC4A2XTEQHLNNF7DS24VULANCNFSM4TS2CAHQ> .

-- Thanks, Qiaosen Huang.

[zhenzou]

Hi, is there anyone who can help to review this PR?
This is a very useful feature, thanks.

[joesonw]

@zhenzou If you do not mind, you can try https:/joesonw/fxx this library. Since this festure does not need to modify uber/fx core features, It can be done out-of-tree. I have been personally using this approach.

[zhenzou]

@joesonw Thanks for your project. It's very helpful. And inspired me that can extend fx in this way, nice work.

[robbertvanginkel]

Hi @joesonw! Thanks for the contribution and sorry for taking so long to get to this.

Although we've internally wanted and debated a shorthand for consuming annotated values, we hadn't yet come up with an good public API. Let me share some of the things we've discussed and relate them back to the WithAnnotated proposal in this pull request.

How should we handle annotations when constructors have multiple parameters? In the existing fx.Annotated, the Name/Group will apply to all non-error values returned by the constructor. Throughout our fx usage we usually see multiple input params and a single (maybe with error) result, so this works well for annotating results. However, for the same reason reason we don't want the new annotation for params API to behave similar. We suspect that in very few cases you would want to apply the same annotation to each input parameter. A more probable usecase is that you want to annotate only one of the params. One way of solving this would be to require a slice of annotations that correspond the constructor's number of parameters. This is probably unavoidable, but a loss for the fx api from a typesafety perspective. Besides that needing to be explicitly documented, it allows for some new class of runtime issues and brings up a few questions like:

1. How should it handle params that don't need to be annotated? Following the proposed API in the PR, say we have fx.WithAnnotated(fx.NameAnnotation("abc"))(func (A, B) {...}) then A gets the name annotation while B gets nothing. Although this works for this order of params, if B needs the annotation you'd need to introduce some sentinel/identity value to allow fx.WithAnnotated(fx.IdentityAnnotation, fx.NameAnnotation("abc"))(func (A, B) {...}).
2. How should it handle the slice length not matching the number of params in the constructor? In the PR's implementation this is ok as long as you provide less options than the function has parameters. This does however lead to the asymmetry where fx.IdentityAnnotation is required only in some cases depending on parameter order. Although I can see that that might allow for some more flexibility an concise code for a few cases, it also makes the code somewhat less readable and more surprising, as you'd have to know the implementation to understand that Annotate(Name("abc"))(func (A, B) {...}) only annotates A and not B, unlike fx.Annotated{Name: "abc"} which annotates all results. A Annotate(Name("abc"), Identity)(func (A, B) {...}) makes the implicit relationship between the number of options and function params more explicit.
3. How should it handle combinations of annotations? This one is somewhat more theoretical as we don't have any combinations of annotations that work today, however both the struct tags and fx.Annotated apis currently allow specifying a combination of annotations. Ideally this new Annotate for params api should allow for that.

What is a good but minmal public API? For the current WithAnnotated proposal, there's two things that stand out:

1. Little symmetry between annotations for parameters and results. For annotations on results you use a struct (fx.Annotate{Target: func(...) {}}), for annotations on parameters a closure that returns a wrapper function (fx.WithAnnotated(...)(func(...) {})). We would like to avoid the closure like approach as in the past we've found they're not so easy to log/compare as compared to structs. Besides the difference in approach, the names Annotated{}/WithAnnotated are not very inidicative that one can only be used for annotating results and the other only for annotating parameters.
2. All the annotation related types (fx.Annotation, fx.GroupAnnotation, fx.NameAnnotation, fx.CombineAnnotations(...)?, and any future ones) end up in the global package scope. Although this is not necessarily problematic, I somewhat prefer the fx.Annotated{} approach where the different annotations are struct fields, so a little more grouped and easier to discover.

Given the considerations above, I don't think we want to merge this as is and go with the proposed fx.WithAnnotated api. However, I do understand that the current status quo like

func doThings(x A, y B) { fmt.Println(x, y) }
func ... {
	fx.Invoke(func(x struct {
		fx.In
		A
		B `name:"rw"`
	}) {
		doThings(x.A, x.B)
	}),
}

isn't very practical or ideal either, so I think we should consider to iterate and come up with an alternative. We could explore extending fx.Annotated with some extra fields and add func (*Annotated) Annotate...() Annotated methods. Some ideas for how that could look:

// annotate argument 1 with name "rw"
fx.Invoke(fx.Annotated{Target: doThings}.ParamName(1, "rw"))

// annotate arguments with respective names "rw", "abc"
fx.Invoke(fx.Annotated{Target: doThings}.ParamNames("rw", "abc"))

// annotate arguments with respective names "rw", "abc"
fx.Invoke(fx.Annotated{Target: doThings}.ParamGroup("rw", "abc"))

// annotate function param at index 3 with name "rw"
fx.Invoke(fx.Annotated{Target: doThings}.Name(fx.In, 3, "rw"))
// annotate function result at index 1 with group "abc"
fx.Invoke(fx.Annotated{Target: doThings}.Group(fx.Out, 1, "abc"))

The change to the api would look something like:

type Annotated struct {
	Name   // existing
	Group  // existing
	Target // existing
	// new fields for the methods below, maybe not public and only allow setting through methods?
	ParamNames  []string
	ParamGroups []string
}

with either

// maybe s/Param/In to match fx.In
func (a *Annotated) ParamName(int, string) Annotated { ... } // param i gets name
func (a *Annotated) ParamNames([]string) Annotated { ... }   // len(slice) should be len(args(a.Target))

func (a *Annotated) ParamGroup(int, string) Annotated { ... }
func (a *Annotated) ParamGroups([]string) Annotated { ... }

// maybe s/Result/Out to match fx.Out
func (a *Annotated) ResultName(string) Annotated { a.Name = s }
func (a *Annotated) ResultGroup(string) Annotated { a.Group = s }

or

// fx.InOut is an interface only implemented by fx.In & fx.Out
func (a *Annotated) Name(fx.InOut, int, string) Annotated { ...} 
func (a *Annotated) Group(fx.InOut, int, string) Annotated { ...} 

The direction above would make annotations for params have an api more similar to annotations for results, and also has some potential to be extended for a fx.Annotated{}.As() (#673).

Let me know what you think!

[joesonw]

@robbertvanginkel
Thanks for the comment!

WithAnnotated is merely a sugar wrap for directly use annotation tags in struct. It uses reflect to map each WithAnnotated pairs into a struct where have all the annotation tags in one place. So It should behave exactly like the following:

fx.Invoke(func(x struct {
		fx.In
		A
		B `name:"rw"`
	}) {
		doThings(x.A, x.B)
	}),

1. Params that don't need to be annotated will be put to the back of the param list, like fx.WithAnnotated(fx.NameAnnotation("abc"))(func (B, A) {...}) instead of explicit annotation.
2. I think this is the same as the last one. All params that are not annotated will be treated as normal params, a rough equivalent as following (This is exactly what fx.WithAnnotated does. By using reflection, it transforms fx.WithAnnotated call into an existing working approach. fx.WithAnnotated can also work with fx.Provide as per my experience with my own projects),

fx.Invoke(fx.WithAnnotated(fx.NameAnnotation("abc"), fx.NameAnnotation("cba"))(func (B, D, A, C) error {...}))
// will be equivalent as below
fx.Invoke(func(x struct {
  fx.In
 Field1  B `name:"abc"`
 Field2 D `name:"cba"`
 Field3 A
 Field4 C
}) error {
  return (func(B, D, A, C) error {

  })(x.Field1, x.Field2, x.Field3, x.Field4)
}) 

3. How do you mean handling combinations of annotations? Could you please provide an example?

I hope these can answer some of your questions.
And I understand your reasons of not merging this. It just seems to me that a lot of people are having the same need. By adding this (or another approach gets the job done) to the uber/fx library, it is a bit easier for those who are struggling with annotations.

Thank you.

[robbertvanginkel]

For 3, conisder the combination of optional & named/grouped. Today you can do:

type Params struct {
	fx.In
	Foo *sql.DB `name:"ro" optional:"true"`
}

where Foo would be both optional and named. Without introducing something like fx.CombineAnnotations(...Annotation) Annotation, I don't see how that would fit in with the current proposal. Currently fx.Annotated{} doesn't have optional as we didn't have a need for it, but due to its design if it is something we want to support in the future we can add a Optional bool to it and it would fit.

I understand how it is syntactic sugar for something that can be achieved today. Pointing out functions can swap argument is a way to address some of considerations of point 1/2, however it doesnt adress the points about non-descriptive naming of two related types and somewhat surprising behavioral difference of applying to one vs all inputs/outputs. For when fx.In/Out can be avoided, so far fx has just worked with existing functions. We would prefer an API for consuming annotations that works for all functions and doesn't require parameter reordering.

It isn't that we don't want to merge anything that makes consuming annotated. However we do want to consider the API for when we do. Fx 1.0 has been stable without breaking changes for 3.5+ years, and the API is still pretty consistent as we've put focus on exploring different APIs for new feature requests. Happy to continue that discussion here and explore other possible forms that would fit both our needs.

[sywhang]

hey @joesonw, thanks for the contribution but I'm closing this PR since we've already added this feature as fx.Annotate (#757). It hasn't been released yet because we want to wait for fx.As (#795), but keep an eye out on the next release as it will do what you were trying to achieve with this PR. Really appreciate you taking the time to work on the PR though!