fx.Annotate: make variadic params optional by default (#831)

* fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * Temporarily pin Dig dependency to master (#827) This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301. * Add fx.Module (#830) This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> * fx.Module: Reorganize code (#832) In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b9cb4eb6eede8e1ca672cb4d74f679ddf2. This reverts commit 8aa68c04a85ac15123bdbaffa70ce7dac478660e. Co-authored-by: Abhinav Gupta <abg@uber.com> * Finish incomplete merge * Only make variadic params optional if no other tags are specified * remove print statements * Update annotated.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * add extra test * Fix test "Provide variadic function with no optional params" * Explain why we're adding the optional tag * Fix test case, verified failing on master Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> Co-authored-by: Sung Yoon Whang <sungyoon@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>
luoboton · Mar 11, 2023 · d847f6b · d847f6b
1 parent 40b8488
commit d847f6b
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 1 deletion.
diff --git a/annotated.go b/annotated.go
@@ -360,7 +360,7 @@ func (ann *annotated) parameters() (
 
 	// No parameter annotations. Return the original types
 	// and an identity function.
-	if len(ann.ParamTags) == 0 {
+	if len(ann.ParamTags) == 0 && !ft.IsVariadic() {
 		return types, func(args []reflect.Value) []reflect.Value {
 			return args
 		}
@@ -383,6 +383,11 @@ func (ann *annotated) parameters() (
 
 		if i < len(ann.ParamTags) {
 			field.Tag = reflect.StructTag(ann.ParamTags[i])
+		} else if i == ft.NumIn()-1 && ft.IsVariadic() {
+			// If a variadic argument is unannotated, mark it optional,
+			// so that just wrapping a function in fx.Annotate does not
+			// suddenly introduce a required []arg dependency.
+			field.Tag = reflect.StructTag(`optional:"true"`)
 		}
 
 		inFields = append(inFields, field)

diff --git a/annotated_test.go b/annotated_test.go
@@ -522,6 +522,10 @@ func TestAnnotate(t *testing.T) {
 	newSliceA := func(sa ...*a) *sliceA {
 		return &sliceA{sa}
 	}
+	newSliceAWithB := func(b *b, sa ...*a) *sliceA {
+		total := append(sa, b.a)
+		return &sliceA{total}
+	}
 
 	t.Run("Provide with optional", func(t *testing.T) {
 		t.Parallel()
@@ -599,6 +603,72 @@ func TestAnnotate(t *testing.T) {
 		assert.Len(t, got.sa, 2)
 	})
 
+	t.Run("Provide variadic function with no optional params", func(t *testing.T) {
+		t.Parallel()
+
+		var got struct {
+			fx.In
+
+			Result *sliceA `name:"as"`
+		}
+		app := fxtest.New(t,
+			fx.Supply([]*a{{}, {}, {}}),
+			fx.Provide(
+				fx.Annotate(newSliceA,
+					fx.ResultTags(`name:"as"`),
+				),
+			),
+			fx.Populate(&got),
+		)
+		defer app.RequireStart().RequireStop()
+		require.NoError(t, app.Err())
+		assert.Len(t, got.Result.sa, 3)
+	})
+
+	t.Run("Provide variadic function named with no given params", func(t *testing.T) {
+		t.Parallel()
+
+		var got *sliceA
+		app := NewForTest(t,
+			fx.Provide(
+				fx.Annotate(newSliceA, fx.ParamTags(`name:"a"`)),
+			),
+			fx.Populate(&got),
+		)
+		err := app.Err()
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), `missing dependencies`)
+		assert.Contains(t, err.Error(), `missing type: []*fx_test.a[name="a"]`)
+	})
+
+	t.Run("Invoke variadic function with multiple params", func(t *testing.T) {
+		t.Parallel()
+
+		app := fxtest.New(t,
+			fx.Supply(
+				fx.Annotate(newB(newA()), fx.ResultTags(`name:"b"`)),
+			),
+			fx.Invoke(fx.Annotate(newSliceAWithB, fx.ParamTags(`name:"b"`))),
+		)
+
+		defer app.RequireStart().RequireStop()
+		require.NoError(t, app.Err())
+	})
+
+	t.Run("Invoke non-optional variadic function with a missing dependency", func(t *testing.T) {
+		t.Parallel()
+
+		app := NewForTest(t,
+			fx.Invoke(
+				fx.Annotate(newSliceA, fx.ParamTags(`optional:"false"`)),
+			),
+		)
+		err := app.Err()
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), `missing dependencies`)
+		assert.Contains(t, err.Error(), `missing type: []*fx_test.a`)
+	})
+
 	t.Run("Invoke with variadic function", func(t *testing.T) {
 		t.Parallel()