Repository: uber-go/dig
Pull request #311 (2022-01-05 18:51:45)

Misc. fixes from #305
This contains some miscellaneous fixes from additional feedback
from #305. Notably:

- Make provider resolution for parameter building iterate the other way around (from self to root) and break once a provider is found.
- Rename appendLeafScopes to appendSubscopes.
- Rename getScopesFromRoot to ancestors
- Change getStoresFromRoot to use storesToRoot.

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

[![Impacted file tree graph](https://codecov.io/gh/uber-go/dig/pull/311/graphs/tree.svg?width=650&height=150&src=pr&token=ARuH7Frwgw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)](https://codecov.io/gh/uber-go/dig/pull/311?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)

```diff
@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
+ Coverage   98.46%   98.61%   +0.15%     
==========================================
  Files          19       19              
  Lines        1234     1230       -4     
==========================================
- Hits         1215     1213       -2     
+ Misses         11       10       -1     
+ Partials        8        7       -1     
```


| [Impacted Files](https://codecov.io/gh/uber-go/dig/pull/311?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) | Coverage Δ | |
|---|---|---|
| [param.go](https://codecov.io/gh/uber-go/dig/pull/311/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-cGFyYW0uZ28=) | `98.98% <100.00%> (+0.97%)` | :arrow_up: |
| [provide.go](https://codecov.io/gh/uber-go/dig/pull/311/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-cHJvdmlkZS5nbw==) | `100.00% <100.00%> (ø)` | |
| [scope.go](https://codecov.io/gh/uber-go/dig/pull/311/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-c2NvcGUuZ28=) | `98.70% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber-go/dig/pull/311?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/311?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Last update [5e5a20d...d737bb4](https://codecov.io/gh/uber-go/dig/pull/311?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/container.go b/container.go
index ef5c5a8..c4b020c 100644
--- a/container.go
+++ b/container.go
@@ -111,7 +111,9 @@ type containerStore interface {
 	// type across all the Scopes that are in effect of this containerStore.
 	getAllValueProviders(name string, t reflect.Type) []provider
 
-	getStoresFromRoot() []containerStore
+	// Reports a list of stores (starting at this store) up to the root
+	// store.
+	storesToRoot() []containerStore
 
 	createGraph() *dot.Graph
 
diff --git a/param.go b/param.go
index bf9d427..62b4737 100644
--- a/param.go
+++ b/param.go
@@ -146,36 +146,28 @@ func (pl paramList) Build(containerStore) (reflect.Value, error) {
 // to the underlying constructor.
 func (pl paramList) BuildList(c containerStore) ([]reflect.Value, error) {
 	args := make([]reflect.Value, len(pl.Params))
-	argsBuilt := make([]bool, len(pl.Params))
-	allContainers := c.getStoresFromRoot()
+	allContainers := c.storesToRoot()
 	for i, p := range pl.Params {
-		var err error
-		var arg reflect.Value
 		// iterate through the tree path of scopes.
+	containerLoop:
 		for _, c := range allContainers {
-			if arg, err = p.Build(c); err == nil {
+			arg, err := p.Build(c)
+			if err == nil {
 				args[i] = arg
-				argsBuilt[i] = true
+				break containerLoop
+			}
+			// If argument has successfully been built, it's possible
+			// for these errors to occur in child scopes that don't
+			// contain the given parameter type. We can safely ignore
+			// these.
+			// If it's an error other than missing types/dependencies,
+			// this means some constructor returned an error that must
+			// be reported.
+			_, isErrMissingTypes := err.(errMissingTypes)
+			_, isErrMissingDeps := err.(errMissingDependencies)
+			if err != nil && !isErrMissingTypes && !isErrMissingDeps {
+				return nil, err
 			}
-		}
-
-		// If an argument failed to build, that means none of the
-		// scopes had the type. This should be reported.
-		if !argsBuilt[i] {
-			return nil, err
-		}
-
-		// If argument has successfully been built, it's possible
-		// for these errors to occur in child scopes that don't
-		// contain the given parameter type. We can safely ignore
-		// these.
-		// If it's an error other than missing types/dependencies,
-		// this means some constructor returned an error that must
-		// be reported.
-		_, isErrMissingTypes := err.(errMissingTypes)
-		_, isErrMissingDeps := err.(errMissingDependencies)
-		if err != nil && !isErrMissingTypes && !isErrMissingDeps {
-			return nil, err
 		}
 	}
 	return args, nil
diff --git a/provide.go b/provide.go
index c861d1c..510476b 100644
--- a/provide.go
+++ b/provide.go
@@ -434,7 +434,7 @@ func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) {
 	// take a snapshot of the current graph state before
 	// we start making changes to it as we may need to
 	// undo them upon encountering errors.
-	allScopes := s.appendLeafScopes(nil)
+	allScopes := s.appendSubscopes(nil)
 	for _, s := range allScopes {
 		s := s
 		s.gh.Snapshot()
diff --git a/scope.go b/scope.go
index 8dcdb7d..278064f 100644
--- a/scope.go
+++ b/scope.go
@@ -115,34 +115,29 @@ func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope {
 	return child
 }
 
-// getScopesFromRoot returns a list of Scopes from the root Container
-// until the current Scope.
-func (s *Scope) getScopesFromRoot() []*Scope {
+// ancestors returns a list of scopes of ancestors of this scope up to the
+// root. The scope at at index 0 is this scope itself.
+func (s *Scope) ancestors() []*Scope {
 	var scopes []*Scope
 	for s := s; s != nil; s = s.parentScope {
 		scopes = append(scopes, s)
 	}
-	for i, j := 0, len(scopes)-1; i < j; i, j = i+1, j-1 {
-		scopes[i], scopes[j] = scopes[j], scopes[i]
-	}
 	return scopes
 }
 
-func (s *Scope) appendLeafScopes(dest []*Scope) []*Scope {
+func (s *Scope) appendSubscopes(dest []*Scope) []*Scope {
 	dest = append(dest, s)
 	for _, cs := range s.childScopes {
-		dest = cs.appendLeafScopes(dest)
+		dest = cs.appendSubscopes(dest)
 	}
 	return dest
 }
 
-func (s *Scope) getStoresFromRoot() []containerStore {
-	var stores []containerStore
-	for s := s; s != nil; s = s.parentScope {
-		stores = append(stores, s)
-	}
-	for i, j := 0, len(stores)-1; i < j; i, j = i+1, j-1 {
-		stores[i], stores[j] = stores[j], stores[i]
+func (s *Scope) storesToRoot() []containerStore {
+	scopes := s.ancestors()
+	stores := make([]containerStore, len(scopes))
+	for i, s := range scopes {
+		stores[i] = s
 	}
 	return stores
 }
@@ -207,7 +202,7 @@ func (s *Scope) getAllValueProviders(name string, t reflect.Type) []provider {
 }
 
 func (s *Scope) getAllProviders(k key) []provider {
-	allScopes := s.getScopesFromRoot()
+	allScopes := s.ancestors()
 	var providers []provider
 	for _, scope := range allScopes {
 		providers = append(providers, scope.getProviders(k)...)


Test patch:
diff --git a/scope_test.go b/scope_test.go
index fede567..f083464 100644
--- a/scope_test.go
+++ b/scope_test.go
@@ -36,8 +36,8 @@ func TestScopedOperations(t *testing.T) {
 		s2 := s1.Scope("child2")
 		s3 := s2.Scope("child2")
 
-		assert.Equal(t, []containerStore{c.scope, s1, s2, s3}, s3.getStoresFromRoot())
-		assert.Equal(t, []*Scope{c.scope, s1, s2, s3}, s3.getScopesFromRoot())
+		assert.Equal(t, []containerStore{s3, s2, s1, c.scope}, s3.storesToRoot())
+		assert.Equal(t, []*Scope{s3, s2, s1, c.scope}, s3.ancestors())
 	})
 
 	t.Run("private provides", func(t *testing.T) {
@@ -130,6 +130,39 @@ func TestScopedOperations(t *testing.T) {
 			assert.NoError(t, scope.Invoke(func(a *A) {}))
 		}
 	})
+
+	t.Run("parent shares values with children", func(t *testing.T) {
+		type (
+			T1 struct{ s string }
+			T2 struct{}
+		)
+
+		parent := New()
+
+		require.NoError(t, parent.Provide(func() T1 {
+			assert.Fail(t, "parent should not be called")
+			return T1{"parent"}
+		}))
+
+		child := parent.Scope("child")
+
+		var childCalled bool
+		defer func() {
+			assert.True(t, childCalled, "child constructor must be called")
+		}()
+		require.NoError(t, child.Provide(func() T1 {
+			childCalled = true
+			return T1{"child"}
+		}))
+
+		require.NoError(t, child.Provide(func(v T1) T2 {
+			assert.Equal(t, "child", v.s,
+				"value should be built by child")
+			return T2{}
+		}))
+
+		require.NoError(t, child.Invoke(func(T2) {}))
+	})
 }
 
 func TestScopeFailures(t *testing.T) {