cmd/cue: repurpose exitOnErr into printError

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í <[email protected]>
Change-Id: I847be07f721fdb8cc64bfd6312b72615d4d70755
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194538
Reviewed-by: Roger Peppe <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>

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) {

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 {

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
 }

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})
-}

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")
 	}

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