diff --git a/cmd/morphic/args.go b/cmd/morphic/args.go new file mode 100644 index 0000000..6089ebb --- /dev/null +++ b/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 +} diff --git a/cmd/morphic/args_test.go b/cmd/morphic/args_test.go new file mode 100644 index 0000000..f635e06 --- /dev/null +++ b/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) + }) + } +} diff --git a/cmd/morphic/compile.go b/cmd/morphic/compile.go index 211b4e2..7e6619b 100644 --- a/cmd/morphic/compile.go +++ b/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:] diff --git a/cmd/morphic/help.go b/cmd/morphic/help.go index b9e20b0..d61c9a4 100644 --- a/cmd/morphic/help.go +++ b/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 } diff --git a/cmd/morphic/main.go b/cmd/morphic/main.go index 20e4a0e..fd52375 100644 --- a/cmd/morphic/main.go +++ b/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) } diff --git a/cmd/morphic/testdata/compile-help.txt b/cmd/morphic/testdata/compile-help.txt index 115d6b1..2393e9e 100644 --- a/cmd/morphic/testdata/compile-help.txt +++ b/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