Repository: uber-go/dig
Pull request #333 (2022-03-15 22:14:40)

Fix issue with multiple decorations of value group in same Scope
This fixes a similar issue to #329 in value groups. When a value group is decorated, and its decorator depends on another decorated type within the same scope, it was skipping over due to incorrect logic in attempt to break the infinite recursion loop. 

This fix uses the concept introduced in #329 - we check if the decorator's "state" to make sure we don't run into infinite recursion, so that we can safely search the current scope for additional decorators instead of dumbly skipping over it all the time when there is a decoration happening.

This also enables us to remove the no-longer used parameter `decorating` from the parameter interfaces. 

codecov[bot]: # [Codecov](https://codecov.io/gh/uber-go/dig/pull/333?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) Report
> Merging [#333](https://codecov.io/gh/uber-go/dig/pull/333?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (be15492) into [master](https://codecov.io/gh/uber-go/dig/commit/ce8b9d75d75c5c7a5441003c836cfb0d5c0bdb57?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (ce8b9d7) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          21       21           
  Lines        1419     1420    +1     
=======================================
+ Hits         1395     1396    +1     
  Misses         15       15           
  Partials        9        9           
```


| [Impacted Files](https://codecov.io/gh/uber-go/dig/pull/333?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) | Coverage Δ | |
|---|---|---|
| [constructor.go](https://codecov.io/gh/uber-go/dig/pull/333/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-Y29uc3RydWN0b3IuZ28=) | `96.96% <100.00%> (ø)` | |
| [decorate.go](https://codecov.io/gh/uber-go/dig/pull/333/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZGVjb3JhdGUuZ28=) | `100.00% <100.00%> (ø)` | |
| [invoke.go](https://codecov.io/gh/uber-go/dig/pull/333/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-aW52b2tlLmdv) | `100.00% <100.00%> (ø)` | |
| [param.go](https://codecov.io/gh/uber-go/dig/pull/333/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-cGFyYW0uZ28=) | `97.24% <100.00%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber-go/dig/pull/333?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber-go/dig/pull/333?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Last update [ce8b9d7...be15492](https://codecov.io/gh/uber-go/dig/pull/333?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).


Patch:
diff --git a/constructor.go b/constructor.go
index 86edc67..cf58bec 100644
--- a/constructor.go
+++ b/constructor.go
@@ -142,7 +142,7 @@ func (n *constructorNode) Call(c containerStore) error {
 		}
 	}
 
-	args, err := n.paramList.BuildList(c, false /* decorating */)
+	args, err := n.paramList.BuildList(c)
 	if err != nil {
 		return errArgumentsFailed{
 			Func:   n.location,
diff --git a/decorate.go b/decorate.go
index 7b3fd63..9752611 100644
--- a/decorate.go
+++ b/decorate.go
@@ -109,7 +109,7 @@ func (n *decoratorNode) Call(s containerStore) error {
 		}
 	}
 
-	args, err := n.params.BuildList(n.s, s == n.s /* decorating */)
+	args, err := n.params.BuildList(n.s)
 	if err != nil {
 		return errArgumentsFailed{
 			Func:   n.location,
diff --git a/invoke.go b/invoke.go
index 7506a50..e154bdd 100644
--- a/invoke.go
+++ b/invoke.go
@@ -82,7 +82,7 @@ func (s *Scope) Invoke(function interface{}, opts ...InvokeOption) error {
 		s.isVerifiedAcyclic = true
 	}
 
-	args, err := pl.BuildList(s, false /* decorating */)
+	args, err := pl.BuildList(s)
 	if err != nil {
 		return errArgumentsFailed{
 			Func:   digreflect.InspectFunc(function),
diff --git a/param.go b/param.go
index b79c32a..54c45be 100644
--- a/param.go
+++ b/param.go
@@ -49,7 +49,7 @@ type param interface {
 	// Container.
 	//
 	// This MAY panic if the param does not produce a single value.
-	Build(store containerStore, decorating bool) (reflect.Value, error)
+	Build(store containerStore) (reflect.Value, error)
 
 	// DotParam returns a slice of dot.Param(s).
 	DotParam() []*dot.Param
@@ -137,18 +137,18 @@ func newParamList(ctype reflect.Type, c containerStore) (paramList, error) {
 	return pl, nil
 }
 
-func (pl paramList) Build(containerStore, bool) (reflect.Value, error) {
+func (pl paramList) Build(containerStore) (reflect.Value, error) {
 	digerror.BugPanicf("paramList.Build() must never be called")
 	panic("") // Unreachable, as BugPanicf above will panic.
 }
 
 // BuildList returns an ordered list of values which may be passed directly
 // to the underlying constructor.
-func (pl paramList) BuildList(c containerStore, decorating bool) ([]reflect.Value, error) {
+func (pl paramList) BuildList(c containerStore) ([]reflect.Value, error) {
 	args := make([]reflect.Value, len(pl.Params))
 	for i, p := range pl.Params {
 		var err error
-		args[i], err = p.Build(c, decorating)
+		args[i], err = p.Build(c)
 		if err != nil {
 			return nil, err
 		}
@@ -221,7 +221,7 @@ func (ps paramSingle) getValue(c containerStore) (reflect.Value, bool) {
 // current scope, if there are any. If there are multiple Scopes that decorates
 // this parameter, the closest one to the Scope that invoked this will be used.
 // If there are no decorators associated with this parameter, _noValue is returned.
-func (ps paramSingle) buildWithDecorators(c containerStore, decorating bool) (v reflect.Value, found bool, err error) {
+func (ps paramSingle) buildWithDecorators(c containerStore) (v reflect.Value, found bool, err error) {
 	var (
 		d               decorator
 		decoratingScope containerStore
@@ -256,8 +256,8 @@ func (ps paramSingle) buildWithDecorators(c containerStore, decorating bool) (v
 	return
 }
 
-func (ps paramSingle) Build(c containerStore, decorating bool) (reflect.Value, error) {
-	v, found, err := ps.buildWithDecorators(c, decorating)
+func (ps paramSingle) Build(c containerStore) (reflect.Value, error) {
+	v, found, err := ps.buildWithDecorators(c)
 	if found {
 		return v, err
 	}
@@ -401,10 +401,10 @@ func newParamObject(t reflect.Type, c containerStore) (paramObject, error) {
 	return po, nil
 }
 
-func (po paramObject) Build(c containerStore, decorating bool) (reflect.Value, error) {
+func (po paramObject) Build(c containerStore) (reflect.Value, error) {
 	dest := reflect.New(po.Type).Elem()
 	for _, f := range po.Fields {
-		v, err := f.Build(c, decorating)
+		v, err := f.Build(c)
 		if err != nil {
 			return dest, err
 		}
@@ -476,8 +476,8 @@ func newParamObjectField(idx int, f reflect.StructField, c containerStore) (para
 	return pof, nil
 }
 
-func (pof paramObjectField) Build(c containerStore, decorating bool) (reflect.Value, error) {
-	v, err := pof.Param.Build(c, decorating)
+func (pof paramObjectField) Build(c containerStore) (reflect.Value, error) {
+	v, err := pof.Param.Build(c)
 	if err != nil {
 		return v, err
 	}
@@ -568,6 +568,11 @@ func (pt paramGroupedSlice) callGroupDecorators(c containerStore) error {
 	for i := len(stores) - 1; i >= 0; i-- {
 		c := stores[i]
 		if d, found := c.getGroupDecorator(pt.Group, pt.Type.Elem()); found {
+			if d.State() == decoratorOnStack {
+				// This decorator is already being run. Avoid cycle
+				// and look further.
+				continue
+			}
 			if err := d.Call(c); err != nil {
 				return errParamGroupFailed{
 					CtorID: d.ID(),
@@ -601,14 +606,12 @@ func (pt paramGroupedSlice) callGroupProviders(c containerStore) (int, error) {
 	return itemCount, nil
 }
 
-func (pt paramGroupedSlice) Build(c containerStore, decorating bool) (reflect.Value, error) {
+func (pt paramGroupedSlice) Build(c containerStore) (reflect.Value, error) {
 	// do not call this if we are already inside a decorator since
 	// it will result in an infinite recursion. (i.e. decorate -> params.BuildList() -> Decorate -> params.BuildList...)
 	// this is safe since a value can be decorated at most once in a given scope.
-	if !decorating {
-		if err := pt.callGroupDecorators(c); err != nil {
-			return _noValue, err
-		}
+	if err := pt.callGroupDecorators(c); err != nil {
+		return _noValue, err
 	}
 
 	// Check if we have decorated values


Test patch:
diff --git a/decorate_test.go b/decorate_test.go
index 53febf6..4208bd4 100644
--- a/decorate_test.go
+++ b/decorate_test.go
@@ -23,6 +23,7 @@ package dig_test
 import (
 	"errors"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -123,6 +124,46 @@ func TestDecorateSuccess(t *testing.T) {
 		})
 	})
 
+	t.Run("value groups with multiple decorations", func(t *testing.T) {
+		type params struct {
+			dig.In
+
+			Strings []string `group:"strings"`
+		}
+
+		type childResult struct {
+			dig.Out
+
+			Strings []string `group:"strings"`
+		}
+
+		type A []string
+
+		parent := digtest.New(t)
+		parent.RequireProvide(func() string { return "a" }, dig.Group("strings"))
+		parent.RequireProvide(func() string { return "b" }, dig.Group("strings"))
+		parent.RequireProvide(func() string { return "c" }, dig.Group("strings"))
+
+		parent.RequireProvide(func(p params) A { return A(p.Strings) })
+
+		child := parent.Scope("child")
+
+		child.RequireDecorate(func(p params) childResult {
+			res := childResult{Strings: make([]string, len(p.Strings))}
+			for i, s := range p.Strings {
+				res.Strings[i] = strings.ToUpper(s)
+			}
+			return res
+		})
+		child.RequireDecorate(func(p params) A {
+			return append(A(p.Strings), "D")
+		})
+
+		require.NoError(t, child.Invoke(func(a A) {
+			assert.ElementsMatch(t, A{"A", "B", "C", "D"}, a)
+		}))
+	})
+
 	t.Run("simple decorate a provider to a scope and its descendants", func(t *testing.T) {
 		type A struct {
 			name string
diff --git a/param_test.go b/param_test.go
index 2ee90a8..7a1f41e 100644
--- a/param_test.go
+++ b/param_test.go
@@ -33,7 +33,7 @@ func TestParamListBuild(t *testing.T) {
 	p, err := newParamList(reflect.TypeOf(func() io.Writer { return nil }), newScope())
 	require.NoError(t, err)
 	assert.Panics(t, func() {
-		p.Build(newScope(), false /* decorating */)
+		p.Build(newScope())
 	})
 }