From 3345599aaf6e2fe265a0fd117ecaae882496aae7 Mon Sep 17 00:00:00 2001 From: Ross Light Date: Thu, 26 Apr 2018 14:23:06 -0400 Subject: [PATCH] goose: remove optional directive This introduces some short-term pain in practice, but I aim to fix that with the goose.Value directive. Reviewed-by: Tuo Shan --- README.md | 19 ------ internal/goose/analyze.go | 3 - internal/goose/parse.go | 58 ++++--------------- .../goose/testdata/OptionalMissing/foo/foo.go | 16 ----- .../testdata/OptionalMissing/foo/foo_goose.go | 7 --- .../goose/testdata/OptionalMissing/out.txt | 1 - internal/goose/testdata/OptionalMissing/pkg | 1 - .../goose/testdata/OptionalPresent/foo/foo.go | 16 ----- .../testdata/OptionalPresent/foo/foo_goose.go | 7 --- .../goose/testdata/OptionalPresent/out.txt | 1 - internal/goose/testdata/OptionalPresent/pkg | 1 - .../testdata/StructOptionalField/foo/foo.go | 23 -------- .../testdata/StructOptionalField/foo/goose.go | 7 --- .../testdata/StructOptionalField/out.txt | 1 - .../goose/testdata/StructOptionalField/pkg | 1 - .../StructOptionalFieldPresent/foo/foo.go | 28 --------- .../StructOptionalFieldPresent/foo/goose.go | 7 --- .../StructOptionalFieldPresent/out.txt | 1 - .../testdata/StructOptionalFieldPresent/pkg | 1 - main.go | 9 --- 20 files changed, 11 insertions(+), 197 deletions(-) delete mode 100644 internal/goose/testdata/OptionalMissing/foo/foo.go delete mode 100644 internal/goose/testdata/OptionalMissing/foo/foo_goose.go delete mode 100644 internal/goose/testdata/OptionalMissing/out.txt delete mode 100644 internal/goose/testdata/OptionalMissing/pkg delete mode 100644 internal/goose/testdata/OptionalPresent/foo/foo.go delete mode 100644 internal/goose/testdata/OptionalPresent/foo/foo_goose.go delete mode 100644 internal/goose/testdata/OptionalPresent/out.txt delete mode 100644 internal/goose/testdata/OptionalPresent/pkg delete mode 100644 internal/goose/testdata/StructOptionalField/foo/foo.go delete mode 100644 internal/goose/testdata/StructOptionalField/foo/goose.go delete mode 100644 internal/goose/testdata/StructOptionalField/out.txt delete mode 100644 internal/goose/testdata/StructOptionalField/pkg delete mode 100644 internal/goose/testdata/StructOptionalFieldPresent/foo/foo.go delete mode 100644 internal/goose/testdata/StructOptionalFieldPresent/foo/goose.go delete mode 100644 internal/goose/testdata/StructOptionalFieldPresent/out.txt delete mode 100644 internal/goose/testdata/StructOptionalFieldPresent/pkg diff --git a/README.md b/README.md index 072a63c..54cd280 100644 --- a/README.md +++ b/README.md @@ -245,22 +245,6 @@ set that provides the concrete type. [type identity]: https://golang.org/ref/spec#Type_identity [return concrete types]: https://github.com/golang/go/wiki/CodeReviewComments#interfaces -### Optional Inputs - -A provider input can be marked optional using `goose:optional`: - -```go -//goose:provide Bar -//goose:optional foo - -func provideBar(foo Foo) Bar { - // ... -} -``` - -If used as part of an injector that does not bring in the `Foo` dependency, then -the injector will pass the provider the zero value as the `foo` argument. - ### Struct Providers Structs can also be marked as providers. Instead of calling a function, an @@ -308,9 +292,6 @@ func injectFooBar() FooBar { And similarly if the injector needed a `*FooBar`. -Like function providers, you can mark dependencies of a struct provider optional -by using the `goose:optional` directive with the field names. - ### Cleanup functions If a provider creates a value that needs to be cleaned up (e.g. closing a file), diff --git a/internal/goose/analyze.go b/internal/goose/analyze.go index 7f1d3bc..f13f278 100644 --- a/internal/goose/analyze.go +++ b/internal/goose/analyze.go @@ -85,9 +85,6 @@ func solve(mc *providerSetCache, out types.Type, given []types.Type, sets []symr p, _ := providers.At(typ).(*Provider) if p == nil { - if trail[len(trail)-1].Optional { - return nil - } if len(trail) == 1 { return fmt.Errorf("no provider found for %s (output of injector)", types.TypeString(typ, nil)) } diff --git a/internal/goose/parse.go b/internal/goose/parse.go index 019dd28..ee63138 100644 --- a/internal/goose/parse.go +++ b/internal/goose/parse.go @@ -79,8 +79,9 @@ type Provider struct { // ProviderInput describes an incoming edge in the provider graph. type ProviderInput struct { - Type types.Type - Optional bool + Type types.Type + + // TODO(light): Move field name into this struct. } // Load finds all the provider sets in the given packages, as well as @@ -203,7 +204,7 @@ func findProviderSets(fctx findContext, files []*ast.File) (map[string]*Provider // processUnassociatedDirective handles any directive that was not associated with a top-level declaration. func processUnassociatedDirective(fctx findContext, sets map[string]*ProviderSet, scope *types.Scope, d directive) error { switch d.kind { - case "provide", "optional": + case "provide": return fmt.Errorf("%v: only functions can be marked as providers", fctx.fset.Position(d.pos)) case "use": // Ignore, picked up by injector flow. @@ -323,11 +324,6 @@ func processDeclDirectives(fctx findContext, sets map[string]*ProviderSet, scope return err } if !p.isValid() { - for _, d := range dg.dirs { - if d.kind == "optional" { - return fmt.Errorf("%v: cannot use goose:%s directive on non-provider", fctx.fset.Position(d.pos), d.kind) - } - } return nil } var providerSetName string @@ -337,18 +333,10 @@ func processDeclDirectives(fctx findContext, sets map[string]*ProviderSet, scope } else if len(args) > 1 { return fmt.Errorf("%v: goose:provide takes at most one argument", fctx.fset.Position(p.pos)) } - optionals := make(map[string]token.Pos) - for _, d := range dg.dirs { - if d.kind == "optional" { - for _, arg := range d.args() { - optionals[arg] = d.pos - } - } - } switch decl := dg.decl.(type) { case *ast.FuncDecl: fn := fctx.typeInfo.ObjectOf(decl.Name).(*types.Func) - provider, err := processFuncProvider(fctx, fn, optionals) + provider, err := processFuncProvider(fctx, fn) if err != nil { return err } @@ -379,7 +367,7 @@ func processDeclDirectives(fctx findContext, sets map[string]*ProviderSet, scope if _, ok := typeName.Type().(*types.Named).Underlying().(*types.Struct); !ok { return fmt.Errorf("%v: only functions and structs can be marked as providers", fctx.fset.Position(p.pos)) } - provider, err := processStructProvider(fctx, typeName, optionals) + provider, err := processStructProvider(fctx, typeName) if err != nil { return err } @@ -410,18 +398,9 @@ func processDeclDirectives(fctx findContext, sets map[string]*ProviderSet, scope return nil } -func processFuncProvider(fctx findContext, fn *types.Func, optionalArgs map[string]token.Pos) (*Provider, error) { +func processFuncProvider(fctx findContext, fn *types.Func) (*Provider, error) { sig := fn.Type().(*types.Signature) - optionals := make([]bool, sig.Params().Len()) - for arg, dpos := range optionalArgs { - pi := paramIndex(sig.Params(), arg) - if pi == -1 { - return nil, fmt.Errorf("%v: %s is not a parameter of func %s", fctx.fset.Position(dpos), arg, fn.Name()) - } - optionals[pi] = true - } - fpos := fn.Pos() r := sig.Results() var hasCleanup, hasErr bool @@ -461,8 +440,7 @@ func processFuncProvider(fctx findContext, fn *types.Func, optionalArgs map[stri } for i := 0; i < params.Len(); i++ { provider.Args[i] = ProviderInput{ - Type: params.At(i).Type(), - Optional: optionals[i], + Type: params.At(i).Type(), } for j := 0; j < i; j++ { if types.Identical(provider.Args[i].Type, provider.Args[j].Type) { @@ -473,21 +451,9 @@ func processFuncProvider(fctx findContext, fn *types.Func, optionalArgs map[stri return provider, nil } -func processStructProvider(fctx findContext, typeName *types.TypeName, optionals map[string]token.Pos) (*Provider, error) { +func processStructProvider(fctx findContext, typeName *types.TypeName) (*Provider, error) { out := typeName.Type() st := out.Underlying().(*types.Struct) - for arg, dpos := range optionals { - found := false - for i := 0; i < st.NumFields(); i++ { - if st.Field(i).Name() == arg { - found = true - break - } - } - if !found { - return nil, fmt.Errorf("%v: %s is not a field of struct %s", fctx.fset.Position(dpos), arg, types.TypeString(st, nil)) - } - } pos := typeName.Pos() provider := &Provider{ @@ -501,10 +467,8 @@ func processStructProvider(fctx findContext, typeName *types.TypeName, optionals } for i := 0; i < st.NumFields(); i++ { f := st.Field(i) - _, optional := optionals[f.Name()] provider.Args[i] = ProviderInput{ - Type: f.Type(), - Optional: optional, + Type: f.Type(), } provider.Fields[i] = f.Name() for j := 0; j < i; j++ { @@ -688,7 +652,7 @@ func parseFile(fset *token.FileSet, f *ast.File) []directiveGroup { // Move directives that don't associate into the unassociated group. n := 0 for i := start; i < len(grp.dirs); i++ { - if k := grp.dirs[i].kind; k == "provide" || k == "optional" || k == "use" { + if k := grp.dirs[i].kind; k == "provide" || k == "use" { grp.dirs[start+n] = grp.dirs[i] n++ } else { diff --git a/internal/goose/testdata/OptionalMissing/foo/foo.go b/internal/goose/testdata/OptionalMissing/foo/foo.go deleted file mode 100644 index e7e839a..0000000 --- a/internal/goose/testdata/OptionalMissing/foo/foo.go +++ /dev/null @@ -1,16 +0,0 @@ -package main - -import "fmt" - -func main() { - fmt.Println(injectBar()) -} - -type foo int -type bar int - -//goose:provide -//goose:optional f -func provideBar(f foo) bar { - return bar(f) -} diff --git a/internal/goose/testdata/OptionalMissing/foo/foo_goose.go b/internal/goose/testdata/OptionalMissing/foo/foo_goose.go deleted file mode 100644 index 0d53f4d..0000000 --- a/internal/goose/testdata/OptionalMissing/foo/foo_goose.go +++ /dev/null @@ -1,7 +0,0 @@ -//+build gooseinject - -package main - -//goose:use provideBar - -func injectBar() bar diff --git a/internal/goose/testdata/OptionalMissing/out.txt b/internal/goose/testdata/OptionalMissing/out.txt deleted file mode 100644 index 573541a..0000000 --- a/internal/goose/testdata/OptionalMissing/out.txt +++ /dev/null @@ -1 +0,0 @@ -0 diff --git a/internal/goose/testdata/OptionalMissing/pkg b/internal/goose/testdata/OptionalMissing/pkg deleted file mode 100644 index 257cc56..0000000 --- a/internal/goose/testdata/OptionalMissing/pkg +++ /dev/null @@ -1 +0,0 @@ -foo diff --git a/internal/goose/testdata/OptionalPresent/foo/foo.go b/internal/goose/testdata/OptionalPresent/foo/foo.go deleted file mode 100644 index c0a9fa4..0000000 --- a/internal/goose/testdata/OptionalPresent/foo/foo.go +++ /dev/null @@ -1,16 +0,0 @@ -package main - -import "fmt" - -func main() { - fmt.Println(injectBar(42)) -} - -type foo int -type bar int - -//goose:provide -//goose:optional f -func provideBar(f foo) bar { - return bar(f) -} diff --git a/internal/goose/testdata/OptionalPresent/foo/foo_goose.go b/internal/goose/testdata/OptionalPresent/foo/foo_goose.go deleted file mode 100644 index c2665b4..0000000 --- a/internal/goose/testdata/OptionalPresent/foo/foo_goose.go +++ /dev/null @@ -1,7 +0,0 @@ -//+build gooseinject - -package main - -//goose:use provideBar - -func injectBar(foo) bar diff --git a/internal/goose/testdata/OptionalPresent/out.txt b/internal/goose/testdata/OptionalPresent/out.txt deleted file mode 100644 index d81cc07..0000000 --- a/internal/goose/testdata/OptionalPresent/out.txt +++ /dev/null @@ -1 +0,0 @@ -42 diff --git a/internal/goose/testdata/OptionalPresent/pkg b/internal/goose/testdata/OptionalPresent/pkg deleted file mode 100644 index 257cc56..0000000 --- a/internal/goose/testdata/OptionalPresent/pkg +++ /dev/null @@ -1 +0,0 @@ -foo diff --git a/internal/goose/testdata/StructOptionalField/foo/foo.go b/internal/goose/testdata/StructOptionalField/foo/foo.go deleted file mode 100644 index f37938a..0000000 --- a/internal/goose/testdata/StructOptionalField/foo/foo.go +++ /dev/null @@ -1,23 +0,0 @@ -package main - -import "fmt" - -func main() { - fb := injectFooBar() - fmt.Println(fb.Foo, fb.OptionalBar) -} - -type Foo int -type Bar int - -//goose:provide Set -//goose:optional OptionalBar -type FooBar struct { - Foo Foo - OptionalBar Bar -} - -//goose:provide Set -func provideFoo() Foo { - return 42 -} diff --git a/internal/goose/testdata/StructOptionalField/foo/goose.go b/internal/goose/testdata/StructOptionalField/foo/goose.go deleted file mode 100644 index 73f5093..0000000 --- a/internal/goose/testdata/StructOptionalField/foo/goose.go +++ /dev/null @@ -1,7 +0,0 @@ -//+build gooseinject - -package main - -//goose:use Set - -func injectFooBar() FooBar diff --git a/internal/goose/testdata/StructOptionalField/out.txt b/internal/goose/testdata/StructOptionalField/out.txt deleted file mode 100644 index 426756f..0000000 --- a/internal/goose/testdata/StructOptionalField/out.txt +++ /dev/null @@ -1 +0,0 @@ -42 0 diff --git a/internal/goose/testdata/StructOptionalField/pkg b/internal/goose/testdata/StructOptionalField/pkg deleted file mode 100644 index 257cc56..0000000 --- a/internal/goose/testdata/StructOptionalField/pkg +++ /dev/null @@ -1 +0,0 @@ -foo diff --git a/internal/goose/testdata/StructOptionalFieldPresent/foo/foo.go b/internal/goose/testdata/StructOptionalFieldPresent/foo/foo.go deleted file mode 100644 index b612fa3..0000000 --- a/internal/goose/testdata/StructOptionalFieldPresent/foo/foo.go +++ /dev/null @@ -1,28 +0,0 @@ -package main - -import "fmt" - -func main() { - fb := injectFooBar() - fmt.Println(fb.Foo, fb.Bar) -} - -type Foo int -type Bar int - -//goose:provide Set -//goose:optional Bar -type FooBar struct { - Foo Foo - Bar Bar -} - -//goose:provide Set -func provideFoo() Foo { - return 41 -} - -//goose:provide Set -func provideBar() Bar { - return 1 -} diff --git a/internal/goose/testdata/StructOptionalFieldPresent/foo/goose.go b/internal/goose/testdata/StructOptionalFieldPresent/foo/goose.go deleted file mode 100644 index 73f5093..0000000 --- a/internal/goose/testdata/StructOptionalFieldPresent/foo/goose.go +++ /dev/null @@ -1,7 +0,0 @@ -//+build gooseinject - -package main - -//goose:use Set - -func injectFooBar() FooBar diff --git a/internal/goose/testdata/StructOptionalFieldPresent/out.txt b/internal/goose/testdata/StructOptionalFieldPresent/out.txt deleted file mode 100644 index b1ae43f..0000000 --- a/internal/goose/testdata/StructOptionalFieldPresent/out.txt +++ /dev/null @@ -1 +0,0 @@ -41 1 diff --git a/internal/goose/testdata/StructOptionalFieldPresent/pkg b/internal/goose/testdata/StructOptionalFieldPresent/pkg deleted file mode 100644 index 257cc56..0000000 --- a/internal/goose/testdata/StructOptionalFieldPresent/pkg +++ /dev/null @@ -1 +0,0 @@ -foo diff --git a/main.go b/main.go index 062b0c7..07a4cce 100644 --- a/main.go +++ b/main.go @@ -198,9 +198,6 @@ func gather(info *goose.Info, key goose.ProviderSetID) (_ []outGroup, imports ma // Try to see if any args haven't been visited. allPresent := true for _, arg := range p.Args { - if arg.Optional { - continue - } if inputVisited.At(arg.Type) == nil { allPresent = false } @@ -208,9 +205,6 @@ func gather(info *goose.Info, key goose.ProviderSetID) (_ []outGroup, imports ma if !allPresent { stk = append(stk, curr) for _, arg := range p.Args { - if arg.Optional { - continue - } if inputVisited.At(arg.Type) == nil { stk = append(stk, arg.Type) } @@ -222,9 +216,6 @@ func gather(info *goose.Info, key goose.ProviderSetID) (_ []outGroup, imports ma in := new(typeutil.Map) in.SetHasher(hash) for _, arg := range p.Args { - if arg.Optional { - continue - } i := inputVisited.At(arg.Type).(int) if i == -1 { in.Set(arg.Type, true)