cmd/cue: fix all "flag used without being added" bugs

Most commands call parseArgs, which ends up calling other methods like
parseFlags and parsePlacementFlags, which always consulted flag values
even when those flags did not exist for the current command.

This was harmless in practice, as we would get their zero value
which resulted in no change in behavior. Still, it was confusing,
and using flags without declaring them should not be allowed.
The panic assertion is our guide here, as all flag names are global.

Teach methods like buildPlan.parseFlags to not consume undeclared flags
based on what the current mode is, via fields like outMode or importing.

No change in behavior should happen here; all tests continue to pass.

As suggested by Matthew, rename outMode to mode as well, since the mode
is not just about output files, and internal/encoding.Config calls it
Mode as well.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: Ib4cfedaef3590e56d00b1c6aefc5cb6a75336782
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1198854
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Matthew Sackman <[email protected]>

Author: mvdan

cmd/cue/cmd/common.go

@@ -333,7 +333,7 @@ func (i *expressionIter) value() cue.Value {
 }
 
 type config struct {
-	outMode filetypes.Mode
+	mode filetypes.Mode
 
 	fileFilter     string
 	reFile         *regexp.Regexp
@@ -458,7 +458,7 @@ func (p *buildPlan) getDecoders(b *build.Instance) (schemas, values []*decoderIn
 		}
 		d := encoding.NewDecoder(p.cmd.ctx, f, &c)
 
-		fi, err := filetypes.FromFile(f, p.cfg.outMode)
+		fi, err := filetypes.FromFile(f, p.cfg.mode)
 		if err != nil {
 			return schemas, values, err
 		}
@@ -651,31 +651,46 @@ func parseArgs(cmd *Command, args []string, cfg *config) (p *buildPlan, err erro
 func (b *buildPlan) parseFlags() (err error) {
 	b.mergeData = !b.cfg.noMerge && flagMerge.Bool(b.cmd)
 
-	out := flagOut.String(b.cmd)
-	outFile := flagOutFile.String(b.cmd)
-
-	if strings.Contains(out, ":") && strings.Contains(outFile, ":") {
-		return errors.Newf(token.NoPos,
-			"cannot specify qualifier in both --out and --outfile")
-	}
-	if outFile == "" {
-		outFile = "-"
-	}
-	if out != "" {
-		outFile = out + ":" + outFile
-	}
-	b.outFile, err = filetypes.ParseFile(outFile, b.cfg.outMode)
-	if err != nil {
-		return err
+	b.encConfig = &encoding.Config{
+		Mode:      b.cfg.mode,
+		Stdin:     b.cmd.InOrStdin(),
+		Stdout:    b.cmd.OutOrStdout(),
+		ProtoPath: flagProtoPath.StringArray(b.cmd),
+		AllErrors: flagAllErrors.Bool(b.cmd),
+		PkgName:   flagPackage.String(b.cmd),
+		Strict:    flagStrict.Bool(b.cmd),
 	}
 
-	for _, e := range flagExpression.StringArray(b.cmd) {
-		expr, err := parser.ParseExpr("--expression", e)
+	// For commands with an output mode, like `cue export` or `cue def`.
+	if b.cfg.mode != filetypes.Input {
+		out := flagOut.String(b.cmd)
+		outFile := flagOutFile.String(b.cmd)
+
+		if strings.Contains(out, ":") && strings.Contains(outFile, ":") {
+			return errors.Newf(token.NoPos,
+				"cannot specify qualifier in both --out and --outfile")
+		}
+		if outFile == "" {
+			outFile = "-"
+		}
+		if out != "" {
+			outFile = out + ":" + outFile
+		}
+		b.outFile, err = filetypes.ParseFile(outFile, b.cfg.mode)
 		if err != nil {
 			return err
 		}
-		b.expressions = append(b.expressions, expr)
+
+		for _, e := range flagExpression.StringArray(b.cmd) {
+			expr, err := parser.ParseExpr("--expression", e)
+			if err != nil {
+				return err
+			}
+			b.expressions = append(b.expressions, expr)
+		}
+		b.encConfig.Force = flagForce.Bool(b.cmd)
 	}
+
 	if s := flagSchema.String(b.cmd); s != "" {
 		b.schema, err = parser.ParseExpr("--schema", s)
 		if err != nil {
@@ -686,17 +701,12 @@ func (b *buildPlan) parseFlags() (err error) {
 		// Set a default file filter to only include json and yaml files
 		b.cfg.fileFilter = s
 	}
-	b.encConfig = &encoding.Config{
-		Force:         flagForce.Bool(b.cmd),
-		Mode:          b.cfg.outMode,
-		Stdin:         b.cmd.InOrStdin(),
-		Stdout:        b.cmd.OutOrStdout(),
-		ProtoPath:     flagProtoPath.StringArray(b.cmd),
-		AllErrors:     flagAllErrors.Bool(b.cmd),
-		PkgName:       flagPackage.String(b.cmd),
-		Strict:        flagStrict.Bool(b.cmd),
-		InlineImports: flagInlineImports.Bool(b.cmd),
-		EscapeHTML:    flagEscape.Bool(b.cmd),
+	// These flags exist only in specific output modes.
+	switch b.cfg.mode {
+	case filetypes.Export:
+		b.encConfig.EscapeHTML = flagEscape.Bool(b.cmd)
+	case filetypes.Def:
+		b.encConfig.InlineImports = flagInlineImports.Bool(b.cmd)
 	}
 	return nil
 }

cmd/cue/cmd/def.go

@@ -52,7 +52,7 @@ The --expression flag is used to only print parts of a configuration.
 }
 
 func runDef(cmd *Command, args []string) error {
-	b, err := parseArgs(cmd, args, &config{outMode: filetypes.Def})
+	b, err := parseArgs(cmd, args, &config{mode: filetypes.Def})
 	if err != nil {
 		return err
 	}

cmd/cue/cmd/eval.go

@@ -85,7 +85,7 @@ const (
 )
 
 func runEval(cmd *Command, args []string) error {
-	b, err := parseArgs(cmd, args, &config{outMode: filetypes.Eval})
+	b, err := parseArgs(cmd, args, &config{mode: filetypes.Eval})
 	if err != nil {
 		return err
 	}

cmd/cue/cmd/export.go

@@ -110,7 +110,7 @@ The following formats are recognized:
 }
 
 func runExport(cmd *Command, args []string) error {
-	b, err := parseArgs(cmd, args, &config{outMode: filetypes.Export})
+	b, err := parseArgs(cmd, args, &config{mode: filetypes.Export})
 	if err != nil {
 		return err
 	}

cmd/cue/cmd/flags.go

@@ -121,38 +121,12 @@ type unaddedFlagUse struct {
 	flag flagName
 }
 
-// TODO(ms) consider each of these in turn and either remove their
-// use, or add the flag to the flagset.
-var todoUnaddedFlagUse = map[unaddedFlagUse]struct{}{
-	{cmd: "def", flag: flagEscape}:           {},
-	{cmd: "def", flag: flagFiles}:            {},
-	{cmd: "eval", flag: flagEscape}:          {},
-	{cmd: "eval", flag: flagFiles}:           {},
-	{cmd: "eval", flag: flagInlineImports}:   {},
-	{cmd: "export", flag: flagFiles}:         {},
-	{cmd: "export", flag: flagInlineImports}: {},
-	{cmd: "import", flag: flagEscape}:        {},
-	{cmd: "import", flag: flagExpression}:    {},
-	{cmd: "import", flag: flagInlineImports}: {},
-	{cmd: "import", flag: flagOut}:           {},
-	{cmd: "vet", flag: flagEscape}:           {},
-	{cmd: "vet", flag: flagExpression}:       {},
-	{cmd: "vet", flag: flagFiles}:            {},
-	{cmd: "vet", flag: flagForce}:            {},
-	{cmd: "vet", flag: flagInlineImports}:    {},
-	{cmd: "vet", flag: flagOut}:              {},
-	{cmd: "vet", flag: flagOutFile}:          {},
-}
-
 // ensureAdded detects if a flag is being used without it first being
 // added to the flagSet. Because flagNames are global, it is quite
 // easy to accidentally use a flag in a command without adding it to
 // the flagSet.
 func (f flagName) ensureAdded(cmd *Command) {
 	if cmd.Flags().Lookup(string(f)) == nil {
-		if _, found := todoUnaddedFlagUse[unaddedFlagUse{cmd.Name(), f}]; found {
-			return
-		}
 		panic(fmt.Sprintf("Cmd %q uses flag %q without adding it", cmd.Name(), f))
 	}
 }

cmd/cue/cmd/orphans.go

@@ -40,7 +40,10 @@ func (b *buildPlan) usePlacement() bool {
 
 func (b *buildPlan) parsePlacementFlags() error {
 	cmd := b.cmd
-	b.perFile = flagFiles.Bool(cmd)
+	// Flags which only exist for `cue import`.
+	if b.importing {
+		b.perFile = flagFiles.Bool(cmd)
+	}
 	b.useList = flagList.Bool(cmd)
 	b.useContext = flagWithContext.Bool(cmd)