Repository: alecthomas/kong
Pull request #494 (2025-01-29 19:23:51)

hooks: Recursively search embedded fields for methods
Follow up to #493 and 840220c

Kong currently supports hooks on embedded fields of a parsed node,
but only at the first level of embedding:

```
type mainCmd struct {
    FooOptions
}

type FooOptions struct {
    BarOptions
}

func (f *FooOptions) BeforeApply() error {
    // this will be called
}

type BarOptions struct {
}

func (b *BarOptions) BeforeApply() error {
    // this will not be called
}
```

This change adds support for hooks to be defined
on embedded fields of embedded fields so that the above
example would work as expected.

Per #493, the definition of "embedded" field is adjusted to mean:

- Any anonymous (Go-embedded) field that is exported
- Any non-anonymous field that is tagged with `embed:""`

*Testing*:
Includes a test case for embedding an anonymous field in an `embed:""`
and an `embed:""` field in an anonymous field.



Patch:
diff --git a/callbacks.go b/callbacks.go
index e541f5a..4644c54 100644
--- a/callbacks.go
+++ b/callbacks.go
@@ -34,8 +34,17 @@ func (b bindings) addTo(impl, iface any) {
 func (b bindings) addProvider(provider any) error {
 	pv := reflect.ValueOf(provider)
 	t := pv.Type()
-	if t.Kind() != reflect.Func || t.NumOut() != 2 || t.Out(1) != reflect.TypeOf((*error)(nil)).Elem() {
-		return fmt.Errorf("%T must be a function with the signature func(...)(T, error)", provider)
+	if t.Kind() != reflect.Func {
+		return fmt.Errorf("%T must be a function", provider)
+	}
+
+	if t.NumOut() == 0 {
+		return fmt.Errorf("%T must be a function with the signature func(...)(T, error) or func(...) T", provider)
+	}
+	if t.NumOut() == 2 {
+		if t.Out(1) != reflect.TypeOf((*error)(nil)).Elem() {
+			return fmt.Errorf("missing error; %T must be a function with the signature func(...)(T, error) or func(...) T", provider)
+		}
 	}
 	rt := pv.Type().Out(0)
 	b[rt] = provider
@@ -68,35 +77,53 @@ func getMethod(value reflect.Value, name string) reflect.Value {
 	return method
 }
 
-// Get methods from the given value and any embedded fields.
+// getMethods gets all methods with the given name from the given value
+// and any embedded fields.
+//
+// Returns a slice of bound methods that can be called directly.
 func getMethods(value reflect.Value, name string) []reflect.Value {
-	// Collect all possible receivers
-	receivers := []reflect.Value{value}
-	if value.Kind() == reflect.Ptr {
-		value = value.Elem()
-	}
-	if value.Kind() == reflect.Struct {
-		t := value.Type()
-		for i := 0; i < value.NumField(); i++ {
-			field := value.Field(i)
-			fieldType := t.Field(i)
-			if !fieldType.IsExported() {
-				continue
-			}
+	// Traverses embedded fields of the struct
+	// starting from the given value to collect all possible receivers
+	// for the given method name.
+	var traverse func(value reflect.Value, receivers []reflect.Value) []reflect.Value
+	traverse = func(value reflect.Value, receivers []reflect.Value) []reflect.Value {
+		// Always consider the current value for hooks.
+		receivers = append(receivers, value)
 
-			// Hooks on exported embedded fields should be called.
-			if fieldType.Anonymous {
-				receivers = append(receivers, field)
-				continue
-			}
+		if value.Kind() == reflect.Ptr {
+			value = value.Elem()
+		}
+
+		// If the current value is a struct, also consider embedded fields.
+		// Two kinds of embedded fields are considered if they're exported:
+		//
+		//   - standard Go embedded fields
+		//   - fields tagged with `embed:""`
+		if value.Kind() == reflect.Struct {
+			t := value.Type()
+			for i := 0; i < value.NumField(); i++ {
+				fieldValue := value.Field(i)
+				field := t.Field(i)
+
+				if !field.IsExported() {
+					continue
+				}
 
-			// Hooks on exported fields that are not exported,
-			// but are tagged with `embed:""` should be called.
-			if _, ok := fieldType.Tag.Lookup("embed"); ok {
-				receivers = append(receivers, field)
+				// Consider a field embedded if it's actually embedded
+				// or if it's tagged with `embed:""`.
+				_, isEmbedded := field.Tag.Lookup("embed")
+				isEmbedded = isEmbedded || field.Anonymous
+				if isEmbedded {
+					receivers = traverse(fieldValue, receivers)
+				}
 			}
 		}
+
+		return receivers
 	}
+
+	receivers := traverse(value, nil /* receivers */)
+
 	// Search all receivers for methods
 	var methods []reflect.Value
 	for _, receiver := range receivers {
@@ -143,7 +170,7 @@ func callAnyFunction(f reflect.Value, bindings bindings) (out []any, err error)
 		if err != nil {
 			return nil, fmt.Errorf("%s: %w", pt, err)
 		}
-		if ferrv := reflect.ValueOf(argv[len(argv)-1]); ferrv.IsValid() && !ferrv.IsNil() {
+		if ferrv := reflect.ValueOf(argv[len(argv)-1]); ferrv.IsValid() && ferrv.Type().Implements(callbackReturnSignature) && !ferrv.IsNil() {
 			return nil, ferrv.Interface().(error) //nolint:forcetypeassert
 		}
 		in = append(in, reflect.ValueOf(argv[0]))
diff --git a/context.go b/context.go
index b6a56e3..ebf4c31 100644
--- a/context.go
+++ b/context.go
@@ -119,6 +119,9 @@ func (c *Context) BindTo(impl, iface any) {
 //
 // This is useful when the Run() function of different commands require different values that may
 // not all be initialisable from the main() function.
+//
+// "provider" must be a function with the signature func(...) (T, error) or func(...) T, where
+// ... will be recursively injected with bound values.
 func (c *Context) BindToProvider(provider any) error {
 	return c.bindings.addProvider(provider)
 }


Test patch:
diff --git a/kong_test.go b/kong_test.go
index 874b90b..6b5f5d6 100644
--- a/kong_test.go
+++ b/kong_test.go
@@ -2405,6 +2405,8 @@ func TestProviderMethods(t *testing.T) {
 }
 
 type EmbeddedCallback struct {
+	Nested NestedCallback `embed:""`
+
 	Embedded bool
 }
 
@@ -2414,6 +2416,8 @@ func (e *EmbeddedCallback) AfterApply() error {
 }
 
 type taggedEmbeddedCallback struct {
+	NestedCallback
+
 	Tagged bool
 }
 
@@ -2422,6 +2426,15 @@ func (e *taggedEmbeddedCallback) AfterApply() error {
 	return nil
 }
 
+type NestedCallback struct {
+	nested bool
+}
+
+func (n *NestedCallback) AfterApply() error {
+	n.nested = true
+	return nil
+}
+
 type EmbeddedRoot struct {
 	EmbeddedCallback
 	Tagged taggedEmbeddedCallback `embed:""`
@@ -2441,9 +2454,15 @@ func TestEmbeddedCallbacks(t *testing.T) {
 	expected := &EmbeddedRoot{
 		EmbeddedCallback: EmbeddedCallback{
 			Embedded: true,
+			Nested: NestedCallback{
+				nested: true,
+			},
 		},
 		Tagged: taggedEmbeddedCallback{
 			Tagged: true,
+			NestedCallback: NestedCallback{
+				nested: true,
+			},
 		},
 		Root: true,
 	}
@@ -2521,3 +2540,23 @@ func TestIssue483EmptyRootNodeNoRun(t *testing.T) {
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "no command selected")
 }
+
+type providerWithoutErrorCLI struct {
+}
+
+func (p *providerWithoutErrorCLI) Run(name string) error {
+	if name == "Bob" {
+		return nil
+	}
+	return fmt.Errorf("name %s is not Bob", name)
+}
+
+func TestProviderWithoutError(t *testing.T) {
+	k := mustNew(t, &providerWithoutErrorCLI{})
+	kctx, err := k.Parse(nil)
+	assert.NoError(t, err)
+	err = kctx.BindToProvider(func() string { return "Bob" })
+	assert.NoError(t, err)
+	err = kctx.Run()
+	assert.NoError(t, err)
+}