cue/errors: print relative positions when used as arguments too

Most positions in CUE errors are reported as a list to be printed
one per line, which is done using relative paths for the sake
of brevity and to make the paths "clickable" on some IDEs. For example:

    builtin package "notinstd" undefined:
        ./missing.cue:1:8

However, sometimes positions are also used as part of error messages:

    list redeclared as imported package name
        previous declaration at /current/dir/missing.cue:4:1:
        ./missing.cue:2:8

Teach the cue/errors package to make these message argument positions
relative as well, which we can do pretty easily by spotting
which of the arguments are of type token.Pos and replacing them.
Note that this requires that the positions aren't stringified too early.

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

Author: mvdan

cmd/cue/cmd/testdata/script/eval_import_bad.txtar

@@ -1,12 +1,10 @@
 # Unknown and redeclared imports should be reported for anonymous packages too.
 
-# TODO: we should not use absolute paths in reported errors, which requires using 'cmpenv' below
-
 ! exec cue export named.cue
-cmpenv stderr named.stderr
+cmp stderr named.stderr
 
 ! exec cue export missing.cue
-cmpenv stderr missing.stderr
+cmp stderr missing.stderr
 
 -- named.cue --
 package p
@@ -19,7 +17,7 @@ list: 123
 builtin package "notinstd" undefined:
     ./named.cue:3:8
 list redeclared as imported package name
-	previous declaration at ${WORK}${/}named.cue:6:1:
+	previous declaration at ./named.cue:6:1:
     ./named.cue:4:8
 -- missing.cue --
 import "notinstd"
@@ -30,5 +28,5 @@ list: 123
 builtin package "notinstd" undefined:
     ./missing.cue:1:8
 list redeclared as imported package name
-	previous declaration at ${WORK}${/}missing.cue:4:1:
+	previous declaration at ./missing.cue:4:1:
     ./missing.cue:2:8

cue/errors/errors.go

@@ -505,12 +505,14 @@ type Config struct {
 	ToSlash bool
 }
 
+var zeroConfig = &Config{}
+
 // Print is a utility function that prints a list of errors to w,
 // one error per line, if the err parameter is an List. Otherwise
 // it prints the err string.
 func Print(w io.Writer, err error, cfg *Config) {
 	if cfg == nil {
-		cfg = &Config{}
+		cfg = zeroConfig
 	}
 	for _, e := range list(Errors(err)).sanitize() {
 		printError(w, e, cfg)
@@ -528,11 +530,11 @@ func Details(err error, cfg *Config) string {
 // String generates a short message from a given Error.
 func String(err Error) string {
 	var b strings.Builder
-	writeErr(&b, err)
+	writeErr(&b, err, zeroConfig)
 	return b.String()
 }
 
-func writeErr(w io.Writer, err Error) {
+func writeErr(w io.Writer, err Error, cfg *Config) {
 	if path := strings.Join(err.Path(), "."); path != "" {
 		_, _ = io.WriteString(w, path)
 		_, _ = io.WriteString(w, ": ")
@@ -542,6 +544,25 @@ func writeErr(w io.Writer, err Error) {
 		u := errors.Unwrap(err)
 
 		msg, args := err.Msg()
+
+		// Just like [printError] does when printing one position per line,
+		// make sure that any position formatting arguments print as relative paths.
+		//
+		// Note that [Error.Msg] isn't clear about whether we should treat args as read-only,
+		// so we make a copy if we need to replace any arguments.
+		didCopy := false
+		for i, arg := range args {
+			if arg, ok := arg.(token.Pos); ok {
+				if !didCopy {
+					args = slices.Clone(args)
+					didCopy = true
+				}
+				pos := arg.Position()
+				pos.Filename = relPath(pos.Filename, cfg)
+				args[i] = pos
+			}
+		}
+
 		n, _ := fmt.Fprintf(w, msg, args...)
 
 		if u == nil {
@@ -573,7 +594,7 @@ func printError(w io.Writer, err error, cfg *Config) {
 	}
 
 	if e, ok := err.(Error); ok {
-		writeErr(w, e)
+		writeErr(w, e, cfg)
 	} else {
 		fprintf(w, "%v", err)
 	}
@@ -586,21 +607,7 @@ func printError(w io.Writer, err error, cfg *Config) {
 	fprintf(w, ":\n")
 	for _, p := range positions {
 		pos := p.Position()
-		path := pos.Filename
-		if cfg.Cwd != "" {
-			if p, err := filepath.Rel(cfg.Cwd, path); err == nil {
-				path = p
-				// Some IDEs (e.g. VSCode) only recognize a path if it starts
-				// with a dot. This also helps to distinguish between local
-				// files and builtin packages.
-				if !strings.HasPrefix(path, ".") {
-					path = fmt.Sprintf(".%c%s", filepath.Separator, path)
-				}
-			}
-		}
-		if cfg.ToSlash {
-			path = filepath.ToSlash(path)
-		}
+		path := relPath(pos.Filename, cfg)
 		fprintf(w, "    %s", path)
 		if pos.IsValid() {
 			if path != "" {
@@ -611,3 +618,21 @@ func printError(w io.Writer, err error, cfg *Config) {
 		fprintf(w, "\n")
 	}
 }
+
+func relPath(path string, cfg *Config) string {
+	if cfg.Cwd != "" {
+		if p, err := filepath.Rel(cfg.Cwd, path); err == nil {
+			path = p
+			// Some IDEs (e.g. VSCode) only recognize a path if it starts
+			// with a dot. This also helps to distinguish between local
+			// files and builtin packages.
+			if !strings.HasPrefix(path, ".") {
+				path = fmt.Sprintf(".%c%s", filepath.Separator, path)
+			}
+		}
+	}
+	if cfg.ToSlash {
+		path = filepath.ToSlash(path)
+	}
+	return path
+}

internal/core/runtime/resolve.go

@@ -90,7 +90,7 @@ func resolveFile(
 		if n, ok := fields[name]; ok {
 			errs = errors.Append(errs, nodeErrorf(spec,
 				"%s redeclared as imported package name\n"+
-					"\tprevious declaration at %v", name, lineStr(idx, n)))
+					"\tprevious declaration at %s", name, n.Pos()))
 			continue
 		}
 		fields[name] = spec
@@ -159,7 +159,3 @@ func resolveFile(
 	// }
 	return errs
 }
-
-func lineStr(idx *index, n ast.Node) string {
-	return n.Pos().String()
-}