Skip to content

Commit

Permalink
Disallow using In/Out structs with From/As annotations (#1068)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
3 people authored Apr 18, 2023
1 parent 195960b commit 475cd1b
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 11 deletions.
24 changes: 17 additions & 7 deletions annotated.go
Original file line number Diff line number Diff line change
Expand Up @@ -1137,6 +1137,8 @@ var _ Annotation = (*asAnnotation)(nil)
// w, r := a()
// return w, r
// }
//
// As annotation cannot be used in a function that returns an [Out] struct as a return type.
func As(interfaces ...interface{}) Annotation {
return &asAnnotation{targets: interfaces}
}
Expand Down Expand Up @@ -1322,6 +1324,9 @@ var _ Annotation = (*fromAnnotation)(nil)
// fx.Provide(func(r1 *FooRunner, r2 *BarRunner) *RunnerWraps {
// return NewRunnerWraps(r1, r2)
// })
//
// From annotation cannot be used in a function that takes an [In] struct as a
// parameter.
func From(interfaces ...interface{}) Annotation {
return &fromAnnotation{targets: interfaces}
}
Expand Down Expand Up @@ -1591,8 +1596,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 a ResultTags or an As annotation
// - takes in an fx.In struct as a parameter and has either a ParamTags or a From annotation
// - has an error result not as the last result.
func (ann *annotated) typeCheckOrigFn() error {
ft := reflect.TypeOf(ann.Target)
Expand All @@ -1608,18 +1613,23 @@ 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 {
return errors.New("fx.Out structs cannot be annotated with fx.ResultTags or fx.As")
}
}

for i := 0; i < ft.NumIn(); i++ {
it := ft.In(i)
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.From) > 0 {
return errors.New("fx.In structs cannot be annotated with fx.ParamTags or fx.From")
}
}
return nil
Expand Down
63 changes: 59 additions & 4 deletions annotated_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1427,7 +1427,7 @@ func TestAnnotate(t *testing.T) {
assert.Contains(t, err.Error(), "must provide constructor function, got 42 (int)")
})

t.Run("annotate a fx.Out", func(t *testing.T) {
t.Run("annotate a fx.Out with ResultTags", func(t *testing.T) {
t.Parallel()

type A struct {
Expand All @@ -1448,10 +1448,40 @@ func TestAnnotate(t *testing.T) {

err := app.Err()
require.Error(t, err)
assert.Contains(t, err.Error(), "fx.Out structs cannot be annotated")
assert.Contains(t, err.Error(), "fx.Out structs cannot be annotated with fx.ResultTags or fx.As")
})

t.Run("annotate a fx.In", func(t *testing.T) {
t.Run("annotate a fx.Out with As", func(t *testing.T) {
t.Parallel()

type I interface{}

type B struct {
// implements I
}

type Res struct {
fx.Out

AB B
}

f := func() Res {
return Res{AB: B{}}
}

app := NewForTest(t,
fx.Provide(
fx.Annotate(f, fx.As(new(I))),
),
)

err := app.Err()
require.Error(t, err)
assert.Contains(t, err.Error(), "fx.Out structs cannot be annotated with fx.ResultTags or fx.As")
})

t.Run("annotate a fx.In with ParamTags", func(t *testing.T) {
t.Parallel()

type A struct {
Expand All @@ -1471,7 +1501,32 @@ func TestAnnotate(t *testing.T) {
require.Error(t, err)
assert.NotContains(t, err.Error(), "invalid annotation function func(fx_test.A) string")
assert.Contains(t, err.Error(), "invalid annotation function func(fx_test.B) string")
assert.Contains(t, err.Error(), "fx.In structs cannot be annotated")
assert.Contains(t, err.Error(), "fx.In structs cannot be annotated with fx.ParamTags or fx.From")
})

t.Run("annotate a fx.In with From", func(t *testing.T) {
t.Parallel()

type I interface{}

type B struct {
// implements I
}

type Param struct {
fx.In
BInterface I
}

app := NewForTest(t,
fx.Provide(
fx.Annotate(func(p Param) string { return "ok" }, fx.From(new(B))),
),
)
err := app.Err()
require.Error(t, err)
assert.Contains(t, err.Error(), "invalid annotation function func(fx_test.Param) string")
assert.Contains(t, err.Error(), "fx.In structs cannot be annotated with fx.ParamTags or fx.From")
})

t.Run("annotate fx.In with fx.ResultTags", func(t *testing.T) {
Expand Down

0 comments on commit 475cd1b

Please sign in to comment.