Misc. fixes from #305 (#311)

* 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.

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
Co-authored-by: Abhinav Gupta <abg@uber.com>

Author: sywhang

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
 

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

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()

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)...)

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) {