fix(cmd/morphic): print help on stdout and exit 0

cmd/morphic/command.go

@@ -1,9 +1,6 @@
 package main
 
-import (
-	"flag"
-	"io"
-)
+import "io"
 
 // command describes one morphic subcommand: how it is invoked, how it is
 // documented, and how it runs. Dispatch and help rendering both read this
@@ -17,21 +14,34 @@ type command struct {
 	usage string
 	// description is the paragraph shown above the flag table in command help.
 	description string
-	// flagSet returns a fresh FlagSet with this command's flags defined. Help
-	// rendering and argument parsing share it, so the flag table printed by
-	// PrintDefaults cannot drift from what Parse accepts.
-	flagSet func() *flag.FlagSet
+	// printFlags writes this command's flag table to w. It renders from the same
+	// constructor that parsing binds, so the documented flags cannot drift from
+	// the flags Parse accepts.
+	//
+	// It hands out the rendered text rather than the *flag.FlagSet because help
+	// needs nothing more: a FlagSet in a caller's hands can be Parsed, and that
+	// writes into an options struct nobody is holding, losing the values with no
+	// error to notice.
+	printFlags func(w io.Writer)
 	// run executes the command with the subcommand word already removed from
 	// args, and returns the process exit code.
 	run func(args []string, stdout, stderr io.Writer) int
 }
 
-// commands is the subcommand table. Adding a subcommand means adding one entry.
-var commands = []command{compileCommand}
+// commands returns the subcommand table. Adding a subcommand means adding one
+// entry.
+//
+// It is a function rather than a var so that a command's own code may reach
+// back into the table — writeRootHelp already does, and any error path that
+// wants to list the commands will too. As a var it sits in the initialization
+// graph, and the first such reference is an initialization cycle
+// (commands → newCompileCommand → runCompile → writeRootHelp → commands) that
+// stops the package compiling. Functions may freely refer to each other.
+func commands() []command { return []command{newCompileCommand()} }
 
 // lookup resolves a subcommand by name.
 func lookup(name string) (command, bool) {
-	for _, c := range commands {
+	for _, c := range commands() {
 		if c.name == name {
 			return c, true
 		}

cmd/morphic/command_test.go

@@ -1,7 +1,9 @@
 package main
 
 import (
+	"bytes"
 	"flag"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -17,7 +19,7 @@ func TestLookup_KnownAndUnknown(t *testing.T) {
 	assert.NotEmpty(t, c.summary, "every command needs a summary for the root help list")
 	assert.NotEmpty(t, c.usage)
 	assert.NotEmpty(t, c.description)
-	require.NotNil(t, c.flagSet)
+	require.NotNil(t, c.printFlags)
 	require.NotNil(t, c.run)
 
 	_, ok = lookup("bogus")
@@ -34,9 +36,10 @@ func TestLookup_KnownAndUnknown(t *testing.T) {
 func TestCommands_TableIsWellFormed(t *testing.T) {
 	t.Parallel()
 
-	require.NotEmpty(t, commands, "the command table must not be empty")
-	seen := make(map[string]bool, len(commands))
-	for _, c := range commands {
+	table := commands()
+	require.NotEmpty(t, table, "the command table must not be empty")
+	seen := make(map[string]bool, len(table))
+	for _, c := range table {
 		require.NotEmpty(t, c.name, "every command needs a name")
 		require.False(t, seen[c.name], "duplicate command %q", c.name)
 		seen[c.name] = true
@@ -64,19 +67,35 @@ func TestNewCompileFlags_DefinesEveryFlag(t *testing.T) {
 // constructor and the command-table entry so the two cannot drift.
 var compileFlagNames = []string{"o", "fail-on", "skip-validate", "explain"}
 
-func TestCommand_FlagSetBindsTheCommandsOwnFlags(t *testing.T) {
+func TestCommand_PrintFlagsDocumentsTheCommandsOwnFlags(t *testing.T) {
 	t.Parallel()
 
 	c, ok := lookup("compile")
 	require.True(t, ok)
+	require.NotNil(t, c.printFlags)
 
-	// The table's flagSet is what help rendering reads. Today it just calls
-	// newCompileFlags, so nothing can drift yet; this holds the line if the
-	// table ever gets its own.
-	fs := c.flagSet()
-	require.NotNil(t, fs)
+	var buf bytes.Buffer
+	c.printFlags(&buf)
+	require.NotEmpty(t, buf.String(), "printFlags must render something")
 
-	var got []string
-	fs.VisitAll(func(f *flag.Flag) { got = append(got, f.Name) })
-	assert.ElementsMatch(t, compileFlagNames, got)
+	// Read back out of the render rather than off a FlagSet: the render is what
+	// a user sees, and it is all the table hands out.
+	assert.ElementsMatch(t, compileFlagNames, flagNamesIn(buf.String()),
+		"the rendered flag table must document exactly the flags compile accepts")
+}
+
+// flagNamesIn returns the flag names documented by a rendered flag table.
+// PrintDefaults writes each flag as "  -name ..." above an indented description
+// line, so a line carrying the "  -" prefix names a flag and nothing else does.
+func flagNamesIn(rendered string) []string {
+	var names []string
+	for _, line := range strings.Split(rendered, "\n") {
+		rest, ok := strings.CutPrefix(line, "  -")
+		if !ok {
+			continue
+		}
+		name, _, _ := strings.Cut(rest, " ")
+		names = append(names, name)
+	}
+	return names
 }

cmd/morphic/compile.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -24,18 +25,28 @@ var newEngine = engine.New
 // not fail after a successful write on the platforms Morphic targets.
 var openOutput = func(path string) (io.WriteCloser, error) { return os.Create(path) }
 
-// compileCommand is compile's entry in the command table.
-var compileCommand = command{
-	name:    "compile",
-	summary: "lower an API spec (OpenAPI 3.x) into Morphic IR JSON",
-	usage:   "morphic compile <spec-file> [flags]",
-	description: "Lower an API spec (OpenAPI 3.x) into Morphic IR JSON on stdout, and write\n" +
-		"diagnostics to stderr.",
-	flagSet: func() *flag.FlagSet {
-		fs, _ := newCompileFlags()
-		return fs
-	},
-	run: runCompile,
+// newCompileCommand builds compile's command-table entry. It is a function,
+// not a package-level var, because its run field refers to runCompile, and
+// runCompile reads this same metadata back to render help and usage text. See
+// commands for why every step of that loop has to stay out of the
+// initialization graph.
+func newCompileCommand() command {
+	return command{
+		name:    "compile",
+		summary: "lower an API spec (OpenAPI 3.x) into Morphic IR JSON",
+		usage:   "morphic compile <spec-file> [flags]",
+		description: "Lower an API spec (OpenAPI 3.x) into Morphic IR JSON on stdout, and write\n" +
+			"diagnostics to stderr.\n\n" +
+			"--explain reports what compiling produced at one source coordinate — the\n" +
+			"type node interned there, the coordinates interned beneath it, and the\n" +
+			"diagnostics stamped at it — instead of writing the document.",
+		printFlags: func(w io.Writer) {
+			fs, _ := newCompileFlags()
+			fs.SetOutput(w)
+			fs.PrintDefaults()
+		},
+		run: runCompile,
+	}
 }
 
 // compileOptions holds the values compile's flags parse into.
@@ -73,25 +84,36 @@ func runCompile(args []string, stdout, stderr io.Writer) int {
 	fs, opts := newCompileFlags()
 
 	positional, err := parseArgs(fs, args)
+	if errors.Is(err, flag.ErrHelp) {
+		writeCommandHelp(stdout, newCompileCommand())
+		return 0
+	}
 	if err != nil {
-		emitf(stderr, "morphic: %v\n", err)
-		printUsage(stderr)
-		return 2
+		return compileUsageError(stderr, err.Error())
 	}
 	if opts.failOn != "error" && opts.failOn != "warning" {
-		emitf(stderr, "morphic: invalid --fail-on %q (want error or warning)\n", opts.failOn)
-		printUsage(stderr)
-		return 2
+		return compileUsageError(stderr,
+			fmt.Sprintf("invalid --fail-on %q (want error or warning)", opts.failOn))
 	}
 	if len(positional) != 1 {
-		emitf(stderr, "morphic: compile requires exactly one spec file\n")
-		printUsage(stderr)
-		return 2
+		return compileUsageError(stderr, "compile requires exactly one spec file")
 	}
 
 	return compileSpec(positional[0], *opts, stdout, stderr)
 }
 
+// compileUsageError reports a misuse of compile: one reason line, one short
+// usage pointer, exit 2. It never touches stdout.
+//
+// reason is a finished string, not a format: a printf-style wrapper here is
+// invisible to vet, so a reason carrying a literal % — a flag value echoed back
+// from the user, say — would be mangled into the output with nothing to catch it.
+func compileUsageError(stderr io.Writer, reason string) int {
+	emitf(stderr, "morphic: %s\n", reason)
+	writeCommandUsage(stderr, newCompileCommand())
+	return 2
+}
+
 // compileSpec runs the pipeline over specPath, writes the IR document and its
 // diagnostics, and returns the process exit code.
 func compileSpec(specPath string, opts compileOptions, stdout, stderr io.Writer) int {

cmd/morphic/compile_test.go

@@ -75,9 +75,36 @@ components:
 
 func TestRun_UsageErrors(t *testing.T) {
 	t.Parallel()
-	var stdout, stderr bytes.Buffer
-	assert.Equal(t, 2, run(nil, &stdout, &stderr))
-	assert.Equal(t, 2, run([]string{"bogus"}, &stdout, &stderr))
-	assert.Equal(t, 2, run([]string{"compile", "x.yaml", "--fail-on", "hint"}, &stdout, &stderr))
-	assert.True(t, strings.Contains(stderr.String(), "usage"))
+
+	spec := writeFile(t, "spec.yaml", testspec.Tiny)
+	tests := []struct {
+		name   string
+		args   []string
+		reason string
+	}{
+		{"unknown command", []string{"bogus"}, `unknown command "bogus"`},
+		{"help of unknown command", []string{"help", "bogus"}, `unknown command "bogus"`},
+		{"help of unknown command with help flag", []string{"help", "bogus", "--help"}, `unknown command "bogus"`},
+		{"help flag of unknown command", []string{"-h", "bogus"}, `unknown command "bogus"`},
+		{"help with extra args", []string{"help", "compile", "extra"}, "help accepts at most one command"},
+		{"help flag with extra args", []string{"--help", "compile", "extra"}, "help accepts at most one command"},
+		{"help with extra args and help flag", []string{"help", "a", "b", "--help"}, "help accepts at most one command"},
+		{"unknown flag", []string{"compile", spec, "--bogus"}, "flag provided but not defined: -bogus"},
+		{"no spec file", []string{"compile"}, "compile requires exactly one spec file"},
+		{"two spec files", []string{"compile", spec, spec}, "compile requires exactly one spec file"},
+		{"bad fail-on", []string{"compile", spec, "--fail-on", "hint"}, `invalid --fail-on "hint"`},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			var stdout, stderr bytes.Buffer
+
+			assert.Equal(t, 2, run(tt.args, &stdout, &stderr))
+			assert.Empty(t, stdout.String(), "usage errors must never write to stdout")
+			assert.Contains(t, stderr.String(), tt.reason)
+			assert.Equal(t, 1, strings.Count(stderr.String(), "usage:"),
+				"exactly one usage block per misuse, got:\n%s", stderr.String())
+		})
+	}
 }

cmd/morphic/edgecases_test.go

@@ -83,7 +83,7 @@ func TestMain_ExitCode(t *testing.T) {
 
 	var got int
 	osExit = func(code int) { got = code }
-	os.Args = []string{"morphic"} // no subcommand → usage → exit 2
+	os.Args = []string{"morphic", "bogus"} // unknown command → usage → exit 2
 
 	main()
 
@@ -121,38 +121,6 @@ func TestRunParse_NilDocumentReturnsOne(t *testing.T) {
 	assert.Contains(t, stderr.String(), "openapi/unsupported-version")
 }
 
-func TestRunParse_UnknownFlagIsUsageError(t *testing.T) {
-	t.Parallel()
-	spec := writeFile(t, "spec.yaml", testspec.Tiny)
-	var stdout, stderr bytes.Buffer
-
-	code := run([]string{"compile", spec, "--bogus"}, &stdout, &stderr)
-
-	assert.Equal(t, 2, code)
-	assert.Contains(t, stderr.String(), "usage")
-}
-
-func TestRunParse_WrongPositionalCount(t *testing.T) {
-	t.Parallel()
-	spec := writeFile(t, "spec.yaml", testspec.Tiny)
-	tests := []struct {
-		name string
-		args []string
-	}{
-		{"no spec file", []string{"compile"}},
-		{"two spec files", []string{"compile", spec, spec}},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			var stdout, stderr bytes.Buffer
-			code := run(tt.args, &stdout, &stderr)
-			assert.Equal(t, 2, code)
-			assert.Contains(t, stderr.String(), "requires exactly one spec file")
-		})
-	}
-}
-
 func TestRunParse_SkipValidateToStdout(t *testing.T) {
 	t.Parallel()
 	spec := writeFile(t, "spec.yaml", testspec.Tiny)