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

Disallow using In/Out structs with From/As annotations #1060

Closed
tchung1118 opened this issue Mar 22, 2023 · 1 comment · Fixed by #1068
Closed

Disallow using In/Out structs with From/As annotations #1060

tchung1118 opened this issue Mar 22, 2023 · 1 comment · Fixed by #1068

Comments

@tchung1118
Copy link
Contributor

tchung1118 commented Mar 22, 2023

In #1053, we eased up the requirement for function signature for applying annotations to allow using In/Out structs with ResultTags/ParamTags. This had an unintended change in behavior where users are now able to use In/Out structs with From/As annotations. For example, the below code used to error out:

type Foo interface { ... }
type Bar struct { ... } // Bar implements Foo
struct Res {
  fx.Out

  ABar Bar
}
fx.Annotate(
  func() Res { ... },
  fx.As(new(Foo)),
)

Currently the code above applies the As annotation to ABar field and transforms the target function's signature accordingly. However, this was an unintended change in behavior, and we should have a design discussion about whether we actually want this before cutting a new release. Since it will require a breaking change to disable this after a new release is cut, I think the safe approach is to disable this for now and enable it after having decided that this is indeed what we want.

@ankit209
Copy link
Contributor

ankit209 commented Mar 23, 2023

Would love to pick up this issue. Suggested change in annotated.go.

@tchung1118 Can you please add me as a contributor to uber/fx repo ?

diff --git a/annotated.go b/annotated.go
index 3c9ecf7..80c5243 100644
--- a/annotated.go
+++ b/annotated.go
@@ -1591,8 +1591,8 @@ func (ann *annotated) cleanUpAsResults() {
 }

 // checks and returns a non-nil error if the target function:
-// - returns an fx.Out struct as a result.
-// - takes in an fx.In struct as a parameter.
+// - returns an fx.Out struct as a result and has either of ResultTags, As or From annotation
+// - takes in an fx.In struct as a parameter and has either of ParamTags, As or From annotation
 // - has an error result not as the last result.
 func (ann *annotated) typeCheckOrigFn() error {
        ft := reflect.TypeOf(ann.Target)
@@ -1608,8 +1608,11 @@ func (ann *annotated) typeCheckOrigFn() error {
                if ot.Kind() != reflect.Struct {
                        continue
                }
-               if len(ann.ResultTags) > 0 && dig.IsOut(reflect.New(ft.Out(i)).Elem().Interface()) {
-                       return errors.New("fx.Out structs cannot be annotated with fx.ResultTags")
+               if !dig.IsOut(reflect.New(ft.Out(i)).Elem().Interface()) {
+                       continue
+               }
+               if len(ann.ResultTags) > 0 || len(ann.As) > 0 || len(ann.From) > 0 {
+                       return errors.New("fx.Out structs cannot be annotated with either of fx.ResultTags, fx.As or fx.From")
                }
        }

@@ -1618,8 +1621,11 @@ func (ann *annotated) typeCheckOrigFn() error {
                if it.Kind() != reflect.Struct {
                        continue
                }
-               if len(ann.ParamTags) > 0 && dig.IsIn(reflect.New(ft.In(i)).Elem().Interface()) {
-                       return errors.New("fx.In structs cannot be annotated with fx.ParamTags")
+               if !dig.IsIn(reflect.New(ft.In(i)).Elem().Interface()) {
+                       continue
+               }
+               if len(ann.ParamTags) > 0 || len(ann.As) > 0 || len(ann.From) > 0 {
+                       return errors.New("fx.In structs cannot be annotated with either of fx.ParamTags, fx.As or fx.From")
                }
        }
        return nil

sywhang added a commit that referenced this issue Apr 18, 2023
In #1053, we eased up the requirement
for function signature for applying annotations to allow using In/Out
structs with ResultTags/ParamTags. This had an unintended change in
behavior where users are now able to use In/Out structs with From/As
annotations. For example, the below code used to error out:

> type Foo interface { ... }
> type Bar struct { ... } // Bar implements Foo
> struct Res {
>   fx.Out
> 
>   ABar Bar
> }
> fx.Annotate(
>   func() Res { ... },
>   fx.As(new(Foo)),
> )

Currently the code above applies the As annotation to ABar field and
transforms the target function's signature accordingly. However, this
was an unintended change in behavior, and we should have a design
discussion about whether we actually want this before cutting a new
release. Since it will require a breaking change to disable this after a
new release is cut, I think the safe approach is to disable this for now
and enable it after having decided that this is indeed what we want.

Fixes #1060

---------

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
Co-authored-by: JacobOaks <jacoboaks.8@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants