wire: improve error message for provider set conflicts (google/go-cloud#500)

internal/wire/analyze.go

@@ -299,22 +299,20 @@ func verifyArgsUsed(set *ProviderSet, used []*providerSetSrc) []error {
 func buildProviderMap(fset *token.FileSet, hasher typeutil.Hasher, set *ProviderSet) (*typeutil.Map, *typeutil.Map, []error) {
 	providerMap := new(typeutil.Map)
 	providerMap.SetHasher(hasher)
-	srcMap := new(typeutil.Map)
+	srcMap := new(typeutil.Map) // to *providerSetSrc
 	srcMap.SetHasher(hasher)
-	setMap := new(typeutil.Map) // to *ProviderSet, for error messages
-	setMap.SetHasher(hasher)
 
 	// Process imports first, verifying that there are no conflicts between sets.
 	ec := new(errorCollector)
 	for _, imp := range set.Imports {
+		src := &providerSetSrc{Import: imp}
 		imp.providerMap.Iterate(func(k types.Type, v interface{}) {
-			if providerMap.At(k) != nil {
+			if prevSrc := srcMap.At(k); prevSrc != nil {
-				ec.add(bindingConflictError(fset, imp.Pos, k, setMap.At(k).(*ProviderSet)))
+				ec.add(bindingConflictError(fset, k, set, src, prevSrc.(*providerSetSrc)))
 				return
 			}
 			providerMap.Set(k, v)
-			srcMap.Set(k, &providerSetSrc{Import: imp})
+			srcMap.Set(k, src)
-			setMap.Set(k, imp)
 		})
 	}
 	if len(ec.errors) > 0 {
@@ -325,23 +323,22 @@ func buildProviderMap(fset *token.FileSet, hasher typeutil.Hasher, set *Provider
 	for _, p := range set.Providers {
 		src := &providerSetSrc{Provider: p}
 		for _, typ := range p.Out {
-			if providerMap.At(typ) != nil {
+			if prevSrc := srcMap.At(typ); prevSrc != nil {
-				ec.add(bindingConflictError(fset, p.Pos, typ, setMap.At(typ).(*ProviderSet)))
+				ec.add(bindingConflictError(fset, typ, set, src, prevSrc.(*providerSetSrc)))
 				continue
 			}
 			providerMap.Set(typ, &ProvidedType{t: typ, p: p})
 			srcMap.Set(typ, src)
-			setMap.Set(typ, set)
 		}
 	}
 	for _, v := range set.Values {
-		if providerMap.At(v.Out) != nil {
+		src := &providerSetSrc{Value: v}
-			ec.add(bindingConflictError(fset, v.Pos, v.Out, setMap.At(v.Out).(*ProviderSet)))
+		if prevSrc := srcMap.At(v.Out); prevSrc != nil {
+			ec.add(bindingConflictError(fset, v.Out, set, src, prevSrc.(*providerSetSrc)))
 			continue
 		}
 		providerMap.Set(v.Out, &ProvidedType{t: v.Out, v: v})
-		srcMap.Set(v.Out, &providerSetSrc{Value: v})
+		srcMap.Set(v.Out, src)
-		setMap.Set(v.Out, set)
 	}
 	if len(ec.errors) > 0 {
 		return nil, nil, ec.errors
@@ -350,8 +347,9 @@ func buildProviderMap(fset *token.FileSet, hasher typeutil.Hasher, set *Provider
 	// Process bindings in set. Must happen after the other providers to
 	// ensure the concrete type is being provided.
 	for _, b := range set.Bindings {
-		if providerMap.At(b.Iface) != nil {
+		src := &providerSetSrc{Binding: b}
-			ec.add(bindingConflictError(fset, b.Pos, b.Iface, setMap.At(b.Iface).(*ProviderSet)))
+		if prevSrc := srcMap.At(b.Iface); prevSrc != nil {
+			ec.add(bindingConflictError(fset, b.Iface, set, src, prevSrc.(*providerSetSrc)))
 			continue
 		}
 		concrete := providerMap.At(b.Provided)
@@ -362,8 +360,7 @@ func buildProviderMap(fset *token.FileSet, hasher typeutil.Hasher, set *Provider
 			continue
 		}
 		providerMap.Set(b.Iface, concrete)
-		srcMap.Set(b.Iface, &providerSetSrc{Binding: b})
+		srcMap.Set(b.Iface, src)
-		setMap.Set(b.Iface, set)
 	}
 	if len(ec.errors) > 0 {
 		return nil, nil, ec.errors
@@ -433,15 +430,15 @@ func verifyAcyclic(providerMap *typeutil.Map, hasher typeutil.Hasher) []error {
 
 // bindingConflictError creates a new error describing multiple bindings
 // for the same output type.
-func bindingConflictError(fset *token.FileSet, pos token.Pos, typ types.Type, prevSet *ProviderSet) error {
+func bindingConflictError(fset *token.FileSet, typ types.Type, set *ProviderSet, cur, prev *providerSetSrc) error {
-	typString := types.TypeString(typ, nil)
+	sb := new(strings.Builder)
-	var err error
+	if set.VarName == "" {
-	if prevSet.VarName == "" {
+		fmt.Fprintf(sb, "wire.Build")
-		err = fmt.Errorf("multiple bindings for %s (previous binding at %v)",
-			typString, fset.Position(prevSet.Pos))
 	} else {
-		err = fmt.Errorf("multiple bindings for %s (previous binding in %q.%s)",
+		fmt.Fprintf(sb, set.VarName)
-			typString, prevSet.PkgPath, prevSet.VarName)
 	}
-	return notePosition(fset.Position(pos), err)
+	fmt.Fprintf(sb, " has multiple bindings for %s (", types.TypeString(typ, nil))
+	fmt.Fprintf(sb, "current binding: %s", strings.Join(cur.trace(fset, typ), " <- "))
+	fmt.Fprintf(sb, "; previous binding: %s", strings.Join(prev.trace(fset, typ), " <- "))
+	return notePosition(fset.Position(set.Pos), errors.New(sb.String()))
 }

internal/wire/parse.go

@@ -38,6 +38,37 @@ type providerSetSrc struct {
 	Import   *ProviderSet
 }
 
+// trace returns a slice of strings describing the (possibly recursive) source
+// of p, including line numbers.
+func (p *providerSetSrc) trace(fset *token.FileSet, typ types.Type) []string {
+	quoted := func(s string) string {
+		if s == "" {
+			return ""
+		}
+		return fmt.Sprintf("%q ", s)
+	}
+	var retval []string
+	if p.Provider != nil {
+		kind := "provider"
+		if p.Provider.IsStruct {
+			kind = "struct provider"
+		}
+		retval = append(retval, fmt.Sprintf("%s %s(%s)", kind, quoted(p.Provider.Name), fset.Position(p.Provider.Pos)))
+	} else if p.Binding != nil {
+		retval = append(retval, fmt.Sprintf("wire.Bind (%s)", fset.Position(p.Binding.Pos)))
+	} else if p.Value != nil {
+		retval = append(retval, fmt.Sprintf("wire.Value (%s)", fset.Position(p.Value.Pos)))
+	} else if p.Import != nil {
+		if parent := p.Import.srcMap.At(typ); parent != nil {
+			retval = append(retval, parent.(*providerSetSrc).trace(fset, typ)...)
+		}
+		retval = append(retval, fmt.Sprintf("provider set %s(%s)", quoted(p.Import.VarName), fset.Position(p.Import.Pos)))
+	} else {
+		panic("providerSetSrc with no fields set")
+	}
+	return retval
+}
+
 // A ProviderSet describes a set of providers.  The zero value is an empty
 // ProviderSet.
 type ProviderSet struct {

internal/wire/testdata/MultipleBindings/foo/foo.go

@@ -0,0 +1,45 @@
+// Copyright 2018 The Go Cloud Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"io"
+	"strings"
+
+	"github.com/google/go-cloud/wire"
+)
+
+type context struct{}
+
+func main() {}
+
+type Foo string
+type Bar io.Reader
+
+var Set = wire.NewSet(provideFoo)
+var SuperSet = wire.NewSet(Set)
+var SetWithDuplicateBindings = wire.NewSet(Set, SuperSet)
+
+func provideFoo() Foo {
+	return Foo("foo")
+}
+
+func provideFooAgain() Foo {
+	return Foo("foo foo")
+}
+
+func provideBar() Bar {
+	return io.Reader(strings.NewReader("hello"))
+}

internal/wire/testdata/MultipleBindings/foo/wire.go

@@ -0,0 +1,53 @@
+// Copyright 2018 The Go Cloud Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//+build wireinject
+
+package main
+
+import (
+	"strings"
+
+	"github.com/google/go-cloud/wire"
+)
+
+func inject() Foo {
+	// fail: provideFoo and provideFooAgain both provide Foo.
+	panic(wire.Build(provideFoo, provideFooAgain))
+}
+
+func injectFromSet() Foo {
+	// fail: provideFoo is also provided by Set.
+	panic(wire.Build(provideFoo, Set))
+}
+
+func injectFromNestedSet() Foo {
+	// fail: provideFoo is also provided by SuperSet, via Set.
+	panic(wire.Build(provideFoo, SuperSet))
+}
+
+func injectFromSetWithDuplicateBindings() Foo {
+	// fail: DuplicateBindingsSet has two providers for Foo.
+	panic(wire.Build(SetWithDuplicateBindings))
+}
+
+func injectDuplicateValues() Foo {
+	// fail: provideFoo and wire.Value both provide Foo.
+	panic(wire.Build(provideFoo, wire.Value(Foo("foo"))))
+}
+
+func injectDuplicateInterface() Bar {
+	// fail: provideBar and wire.Bind both provide Bar.
+	panic(wire.Build(provideBar, wire.Bind(new(Bar), strings.NewReader("hello"))))
+}

internal/wire/testdata/MultipleBindings/pkg

@@ -0,0 +1 @@
+example.com/foo

internal/wire/testdata/MultipleBindings/want/wire_errs.txt

@@ -0,0 +1,6 @@
+/wire_gopath/src/example.com/foo/wire.go:27:8: wire.Build has multiple bindings for example.com/foo.Foo (current binding: provider "provideFooAgain" (/wire_gopath/src/example.com/foo/foo.go:39:6); previous binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6)
+/wire_gopath/src/example.com/foo/wire.go:32:8: wire.Build has multiple bindings for example.com/foo.Foo (current binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6); previous binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6) <- provider set "Set" (/wire_gopath/src/example.com/foo/foo.go:31:11)
+/wire_gopath/src/example.com/foo/wire.go:37:8: wire.Build has multiple bindings for example.com/foo.Foo (current binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6); previous binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6) <- provider set "Set" (/wire_gopath/src/example.com/foo/foo.go:31:11) <- provider set "SuperSet" (/wire_gopath/src/example.com/foo/foo.go:32:16)
+/wire_gopath/src/example.com/foo/foo.go:33:32: SetWithDuplicateBindings has multiple bindings for example.com/foo.Foo (current binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6) <- provider set "Set" (/wire_gopath/src/example.com/foo/foo.go:31:11) <- provider set "SuperSet" (/wire_gopath/src/example.com/foo/foo.go:32:16); previous binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6) <- provider set "Set" (/wire_gopath/src/example.com/foo/foo.go:31:11)
+/wire_gopath/src/example.com/foo/wire.go:47:8: wire.Build has multiple bindings for example.com/foo.Foo (current binding: wire.Value (/wire_gopath/src/example.com/foo/wire.go:47:42); previous binding: provider "provideFoo" (/wire_gopath/src/example.com/foo/foo.go:35:6)
+/wire_gopath/src/example.com/foo/wire.go:52:8: wire.Build has multiple bindings for example.com/foo.Bar (current binding: wire.Bind (/wire_gopath/src/example.com/foo/wire.go:52:31); previous binding: provider "provideBar" (/wire_gopath/src/example.com/foo/foo.go:43:6)