From 339b30a42367cfcc3468d2b535c3ead607d62396 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Mon, 6 Dec 2021 12:47:29 -0800 Subject: [PATCH 01/15] add dig.Scope --- container.go | 37 +++++++++----------------- graph.go | 7 +++-- scope.go | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 29 deletions(-) create mode 100644 scope.go diff --git a/container.go b/container.go index f4f59558..f46ba8d9 100644 --- a/container.go +++ b/container.go @@ -67,32 +67,12 @@ type optionFunc func(*Container) func (f optionFunc) applyOption(c *Container) { f(c) } // Container is a directed acyclic graph of types and their dependencies. +// A Container is the root Scope that represents the top-level scoped +// directed acyclic graph of the dependencies. type Container struct { - // Mapping from key to all the constructor node that can provide a value for that - // key. - providers map[key][]*constructorNode - - nodes []*constructorNode - - // Values that have already been generated in the container. - values map[key]reflect.Value - - // Values groups that have already been generated in the container. - groups map[key][]reflect.Value - - // Source of randomness. - rand *rand.Rand - - // Flag indicating whether the graph has been checked for cycles. - isVerifiedAcyclic bool - - // Defer acyclic check on provide until Invoke. - deferAcyclicVerification bool - - // invokerFn calls a function with arguments provided to Provide or Invoke. - invokerFn invokerFn - - gh *graphHolder + // this is the "root" Scope that represents the + // root of the scope tree. + scope *Scope } // containerWriter provides write access to the Container's underlying data @@ -142,6 +122,13 @@ type containerStore interface { // New constructs a Container. func New(opts ...Option) *Container { + s := &Scope{ + providers: make(map[key][]*constructorNode), + values: make(map[key]reflect.Value), + groups: make(map[key][]reflect.Value), + invokerFn: defaultInvoker, + rand: rand.New(rand.NewSource(time.Now().UnixNano())), + } c := &Container{ providers: make(map[key][]*constructorNode), values: make(map[key]reflect.Value), diff --git a/graph.go b/graph.go index 755470b5..d3d6250a 100644 --- a/graph.go +++ b/graph.go @@ -33,13 +33,13 @@ type graphNode struct { // It saves constructorNodes and paramGroupedSlice (value groups) // as nodes in the graph. // It implements the graph interface defined by internal/graph. -// It has 1-1 correspondence with the Container whose graph it represents. +// It has 1-1 correspondence with the Scope whose graph it represents. type graphHolder struct { // all the nodes defined in the graph. nodes []*graphNode - // Container whose graph this holder contains. - c *Container + // Scope whose graph this holder contains. + c *Scope // Number of nodes in the graph at last snapshot. // -1 if no snapshot has been taken. @@ -50,7 +50,6 @@ var _ graph.Graph = (*graphHolder)(nil) func newGraphHolder(c *Container) *graphHolder { return &graphHolder{c: c, snap: -1} - } func (gh *graphHolder) Order() int { return len(gh.nodes) } diff --git a/scope.go b/scope.go new file mode 100644 index 00000000..1aeab5ca --- /dev/null +++ b/scope.go @@ -0,0 +1,73 @@ +// Copyright (c) 2021 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package dig + +import ( + "math/rand" + "reflect" +) + +// Scope is a scoped DAG of types and their dependencies. +// A Scope may also have one or more child Scopes that "inherit" +// from it. +type Scope struct { + // Mapping from key to all the constructor node that can provide a value for that + // key. + providers map[key][]*constructorNode + + nodes []*constructorNode + + // Values that have already been generated in the container. + values map[key]reflect.Value + + // Values groups that have already been generated in the container. + groups map[key][]reflect.Value + + // Source of randomness. + rand *rand.Rand + + // Flag indicating whether the graph has been checked for cycles. + isVerifiedAcyclic bool + + // Defer acyclic check on provide until Invoke. + deferAcyclicVerification bool + + // invokerFn calls a function with arguments provided to Provide or Invoke. + invokerFn invokerFn + + gh *graphHolder + + // Parent of this Scope. + parentScope *Scope + + // All the child scopes of this Scope. + childScopes []*Scope +} + +// ScopeOption configures a Scope. It's included for future functionality; +// currently, there are no concrete implementations. +type ScopeOption interface { + applyOption(*Scope) +} + +type scopeOptionFunc func(*Scope) + +func (f scopeOptionFunc) applyOption(s *Scope) { f(s) } From 06ffea54ea96d415b64d1f267003d39b98a7555a Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 7 Dec 2021 11:54:40 -0800 Subject: [PATCH 02/15] tests are all passing --- constructor.go | 9 +++-- container.go | 15 ++++++++ invoke.go | 2 +- param.go | 8 ++++- scope.go | 8 +++++ scope_test.go | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 scope_test.go diff --git a/constructor.go b/constructor.go index 8e694a26..4e5a8afc 100644 --- a/constructor.go +++ b/constructor.go @@ -54,6 +54,8 @@ type constructorNode struct { resultList resultList order int // order of this node in graphHolder + + s *Scope // scope this node was originally provided to. } type constructorOptions struct { @@ -65,12 +67,12 @@ type constructorOptions struct { Location *digreflect.Func } -func newConstructorNode(ctor interface{}, c containerStore, opts constructorOptions) (*constructorNode, error) { +func newConstructorNode(ctor interface{}, s *Scope, opts constructorOptions) (*constructorNode, error) { cval := reflect.ValueOf(ctor) ctype := cval.Type() cptr := cval.Pointer() - params, err := newParamList(ctype, c) + params, err := newParamList(ctype, s) if err != nil { return nil, err } @@ -99,8 +101,9 @@ func newConstructorNode(ctor interface{}, c containerStore, opts constructorOpti id: dot.CtorID(cptr), paramList: params, resultList: results, + s: s, } - n.order = c.newGraphNode(n) + n.order = s.newGraphNode(n) return n, nil } diff --git a/container.go b/container.go index 8f9035ff..a5bc237c 100644 --- a/container.go +++ b/container.go @@ -108,6 +108,16 @@ type containerStore interface { // type. getGroupProviders(name string, t reflect.Type) []provider + // Returns the providers that can produce a value with the given name and + // type across all the Scopes that are in effect of this containerStore. + getAllValueProviders(name string, t reflect.Type) []provider + + // Returns the providers that can produce values for the given group and + // type across all the Scopes that are in effect of this containerStore. + getAllGroupProviders(name string, t reflect.Type) []provider + + getStoresUntilRoot() []containerStore + createGraph() *dot.Graph // Returns invokerFn function to use when calling arguments. @@ -220,6 +230,11 @@ func (c *Container) String() string { return c.scope.String() } +// Scope creates a child scope of the Container with the given name. +func (c *Container) Scope(name string, opts ...ScopeOption) *Scope { + return c.scope.Scope(name, opts...) +} + type byTypeName []reflect.Type func (bs byTypeName) Len() int { diff --git a/invoke.go b/invoke.go index 9df8fc1a..5fdcaa59 100644 --- a/invoke.go +++ b/invoke.go @@ -116,7 +116,7 @@ func findMissingDependencies(c containerStore, params ...param) []paramSingle { for _, param := range params { switch p := param.(type) { case paramSingle: - if ns := c.getValueProviders(p.Name, p.Type); len(ns) == 0 && !p.Optional { + if ns := c.getAllValueProviders(p.Name, p.Type); len(ns) == 0 && !p.Optional { missingDeps = append(missingDeps, p) } case paramObject: diff --git a/param.go b/param.go index cefeab81..7a26175d 100644 --- a/param.go +++ b/param.go @@ -146,9 +146,15 @@ 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)) + allContainers := c.getStoresUntilRoot() for i, p := range pl.Params { var err error - args[i], err = p.Build(c) + // iterate through the tree path of scopes. + for _, ac := range allContainers { + if args[i], err = p.Build(ac); err == nil { + break + } + } if err != nil { return nil, err } diff --git a/scope.go b/scope.go index 2d9afc10..b3ce2898 100644 --- a/scope.go +++ b/scope.go @@ -105,6 +105,14 @@ func (s *Scope) getScopesUntilRoot() []*Scope { return []*Scope{s} } +func (s *Scope) getStoresUntilRoot() []containerStore { + if s.parentScope != nil { + return append(s.parentScope.getStoresUntilRoot(), s) + } + return []containerStore{s} + +} + func (s *Scope) knownTypes() []reflect.Type { typeSet := make(map[reflect.Type]struct{}, len(s.providers)) for k := range s.providers { diff --git a/scope_test.go b/scope_test.go new file mode 100644 index 00000000..26c08e70 --- /dev/null +++ b/scope_test.go @@ -0,0 +1,97 @@ +// Copyright (c) 2021 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package dig + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestScopeTree(t *testing.T) { + t.Parallel() + c := New() + s1 := c.Scope("child 1") + s2 := c.Scope("child 2") + s3 := s1.Scope("grandchild") + + t.Run("verify Container tree", func(t *testing.T) { + t.Parallel() + + assert.Equal(t, s1.parentScope, c.scope) + assert.Equal(t, s2.parentScope, c.scope) + + assert.Equal(t, s3.parentScope, s1) + assert.NotEqual(t, s3.parentScope, s2) + }) + + t.Run("getScopesUntilRoot returns scopes in tree path in order of distance from root", func(t *testing.T) { + t.Parallel() + + assert.Equal(t, []*Scope{c.scope, s1, s3}, s3.getScopesUntilRoot()) + assert.Equal(t, []*Scope{c.scope, s1, s3}, s3.getScopesUntilRoot()) + }) +} + +func TestScopedOperations(t *testing.T) { + t.Parallel() + + t.Run("verify private provides", func(t *testing.T) { + c := New() + s := c.Scope("child") + type A struct{} + + f := func(a *A) { + assert.NotEqual(t, nil, a) + } + + require.NoError(t, s.Provide(func() *A { return &A{} })) + assert.NoError(t, s.Invoke(f)) + assert.Error(t, c.Invoke(f)) + }) + + t.Run("verify private provides inherits", func(t *testing.T) { + type A struct{} + type B struct{} + + useA := func(a *A) { + assert.NotEqual(t, nil, a) + } + useB := func(b *B) { + assert.NotEqual(t, nil, b) + } + + c := New() + c.Provide(func() *A { return &A{} }) + + child := c.Scope("child") + child.Provide(func() *B { return &B{} }) + assert.NoError(t, child.Invoke(useA)) + assert.NoError(t, child.Invoke(useB)) + + grandchild := child.Scope("grandchild") + + assert.NoError(t, grandchild.Invoke(useA)) + assert.NoError(t, grandchild.Invoke(useB)) + assert.Error(t, c.Invoke(useB)) + }) +} From df3e502248a6b0215591ba15641941a7761d834d Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 7 Dec 2021 16:53:24 -0800 Subject: [PATCH 03/15] add more tests, private provides and cross-Scope cycle checks fully functional --- container.go | 4 --- invoke.go | 2 +- provide.go | 11 ++++++++ scope.go | 31 +++++++++++++++++++--- scope_test.go | 73 +++++++++++++++++++++++++++++++++------------------ 5 files changed, 87 insertions(+), 34 deletions(-) diff --git a/container.go b/container.go index a5bc237c..e3a3ad8f 100644 --- a/container.go +++ b/container.go @@ -112,10 +112,6 @@ type containerStore interface { // type across all the Scopes that are in effect of this containerStore. getAllValueProviders(name string, t reflect.Type) []provider - // Returns the providers that can produce values for the given group and - // type across all the Scopes that are in effect of this containerStore. - getAllGroupProviders(name string, t reflect.Type) []provider - getStoresUntilRoot() []containerStore createGraph() *dot.Graph diff --git a/invoke.go b/invoke.go index 5fdcaa59..9df8fc1a 100644 --- a/invoke.go +++ b/invoke.go @@ -116,7 +116,7 @@ func findMissingDependencies(c containerStore, params ...param) []paramSingle { for _, param := range params { switch p := param.(type) { case paramSingle: - if ns := c.getAllValueProviders(p.Name, p.Type); len(ns) == 0 && !p.Optional { + if ns := c.getValueProviders(p.Name, p.Type); len(ns) == 0 && !p.Optional { missingDeps = append(missingDeps, p) } case paramObject: diff --git a/provide.go b/provide.go index 45776b57..1a15dd94 100644 --- a/provide.go +++ b/provide.go @@ -433,6 +433,17 @@ func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) { } s.isVerifiedAcyclic = true } + + // Before appending to the nodes, check if there are + // any child Scopes. If there are any, recurse down + // the descendents to provide the constructor to them. + if len(s.childScopes) != 0 { + for _, childScope := range s.childScopes { + if err := childScope.provide(ctor, opts); err != nil { + return err + } + } + } s.nodes = append(s.nodes, n) // Record introspection info for caller if Info option is specified diff --git a/scope.go b/scope.go index b3ce2898..a00c2f05 100644 --- a/scope.go +++ b/scope.go @@ -81,15 +81,38 @@ func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { child := &Scope{ name: name, parentScope: s, - providers: make(map[key][]*constructorNode), - values: make(map[key]reflect.Value), - groups: make(map[key][]reflect.Value), + providers: make(map[key][]*constructorNode, len(s.providers)), + values: make(map[key]reflect.Value, len(s.values)), + groups: make(map[key][]reflect.Value, len(s.groups)), invokerFn: s.invokerFn, } + // Copy over providers, values, and groups. + for key, nodes := range s.providers { + child.providers[key] = make([]*constructorNode, len(nodes)) + for i, node := range nodes { + child.providers[key][i] = node + } + } + for key, value := range s.values { + child.values[key] = value + } + + for key, groups := range s.groups { + child.groups[key] = make([]reflect.Value, len(groups)) + for i, group := range groups { + child.groups[key][i] = group + } + } + // child should hold a separate graph holder child.gh = &graphHolder{ - s: child, + s: child, + snap: -1, + nodes: make([]*graphNode, len(s.gh.nodes)), + } + for i, graphNode := range s.gh.nodes { + child.gh.nodes[i] = graphNode } s.childScopes = append(s.childScopes, child) diff --git a/scope_test.go b/scope_test.go index 26c08e70..e4d43756 100644 --- a/scope_test.go +++ b/scope_test.go @@ -27,31 +27,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestScopeTree(t *testing.T) { - t.Parallel() - c := New() - s1 := c.Scope("child 1") - s2 := c.Scope("child 2") - s3 := s1.Scope("grandchild") - - t.Run("verify Container tree", func(t *testing.T) { - t.Parallel() - - assert.Equal(t, s1.parentScope, c.scope) - assert.Equal(t, s2.parentScope, c.scope) - - assert.Equal(t, s3.parentScope, s1) - assert.NotEqual(t, s3.parentScope, s2) - }) - - t.Run("getScopesUntilRoot returns scopes in tree path in order of distance from root", func(t *testing.T) { - t.Parallel() - - assert.Equal(t, []*Scope{c.scope, s1, s3}, s3.getScopesUntilRoot()) - assert.Equal(t, []*Scope{c.scope, s1, s3}, s3.getScopesUntilRoot()) - }) -} - func TestScopedOperations(t *testing.T) { t.Parallel() @@ -94,4 +69,52 @@ func TestScopedOperations(t *testing.T) { assert.NoError(t, grandchild.Invoke(useB)) assert.Error(t, c.Invoke(useB)) }) + + t.Run("private provides do not propagate upstream", func(t *testing.T) { + type A struct{} + + c := New() + s := c.Scope("child") + s.Provide(func() *A { return &A{} }) + + assert.Error(t, c.Invoke(func(a *A) {}), "invoking on child's private-provided type should fail") + }) + + t.Run("export provides propogate upstream", func(t *testing.T) { + }) +} + +func TestScopeFailures(t *testing.T) { + t.Parallel() + + t.Run("cycle with child", func(t *testing.T) { + // what root sees: + // A <- B C + // | ^ + // |_________| + // + // what child sees: + // A <- B <- C + // | ^ + // |_________| + type A struct{} + type B struct{} + type C struct{} + newA := func(*C) *A { return &A{} } + newB := func(*A) *B { return &B{} } + newC := func(*B) *C { return &C{} } + + c := New() + s := c.Scope("child") + assert.NoError(t, c.Provide(newA)) + assert.NoError(t, s.Provide(newB)) + assert.Error(t, c.Provide(newC), "expected a cycle to be introduced in the child") + + // Try again, this time with child inheriting parent-provided constructors. + c = New() + assert.NoError(t, c.Provide(newA)) + s = c.Scope("child") + assert.NoError(t, s.Provide(newB)) + assert.Error(t, c.Provide(newC), "expected a cycle to be introduced in the child") + }) } From 4d4df2fb640c12abe16c189dffee64a13c2e2c25 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 7 Dec 2021 23:28:10 -0800 Subject: [PATCH 04/15] more tests --- invoke.go | 8 +++++++ scope_test.go | 62 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/invoke.go b/invoke.go index 9df8fc1a..5dcc5f65 100644 --- a/invoke.go +++ b/invoke.go @@ -46,6 +46,14 @@ func (c *Container) Invoke(function interface{}, opts ...InvokeOption) error { return c.scope.Invoke(function, opts...) } +// Invoke runs the given function after instantiating its dependencies. +// +// Any arguments that the function has are treated as its dependencies. The +// dependencies are instantiated in an unspecified order along with any +// dependencies that they might have. +// +// The function may return an error to indicate failure. The error will be +// returned to the caller as-is. func (s *Scope) Invoke(function interface{}, opts ...InvokeOption) error { ftype := reflect.TypeOf(function) if ftype == nil { diff --git a/scope_test.go b/scope_test.go index e4d43756..a174cd12 100644 --- a/scope_test.go +++ b/scope_test.go @@ -70,24 +70,57 @@ func TestScopedOperations(t *testing.T) { assert.Error(t, c.Invoke(useB)) }) - t.Run("private provides do not propagate upstream", func(t *testing.T) { + t.Run("provides to top-level Container propogates to all scopes", func(t *testing.T) { type A struct{} - c := New() - s := c.Scope("child") - s.Provide(func() *A { return &A{} }) - - assert.Error(t, c.Invoke(func(a *A) {}), "invoking on child's private-provided type should fail") + // Scope tree: + // root <-- Provide(func() *A) + // / \ + // c1 c2 + // | / \ + // gc1 gc2 gc3 + var allScopes []*Scope + root := New() + + allScopes = append(allScopes, root.Scope("child 1"), root.Scope("child 2")) + allScopes = append(allScopes, allScopes[0].Scope("grandchild 1"), allScopes[1].Scope("grandchild 2"), allScopes[1].Scope("grandchild 3")) + + root.Provide(func() *A { + return &A{} + }) + + // top-level provide should be available in all the scopes. + for _, scope := range allScopes { + assert.NoError(t, scope.Invoke(func(a *A) {})) + } }) - t.Run("export provides propogate upstream", func(t *testing.T) { + t.Run("private provides to child should be available to grandchildren, but not root", func(t *testing.T) { + type A struct{} + // Scope tree: + // root + // | + // child <-- Provide(func() *A) + // / \ + // gc1 gc2 + root := New() + c := root.Scope("child") + gc := c.Scope("grandchild") + + c.Provide(func() *A { return &A{} }) + + err := root.Invoke(func(a *A) {}) + assert.Error(t, err, "expected Invoke in root container on child's private-provided type to fail") + assert.Contains(t, err.Error(), "missing type: *dig.A") + + assert.NoError(t, gc.Invoke(func(a *A) {}), "expected Invoke in grandchild container on child's private-provided type to fail") }) } func TestScopeFailures(t *testing.T) { t.Parallel() - t.Run("cycle with child", func(t *testing.T) { + t.Run("introduce a cycle with child", func(t *testing.T) { // what root sees: // A <- B C // | ^ @@ -117,4 +150,17 @@ func TestScopeFailures(t *testing.T) { assert.NoError(t, s.Provide(newB)) assert.Error(t, c.Provide(newC), "expected a cycle to be introduced in the child") }) + + t.Run("private provides do not propagate upstream", func(t *testing.T) { + type A struct{} + + root := New() + c := root.Scope("child") + gc := c.Scope("grandchild") + gc.Provide(func() *A { return &A{} }) + + assert.Error(t, root.Invoke(func(a *A) {}), "invoking on grandchild's private-provided type should fail") + assert.Error(t, c.Invoke(func(a *A) {}), "invoking on child's private-provided type should fail") + }) + } From 374f584b4163677749d6ea4c5084825ece4f46a3 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 7 Dec 2021 23:36:19 -0800 Subject: [PATCH 05/15] make linter happy --- container.go | 6 ------ provide.go | 21 +++++++++++++++++++++ scope.go | 21 ++++++++++----------- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/container.go b/container.go index e3a3ad8f..f5a6a3ae 100644 --- a/container.go +++ b/container.go @@ -215,12 +215,6 @@ func dryInvoker(fn reflect.Value, _ []reflect.Value) []reflect.Value { return results } -// invokerFn return a function to run when calling function provided to Provide or Invoke. Used for -// running container in dry mode. -func (c *Container) invoker() invokerFn { - return c.scope.invokerFn -} - // String representation of the entire Container func (c *Container) String() string { return c.scope.String() diff --git a/provide.go b/provide.go index 1a15dd94..2b34bc46 100644 --- a/provide.go +++ b/provide.go @@ -351,6 +351,27 @@ func (c *Container) Provide(constructor interface{}, opts ...ProvideOption) erro return c.scope.Provide(constructor, opts...) } +// Provide teaches the Scope how to build values of one or more types and +// expresses their dependencies. +// +// The first argument of Provide is a function that accepts zero or more +// parameters and returns one or more results. The function may optionally +// return an error to indicate that it failed to build the value. This +// function will be treated as the constructor for all the types it returns. +// This function will be called AT MOST ONCE when a type produced by it, or a +// type that consumes this function's output, is requested via Invoke. If the +// same types are requested multiple times, the previously produced value will +// be reused. +// +// In addition to accepting constructors that accept dependencies as separate +// arguments and produce results as separate return values, Provide also +// accepts constructors that specify dependencies as dig.In structs and/or +// specify results as dig.Out structs. +// +// When a constructor is Provided to a Scope, it will propagate this to any +// Scopes that are descendents, but not ancestors of this Scope. +// To provide a constructor to all the Scopes available, provide it to +// Container, which is the root Scope. func (s *Scope) Provide(constructor interface{}, opts ...ProvideOption) error { ctype := reflect.TypeOf(constructor) if ctype == nil { diff --git a/scope.go b/scope.go index a00c2f05..047bca32 100644 --- a/scope.go +++ b/scope.go @@ -28,18 +28,17 @@ import ( "sort" ) -// ScopeOption configures a Scope. It's included for future functionality; -// currently, there are no concrete implementations. +// A ScopeOption modifies the default behavior of Scope; currently, +// there are no implementations. type ScopeOption interface { - applyOption(*Scope) + applyScopeOption(*scopeOptions) } -type scopeOptionFunc func(*Scope) - -func (f scopeOptionFunc) applyOption(s *Scope) { f(s) } +type scopeOptions struct { +} // Scope is a scoped DAG of types and their dependencies. -// A Scope may also have one or more child Scopes that "inherit" +// A Scope may also have one or more child Scopes that inherit // from it. type Scope struct { name string @@ -77,6 +76,10 @@ type Scope struct { } // Scope creates a new Scope with the given name and options from current Scope. +// Any constructors that the current Scope knows about, as well as any modifications +// made to it in the future will be propagated to the child scope. +// However, no modifications made to the child scope being created will be propagated +// to the parent Scope. func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { child := &Scope{ name: name, @@ -191,10 +194,6 @@ func (s *Scope) getAllValueProviders(name string, t reflect.Type) []provider { return s.getAllProviders(key{name: name, t: t}) } -func (s *Scope) getAllGroupProviders(name string, t reflect.Type) []provider { - return s.getAllProviders(key{group: name, t: t}) -} - func (s *Scope) getAllProviders(k key) []provider { allScopes := s.getScopesUntilRoot() var providers []provider From 5dc219e0510fa9ee4e10444d19427328187618cd Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Thu, 9 Dec 2021 22:57:00 -0800 Subject: [PATCH 06/15] avoid copying over providers,value,groups for creating child scope --- constructor.go | 7 ++++--- container.go | 2 +- graph.go | 2 +- invoke.go | 2 +- param.go | 12 ++++++------ provide.go | 2 +- scope.go | 31 +++++++++++-------------------- 7 files changed, 25 insertions(+), 33 deletions(-) diff --git a/constructor.go b/constructor.go index 4e5a8afc..670b5f98 100644 --- a/constructor.go +++ b/constructor.go @@ -53,7 +53,7 @@ type constructorNode struct { // Type information about constructor results. resultList resultList - order int // order of this node in graphHolder + orders map[*Scope]int // order of this node in each Scopes' graphHolders. s *Scope // scope this node was originally provided to. } @@ -101,9 +101,10 @@ func newConstructorNode(ctor interface{}, s *Scope, opts constructorOptions) (*c id: dot.CtorID(cptr), paramList: params, resultList: results, + orders: make(map[*Scope]int), s: s, } - n.order = s.newGraphNode(n) + s.newGraphNode(n, n.orders) return n, nil } @@ -112,7 +113,7 @@ func (n *constructorNode) ParamList() paramList { return n.paramList } func (n *constructorNode) ResultList() resultList { return n.resultList } func (n *constructorNode) ID() dot.CtorID { return n.id } func (n *constructorNode) CType() reflect.Type { return n.ctype } -func (n *constructorNode) Order() int { return n.order } +func (n *constructorNode) Order(s *Scope) int { return n.orders[s] } func (n *constructorNode) String() string { return fmt.Sprintf("deps: %v, ctor: %v", n.paramList, n.ctype) diff --git a/container.go b/container.go index f5a6a3ae..e522ba71 100644 --- a/container.go +++ b/container.go @@ -87,7 +87,7 @@ type containerStore interface { containerWriter // Adds a new graph node to the Container and returns its order. - newGraphNode(w interface{}) int + newGraphNode(w interface{}, orders map[*Scope]int) // Returns a slice containing all known types. knownTypes() []reflect.Type diff --git a/graph.go b/graph.go index 5eed33c3..50a7ac99 100644 --- a/graph.go +++ b/graph.go @@ -73,7 +73,7 @@ func (gh *graphHolder) EdgesFrom(u int) []int { case *paramGroupedSlice: providers := gh.s.getGroupProviders(w.Group, w.Type.Elem()) for _, provider := range providers { - orders = append(orders, provider.Order()) + orders = append(orders, provider.Order(gh.s)) } } return orders diff --git a/invoke.go b/invoke.go index 5dcc5f65..acfc25af 100644 --- a/invoke.go +++ b/invoke.go @@ -124,7 +124,7 @@ func findMissingDependencies(c containerStore, params ...param) []paramSingle { for _, param := range params { switch p := param.(type) { case paramSingle: - if ns := c.getValueProviders(p.Name, p.Type); len(ns) == 0 && !p.Optional { + if ns := c.getAllValueProviders(p.Name, p.Type); len(ns) == 0 && !p.Optional { missingDeps = append(missingDeps, p) } case paramObject: diff --git a/param.go b/param.go index 7a26175d..b68004bf 100644 --- a/param.go +++ b/param.go @@ -270,14 +270,14 @@ func getParamOrder(gh *graphHolder, param param) []int { var orders []int switch p := param.(type) { case paramSingle: - providers := gh.s.getValueProviders(p.Name, p.Type) + providers := gh.s.getAllValueProviders(p.Name, p.Type) for _, provider := range providers { - orders = append(orders, provider.Order()) + orders = append(orders, provider.Order(gh.s)) } case paramGroupedSlice: // value group parameters have nodes of their own. // We can directly return that here. - orders = append(orders, p.order) + orders = append(orders, p.orders[gh.s]) case paramObject: for _, pf := range p.Fields { orders = append(orders, getParamOrder(gh, pf.Param)...) @@ -416,7 +416,7 @@ type paramGroupedSlice struct { // Type of the slice. Type reflect.Type - order int + orders map[*Scope]int } func (pt paramGroupedSlice) String() string { @@ -444,7 +444,7 @@ func newParamGroupedSlice(f reflect.StructField, c containerStore) (paramGrouped if err != nil { return paramGroupedSlice{}, err } - pg := paramGroupedSlice{Group: g.Name, Type: f.Type} + pg := paramGroupedSlice{Group: g.Name, Type: f.Type, orders: make(map[*Scope]int)} name := f.Tag.Get(_nameTag) optional, _ := isFieldOptional(f) @@ -463,7 +463,7 @@ func newParamGroupedSlice(f reflect.StructField, c containerStore) (paramGrouped case optional: return pg, errors.New("value groups cannot be optional") } - pg.order = c.newGraphNode(&pg) + c.newGraphNode(&pg, pg.orders) return pg, nil } diff --git a/provide.go b/provide.go index 2b34bc46..8d89bf77 100644 --- a/provide.go +++ b/provide.go @@ -308,7 +308,7 @@ type provider interface { // Order reports the order of this provider in the graphHolder. // This value is usually returned by the graphHolder.NewNode method. - Order() int + Order(*Scope) int // Location returns where this constructor was defined. Location() *digreflect.Func diff --git a/scope.go b/scope.go index 047bca32..3f4a8107 100644 --- a/scope.go +++ b/scope.go @@ -90,30 +90,13 @@ func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { invokerFn: s.invokerFn, } - // Copy over providers, values, and groups. - for key, nodes := range s.providers { - child.providers[key] = make([]*constructorNode, len(nodes)) - for i, node := range nodes { - child.providers[key][i] = node - } - } - for key, value := range s.values { - child.values[key] = value - } - - for key, groups := range s.groups { - child.groups[key] = make([]reflect.Value, len(groups)) - for i, group := range groups { - child.groups[key][i] = group - } - } - // child should hold a separate graph holder child.gh = &graphHolder{ s: child, snap: -1, nodes: make([]*graphNode, len(s.gh.nodes)), } + for i, graphNode := range s.gh.nodes { child.gh.nodes[i] = graphNode } @@ -207,8 +190,16 @@ func (s *Scope) invoker() invokerFn { return s.invokerFn } -func (s *Scope) newGraphNode(wrapped interface{}) int { - return s.gh.NewNode(wrapped) +// adds a new graphNode to this Scope and all of its descendent +// scope. +func (s *Scope) newGraphNode(wrapped interface{}, orders map[*Scope]int) { + orders[s] = s.gh.NewNode(wrapped) + + if len(s.childScopes) > 0 { + for _, cs := range s.childScopes { + cs.newGraphNode(wrapped, orders) + } + } } func (s *Scope) cycleDetectedError(cycle []int) error { From 7e82f37dd2c435b2ae12bc3558390c8211e7ac72 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 10 Dec 2021 01:11:09 -0800 Subject: [PATCH 07/15] each ctor gets provided exactly once; change err message to include scope name --- constructor.go | 6 ++++-- cycle_error.go | 4 +++- graph.go | 2 +- provide.go | 32 ++++++++++++++------------------ scope.go | 17 +++++++++++++++-- scope_test.go | 8 ++++++-- 6 files changed, 43 insertions(+), 26 deletions(-) diff --git a/constructor.go b/constructor.go index 670b5f98..00bba546 100644 --- a/constructor.go +++ b/constructor.go @@ -53,9 +53,11 @@ type constructorNode struct { // Type information about constructor results. resultList resultList - orders map[*Scope]int // order of this node in each Scopes' graphHolders. + // order of this node in each Scopes' graphHolders. + orders map[*Scope]int - s *Scope // scope this node was originally provided to. + // scope this node was originally provided to. + s *Scope } type constructorOptions struct { diff --git a/cycle_error.go b/cycle_error.go index d6916bbf..c1d41abf 100644 --- a/cycle_error.go +++ b/cycle_error.go @@ -33,7 +33,8 @@ type cycleErrPathEntry struct { } type errCycleDetected struct { - Path []cycleErrPathEntry + Path []cycleErrPathEntry + scope *Scope } func (e errCycleDetected) Error() string { @@ -46,6 +47,7 @@ func (e errCycleDetected) Error() string { // b := new(bytes.Buffer) + fmt.Fprintf(b, "In Scope %s: \n", e.scope.name) for i, entry := range e.Path { if i > 0 { b.WriteString("\n\tdepends on ") diff --git a/graph.go b/graph.go index 50a7ac99..6c8e7e13 100644 --- a/graph.go +++ b/graph.go @@ -71,7 +71,7 @@ func (gh *graphHolder) EdgesFrom(u int) []int { orders = append(orders, getParamOrder(gh, param)...) } case *paramGroupedSlice: - providers := gh.s.getGroupProviders(w.Group, w.Type.Elem()) + providers := gh.s.getAllGroupProviders(w.Group, w.Type.Elem()) for _, provider := range providers { orders = append(orders, provider.Order(gh.s)) } diff --git a/provide.go b/provide.go index 8d89bf77..dcb8c971 100644 --- a/provide.go +++ b/provide.go @@ -440,31 +440,27 @@ func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) { s.providers[k] = append(s.providers[k], n) } - s.isVerifiedAcyclic = false - if !s.deferAcyclicVerification { - if ok, cycle := graph.IsAcyclic(s.gh); !ok { - // When a cycle is detected, recover the old providers to reset - // the providers map back to what it was before this node was - // introduced. - for k, ops := range oldProviders { - s.providers[k] = ops + allScopes := s.getAllLeafScopes() + for _, s := range allScopes { + s.isVerifiedAcyclic = false + if !s.deferAcyclicVerification { + if ok, cycle := graph.IsAcyclic(s.gh); !ok { + // When a cycle is detected, recover the old providers to reset + // the providers map back to what it was before this node was + // introduced. + for k, ops := range oldProviders { + s.providers[k] = ops + } + + return errf("this function introduces a cycle", s.cycleDetectedError(cycle)) } - - return errf("this function introduces a cycle", s.cycleDetectedError(cycle)) + s.isVerifiedAcyclic = true } - s.isVerifiedAcyclic = true } // Before appending to the nodes, check if there are // any child Scopes. If there are any, recurse down // the descendents to provide the constructor to them. - if len(s.childScopes) != 0 { - for _, childScope := range s.childScopes { - if err := childScope.provide(ctor, opts); err != nil { - return err - } - } - } s.nodes = append(s.nodes, n) // Record introspection info for caller if Info option is specified diff --git a/scope.go b/scope.go index 3f4a8107..f2c5a697 100644 --- a/scope.go +++ b/scope.go @@ -114,12 +114,21 @@ func (s *Scope) getScopesUntilRoot() []*Scope { return []*Scope{s} } +func (s *Scope) getAllLeafScopes() []*Scope { + allScopes := []*Scope{s} + if len(s.childScopes) > 0 { + for _, cs := range s.childScopes { + allScopes = append(allScopes, cs.getAllLeafScopes()...) + } + } + return allScopes +} + func (s *Scope) getStoresUntilRoot() []containerStore { if s.parentScope != nil { return append(s.parentScope.getStoresUntilRoot(), s) } return []containerStore{s} - } func (s *Scope) knownTypes() []reflect.Type { @@ -173,6 +182,10 @@ func (s *Scope) getProviders(k key) []provider { return providers } +func (s *Scope) getAllGroupProviders(name string, t reflect.Type) []provider { + return s.getAllProviders(key{group: name, t: t}) +} + func (s *Scope) getAllValueProviders(name string, t reflect.Type) []provider { return s.getAllProviders(key{name: name, t: t}) } @@ -214,7 +227,7 @@ func (s *Scope) cycleDetectedError(cycle []int) error { }) } } - return errCycleDetected{Path: path} + return errCycleDetected{Path: path, scope: s} } // String representation of the entire Scope diff --git a/scope_test.go b/scope_test.go index a174cd12..61665543 100644 --- a/scope_test.go +++ b/scope_test.go @@ -141,14 +141,18 @@ func TestScopeFailures(t *testing.T) { s := c.Scope("child") assert.NoError(t, c.Provide(newA)) assert.NoError(t, s.Provide(newB)) - assert.Error(t, c.Provide(newC), "expected a cycle to be introduced in the child") + err := c.Provide(newC) + assert.Error(t, err, "expected a cycle to be introduced in the child") + assert.Contains(t, err.Error(), "In Scope child") // Try again, this time with child inheriting parent-provided constructors. c = New() assert.NoError(t, c.Provide(newA)) s = c.Scope("child") assert.NoError(t, s.Provide(newB)) - assert.Error(t, c.Provide(newC), "expected a cycle to be introduced in the child") + err = c.Provide(newC) + assert.Error(t, err, "expected a cycle to be introduced in the child") + assert.Contains(t, err.Error(), "In Scope child") }) t.Run("private provides do not propagate upstream", func(t *testing.T) { From b53180055e98b953f8b47c973675b5e77f3a5b03 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 10 Dec 2021 03:00:43 -0800 Subject: [PATCH 08/15] update comments --- scope.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scope.go b/scope.go index f2c5a697..ecff3bc8 100644 --- a/scope.go +++ b/scope.go @@ -46,12 +46,14 @@ type Scope struct { // key. providers map[key][]*constructorNode + // constructorNodes provided directly to this Scope. i.e. it does not include + // any nodes that were provided to the parent Scope this inherited from. nodes []*constructorNode - // Values that have already been generated in the container. + // Values that have already been generated directly in the Scope. values map[key]reflect.Value - // Values groups that have already been generated in the container. + // Values groups that have already been generated directly in the Scope. groups map[key][]reflect.Value // Source of randomness. @@ -66,6 +68,8 @@ type Scope struct { // invokerFn calls a function with arguments provided to Provide or Invoke. invokerFn invokerFn + // graph of this Scope. Note that this holds the dependency graph of all the + // nodes that affect this Scope, not just the ones provided directly to this Scope. gh *graphHolder // Parent of this Scope. From 050ec0ccd3d4e3e2c3ff26383f00eceece72d41b Mon Sep 17 00:00:00 2001 From: Sung Whang Date: Fri, 10 Dec 2021 16:14:30 -0800 Subject: [PATCH 09/15] save/restore state for all Scopes affected --- provide.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/provide.go b/provide.go index dcb8c971..b3107a4b 100644 --- a/provide.go +++ b/provide.go @@ -399,15 +399,19 @@ func (s *Scope) Provide(constructor interface{}, opts ...ProvideOption) error { } func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) { + // For all scopes affected by this change, // 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. - s.gh.Snapshot() - defer func() { - if err != nil { - s.gh.Rollback() - } - }() + allScopes := s.getAllLeafScopes() + for _, s := range allScopes { + s.gh.Snapshot() + defer func() { + if err != nil { + s.gh.Rollback() + } + }() + } n, err := newConstructorNode( ctor, @@ -440,7 +444,6 @@ func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) { s.providers[k] = append(s.providers[k], n) } - allScopes := s.getAllLeafScopes() for _, s := range allScopes { s.isVerifiedAcyclic = false if !s.deferAcyclicVerification { From 487391d04b5d0f83c71993370f832a89a30749af Mon Sep 17 00:00:00 2001 From: Sung Whang Date: Fri, 10 Dec 2021 16:24:02 -0800 Subject: [PATCH 10/15] loop literal --- provide.go | 1 + 1 file changed, 1 insertion(+) diff --git a/provide.go b/provide.go index b3107a4b..f21dd707 100644 --- a/provide.go +++ b/provide.go @@ -405,6 +405,7 @@ func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) { // undo them upon encountering errors. allScopes := s.getAllLeafScopes() for _, s := range allScopes { + s := s s.gh.Snapshot() defer func() { if err != nil { From cbf60a8a2d06cb81b540fdcaa3bc4817b41a130f Mon Sep 17 00:00:00 2001 From: Sung Whang Date: Mon, 20 Dec 2021 17:05:19 -0800 Subject: [PATCH 11/15] address code review feedback --- container.go | 13 +------ dig_test.go | 2 +- param.go | 29 +++++++++++--- param_test.go | 16 ++++---- provide.go | 40 +++++++++----------- scope.go | 96 ++++++++++++++++++++++++----------------------- scope_test.go | 14 ++++++- visualize_test.go | 2 +- 8 files changed, 114 insertions(+), 98 deletions(-) diff --git a/container.go b/container.go index e522ba71..56b37156 100644 --- a/container.go +++ b/container.go @@ -24,7 +24,6 @@ import ( "fmt" "math/rand" "reflect" - "time" "go.uber.org/dig/internal/dot" ) @@ -112,7 +111,7 @@ type containerStore interface { // type across all the Scopes that are in effect of this containerStore. getAllValueProviders(name string, t reflect.Type) []provider - getStoresUntilRoot() []containerStore + getStoresFromRoot() []containerStore createGraph() *dot.Graph @@ -122,15 +121,7 @@ type containerStore interface { // New constructs a Container. func New(opts ...Option) *Container { - s := &Scope{ - providers: make(map[key][]*constructorNode), - values: make(map[key]reflect.Value), - groups: make(map[key][]reflect.Value), - invokerFn: defaultInvoker, - rand: rand.New(rand.NewSource(time.Now().UnixNano())), - } - - s.gh = newGraphHolder(s) + s := newScope() c := &Container{scope: s} for _, opt := range opts { diff --git a/dig_test.go b/dig_test.go index 5df65412..2e3dd8d6 100644 --- a/dig_test.go +++ b/dig_test.go @@ -3105,7 +3105,7 @@ func TestNodeAlreadyCalled(t *testing.T) { type type1 struct{} f := func() type1 { return type1{} } - n, err := newConstructorNode(f, New().scope, constructorOptions{}) + n, err := newConstructorNode(f, newScope(), constructorOptions{}) require.NoError(t, err, "failed to build node") require.False(t, n.called, "node must not have been called") diff --git a/param.go b/param.go index b68004bf..12d720ab 100644 --- a/param.go +++ b/param.go @@ -146,16 +146,35 @@ 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)) - allContainers := c.getStoresUntilRoot() + argsBuilt := make([]bool, len(pl.Params)) + allContainers := c.getStoresFromRoot() for i, p := range pl.Params { var err error + var arg reflect.Value // iterate through the tree path of scopes. - for _, ac := range allContainers { - if args[i], err = p.Build(ac); err == nil { - break + for _, c := range allContainers { + if arg, err = p.Build(c); err == nil { + args[i] = arg + argsBuilt[i] = true } } - if err != nil { + + // 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. + // kf 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 } } diff --git a/param_test.go b/param_test.go index 4ed3b686..7a1f41ed 100644 --- a/param_test.go +++ b/param_test.go @@ -30,10 +30,10 @@ import ( ) func TestParamListBuild(t *testing.T) { - p, err := newParamList(reflect.TypeOf(func() io.Writer { return nil }), New().scope) + p, err := newParamList(reflect.TypeOf(func() io.Writer { return nil }), newScope()) require.NoError(t, err) assert.Panics(t, func() { - p.Build(New().scope) + p.Build(newScope()) }) } @@ -57,7 +57,7 @@ func TestParamObjectSuccess(t *testing.T) { } `name:"bar"` } - po, err := newParamObject(reflect.TypeOf(in{}), New().scope) + po, err := newParamObject(reflect.TypeOf(in{}), newScope()) require.NoError(t, err) require.Len(t, po.Fields, 4) @@ -114,7 +114,7 @@ func TestParamObjectWithUnexportedFieldsSuccess(t *testing.T) { _ = in{}.t2 // unused - po, err := newParamObject(reflect.TypeOf(in{}), New().scope) + po, err := newParamObject(reflect.TypeOf(in{}), newScope()) require.NoError(t, err) require.Len(t, po.Fields, 1) @@ -138,7 +138,7 @@ func TestParamObjectFailure(t *testing.T) { _ = in{}.a2 // unused but needed - _, err := newParamObject(reflect.TypeOf(in{}), New().scope) + _, err := newParamObject(reflect.TypeOf(in{}), newScope()) require.Error(t, err) assert.Contains(t, err.Error(), `bad field "a2" of dig.in: unexported fields not allowed in dig.In, did you mean to export "a2" (dig.A)`) @@ -155,7 +155,7 @@ func TestParamObjectFailure(t *testing.T) { _ = in{}.a2 // unused but needed - _, err := newParamObject(reflect.TypeOf(in{}), New().scope) + _, err := newParamObject(reflect.TypeOf(in{}), newScope()) require.Error(t, err) assert.Contains(t, err.Error(), `bad field "a2" of dig.in: unexported fields not allowed in dig.In, did you mean to export "a2" (dig.A)`) @@ -172,7 +172,7 @@ func TestParamObjectFailure(t *testing.T) { _ = in{}.a2 // unused but needed - _, err := newParamObject(reflect.TypeOf(in{}), New().scope) + _, err := newParamObject(reflect.TypeOf(in{}), newScope()) require.Error(t, err) assert.Contains(t, err.Error(), `invalid value "foo" for "ignore-unexported" tag on field In: strconv.ParseBool: parsing "foo": invalid syntax`) @@ -227,7 +227,7 @@ func TestParamGroupSliceErrors(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - _, err := newParamObject(reflect.TypeOf(tt.shape), New().scope) + _, err := newParamObject(reflect.TypeOf(tt.shape), newScope()) require.Error(t, err, "expected failure") assert.Contains(t, err.Error(), tt.wantErr) }) diff --git a/provide.go b/provide.go index f21dd707..bcf28af5 100644 --- a/provide.go +++ b/provide.go @@ -343,10 +343,8 @@ type provider interface { // same types are requested multiple times, the previously produced value will // be reused. // -// In addition to accepting constructors that accept dependencies as separate -// arguments and produce results as separate return values, Provide also -// accepts constructors that specify dependencies as dig.In structs and/or -// specify results as dig.Out structs. +// Provide accepts argument types or dig.In structs as dependencies, and +// separate return values or dig.Out structs for results. func (c *Container) Provide(constructor interface{}, opts ...ProvideOption) error { return c.scope.Provide(constructor, opts...) } @@ -363,10 +361,8 @@ func (c *Container) Provide(constructor interface{}, opts ...ProvideOption) erro // same types are requested multiple times, the previously produced value will // be reused. // -// In addition to accepting constructors that accept dependencies as separate -// arguments and produce results as separate return values, Provide also -// accepts constructors that specify dependencies as dig.In structs and/or -// specify results as dig.Out structs. +// Provide accepts argument types or dig.In structs as dependencies, and +// separate return values or dig.Out structs for results. // // When a constructor is Provided to a Scope, it will propagate this to any // Scopes that are descendents, but not ancestors of this Scope. @@ -403,7 +399,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.getAllLeafScopes() + allScopes := s.appendLeafScopes(nil) for _, s := range allScopes { s := s s.gh.Snapshot() @@ -447,24 +443,22 @@ func (s *Scope) provide(ctor interface{}, opts provideOptions) (err error) { for _, s := range allScopes { s.isVerifiedAcyclic = false - if !s.deferAcyclicVerification { - if ok, cycle := graph.IsAcyclic(s.gh); !ok { - // When a cycle is detected, recover the old providers to reset - // the providers map back to what it was before this node was - // introduced. - for k, ops := range oldProviders { - s.providers[k] = ops - } - - return errf("this function introduces a cycle", s.cycleDetectedError(cycle)) + if s.deferAcyclicVerification { + continue + } + if ok, cycle := graph.IsAcyclic(s.gh); !ok { + // When a cycle is detected, recover the old providers to reset + // the providers map back to what it was before this node was + // introduced. + for k, ops := range oldProviders { + s.providers[k] = ops } - s.isVerifiedAcyclic = true + + return errf("this function introduces a cycle", s.cycleDetectedError(cycle)) } + s.isVerifiedAcyclic = true } - // Before appending to the nodes, check if there are - // any child Scopes. If there are any, recurse down - // the descendents to provide the constructor to them. s.nodes = append(s.nodes, n) // Record introspection info for caller if Info option is specified diff --git a/scope.go b/scope.go index ecff3bc8..f3b10898 100644 --- a/scope.go +++ b/scope.go @@ -26,15 +26,13 @@ import ( "math/rand" "reflect" "sort" + "time" ) // A ScopeOption modifies the default behavior of Scope; currently, // there are no implementations. type ScopeOption interface { - applyScopeOption(*scopeOptions) -} - -type scopeOptions struct { + noScopeOption() //yet } // Scope is a scoped DAG of types and their dependencies. @@ -50,10 +48,10 @@ type Scope struct { // any nodes that were provided to the parent Scope this inherited from. nodes []*constructorNode - // Values that have already been generated directly in the Scope. + // Values that generated directly in the Scope. values map[key]reflect.Value - // Values groups that have already been generated directly in the Scope. + // Values groups that generated directly in the Scope. groups map[key][]reflect.Value // Source of randomness. @@ -79,60 +77,67 @@ type Scope struct { childScopes []*Scope } +func newScope() *Scope { + s := &Scope{ + name: "container", + providers: make(map[key][]*constructorNode), + values: make(map[key]reflect.Value), + groups: make(map[key][]reflect.Value), + invokerFn: defaultInvoker, + rand: rand.New(rand.NewSource(time.Now().UnixNano())), + } + s.gh = newGraphHolder(s) + return s +} + // Scope creates a new Scope with the given name and options from current Scope. // Any constructors that the current Scope knows about, as well as any modifications // made to it in the future will be propagated to the child scope. // However, no modifications made to the child scope being created will be propagated // to the parent Scope. func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { - child := &Scope{ - name: name, - parentScope: s, - providers: make(map[key][]*constructorNode, len(s.providers)), - values: make(map[key]reflect.Value, len(s.values)), - groups: make(map[key][]reflect.Value, len(s.groups)), - invokerFn: s.invokerFn, - } - - // child should hold a separate graph holder - child.gh = &graphHolder{ - s: child, - snap: -1, - nodes: make([]*graphNode, len(s.gh.nodes)), - } + child := newScope() + child.name = name + child.parentScope = s + child.invokerFn = s.invokerFn - for i, graphNode := range s.gh.nodes { - child.gh.nodes[i] = graphNode - } + // child copies the parent's graph nodes. + child.gh.nodes = append(child.gh.nodes, s.gh.nodes...) s.childScopes = append(s.childScopes, child) return child } -// getScopesUntilRoot creates a list of Scopes -// have to traverse through until the current node. -func (s *Scope) getScopesUntilRoot() []*Scope { - if s.parentScope != nil { - return append(s.parentScope.getScopesUntilRoot(), s) +// getScopesFromRoot creates a list of Scopes +// have to traverse through from root until the current node. +func (s *Scope) getScopesFromRoot() []*Scope { + var scopes []*Scope + for s := s; s != nil; s = s.parentScope { + scopes = append(scopes, s) } - return []*Scope{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) getAllLeafScopes() []*Scope { - allScopes := []*Scope{s} - if len(s.childScopes) > 0 { - for _, cs := range s.childScopes { - allScopes = append(allScopes, cs.getAllLeafScopes()...) - } +func (s *Scope) appendLeafScopes(dest []*Scope) []*Scope { + dest = append(dest, s) + for _, cs := range s.childScopes { + dest = cs.appendLeafScopes(dest) } - return allScopes + return dest } -func (s *Scope) getStoresUntilRoot() []containerStore { - if s.parentScope != nil { - return append(s.parentScope.getStoresUntilRoot(), s) +func (s *Scope) getStoresFromRoot() []containerStore { + var stores []containerStore + for s := s; s != nil; s = s.parentScope { + stores = append(stores, s) } - return []containerStore{s} + for i, j := 0, len(stores)-1; i < j; i, j = i+1, j-1 { + stores[i], stores[j] = stores[j], stores[i] + } + return stores } func (s *Scope) knownTypes() []reflect.Type { @@ -195,7 +200,7 @@ func (s *Scope) getAllValueProviders(name string, t reflect.Type) []provider { } func (s *Scope) getAllProviders(k key) []provider { - allScopes := s.getScopesUntilRoot() + allScopes := s.getScopesFromRoot() var providers []provider for _, scope := range allScopes { providers = append(providers, scope.getProviders(k)...) @@ -211,11 +216,8 @@ func (s *Scope) invoker() invokerFn { // scope. func (s *Scope) newGraphNode(wrapped interface{}, orders map[*Scope]int) { orders[s] = s.gh.NewNode(wrapped) - - if len(s.childScopes) > 0 { - for _, cs := range s.childScopes { - cs.newGraphNode(wrapped, orders) - } + for _, cs := range s.childScopes { + cs.newGraphNode(wrapped, orders) } } diff --git a/scope_test.go b/scope_test.go index 61665543..84f49c03 100644 --- a/scope_test.go +++ b/scope_test.go @@ -30,7 +30,17 @@ import ( func TestScopedOperations(t *testing.T) { t.Parallel() - t.Run("verify private provides", func(t *testing.T) { + t.Run("getStores/ScopesFromRoot returns scopes from root in order of distance from root", func(t *testing.T) { + c := New() + s1 := c.Scope("child1") + 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()) + }) + + t.Run("private provides", func(t *testing.T) { c := New() s := c.Scope("child") type A struct{} @@ -44,7 +54,7 @@ func TestScopedOperations(t *testing.T) { assert.Error(t, c.Invoke(f)) }) - t.Run("verify private provides inherits", func(t *testing.T) { + t.Run("private provides inherits", func(t *testing.T) { type A struct{} type B struct{} diff --git a/visualize_test.go b/visualize_test.go index 9e896e64..128ac7ac 100644 --- a/visualize_test.go +++ b/visualize_test.go @@ -377,7 +377,7 @@ func TestNewDotCtor(t *testing.T) { type t1 struct{} type t2 struct{} - n, err := newConstructorNode(func(A t1) t2 { return t2{} }, New().scope, constructorOptions{}) + n, err := newConstructorNode(func(A t1) t2 { return t2{} }, newScope(), constructorOptions{}) require.NoError(t, err) n.location = &digreflect.Func{ From 0ba222a3dff445c84b91fe9bc35139542ac4d2b5 Mon Sep 17 00:00:00 2001 From: Sung Whang Date: Tue, 21 Dec 2021 11:23:30 -0800 Subject: [PATCH 12/15] typo --- param.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/param.go b/param.go index 12d720ab..bf9d4272 100644 --- a/param.go +++ b/param.go @@ -169,7 +169,7 @@ func (pl paramList) BuildList(c containerStore) ([]reflect.Value, error) { // for these errors to occur in child scopes that don't // contain the given parameter type. We can safely ignore // these. - // kf it's an error other than missing types/dependencies, + // If it's an error other than missing types/dependencies, // this means some constructor returned an error that must // be reported. _, isErrMissingTypes := err.(errMissingTypes) From 9c35896a6c15e0e3c7b11dfdad3230c601d940b9 Mon Sep 17 00:00:00 2001 From: Sung Whang Date: Tue, 21 Dec 2021 13:35:46 -0800 Subject: [PATCH 13/15] fix docstrings --- container.go | 2 +- scope.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/container.go b/container.go index 56b37156..ef5c5a85 100644 --- a/container.go +++ b/container.go @@ -85,7 +85,7 @@ type containerWriter interface { type containerStore interface { containerWriter - // Adds a new graph node to the Container and returns its order. + // Adds a new graph node to the Container newGraphNode(w interface{}, orders map[*Scope]int) // Returns a slice containing all known types. diff --git a/scope.go b/scope.go index f3b10898..a2f86576 100644 --- a/scope.go +++ b/scope.go @@ -108,8 +108,8 @@ func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { return child } -// getScopesFromRoot creates a list of Scopes -// have to traverse through from root until the current node. +// getScopesFromRoot returns a list of Scopes from the root Container +// until the current Scope. func (s *Scope) getScopesFromRoot() []*Scope { var scopes []*Scope for s := s; s != nil; s = s.parentScope { From 2a86ead7d7280e86acdd7ac9d31a8220d353f216 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 21 Dec 2021 16:21:57 -0800 Subject: [PATCH 14/15] address code review feedback --- scope.go | 7 +++++++ scope_test.go | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/scope.go b/scope.go index a2f86576..f0483233 100644 --- a/scope.go +++ b/scope.go @@ -39,6 +39,9 @@ type ScopeOption interface { // A Scope may also have one or more child Scopes that inherit // from it. type Scope struct { + // This implements containerStore interface. + + // Name of the Scope name string // Mapping from key to all the constructor node that can provide a value for that // key. @@ -104,6 +107,10 @@ func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { // child copies the parent's graph nodes. child.gh.nodes = append(child.gh.nodes, s.gh.nodes...) + for _, opt := range opts { + opt.noScopeOption() + } + s.childScopes = append(s.childScopes, child) return child } diff --git a/scope_test.go b/scope_test.go index 84f49c03..ffc341a5 100644 --- a/scope_test.go +++ b/scope_test.go @@ -66,7 +66,7 @@ func TestScopedOperations(t *testing.T) { } c := New() - c.Provide(func() *A { return &A{} }) + require.NoError(t, c.Provide(func() *A { return &A{} })) child := c.Scope("child") child.Provide(func() *B { return &B{} }) From 5b172dd19024c412ccd83f9f4501ba74a3886b41 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 21 Dec 2021 17:33:08 -0800 Subject: [PATCH 15/15] Add more permutations to scope test with DeferAcyclicVerification and DryRun --- scope.go | 1 + scope_test.go | 120 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 80 insertions(+), 41 deletions(-) diff --git a/scope.go b/scope.go index f0483233..9ea08343 100644 --- a/scope.go +++ b/scope.go @@ -103,6 +103,7 @@ func (s *Scope) Scope(name string, opts ...ScopeOption) *Scope { child.name = name child.parentScope = s child.invokerFn = s.invokerFn + child.deferAcyclicVerification = s.deferAcyclicVerification // child copies the parent's graph nodes. child.gh.nodes = append(child.gh.nodes, s.gh.nodes...) diff --git a/scope_test.go b/scope_test.go index ffc341a5..bd8f5106 100644 --- a/scope_test.go +++ b/scope_test.go @@ -69,7 +69,7 @@ func TestScopedOperations(t *testing.T) { require.NoError(t, c.Provide(func() *A { return &A{} })) child := c.Scope("child") - child.Provide(func() *B { return &B{} }) + require.NoError(t, child.Provide(func() *B { return &B{} })) assert.NoError(t, child.Invoke(useA)) assert.NoError(t, child.Invoke(useB)) @@ -95,36 +95,15 @@ func TestScopedOperations(t *testing.T) { allScopes = append(allScopes, root.Scope("child 1"), root.Scope("child 2")) allScopes = append(allScopes, allScopes[0].Scope("grandchild 1"), allScopes[1].Scope("grandchild 2"), allScopes[1].Scope("grandchild 3")) - root.Provide(func() *A { + require.NoError(t, root.Provide(func() *A { return &A{} - }) + })) // top-level provide should be available in all the scopes. for _, scope := range allScopes { assert.NoError(t, scope.Invoke(func(a *A) {})) } }) - - t.Run("private provides to child should be available to grandchildren, but not root", func(t *testing.T) { - type A struct{} - // Scope tree: - // root - // | - // child <-- Provide(func() *A) - // / \ - // gc1 gc2 - root := New() - c := root.Scope("child") - gc := c.Scope("grandchild") - - c.Provide(func() *A { return &A{} }) - - err := root.Invoke(func(a *A) {}) - assert.Error(t, err, "expected Invoke in root container on child's private-provided type to fail") - assert.Contains(t, err.Error(), "missing type: *dig.A") - - assert.NoError(t, gc.Invoke(func(a *A) {}), "expected Invoke in grandchild container on child's private-provided type to fail") - }) } func TestScopeFailures(t *testing.T) { @@ -147,22 +126,61 @@ func TestScopeFailures(t *testing.T) { newB := func(*A) *B { return &B{} } newC := func(*B) *C { return &C{} } - c := New() - s := c.Scope("child") - assert.NoError(t, c.Provide(newA)) - assert.NoError(t, s.Provide(newB)) - err := c.Provide(newC) - assert.Error(t, err, "expected a cycle to be introduced in the child") - assert.Contains(t, err.Error(), "In Scope child") - - // Try again, this time with child inheriting parent-provided constructors. - c = New() - assert.NoError(t, c.Provide(newA)) - s = c.Scope("child") - assert.NoError(t, s.Provide(newB)) - err = c.Provide(newC) - assert.Error(t, err, "expected a cycle to be introduced in the child") - assert.Contains(t, err.Error(), "In Scope child") + // Create a child Scope, and introduce a cycle + // in the child only. + check := func(c *Container, fails bool) { + s := c.Scope("child") + assert.NoError(t, c.Provide(newA)) + assert.NoError(t, s.Provide(newB)) + err := c.Provide(newC) + + if fails { + assert.Error(t, err, "expected a cycle to be introduced in the child") + assert.Contains(t, err.Error(), "In Scope child") + } else { + assert.NoError(t, err) + } + } + + // Same as check, but this time child should inherit + // parent-provided constructors upon construction. + checkWithInheritance := func(c *Container, fails bool) { + assert.NoError(t, c.Provide(newA)) + s := c.Scope("child") + assert.NoError(t, s.Provide(newB)) + err := c.Provide(newC) + if fails { + assert.Error(t, err, "expected a cycle to be introduced in the child") + assert.Contains(t, err.Error(), "In Scope child") + } else { + assert.NoError(t, err) + } + } + + // Test using different permutations + nodeferContainers := []func() *Container{ + func() *Container { return New() }, + func() *Container { return New(DryRun(true)) }, + func() *Container { return New(DryRun(false)) }, + } + // Container permutations with DeferAcyclicVerification. + deferredContainers := []func() *Container{ + func() *Container { return New(DeferAcyclicVerification()) }, + func() *Container { return New(DeferAcyclicVerification(), DryRun(true)) }, + func() *Container { return New(DeferAcyclicVerification(), DryRun(false)) }, + } + + for _, c := range nodeferContainers { + check(c(), true) + checkWithInheritance(c(), true) + } + + // with deferAcyclicVerification, these should not + // error on Provides. + for _, c := range deferredContainers { + check(c(), false) + checkWithInheritance(c(), false) + } }) t.Run("private provides do not propagate upstream", func(t *testing.T) { @@ -171,10 +189,30 @@ func TestScopeFailures(t *testing.T) { root := New() c := root.Scope("child") gc := c.Scope("grandchild") - gc.Provide(func() *A { return &A{} }) + require.NoError(t, gc.Provide(func() *A { return &A{} })) assert.Error(t, root.Invoke(func(a *A) {}), "invoking on grandchild's private-provided type should fail") assert.Error(t, c.Invoke(func(a *A) {}), "invoking on child's private-provided type should fail") }) + t.Run("private provides to child should be available to grandchildren, but not root", func(t *testing.T) { + type A struct{} + // Scope tree: + // root + // | + // child <-- Provide(func() *A) + // / \ + // gc1 gc2 + root := New() + c := root.Scope("child") + gc := c.Scope("grandchild") + + require.NoError(t, c.Provide(func() *A { return &A{} })) + + err := root.Invoke(func(a *A) {}) + assert.Error(t, err, "expected Invoke in root container on child's private-provided type to fail") + assert.Contains(t, err.Error(), "missing type: *dig.A") + + assert.NoError(t, gc.Invoke(func(a *A) {}), "expected Invoke in grandchild container on child's private-provided type to fail") + }) }