breaking change: introduce OverridesError() to mark unsafe wrappers

Author: muir

bind.go

@@ -34,6 +34,11 @@ func doBind(sc *Collection, originalInvokeF *provider, originalInitF *provider,
 			return err
 		}
 
+		err = checkForMissingOverridesError(afterInvoke)
+		if err != nil {
+			return err
+		}
+
 		// Add debugging provider
 		{
 			d := newProvider(func() *Debugging { return nil }, -1, "Debugging")

nject.go

@@ -25,6 +25,7 @@ type provider struct {
 	callsInner          bool
 	memoize             bool
 	loose               bool
+	overridesError      bool
 	desired             bool
 	shun                bool
 	notCacheable        bool
@@ -363,6 +364,9 @@ func (fm provider) DownFlows() ([]reflect.Type, []reflect.Type) {
 	}
 	t := v.Type()
 	if t.Kind() == reflect.Func {
+		if fm.group == finalGroup {
+			return typesIn(t), nil
+		}
 		return effectiveOutputs(t)
 	}
 	return nil, []reflect.Type{t}
@@ -439,6 +443,9 @@ func (fm provider) UpFlows() ([]reflect.Type, []reflect.Type) {
 	}
 	t := v.Type()
 	if t.Kind() == reflect.Func {
+		if fm.group == finalGroup {
+			return nil, typesOut(t)
+		}
 		return effectiveReturns(t)
 	}
 	return nil, []reflect.Type{t}

overrides_error.go

@@ -0,0 +1,86 @@
+package nject
+
+import (
+	"fmt"
+	"reflect"
+)
+
+// OverridesError marks a provider that is okay for that provider to override
+// error returns.  Without this decorator, a wrapper that returns error but
+// does not expect to receive an error will cause the injection chain
+// compilation to fail.
+//
+// A common mistake is to have an wrapper that accidently returns error.  It
+// looks like this:
+//
+//	func AutoCloseThing(inner func(someType), param anotherType) error {
+//		thing, err := getThing(param)
+//		if err != nil {
+//			return err
+//		}
+//		defer thing.Close()
+//		inner(thing)
+//		return nil
+//	}
+//
+// The above function has two problems.  The big problem is that it will
+// override any returned errors coming up from below in the call chain
+// by returning nil.  The fix for this is to have the inner function return
+// error.  If you aren't sure there will be something below that will
+// definitely return error, then you can inject something to provide a nil
+// error.  Put the following at the end of the sequence:
+//
+//	nject.Shun(nject.NotFinal(func () error { return nil }))
+//
+// The second issue is that thing.Close() probably returns error.  A correct
+// wrapper for this looks like this:
+//
+//	func AutoCloseThing(inner func(someType) error, param anotherType) (err error) {
+//		var thing someType
+//		thing, err = getThing(param)
+//		if err != nil {
+//			return err
+//		}
+//		defer func() {
+//			e := thing.Close()
+//			if err == nil && e != nil {
+//				err = e
+//			}
+//		}()
+//		return inner(thing)
+//	}
+//
+func OverridesError(fn interface{}) Provider {
+	return newThing(fn).modify(func(fm *provider) {
+		fm.overridesError = true
+	})
+}
+
+func checkForMissingOverridesError(collection []*provider) error {
+	var errorReturnSeen bool
+	for i := len(collection) - 1; i >= 0; i-- {
+		fm := collection[i]
+		if errorReturnSeen && !fm.overridesError && fm.class == wrapperFunc {
+			consumes, returns := fm.UpFlows()
+			if hasError(returns) && !hasError(consumes) {
+				return fmt.Errorf("wrapper returns error but does not consume error.  Decorate with OverridesError() if this is intentional. %s", fm)
+			}
+		}
+		if !errorReturnSeen {
+			_, returns := fm.UpFlows()
+			if hasError(returns) {
+				errorReturnSeen = true
+			}
+		}
+	}
+	return nil
+}
+
+func hasError(types []reflect.Type) bool {
+	for _, typ := range types {
+		if typ == errorType {
+			return true
+		}
+	}
+	return false
+}

overrides_error_test.go

@@ -0,0 +1,40 @@
+package nject
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestOverridesError(t *testing.T) {
+	type someType string
+	type anotherType string
+	getThing := func(_ anotherType) (someType, error) { return "", nil }
+	danger := Required(func(inner func(someType), param anotherType) error {
+		thing, err := getThing(param)
+		if err != nil {
+			return err
+		}
+		inner(thing)
+		return nil
+	})
+	finalWithError := func() error { return nil }
+	returnsTerminal := Required(func() TerminalError { return nil })
+	finalWithoutError := func() {}
+	var target func(anotherType) error
+
+	t.Log("test: okay because no error bubbling up")
+	assert.NoError(t, Sequence("A", danger, finalWithoutError).Bind(&target, nil))
+
+	t.Log("test: should fail because the final function returns error that gets clobbered")
+	assert.Error(t, Sequence("B", danger, finalWithError).Bind(&target, nil))
+
+	t.Log("test: should fail because there is a terminal-error injector that gets clobbered")
+	assert.Error(t, Sequence("C", danger, returnsTerminal, finalWithoutError).Bind(&target, nil))
+
+	t.Log("test: okay because marked even thoguh the final function returns error that gets clobbered")
+	assert.Error(t, Sequence("B", OverridesError(danger), finalWithError).Bind(&target, nil))
+
+	t.Log("test: okay because marked even though there is a terminal-error injector that gets clobbered")
+	assert.Error(t, Sequence("C", OverridesError(danger), returnsTerminal, finalWithoutError).Bind(&target, nil))
+}