pkg/diag: Include line:col of start position in Error and Show methods.

The Error methods used to show the start and end indices, while the Show methods
used to show line ranges.

Showing line:col of the start position seems to be pretty standard; both Go and
Rust do that.

I tried including the line:col of the end position too, but can't find a good
enough format.

pkg/diag/cognate_errors.go

@@ -51,8 +51,7 @@ func (err cognateErrors) Error() string {
 		if i > 0 {
 			sb.WriteString("; ")
 		}
-		// TODO: Include line and column numbers instead of byte indices.
+		fmt.Fprintf(&sb, "%s: %s", e.Context.culprit.describeStart(), e.Message)
-		fmt.Fprintf(&sb, "%d-%d: %s", e.Context.From, e.Context.To, e.Message)
 	}
 	return sb.String()
 }
@@ -62,12 +61,12 @@ func (err cognateErrors) Show(indent string) string {
 	fmt.Fprintf(&sb, "Multiple %ss in %s:", err[0].Type, err[0].Context.Name)
 	for _, e := range err {
 		sb.WriteString("\n" + indent + "  ")
-		fmt.Fprintf(&sb, "\033[31;1m%s\033[m", e.Message)
+		sb.WriteString(messageStart + e.Message + messageEnd)
 		sb.WriteString("\n" + indent + "    ")
 		// This duplicates part of [Context.ShowCompact].
-		desc := e.Context.lineRange() + " "
+		desc := e.Context.culprit.describeStart() + ": "
 		descIndent := strings.Repeat(" ", wcwidth.Of(desc))
-		sb.WriteString(desc + e.Context.relevantSource(indent+"  "+descIndent))
+		sb.WriteString(desc + e.Context.culprit.Show(indent+"  "+descIndent))
 	}
 	return sb.String()
 }

pkg/diag/cognate_errors_test.go

@@ -22,51 +22,33 @@ var (
 )
 
 var cognateErrorsTests = []struct {
-	name      string
+	name    string
-	errs      []*Error
+	errs    []*Error
-	wantError string
+	wantErr error
-	wantShow  string
 }{
 	{
-		name:      "no error",
+		name:    "no error",
-		errs:      nil,
+		errs:    nil,
-		wantError: "",
+		wantErr: nil,
-		wantShow:  "",
 	},
 	{
-		name:      "one error",
+		name:    "one error",
-		errs:      []*Error{err1},
+		errs:    []*Error{err1},
-		wantError: err1.Error(),
+		wantErr: err1,
-		wantShow:  err1.Show(""),
 	},
 	{
-		name:      "multiple errors",
+		name:    "multiple errors",
-		errs:      []*Error{err1, err2},
+		errs:    []*Error{err1, err2},
-		wantError: "multiple foo errors in [test]: 5-8: bad 1; 5-8: bad 2",
+		wantErr: cognateErrors{err1, err2},
-		wantShow: lines(
-			"Multiple foo errors in [test]:",
-			"  \x1b[31;1mbad 1\x1b[m",
-			"    line 1: echo \x1b[1;4m(1)\x1b[m",
-			"  \x1b[31;1mbad 2\x1b[m",
-			"    line 1: echo \x1b[1;4m(2)\x1b[m"),
 	},
 }
 
-func TestCognateErrors(t *testing.T) {
+func TestPackAndUnpackCognateErrors(t *testing.T) {
 	for _, tc := range cognateErrorsTests {
 		t.Run(tc.name, func(t *testing.T) {
 			err := PackCognateErrors(tc.errs)
-			if err == nil {
+			if !reflect.DeepEqual(err, tc.wantErr) {
-				if tc.wantError != "" || tc.wantShow != "" {
+				t.Errorf("got packed error %#v, want %#v", err, tc.wantErr)
-					t.Errorf("Want non-nil error, got nil")
-				}
-			} else {
-				if got := err.Error(); got != tc.wantError {
-					t.Errorf("Error() (-want +got):\n%s", cmp.Diff(tc.wantError, got))
-				}
-				if got := err.(Shower).Show(""); got != tc.wantShow {
-					t.Errorf("Show() (-want +got):\n%s", cmp.Diff(tc.wantShow, got))
-				}
 			}
 			unpacked := UnpackCognateErrors(err)
 			if !reflect.DeepEqual(unpacked, tc.errs) {
@@ -82,3 +64,24 @@ func TestUnpackCognateErrors_CalledWithOtherErrorType(t *testing.T) {
 		t.Errorf("want nil, got %v", unpacked)
 	}
 }
+
+func TestCognateErrors(t *testing.T) {
+	setCulpritMarkers(t, "<", ">")
+	setMessageMarkers(t, "{", "}")
+	err := PackCognateErrors([]*Error{err1, err2})
+	wantError := "multiple foo errors in [test]: 1:6: bad 1; 1:6: bad 2"
+	if s := err.Error(); s != wantError {
+		t.Errorf(".Error() returns unexpected result (-want +got):\n%s",
+			cmp.Diff(wantError, s))
+	}
+	wantShow := dedent(`
+			Multiple foo errors in [test]:
+			  {bad 1}
+			    1:6: echo <(1)>
+			  {bad 2}
+			    1:6: echo <(2)>`)
+	if show := err.(Shower).Show(""); show != wantShow {
+		t.Errorf(".Show(\"\") returns unexpected result (-want +got):\n%s",
+			cmp.Diff(wantShow, show))
+	}
+}

pkg/diag/context.go

@@ -1,7 +1,6 @@
 package diag
 
 import (
-	"bytes"
 	"fmt"
 	"strings"
 
@@ -11,132 +10,102 @@ import (
 // Context is a range of text in a source code. It is typically used for
 // errors that can be associated with a part of the source code, like parse
 // errors and a traceback entry.
+//
+// Context values should only be constructed using [NewContext].
 type Context struct {
 	Name   string
 	Source string
 	Ranging
 
-	savedShowInfo *rangeShowInfo
+	culprit culprit
 }
 
 // NewContext creates a new Context.
 func NewContext(name, source string, r Ranger) *Context {
-	return &Context{name, source, r.Range(), nil}
+	rg := r.Range()
+	return &Context{name, source, rg, makeCulprit(source, rg)}
 }
 
-// Information about the source range that are needed for showing.
+// Show shows a SourceContext.
-type rangeShowInfo struct {
+func (c *Context) Show(indent string) string {
-	// Head is the piece of text immediately before Culprit, extending to, but
+	return fmt.Sprintf("%s:%s:\n%s%s",
-	// not including the closest line boundary. If Culprit already starts after
+		c.Name, c.culprit.describeStart(), indent+"  ", c.culprit.Show(indent+"  "))
-	// a line boundary, Head is an empty string.
+}
+
+// ShowCompact shows a Context, with no line break between the culprit range
+// description and relevant source excerpt.
+func (c *Context) ShowCompact(indent string) string {
+	desc := fmt.Sprintf("%s:%s: ", c.Name, c.culprit.describeStart())
+	// Extra indent so that following lines line up with the first line.
+	descIndent := strings.Repeat(" ", wcwidth.Of(desc))
+	return desc + c.culprit.Show(indent+descIndent)
+}
+
+// Information about the lines that contain the culprit.
+type culprit struct {
+	// The actual culprit text.
+	Body string
+	// Text before Body on its first line.
 	Head string
-	// Culprit is Source[Begin:End], with any trailing newlines stripped.
+	// Text after Body on its last line.
-	Culprit string
-	// Tail is the piece of text immediately after Culprit, extending to, but
-	// not including the closet line boundary. If Culprit already ends before a
-	// line boundary, Tail is an empty string.
 	Tail string
-	// BeginLine is the (1-based) line number that the first character of Culprit is on.
+	// 1-based line and column numbers of the start position.
-	BeginLine int
+	StartLine, StartCol int
-	// EndLine is the (1-based) line number that the last character of Culprit is on.
-	EndLine int
 }
 
-// Variables controlling the style of the culprit.
+func makeCulprit(source string, r Ranging) culprit {
-var (
+	before := source[:r.From]
-	culpritLineBegin   = "\033[1;4m"
+	body := source[r.From:r.To]
-	culpritLineEnd     = "\033[m"
+	after := source[r.To:]
-	culpritPlaceHolder = "^"
-)
-
-func (c *Context) showInfo() *rangeShowInfo {
-	if c.savedShowInfo != nil {
-		return c.savedShowInfo
-	}
-
-	before := c.Source[:c.From]
-	culprit := c.Source[c.From:c.To]
-	after := c.Source[c.To:]
 
 	head := lastLine(before)
-	beginLine := strings.Count(before, "\n") + 1
+	fromLine := strings.Count(before, "\n") + 1
+	fromCol := 1 + wcwidth.Of(head)
 
-	// If the culprit ends with a newline, stripe it. Otherwise, tail is nonempty.
+	// If the culprit ends with a newline, stripe it, and tail is empty.
+	// Otherwise, tail is nonempty.
 	var tail string
-	if strings.HasSuffix(culprit, "\n") {
+	if strings.HasSuffix(body, "\n") {
-		culprit = culprit[:len(culprit)-1]
+		body = body[:len(body)-1]
 	} else {
 		tail = firstLine(after)
 	}
 
-	endLine := beginLine + strings.Count(culprit, "\n")
+	return culprit{body, head, tail, fromLine, fromCol}
-
-	c.savedShowInfo = &rangeShowInfo{head, culprit, tail, beginLine, endLine}
-	return c.savedShowInfo
 }
 
-// Show shows a SourceContext.
+// Variables controlling the style of the culprit.
-func (c *Context) Show(sourceIndent string) string {
+var (
-	if err := c.checkPosition(); err != nil {
+	culpritStart       = "\033[1;4m"
-		return err.Error()
+	culpritEnd         = "\033[m"
-	}
+	culpritPlaceHolder = "^"
-	return (c.Name + ", " + c.lineRange() +
+)
-		"\n" + sourceIndent + c.relevantSource(sourceIndent))
+
+func (cl *culprit) describeStart() string {
+	return fmt.Sprintf("%d:%d", cl.StartLine, cl.StartCol)
 }
 
-// ShowCompact shows a SourceContext, with no line break between the
+func (cl *culprit) Show(indent string) string {
-// source position range description and relevant source excerpt.
+	var sb strings.Builder
-func (c *Context) ShowCompact(sourceIndent string) string {
+	sb.WriteString(cl.Head)
-	if err := c.checkPosition(); err != nil {
-		return err.Error()
-	}
-	desc := c.Name + ", " + c.lineRange() + " "
-	// Extra indent so that following lines line up with the first line.
-	descIndent := strings.Repeat(" ", wcwidth.Of(desc))
-	return desc + c.relevantSource(sourceIndent+descIndent)
-}
 
-func (c *Context) checkPosition() error {
+	body := cl.Body
-	if c.From == -1 {
+	if body == "" {
-		return fmt.Errorf("%s, unknown position", c.Name)
+		body = culpritPlaceHolder
-	} else if c.From < 0 || c.To > len(c.Source) || c.From > c.To {
-		return fmt.Errorf("%s, invalid position %d-%d", c.Name, c.From, c.To)
-	}
-	return nil
-}
-
-func (c *Context) lineRange() string {
-	info := c.showInfo()
-
-	if info.BeginLine == info.EndLine {
-		return fmt.Sprintf("line %d:", info.BeginLine)
-	}
-	return fmt.Sprintf("line %d-%d:", info.BeginLine, info.EndLine)
-}
-
-func (c *Context) relevantSource(sourceIndent string) string {
-	info := c.showInfo()
-
-	var buf bytes.Buffer
-	buf.WriteString(info.Head)
-
-	culprit := info.Culprit
-	if culprit == "" {
-		culprit = culpritPlaceHolder
 	}
 
-	for i, line := range strings.Split(culprit, "\n") {
+	for i, line := range strings.Split(body, "\n") {
 		if i > 0 {
-			buf.WriteByte('\n')
+			sb.WriteByte('\n')
-			buf.WriteString(sourceIndent)
+			sb.WriteString(indent)
 		}
-		buf.WriteString(culpritLineBegin)
+		sb.WriteString(culpritStart)
-		buf.WriteString(line)
+		sb.WriteString(line)
-		buf.WriteString(culpritLineEnd)
+		sb.WriteString(culpritEnd)
 	}
 
-	buf.WriteString(info.Tail)
+	sb.WriteString(cl.Tail)
-	return buf.String()
+	return sb.String()
 }
 
 func firstLine(s string) string {

pkg/diag/context_test.go

@@ -18,69 +18,47 @@ var sourceRangeTests = []struct {
 		Context: contextInParen("[test]", "echo (bad)"),
 		Indent:  "_",
 
-		WantShow: lines(
+		WantShow: dedent(`
-			"[test], line 1:",
+			[test]:1:6:
-			"_echo <(bad)>",
+			_  echo <(bad)>`),
-		),
+		WantShowCompact: "[test]:1:6: echo <(bad)>",
-		WantShowCompact: "[test], line 1: echo <(bad)>",
 	},
 	{
 		Name:    "multi-line culprit",
 		Context: contextInParen("[test]", "echo (bad\nbad)\nmore"),
 		Indent:  "_",
 
-		WantShow: lines(
+		WantShow: dedent(`
-			"[test], line 1-2:",
+			[test]:1:6:
-			"_echo <(bad>",
+			_  echo <(bad>
-			"_<bad)>",
+			_  <bad)>`),
-		),
+		WantShowCompact: dedent(`
-		WantShowCompact: lines(
+			[test]:1:6: echo <(bad>
-			"[test], line 1-2: echo <(bad>",
+			_            <bad)>`),
-			"_                  <bad)>",
-		),
 	},
 	{
-		Name: "trailing newline in culprit is removed",
+		Name:    "trailing newline in culprit is removed",
-		//                             012345678 9
 		Context: NewContext("[test]", "echo bad\n", Ranging{5, 9}),
 		Indent:  "_",
 
-		WantShow: lines(
+		WantShow: dedent(`
-			"[test], line 1:",
+			[test]:1:6:
-			"_echo <bad>",
+			_  echo <bad>`),
-		),
+		WantShowCompact: "[test]:1:6: echo <bad>",
-		WantShowCompact: lines(
-			"[test], line 1: echo <bad>",
-		),
 	},
 	{
-		Name: "empty culprit",
+		Name:    "empty culprit",
-		//                             012345
 		Context: NewContext("[test]", "echo x", Ranging{5, 5}),
 
-		WantShow: lines(
+		WantShow: dedent(`
-			"[test], line 1:",
+			[test]:1:6:
-			"echo <^>x",
+			  echo <^>x`),
-		),
+		WantShowCompact: "[test]:1:6: echo <^>x",
-		WantShowCompact: "[test], line 1: echo <^>x",
-	},
-	{
-		Name:            "unknown culprit range",
-		Context:         NewContext("[test]", "echo", Ranging{-1, -1}),
-		WantShow:        "[test], unknown position",
-		WantShowCompact: "[test], unknown position",
-	},
-	{
-		Name:            "invalid culprit range",
-		Context:         NewContext("[test]", "echo", Ranging{2, 1}),
-		WantShow:        "[test], invalid position 2-1",
-		WantShowCompact: "[test], invalid position 2-1",
 	},
 }
 
 func TestContext(t *testing.T) {
-	culpritLineBegin = "<"
+	setCulpritMarkers(t, "<", ">")
-	culpritLineEnd = ">"
 	for _, test := range sourceRangeTests {
 		t.Run(test.Name, func(t *testing.T) {
 			gotShow := test.Context.Show(test.Indent)

pkg/diag/error.go

@@ -15,9 +15,8 @@ type Error struct {
 
 // Error returns a plain text representation of the error.
 func (e *Error) Error() string {
-	// TODO: Include line and column numbers instead of byte indices.
+	return fmt.Sprintf("%s: %s:%s: %s",
-	return fmt.Sprintf("%s: %d-%d in %s: %s",
+		e.Type, e.Context.Name, e.Context.culprit.describeStart(), e.Message)
-		e.Type, e.Context.From, e.Context.To, e.Context.Name, e.Message)
 }
 
 // Range returns the range of the error.
@@ -25,8 +24,14 @@ func (e *Error) Range() Ranging {
 	return e.Context.Range()
 }
 
+var (
+	messageStart = "\033[31;1m"
+	messageEnd   = "\033[m"
+)
+
 // Show shows the error.
 func (e *Error) Show(indent string) string {
-	header := fmt.Sprintf("%s: \033[31;1m%s\033[m\n", strutil.Title(e.Type), e.Message)
+	return fmt.Sprintf("%s: %s%s%s\n%s%s",
-	return header + e.Context.ShowCompact(indent+"  ")
+		strutil.Title(e.Type), messageStart, e.Message, messageEnd,
+		indent+"  ", e.Context.ShowCompact(indent+"  "))
 }

pkg/diag/error_test.go

@@ -1,15 +1,12 @@
 package diag
 
 import (
-	"strings"
 	"testing"
-
-	"src.elv.sh/pkg/testutil"
 )
 
 func TestError(t *testing.T) {
-	testutil.Set(t, &culpritLineBegin, "<")
+	setCulpritMarkers(t, "<", ">")
-	testutil.Set(t, &culpritLineEnd, ">")
+	setMessageMarkers(t, "{", "}")
 
 	err := &Error{
 		Type:    "some error",
@@ -17,7 +14,7 @@ func TestError(t *testing.T) {
 		Context: *contextInParen("[test]", "echo (x)"),
 	}
 
-	wantErrorString := "some error: 5-8 in [test]: bad list"
+	wantErrorString := "some error: [test]:1:6: bad list"
 	if gotErrorString := err.Error(); gotErrorString != wantErrorString {
 		t.Errorf("Error() -> %q, want %q", gotErrorString, wantErrorString)
 	}
@@ -27,16 +24,11 @@ func TestError(t *testing.T) {
 		t.Errorf("Range() -> %v, want %v", gotRanging, wantRanging)
 	}
 
-	wantShow := lines(
+	// Type is capitalized in return value of Show
-		// Type is capitalized in return value of Show
+	wantShow := dedent(`
-		"Some error: \033[31;1mbad list\033[m",
+		Some error: {bad list}
-		"[test], line 1: echo <(x)>",
+		  [test]:1:6: echo <(x)>`)
-	)
 	if gotShow := err.Show(""); gotShow != wantShow {
 		t.Errorf("Show() -> %q, want %q", gotShow, wantShow)
 	}
 }
-
-func lines(lines ...string) string {
-	return strings.Join(lines, "\n")
-}

pkg/diag/testutil_test.go

@@ -0,0 +1,19 @@
+package diag
+
+import (
+	"testing"
+
+	"src.elv.sh/pkg/testutil"
+)
+
+var dedent = testutil.Dedent
+
+func setCulpritMarkers(t *testing.T, start, end string) {
+	testutil.Set(t, &culpritStart, start)
+	testutil.Set(t, &culpritEnd, end)
+}
+
+func setMessageMarkers(t *testing.T, start, end string) {
+	testutil.Set(t, &messageStart, start)
+	testutil.Set(t, &messageEnd, end)
+}

pkg/edit/filter/compile_test.go

@@ -58,7 +58,7 @@ func TestCompile(t *testing.T) {
 			Filter("[re '[']").
 			DoesNotCompile("error parsing regexp: missing closing ]: `[`"),
 		That("invalid syntax results in parse error").
-			Filter("[and").DoesNotParse("parse error: 4-4 in [filter]: should be ']'"),
+			Filter("[and").DoesNotParse("parse error: [filter]:1:5: should be ']'"),
 
 		// Unsupported for now, but may be in future
 		That("options are not supported yet").

pkg/edit/highlight_test.go

@@ -30,7 +30,7 @@ func TestHighlighter(t *testing.T) {
 	f.TestTTY(t,
 		"~> put $truex", Styles,
 		"   vvv ??????", term.DotHere, "\n",
-		"compilation error: 4-10 in [interactive]: variable $truex not found",
+		"compilation error: [interactive]:1:5: variable $truex not found",
 	)
 }
 

pkg/eval/compiler.go

@@ -137,8 +137,7 @@ func (cp *compiler) deprecate(r diag.Ranger, msg string, minLevel int) {
 	if prog.DeprecationLevel >= minLevel && cp.deprecations.register(dep) {
 		err := diag.Error{
 			Type: "deprecation", Message: msg,
-			Context: diag.Context{
+			Context: *diag.NewContext(cp.srcMeta.Name, cp.srcMeta.Code, r.Range())}
-				Name: cp.srcMeta.Name, Source: cp.srcMeta.Code, Ranging: r.Range()}}
 		fmt.Fprintln(cp.warn, err.Show(""))
 	}
 }

pkg/eval/exception.go

@@ -91,7 +91,7 @@ func (exc *exception) Show(indent string) string {
 			buf.WriteString(indent + "Traceback:")
 			for tb := exc.stackTrace; tb != nil; tb = tb.Next {
 				buf.WriteString("\n" + indent + "  ")
-				buf.WriteString(tb.Head.Show(indent + "    "))
+				buf.WriteString(tb.Head.Show(indent + "  "))
 			}
 		}
 	}