From e9bf33c50d3fba9b5d400bfcb97128364dcb0df8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Fri, 10 May 2024 10:28:57 +0100 Subject: [PATCH] cmd/cue: repurpose exitOnErr into printError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only call site with fatal=true was in buildInstances, which now returns an error just like all the others. exitOnErr is now printError, since fatal was always false at this point, and panicExit and recoverError can be deleted as unused now. We now "just" have two ways to stop a command with errors: returning an arbitrary error, and returning ErrPrintedError. Stopping a command by panicking with panicExit is no longer possible. Note that fmt.go replaces uses of the non-fatal exitOnErr with error returns which stop further processing. This is intentional; after recent refactors, `cue fmt` is now inconsistent in terms of which errors it considers fatal. For the time being, it makes sense for fmt to stop at the first error. If we want to start treating some errors as non-fatal, such as parsing or formatting errors, we should first have tests in place that show the tool continuing to format more files. Signed-off-by: Daniel Martí Change-Id: I847be07f721fdb8cc64bfd6312b72615d4d70755 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194538 Reviewed-by: Roger Peppe TryBot-Result: CUEcueckoo --- cmd/cue/cmd/common.go | 63 ++++++++++++++++--------------------------- cmd/cue/cmd/eval.go | 6 ++--- cmd/cue/cmd/fmt.go | 30 ++++++++++++--------- cmd/cue/cmd/root.go | 51 +++++++++++++++++++---------------- cmd/cue/cmd/trim.go | 10 +++++-- cmd/cue/cmd/vet.go | 4 +-- 6 files changed, 81 insertions(+), 83 deletions(-) diff --git a/cmd/cue/cmd/common.go b/cmd/cue/cmd/common.go index 590fcee2c33..88a48ecc261 100644 --- a/cmd/cue/cmd/common.go +++ b/cmd/cue/cmd/common.go @@ -15,17 +15,14 @@ package cmd import ( - "io" "os" "path/filepath" "regexp" "strconv" "strings" - "testing" "github.com/spf13/pflag" "golang.org/x/text/language" - "golang.org/x/text/message" "cuelang.org/go/cue" "cuelang.org/go/cue/ast" @@ -82,34 +79,6 @@ func getLang() language.Tag { return language.Make(loc) } -// exitOnErr uses cue/errors to print any error to stderr, -// and if fatal is true, it causes the command's exit code to be 1. -// Note that os.Exit is not called, as that would prevent defers from running. -// -// TODO(mvdan): can we avoid the panicError and recover shenanigans? -// They work, but they make the code flow somewhat confusing to follow. -func exitOnErr(cmd *Command, err error, fatal bool) { - if err == nil { - return - } - - // Link x/text as our localizer. - p := message.NewPrinter(getLang()) - format := func(w io.Writer, format string, args ...interface{}) { - p.Fprintf(w, format, args...) - } - - cwd, _ := os.Getwd() - errors.Print(cmd.Stderr(), err, &errors.Config{ - Format: format, - Cwd: cwd, - ToSlash: testing.Testing(), - }) - if fatal { - panicExit() - } -} - func loadFromArgs(args []string, cfg *load.Config) []*build.Instance { binst := load.Instances(args, cfg) if len(binst) == 0 { @@ -168,9 +137,11 @@ func (b *buildPlan) instances() iterator { case len(b.orphaned) > 0: i = newStreamingIterator(b) case len(b.insts) > 0: + insts, err := buildInstances(b.cmd, b.insts, false) i = &instanceIterator{ inst: b.instance, - a: buildInstances(b.cmd, b.insts, false), + a: insts, + e: err, i: -1, } default: @@ -625,11 +596,14 @@ func parseArgs(cmd *Command, args []string, cfg *config) (p *buildPlan, err erro // TODO: ignore errors here for now until reporting of concreteness // of errors is correct. // See https://github.com/cue-lang/cue/issues/1483. - inst := buildInstances( + insts, err := buildInstances( p.cmd, []*build.Instance{schema}, - true)[0] - + true) + if err != nil { + return nil, err + } + inst := insts[0] if err := inst.err; err != nil { return nil, err } @@ -729,12 +703,14 @@ func (b *buildPlan) parseFlags() (err error) { return nil } -func buildInstances(cmd *Command, binst []*build.Instance, ignoreErrors bool) []*instance { +func buildInstances(cmd *Command, binst []*build.Instance, ignoreErrors bool) ([]*instance, error) { // TODO: // If there are no files and User is true, then use those? // Always use all files in user mode? instances, err := cmd.ctx.BuildInstances(binst) - exitOnErr(cmd, err, true) + if err != nil { + return nil, err + } insts := make([]*instance, len(instances)) for i, v := range instances { @@ -748,15 +724,22 @@ func buildInstances(cmd *Command, binst []*build.Instance, ignoreErrors bool) [] // TODO: remove ignoreErrors flag and always return here, leaving it up to // clients to check for errors down the road. if ignoreErrors || flagIgnore.Bool(cmd) { - return insts + return insts, nil } for _, inst := range instances { // TODO: consider merging errors of multiple files, but ensure // duplicates are removed. - exitOnErr(cmd, inst.Validate(), !flagIgnore.Bool(cmd)) + err := inst.Validate() + if err != nil { + if flagIgnore.Bool(cmd) { + printError(cmd, err) + } else { + return nil, err + } + } } - return insts + return insts, nil } func buildToolInstances(binst []*build.Instance) ([]*cue.Instance, error) { diff --git a/cmd/cue/cmd/eval.go b/cmd/cue/cmd/eval.go index 9ef3f147f22..9b8afe0fe84 100644 --- a/cmd/cue/cmd/eval.go +++ b/cmd/cue/cmd/eval.go @@ -132,7 +132,7 @@ func runEval(cmd *Command, args []string) error { err := e.Encode(v) if err != nil { errHeader() - exitOnErr(cmd, err, false) + printError(cmd, err) } continue } @@ -160,7 +160,7 @@ func runEval(cmd *Command, args []string) error { if e.IsConcrete() || flagConcrete.Bool(cmd) { if err := v.Validate(cue.Concrete(true)); err != nil { errHeader() - exitOnErr(cmd, err, false) + printError(cmd, err) continue } } @@ -171,7 +171,7 @@ func runEval(cmd *Command, args []string) error { err := e.EncodeFile(f) if err != nil { errHeader() - exitOnErr(cmd, err, false) + printError(cmd, err) } } if err := iter.err(); err != nil { diff --git a/cmd/cue/cmd/fmt.go b/cmd/cue/cmd/fmt.go index e4d8ee9f3a2..a70c240e317 100644 --- a/cmd/cue/cmd/fmt.go +++ b/cmd/cue/cmd/fmt.go @@ -71,14 +71,13 @@ given as explicit arguments. } for _, inst := range builds { - if inst.Err != nil { + if err := inst.Err; err != nil { switch { - case errors.As(inst.Err, new(*load.PackageError)) && len(inst.BuildFiles) != 0: + case errors.As(err, new(*load.PackageError)) && len(inst.BuildFiles) != 0: // Ignore package errors if there are files to format. - case errors.As(inst.Err, new(*load.NoFilesError)): + case errors.As(err, new(*load.NoFilesError)): default: - exitOnErr(cmd, inst.Err, false) - continue + return err } } for _, file := range inst.BuildFiles { @@ -116,7 +115,9 @@ given as explicit arguments. if path == "-" { contents, err := io.ReadAll(cmd.InOrStdin()) - exitOnErr(cmd, err, false) + if err != nil { + return err + } file.Source = contents } @@ -131,13 +132,14 @@ given as explicit arguments. } for _, arg := range args { if arg == "-" { - err := processFile(arg) - exitOnErr(cmd, err, false) + if err := processFile(arg); err != nil { + return err + } continue } arg = filepath.Clean(arg) - err := filepath.WalkDir(arg, func(path string, d fs.DirEntry, err error) error { + if err := filepath.WalkDir(arg, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } @@ -157,8 +159,9 @@ given as explicit arguments. } return processFile(path) - }) - exitOnErr(cmd, err, false) + }); err != nil { + return err + } } } @@ -227,8 +230,9 @@ func formatFile(file *build.File, opts []format.Option, cwd string, doDiff, chec case file.Filename == "-": // already wrote the formatted source to stdout above default: - err := os.WriteFile(file.Filename, formatted, 0644) - exitOnErr(cmd, err, false) + if err := os.WriteFile(file.Filename, formatted, 0644); err != nil { + return false, err + } } return true, nil } diff --git a/cmd/cue/cmd/root.go b/cmd/cue/cmd/root.go index 53804baf406..104bdb708e9 100644 --- a/cmd/cue/cmd/root.go +++ b/cmd/cue/cmd/root.go @@ -25,6 +25,7 @@ import ( "testing" "github.com/spf13/cobra" + "golang.org/x/text/message" "cuelang.org/go/cue" "cuelang.org/go/cue/cuecontext" @@ -300,7 +301,7 @@ type errWriter Command func (w *errWriter) Write(b []byte) (int, error) { c := (*Command)(w) - c.hasErr = true + c.hasErr = len(b) > 0 return c.Command.OutOrStderr().Write(b) } @@ -309,6 +310,9 @@ func (w *errWriter) Write(b []byte) (int, error) { // be used directly. // Stderr returns a writer that should be used for error messages. +// Writing to it will result in the command's exit code being 1. +// +// TODO(mvdan): provide an alternative to write to stderr without setting the exit code to 1. func (c *Command) Stderr() io.Writer { return (*errWriter)(c) } @@ -331,17 +335,37 @@ func (c *Command) SetInput(r io.Reader) { c.root.SetIn(r) } -// ErrPrintedError indicates error messages have been printed to stderr. +// ErrPrintedError indicates error messages have been printed directly to stderr, +// and can be used so that the returned error itself isn't printed as well. var ErrPrintedError = errors.New("terminating because of errors") +// printError uses cue/errors to print an error to stderr when non-nil. +func printError(cmd *Command, err error) { + if err == nil { + return + } + + // Link x/text as our localizer. + p := message.NewPrinter(getLang()) + format := func(w io.Writer, format string, args ...interface{}) { + p.Fprintf(w, format, args...) + } + + // TODO(mvdan): call os.Getwd only once at the start of the process. + cwd, _ := os.Getwd() + errors.Print(cmd.Stderr(), err, &errors.Config{ + Format: format, + Cwd: cwd, + ToSlash: testing.Testing(), + }) +} + func (c *Command) Run(ctx context.Context) (err error) { // Three categories of commands: // - normal // - user defined // - help // For the latter two, we need to use the default loading. - defer recoverError(&err) - if err := c.root.Execute(); err != nil { return err } @@ -350,22 +374,3 @@ func (c *Command) Run(ctx context.Context) (err error) { } return nil } - -func recoverError(err *error) { - switch e := recover().(type) { - case nil: - case panicError: - *err = e.Err - default: - panic(e) - } - // We use panic to escape, instead of os.Exit -} - -type panicError struct { - Err error -} - -func panicExit() { - panic(panicError{ErrPrintedError}) -} diff --git a/cmd/cue/cmd/trim.go b/cmd/cue/cmd/trim.go index a263bd50ef2..bbee57fbb99 100644 --- a/cmd/cue/cmd/trim.go +++ b/cmd/cue/cmd/trim.go @@ -100,7 +100,10 @@ func runTrim(cmd *Command, args []string) error { if binst == nil { return nil } - instances := buildInstances(cmd, binst, false) + instances, err := buildInstances(cmd, binst, false) + if err != nil { + return err + } dst := flagOutFile.String(cmd) if dst != "" && dst != "-" && !flagForce.Bool(cmd) { @@ -130,7 +133,10 @@ func runTrim(cmd *Command, args []string) error { cfg := *defCfg.loadCfg cfg.Overlay = overlay - tinsts := buildInstances(cmd, load.Instances(args, &cfg), false) + tinsts, err := buildInstances(cmd, load.Instances(args, &cfg), false) + if err != nil { + return err + } if len(tinsts) != len(binst) { return errors.New("unexpected number of new instances") } diff --git a/cmd/cue/cmd/vet.go b/cmd/cue/cmd/vet.go index 7df0918f65b..2c39cc711be 100644 --- a/cmd/cue/cmd/vet.go +++ b/cmd/cue/cmd/vet.go @@ -134,7 +134,7 @@ func doVet(cmd *Command, args []string) error { "some instances are incomplete; use the -c flag to show errors or suppress this message") } } - exitOnErr(cmd, err, false) + printError(cmd, err) } if err := iter.err(); err != nil { return err @@ -156,7 +156,7 @@ func vetFiles(cmd *Command, b *buildPlan) error { // Always concrete when checking against concrete files. err := v.Validate(cue.Concrete(true)) - exitOnErr(cmd, err, false) + printError(cmd, err) } if err := iter.err(); err != nil { return err