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

Annotation: Param/ResultTags with In/Out #980

Closed
sywhang opened this issue Oct 27, 2022 · 7 comments · Fixed by #1053
Closed

Annotation: Param/ResultTags with In/Out #980

sywhang opened this issue Oct 27, 2022 · 7 comments · Fixed by #1053

Comments

@sywhang
Copy link
Contributor

sywhang commented Oct 27, 2022

Currently, fx.ParamTags and fx.ResultTags error out when they're used with fx.In and fx.Out.

It makes sense that fx.ParamTags shouldn't be used with fx.In, and fx.ResultTags with fx.Out, but there's no reason for fx.ParamTags to not be able to be used with fx.Out.

We should allow the usage of fx.ParamTags annotation with fx.Out, and similarly, fx.ResultTags with fx.In.

@recht
Copy link

recht commented Oct 28, 2022

Related to this, if I have a constructor that takes two args, the second of them being an fx.In struct, but I want to put a ParamTags on the first one then that's also not possible, which also seems overly restrictive.

@sywhang
Copy link
Contributor Author

sywhang commented Oct 28, 2022

Thanks for the feedback @recht. We can also look into making that possible.

@abhinav
Copy link
Collaborator

abhinav commented Oct 28, 2022

EDIT: Disregard. I misread.

I think there may be some annoying cases with supporting ParamTags on fx.In.

type Foo struct{ fx.In; /* ... */ }
type Bar struct{ fx.In; /* ... */ }

func New(Foo, Bar) *Baz { ... }

This is valid. If ParamTags supports fx.In structs, then the position of ParamTags has to match the fields of Foo first, and Bar next.

This is also valid:

type Foo struct { fx.In; /* ... */ }

type Bar struct{
  fx.In

  Foo Foo
}

func New(Bar) *Baz

In this case, ParamTags will have to recurse down.

These are both possible but annoying. I suggest moving support for those into a separate ticket, and scoping this to just allowing ParamTags with fx.Out, and ResultTags with fx.In.

@sywhang
Copy link
Contributor Author

sywhang commented Oct 28, 2022

@abhinav Perhaps I misunderstood, but isn't @recht's request that we be able to annotate types with fx.ParamTags when they're not an fx.In struct? i.e.

type Foo struct {
  // not an fx.In struct
}

type Bar struct {
  fx.In
  // stuff
}

func New(Foo, Bar) FooBar { ... }

fx.Provide(
  New,
  fx.ParamTags(`name:"myFoo"`),
)

@abhinav
Copy link
Collaborator

abhinav commented Oct 29, 2022

Oops, you're right. I misread.

@sywhang
Copy link
Contributor Author

sywhang commented Mar 9, 2023

@jkanywhere reported this issue as well. We'll have someone take a look at this

@sywhang
Copy link
Contributor Author

sywhang commented Mar 9, 2023

@JacobOaks if you're looking for another Fx issue to handle, this may be a good one.

sywhang added a commit to sywhang/fx that referenced this issue Mar 10, 2023
…tively.

It is currently not possible for fx.Annotate to be used on a function that
takes in or returns fx.In and fx.Out structs.

This limitation, however, is not necessary when the annotation is different
from the struct defined annotations, such as using fx.In with fx.ResultTags
Annotation or fx.Out with fx.ParamTags Annotation.

This loosens the constraint so that these Annotations become valid.

Fixes uber-go#980.
sywhang added a commit that referenced this issue Mar 10, 2023
…tively (#1053)

It is currently not possible for fx.Annotate to be used on a function
that takes in or returns fx.In and fx.Out structs.

This limitation, however, is not necessary when the annotation is
different from the struct defined annotations, such as using fx.In with
fx.ResultTags Annotation or fx.Out with fx.ParamTags Annotation.

This loosens the constraint so that these combinations of Annotations
become valid.

Fixes #980.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants