fix(cli): make -- terminate flag parsing for all later args

cmd/morphic/args.go

@@ -0,0 +1,80 @@
+package main
+
+import (
+	"flag"
+	"strings"
+)
+
+// flagTerminator is the end-of-flags marker every POSIX utility accepts: the
+// arguments after it are operands however they are spelled, which is the only
+// way to name a file that begins with "-".
+const flagTerminator = "--"
+
+// boolFlag is the flag package's own test for a flag set by its own presence
+// rather than by the argument after it. The interface is unexported there, so
+// it is restated rather than reached for; every flag registered by BoolVar
+// satisfies it.
+type boolFlag interface{ IsBoolFlag() bool }
+
+// cutTerminator returns args with a leading flagTerminator removed, and reports
+// whether one was there. It is the whole of terminator handling for an argument
+// list that defines no flags: there is nothing to stop parsing, so the marker's
+// only job is to say that what follows is not a flag.
+func cutTerminator(args []string) ([]string, bool) {
+	if len(args) > 0 && args[0] == flagTerminator {
+		return args[1:], true
+	}
+	return args, false
+}
+
+// splitAtTerminator splits args at the first flagTerminator standing as an
+// argument of its own, returning what precedes it and the operands that follow.
+// A "--" some flag asked for is that flag's value and not a marker, so the scan
+// steps over it — which is the whole reason it needs fs rather than a plain
+// search for the token.
+func splitAtTerminator(fs *flag.FlagSet, args []string) (before, operands []string) {
+	for i := 0; i < len(args); i++ {
+		if args[i] == flagTerminator {
+			return args[:i], args[i+1:]
+		}
+		if takesNextValue(fs, args[i]) {
+			i++
+		}
+	}
+	return args, nil
+}
+
+// takesNextValue reports whether arg is a flag fs defines that reads its value
+// from the following argument: spelled without an inline "=value" and not
+// boolean. An argument fs does not define is left alone, since Parse will
+// reject it and the split cannot change that.
+func takesNextValue(fs *flag.FlagSet, arg string) bool {
+	name, ok := flagName(arg)
+	if !ok {
+		return false
+	}
+	f := fs.Lookup(name)
+	if f == nil {
+		return false
+	}
+	b, isBool := f.Value.(boolFlag)
+	return !isBool || !b.IsBoolFlag()
+}
+
+// flagName returns the flag name arg spells, and whether arg is a flag whose
+// value would come from the next argument at all. It mirrors the flag package's
+// own syntax: one or two leading dashes, a name that starts with neither "-"
+// nor "=", and no inline "=value".
+func flagName(arg string) (string, bool) {
+	if len(arg) < 2 || arg[0] != '-' {
+		return "", false
+	}
+	name := strings.TrimPrefix(arg[1:], "-")
+	if name == "" || name[0] == '-' || name[0] == '=' {
+		return "", false
+	}
+	if strings.Contains(name, "=") {
+		return "", false
+	}
+	return name, true
+}

cmd/morphic/args_test.go

@@ -0,0 +1,203 @@
+package main
+
+import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/dexpace/morphic/internal/testspec"
+)
+
+// TestRun_TerminatorEndsFlagParsing pins what "--" means everywhere morphic
+// reads an argument list: it ends flag parsing for the rest of the invocation,
+// not for the next argument. Each case names the layer it exercises, because
+// the three read argv independently — the subcommand's own flags, the root
+// command word, and help's command word — and a terminator honoured in one is
+// no evidence about the others.
+func TestRun_TerminatorEndsFlagParsing(t *testing.T) {
+	t.Parallel()
+
+	spec := writeFile(t, "spec.yaml", testspec.Tiny)
+	outDir := t.TempDir()
+	shielded := filepath.Join(outDir, "shielded.json")
+	trailing := filepath.Join(outDir, "trailing.json")
+	written := filepath.Join(outDir, "written.json")
+
+	tests := []struct {
+		name     string
+		args     []string
+		wantCode int
+		wantErr  string
+		// wantFile, when set, must exist afterwards; wantNoFile must not.
+		wantFile   string
+		wantNoFile string
+	}{
+		{
+			name:       "flags after a shielded spec are operands",
+			args:       []string{"compile", "--", spec, "-o", shielded},
+			wantCode:   2,
+			wantErr:    "compile requires exactly one spec file",
+			wantNoFile: shielded,
+		},
+		{
+			name:     "every shielded operand is an operand",
+			args:     []string{"compile", "--", "-nope.yaml", "-second.yaml"},
+			wantCode: 2,
+			wantErr:  "compile requires exactly one spec file",
+		},
+		{
+			name:     "a shielded dash-named spec reaches the engine",
+			args:     []string{"compile", "--", "-nope.yaml"},
+			wantCode: 2,
+			wantErr:  `read spec "-nope.yaml"`,
+		},
+		{
+			name:       "a terminator after the spec still shields",
+			args:       []string{"compile", spec, "--", "-o", trailing},
+			wantCode:   2,
+			wantErr:    "compile requires exactly one spec file",
+			wantNoFile: trailing,
+		},
+		{
+			name:     "flags before the terminator still parse",
+			args:     []string{"compile", "-o", written, "--", spec},
+			wantCode: 0,
+			wantFile: written,
+		},
+		{
+			name:     "a root terminator shields the command word",
+			args:     []string{"--", "compile", spec},
+			wantCode: 0,
+		},
+		{
+			name:     "a root terminator ends help flags",
+			args:     []string{"--", "-h"},
+			wantCode: 2,
+			wantErr:  `unknown command "-h"`,
+		},
+		{
+			name:     "a help terminator ends help flags",
+			args:     []string{"help", "--", "--help"},
+			wantCode: 2,
+			wantErr:  `unknown command "--help"`,
+		},
+		{
+			name:     "a help terminator shields the command word",
+			args:     []string{"help", "--", "compile"},
+			wantCode: 0,
+		},
+	}
+
+	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)
+
+			require.Equal(t, tt.wantCode, code, "stderr: %s", stderr.String())
+			if tt.wantErr != "" {
+				assert.Contains(t, stderr.String(), tt.wantErr)
+			}
+			if tt.wantFile != "" {
+				assert.FileExists(t, tt.wantFile)
+			}
+			if tt.wantNoFile != "" {
+				assert.NoFileExists(t, tt.wantNoFile,
+					"an operand the user shielded must never be honoured as -o")
+			}
+		})
+	}
+}
+
+// TestRun_TerminatorAsFlagValue pins the half of the rule that is easy to lose
+// while fixing the other half: the "--" a flag asked for is that flag's value,
+// not a terminator, so scanning argv for the token without tracking which flags
+// consume the argument after them would break this. It passes both before and
+// after the terminator fix, and reddens on a fix that pre-scans instead.
+//
+// Not run in parallel: it changes the process working directory so -o's value
+// resolves to a file literally named "--".
+func TestRun_TerminatorAsFlagValue(t *testing.T) {
+	dir := t.TempDir()
+	prevWD, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.Chdir(dir))
+	t.Cleanup(func() { require.NoError(t, os.Chdir(prevWD)) })
+
+	spec := writeFile(t, "spec.yaml", testspec.Tiny)
+	var stdout, stderr bytes.Buffer
+
+	code := run([]string{"compile", "-o", "--", spec, "--skip-validate"}, &stdout, &stderr)
+
+	require.Equal(t, 0, code, "stderr: %s", stderr.String())
+	raw, err := os.ReadFile(filepath.Join(dir, "--"))
+	require.NoError(t, err)
+	assert.Contains(t, string(raw), `"name": "Tiny"`,
+		`"--" must be consumed as -o's value, leaving the flags after the spec to parse`)
+}
+
+func TestTakesNextValue_FlagSpellings(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		arg  string
+		want bool
+	}{
+		{"a value flag", "-o", true},
+		{"a value flag spelled with two dashes", "--o", true},
+		{"a value flag carrying its value inline", "-o=x", false},
+		{"a boolean flag", "-skip-validate", false},
+		{"a flag this command does not define", "-bogus", false},
+		{"an operand", "spec.yaml", false},
+		{"a bare dash", "-", false},
+		{"the terminator", "--", false},
+		{"more dashes than a flag can have", "---o", false},
+		{"a flag with no name", "-=x", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			fs, _ := newCompileFlags()
+			assert.Equal(t, tt.want, takesNextValue(fs, tt.arg))
+		})
+	}
+}
+
+func TestSplitAtTerminator_Cases(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name         string
+		args         []string
+		wantBefore   []string
+		wantOperands []string
+	}{
+		{"no terminator", []string{"-o", "x", "spec.yaml"}, []string{"-o", "x", "spec.yaml"}, nil},
+		{"leading terminator", []string{"--", "-a", "-b"}, []string{}, []string{"-a", "-b"}},
+		{"terminator with nothing after it", []string{"spec.yaml", "--"}, []string{"spec.yaml"}, []string{}},
+		{"only the first terminator splits", []string{"--", "a", "--", "b"}, []string{}, []string{"a", "--", "b"}},
+		{"a terminator a flag asked for is its value", []string{"-o", "--", "spec.yaml"},
+			[]string{"-o", "--", "spec.yaml"}, nil},
+		{"a boolean flag does not swallow the terminator", []string{"-skip-validate", "--", "-a"},
+			[]string{"-skip-validate"}, []string{"-a"}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			fs, _ := newCompileFlags()
+
+			before, operands := splitAtTerminator(fs, tt.args)
+
+			assert.Equal(t, tt.wantBefore, before)
+			assert.Equal(t, tt.wantOperands, operands)
+		})
+	}
+}

cmd/morphic/compile.go

@@ -70,7 +70,10 @@ func newCompileCommand() command {
 			"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.",
+			"diagnostics stamped at it — instead of writing the document.\n\n" +
+			"A -- argument ends flag parsing: every argument after it is an operand,\n" +
+			"even one that begins with a dash, which is how a spec file named like a\n" +
+			"flag is passed.",
 		printFlags: func(w io.Writer) {
 			fs, _ := newCompileFlags()
 			fs.SetOutput(w)
@@ -208,9 +211,18 @@ func compileSpec(specPath string, opts compileOptions, stdout, stderr io.Writer)
 // parseArgs binds fs and collects positional arguments, tolerating flags that
 // appear either before or after the spec path (stdlib flag stops at the first
 // non-flag argument, so it is invoked once per positional).
+//
+// A "--" ends flag parsing for the whole invocation rather than for one round
+// of it, so it is split off before that loop starts. Leaving it to Parse would
+// shield exactly one argument: Parse consumes the marker and reports nothing
+// about having seen one, so the next round cannot tell a terminated list from a
+// list that merely stopped at a positional, and re-enables flag parsing for
+// everything the user had marked as operands.
 func parseArgs(fs *flag.FlagSet, args []string) ([]string, error) {
+	before, operands := splitAtTerminator(fs, args)
+
 	var positional []string
-	rest := args
+	rest := before
 	for {
 		if err := fs.Parse(rest); err != nil {
 			// Returned verbatim, not wrapped: this error is rendered straight to
@@ -219,7 +231,7 @@ func parseArgs(fs *flag.FlagSet, args []string) ([]string, error) {
 		}
 		rest = fs.Args()
 		if len(rest) == 0 {
-			return positional, nil
+			return append(positional, operands...), nil
 		}
 		positional = append(positional, rest[0])
 		rest = rest[1:]

cmd/morphic/help.go

@@ -82,9 +82,16 @@ func writeCommandUsage(w io.Writer, c command) {
 // here specifically because help has no flags; dispatch must keep detecting a
 // subcommand's help request via errors.Is(err, flag.ErrHelp) from that
 // subcommand's own Parse instead of pre-scanning argv.
+//
+// A "--" stops the filtering, since past it a help-flag token is a command name
+// like any other: "morphic help -- --help" reports an unknown command called
+// "--help" rather than dropping the token and being left with the marker.
 func filterHelpTokens(args []string) []string {
 	names := make([]string, 0, len(args))
-	for _, arg := range args {
+	for i, arg := range args {
+		if arg == flagTerminator {
+			return append(names, args[i+1:]...)
+		}
 		if isHelpFlag(arg) {
 			continue
 		}

cmd/morphic/main.go

@@ -22,6 +22,11 @@ func main() {
 // run dispatches subcommands and returns the process exit code. It exists so
 // tests can drive the CLI without a subprocess; only main calls os.Exit.
 func run(args []string, stdout, stderr io.Writer) int {
+	// A leading "--" ends flag parsing for morphic itself, so what follows names
+	// a command however it is spelled: "morphic -- -h" reports an unknown
+	// command rather than printing help. It does not stop that name being
+	// "help", which is a command word and never was a flag.
+	args, terminated := cutTerminator(args)
 	if len(args) == 0 {
 		writeRootHelp(stdout)
 		return 0
@@ -30,7 +35,7 @@ func run(args []string, stdout, stderr io.Writer) int {
 	// the same path rather than a shortcut to root help. That is what makes
 	// "morphic -h compile" print compile's help instead of silently dropping
 	// the name, and "morphic -h bogus" report it instead of masking it.
-	if args[0] == "help" || isHelpFlag(args[0]) {
+	if args[0] == "help" || (!terminated && isHelpFlag(args[0])) {
 		return runHelp(args[1:], stdout, stderr)
 	}
 

cmd/morphic/testdata/compile-help.txt

@@ -8,6 +8,10 @@ diagnostics to stderr.
 type node interned there, the coordinates interned beneath it, and the
 diagnostics stamped at it — instead of writing the document.
 
+A -- argument ends flag parsing: every argument after it is an operand,
+even one that begins with a dash, which is how a spec file named like a
+flag is passed.
+
 flags:
   -explain string
     	report what compiling produced at this source pointer instead of writing IR JSON