From 657d5ec6b041767fd380df716f8e2c96ca81511c Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Tue, 23 Jul 2024 14:21:19 +0100 Subject: [PATCH] cue/cmd: ensure flags are added if they are used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: trim used the dry-run flag but did not add it. This change ensures that when a value is fetched for any flag, we ensure that the flag has been added to the flagset. We panic if it has not been. Although better than the status quo, this is not quite perfect because it is only when the value is fetched at runtime that we perform this checking, and not all code paths require every flag value. So in the case of trim, if trim exits early due to some bug or divergence, then the dry-run flag value is never inspected and so we never do this checking. Trim has been fixed: it now adds the dry-run flag to the flagSet. There are quite a few cases of commands "using" flags which have not been added, much of which is because of some flags being "used" in common code (e.g. in common.go). So this change whitelists all these existing uses so that we can slowly work towards eliminating them all, and ensures the situation doesn't get worse in the mean time. Change-Id: I21263c4da9f7550e994d4bd4cbe0195f8946b2c3 Signed-off-by: Matthew Sackman Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1198283 Reviewed-by: Daniel Martí TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine --- cmd/cue/cmd/flags.go | 46 ++++++++++++++++++++++++++++++++++++++++++++ cmd/cue/cmd/trim.go | 1 + 2 files changed, 47 insertions(+) diff --git a/cmd/cue/cmd/flags.go b/cmd/cue/cmd/flags.go index 42d3c601e76..13ab56fdccc 100644 --- a/cmd/cue/cmd/flags.go +++ b/cmd/cue/cmd/flags.go @@ -15,6 +15,8 @@ package cmd import ( + "fmt" + "github.com/spf13/pflag" ) @@ -114,17 +116,61 @@ func addInjectionFlags(f *pflag.FlagSet, auto, hidden bool) { type flagName string +type unaddedFlagUse struct { + cmd string + 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)) + } +} + func (f flagName) Bool(cmd *Command) bool { + f.ensureAdded(cmd) v, _ := cmd.Flags().GetBool(string(f)) return v } func (f flagName) String(cmd *Command) string { + f.ensureAdded(cmd) v, _ := cmd.Flags().GetString(string(f)) return v } func (f flagName) StringArray(cmd *Command) []string { + f.ensureAdded(cmd) v, _ := cmd.Flags().GetStringArray(string(f)) return v } diff --git a/cmd/cue/cmd/trim.go b/cmd/cue/cmd/trim.go index 9ee0b956d5e..b87486d962f 100644 --- a/cmd/cue/cmd/trim.go +++ b/cmd/cue/cmd/trim.go @@ -87,6 +87,7 @@ removal. } addOutFlags(cmd.Flags(), false) + cmd.Flags().BoolP(string(flagDryRun), "n", false, "only run simulation") return cmd }