Repository: alecthomas/kong
Pull request #298 (2022-05-01 23:32:29)

fix: always help
Help fails sometimes due to `kong.Parse()` calling `context.go:Context.Reset()` early in the function; this call to `Context.Reset()` makes subsequent calls to `model.go:Value.Reset()`; `Value.Reset()` calls `Parse` for environment variables and default values; `Parse` in turn calls `Value.Mapper.Decode()`, which executes the resolver; if the resolver fails, then `Parse` fails during `Reset`; originally, `--help` was implemented as a `BeforeApply` hook, but due to the above described call order, it wasn't getting called in some failure conditions; if `Reset` is moved to a later stage in `kong.Parse`, the hook can be called, allowing for usage to be printed.

Initially opening as a draft, as there may be unintended consequences of moving `Reset` around in `Parse` and changing `--help` to a `BeforeResolve` hook.


Patch:
diff --git a/README.md b/README.md
index 372db71..40d4c46 100644
--- a/README.md
+++ b/README.md
@@ -248,7 +248,7 @@ func main() {
 
 If a node in the grammar has a `BeforeResolve(...)`, `BeforeApply(...) error` and/or `AfterApply(...) error` method, those methods will be called before validation/assignment and after validation/assignment, respectively.
 
-The `--help` flag is implemented with a `BeforeApply` hook.
+The `--help` flag is implemented with a `BeforeResolve` hook.
 
 Arguments to hooks are provided via the `Run(...)` method or `Bind(...)` option. `*Kong`, `*Context` and `*Path` are also bound and finally, hooks can also contribute bindings via `kong.Context.Bind()` and `kong.Context.BindTo()`.
 
diff --git a/help.go b/help.go
index d65f91c..473df92 100644
--- a/help.go
+++ b/help.go
@@ -16,7 +16,7 @@ const (
 // Help flag.
 type helpValue bool
 
-func (h helpValue) BeforeApply(ctx *Context) error {
+func (h helpValue) BeforeResolve(ctx *Context) error {
 	options := ctx.Kong.helpOptions
 	options.Summary = false
 	err := ctx.Kong.help(options, ctx)
diff --git a/kong.go b/kong.go
index eeeaa8c..bea2998 100644
--- a/kong.go
+++ b/kong.go
@@ -251,15 +251,15 @@ func (k *Kong) Parse(args []string) (ctx *Context, err error) {
 	if ctx.Error != nil {
 		return nil, &ParseError{error: ctx.Error, Context: ctx}
 	}
-	if err = ctx.Reset(); err != nil {
-		return nil, &ParseError{error: err, Context: ctx}
-	}
 	if err = k.applyHook(ctx, "BeforeResolve"); err != nil {
 		return nil, &ParseError{error: err, Context: ctx}
 	}
 	if err = ctx.Resolve(); err != nil {
 		return nil, &ParseError{error: err, Context: ctx}
 	}
+	if err = ctx.Reset(); err != nil {
+		return nil, &ParseError{error: err, Context: ctx}
+	}
 	if err = k.applyHook(ctx, "BeforeApply"); err != nil {
 		return nil, &ParseError{error: err, Context: ctx}
 	}


Test patch:
diff --git a/kong_test.go b/kong_test.go
index 6ce73f5..9848356 100644
--- a/kong_test.go
+++ b/kong_test.go
@@ -1542,3 +1542,22 @@ func TestPassthroughCmdOnlyStringArgs(t *testing.T) {
 	_, err := kong.New(&cli)
 	require.EqualError(t, err, "<anonymous struct>.Command: passthrough command command [<args> ...] must contain exactly one positional argument of []string type")
 }
+
+func TestHelpShouldStillWork(t *testing.T) {
+	type CLI struct {
+		Dir string `type:"existingdir" default:"missing-dir"`
+	}
+	var cli CLI
+	k := mustNew(t, &cli)
+	rc := -1 // init nonzero to help assert help hook was called
+	k.Exit = func(i int) {
+		rc = i
+	}
+	_, err := k.Parse([]string{"--help"})
+	// checking return code validates the help hook was called
+	require.Zero(t, rc)
+	// allow for error propagation from other validation (only for the
+	// sake of this test, due to the exit function not actually exiting the
+	// program; errors will not propagate in the real world).
+	require.Error(t, err)
+}