Make testing compilation errors more deterministic

A year ago I submitted a change to replace AnyError with tests for specific
errors (see https://github.com/elves/elvish/commit/87656c99).  This does
something similar for DoesNotCompile. This ensures the test does what
is implied and makes correlating specific unit tests with compilation
errors easier.

This includes a couple of changes to compilation error messages to improve
readability (IMHO) but those are not the primary purpose of this change.

Related #1560

pkg/eval/builtin_fn_misc_test.go

@@ -53,7 +53,7 @@ func TestEval(t *testing.T) {
 		// Altering variables in the specified namespace.
 		That("var n = (ns [&x=foo]); eval 'set x = bar' &ns=$n; put $n[x]").Puts("bar"),
 		// Newly created variables do not appear in the local namespace.
-		That("eval 'x = foo'; put $x").DoesNotCompile(),
+		That("eval 'x = foo'; put $x").DoesNotCompile("variable $x not found"),
 		// Newly created variables do not alter the specified namespace, either.
 		That("var n = (ns [&]); eval &ns=$n 'var x = foo'; put $n[x]").
 			Throws(vals.NoSuchKey("x"), "$n[x]"),

pkg/eval/builtin_special.go

@@ -133,7 +133,7 @@ func compileDel(cp *compiler, fn *parse.Form) effectOp {
 		}
 		head, indices := cn.Indexings[0].Head, cn.Indexings[0].Indices
 		if head.Type == parse.Variable {
-			cp.errorpf(cn, "arguments to del must drop $")
+			cp.errorpf(cn, "arguments to del must omit the dollar-sign")
 		} else if !parse.ValidLHSVariable(head, false) {
 			cp.errorpf(cn, delArgMsg)
 		}
@@ -279,7 +279,7 @@ func compileUse(cp *compiler, fn *parse.Form) effectOp {
 	switch len(fn.Args) {
 	case 0:
 		end := fn.Head.Range().To
-		cp.errorpf(diag.PointRanging(end), "lack module name")
+		cp.errorpf(diag.PointRanging(end), "use requires a module name")
 	case 1:
 		spec = stringLiteralOrError(cp, fn.Args[0], "module spec")
 		// Use the last path component as the name; for instance, if path =
@@ -291,7 +291,7 @@ func compileUse(cp *compiler, fn *parse.Form) effectOp {
 		name = stringLiteralOrError(cp, fn.Args[1], "module name")
 	default: // > 2
 		cp.errorpf(diag.MixedRanging(fn.Args[2], fn.Args[len(fn.Args)-1]),
-			"superfluous argument(s)")
+			"use has superfluous argument(s)")
 	}
 
 	return useOp{fn.Range(), cp.thisScope().add(name + NsSuffix), spec}

pkg/eval/builtin_special_test.go

@@ -20,11 +20,11 @@ import (
 
 func TestPragma(t *testing.T) {
 	Test(t,
-		That("pragma unknown-command").DoesNotCompile(),
-		That("pragma unknown-command =").DoesNotCompile(),
-		That("pragma unknown-command x").DoesNotCompile(),
-		That("pragma bad-name = some-value").DoesNotCompile(),
-		That("pragma unknown-command = bad").DoesNotCompile(),
+		That("pragma unknown-command").DoesNotCompile("need more arguments"),
+		That("pragma unknown-command =").DoesNotCompile("need more arguments"),
+		That("pragma unknown-command x").DoesNotCompile("must be literal ="),
+		That("pragma bad-name = some-value").DoesNotCompile("unknown pragma bad-name"),
+		That("pragma unknown-command = bad").DoesNotCompile("invalid value for unknown-command: bad"),
 	)
 	// Actual effect of the unknown-command pragma is tested in TestCommand_External
 }
@@ -82,19 +82,19 @@ func TestVar(t *testing.T) {
 			"var x y @z = 1"),
 
 		// Variable name must not be empty
-		That("var ''").DoesNotCompile(),
+		That("var ''").DoesNotCompile("variable name must not be empty"),
 		// Variable name that must be quoted after $ must be quoted
-		That("var a/b").DoesNotCompile(),
+		That("var a/b").DoesNotCompile("lvalue must be valid literal variable names"),
 		// Multiple @ not allowed
-		That("var x @y @z = a b c d").DoesNotCompile(),
+		That("var x @y @z = a b c d").DoesNotCompile("at most one rest variable is allowed"),
 		// Non-local not allowed
-		That("var ns:a").DoesNotCompile(),
+		That("var ns:a").DoesNotCompile("cannot create variable $ns:a; new variables can only be created in the current scope"),
 		// Index not allowed
-		That("var a[0]").DoesNotCompile(),
+		That("var a[0]").DoesNotCompile("new variable $a must not have indices"),
 		// Composite expression not allowed
-		That("var a'b'").DoesNotCompile(),
+		That("var a'b'").DoesNotCompile("lvalue may not be composite expressions"),
 		// Braced lists must not have any indices when used as a lvalue.
-		That("var {a b}[0] = x y").DoesNotCompile(),
+		That("var {a b}[0] = x y").DoesNotCompile("braced list may not have indices when used as lvalue"),
 	)
 }
 
@@ -105,12 +105,12 @@ func TestSet(t *testing.T) {
 		// An empty RHS is technically legal although rarely useful.
 		That("var x; set @x =", "put $x").Puts(vals.EmptyList),
 		// Variable must already exist
-		That("set x = foo").DoesNotCompile(),
+		That("set x = foo").DoesNotCompile("cannot find variable $x"),
 		// List element assignment
 		That("var li = [foo bar]; set li[0] = 233; put $@li").Puts("233", "bar"),
 		// Variable in list assignment must already be defined. Regression test
 		// for b.elv.sh/889.
-		That("set foobarlorem[0] = a").DoesNotCompile(),
+		That("set y[0] = a").DoesNotCompile("cannot find variable $y"),
 		// Map element assignment
 		That("var di = [&k=v]; set di[k] = lorem; set di[k2] = ipsum",
 			"put $di[k] $di[k2]").Puts("lorem", "ipsum"),
@@ -124,16 +124,16 @@ func TestSet(t *testing.T) {
 				ValidLow: "0", ValidHigh: "0", Actual: "1"}, "li[1][2]"),
 
 		// Assignment to read-only var is a compile-time error.
-		That("set nil = 1").DoesNotCompile(),
-		That("var a b; set a true b = 1 2 3").DoesNotCompile(),
-		That("set @true = 1").DoesNotCompile(),
-		That("var r; set true @r = 1").DoesNotCompile(),
-		That("var r; set @r true = 1").DoesNotCompile(),
+		That("set nil = 1").DoesNotCompile("variable $nil is read-only"),
+		That("var a b; set a true b = 1 2 3").DoesNotCompile("variable $true is read-only"),
+		That("set @true = 1").DoesNotCompile("variable $true is read-only"),
+		That("var r; set true @r = 1").DoesNotCompile("variable $true is read-only"),
+		That("var r; set @r true = 1").DoesNotCompile("variable $true is read-only"),
 
 		// Error conditions already covered by TestVar are not repeated.
 
 		// = is required.
-		That("var x; set x").DoesNotCompile(),
+		That("var x; set x").DoesNotCompile("need = and right-hand-side"),
 
 		// set a non-exist environment
 		That("has-env X; set E:X = x; get-env X; unset-env X").
@@ -157,8 +157,8 @@ func TestTmp(t *testing.T) {
 		That("var x = foo; put $x; { tmp x = bar; put $x }; put $x").
 			Puts("foo", "bar", "foo"),
 
-		That("var x; tmp x = y").DoesNotCompile(),
-		That("{ tmp x = y }").DoesNotCompile(),
+		That("var x; tmp x = y").DoesNotCompile("tmp may only be used inside a function"),
+		That("{ tmp x = y }").DoesNotCompile("cannot find variable $x"),
 
 		That("has-env X; { tmp E:X = y; put $E:X }; has-env X; put $E:X").
 			Puts(false, "y", false, ""),
@@ -206,7 +206,7 @@ func TestDel(t *testing.T) {
 	Test(t,
 		// Deleting variable
 		That("var x = 1; del x").DoesNothing(),
-		That("var x = 1; del x; echo $x").DoesNotCompile(),
+		That("var x = 1; del x; echo $x").DoesNotCompile("variable $x not found"),
 		// Deleting environment variable
 		That("has-env TEST_ENV", "del E:TEST_ENV", "has-env TEST_ENV").Puts(true, false),
 		// Deleting variable whose name contains special characters
@@ -218,21 +218,21 @@ func TestDel(t *testing.T) {
 		// Error cases
 
 		// Deleting nonexistent variable
-		That("del x").DoesNotCompile(),
+		That("del x").DoesNotCompile("no variable $x"),
 		// Deleting element of nonexistent variable
-		That("del x[0]").DoesNotCompile(),
+		That("del x[0]").DoesNotCompile("no variable $x"),
 		// Deleting variable in non-local namespace
-		That("var a: = (ns [&b=$nil])", "del a:b").DoesNotCompile(),
+		That("var a: = (ns [&b=$nil])", "del a:b").DoesNotCompile("only variables in the local scope or E: can be deleted"),
 		// Variable name given with $
-		That("var x = 1; del $x").DoesNotCompile(),
+		That("var x = 1; del $x").DoesNotCompile("arguments to del must omit the dollar-sign"),
 		// Variable name not given as a single primary expression
-		That("var ab = 1; del a'b'").DoesNotCompile(),
+		That("var ab = 1; del a'b'").DoesNotCompile("arguments to del must be variable or variable elements"),
 		// Variable name not a string
-		That("del [a]").DoesNotCompile(),
+		That("del [a]").DoesNotCompile("arguments to del must be variable or variable elements"),
 		// Variable name has sigil
-		That("var x = []; del @x").DoesNotCompile(),
+		That("var x = []; del @x").DoesNotCompile("arguments to del must be variable or variable elements"),
 		// Variable name not quoted when it should be
-		That("var 'a/b' = foo; del a/b").DoesNotCompile(),
+		That("var 'a/b' = foo; del a/b").DoesNotCompile("arguments to del must be variable or variable elements"),
 
 		// Index is multiple values
 		That("var x = [&k1=v1 &k2=v2]", "del x[k1 k2]").Throws(
@@ -301,8 +301,8 @@ func TestCoalesce(t *testing.T) {
 func TestSpecialFormThunks(t *testing.T) {
 	// Regression test for b.elv.sh/1456
 	Test(t,
-		That("for x [] {|arg| }").DoesNotCompile(),
-		That("for x [] {|&opt=val| }").DoesNotCompile(),
+		That("for x [] {|arg| }").DoesNotCompile("for body must not have arguments"),
+		That("for x [] {|&opt=val| }").DoesNotCompile("for body must not have options"),
 		// The other special forms use the same utility under the hood and are
 		// not repeated
 	)
@@ -345,13 +345,13 @@ func TestTry(t *testing.T) {
 		That("try { fail tr } except { put bad }").
 			Puts("bad").PrintsStderrWith("deprecated"),
 		// Must have catch or finally
-		That("try { fail tr }").DoesNotCompile(),
+		That("try { fail tr }").DoesNotCompile("try must be followed by a catch block or a finally block"),
 		// Rest variable not allowed
-		That("try { nop } catch @a { }").DoesNotCompile(),
+		That("try { nop } catch @a { }").DoesNotCompile("rest variable not allowed"),
 
 		// A readonly var as a target for the "catch" clause is a compile-time
 		// error.
-		That("try { fail reason } catch nil { }").DoesNotCompile(),
+		That("try { fail reason } catch nil { }").DoesNotCompile("variable $nil is read-only"),
 		That("try { fail reason } catch x { }").DoesNothing(),
 
 		// A quoted var name, that would be invalid as a bareword, should be allowed as the referent
@@ -407,10 +407,10 @@ func TestFor(t *testing.T) {
 		That("for x [a] { fail foo }").Throws(FailError{"foo"}),
 
 		// More than one iterator.
-		That("for {x,y} [] { }").DoesNotCompile(),
+		That("for {x,y} [] { }").DoesNotCompile("must be exactly one lvalue"),
 		// Invalid for loop lvalue. You can't use a var in a namespace other
 		// than the local namespace as the lvalue in a for loop.
-		That("for no-such-namespace:x [a b] { }").DoesNotCompile(),
+		That("for no-such-namespace:x [a b] { }").DoesNotCompile("cannot create variable $no-such-namespace:x; new variables can only be created in the current scope"),
 		// Exception with the variable
 		That("var a: = (ns [&])", "for a:b [] { }").Throws(
 			ErrorWithMessage("no variable $a:b"),
@@ -513,7 +513,7 @@ func TestUse(t *testing.T) {
 		That(`use lorem; put $lorem:name`).Puts("lorem"),
 		// imports are lexically scoped
 		// TODO: Support testing for compilation error
-		That(`{ use lorem }; put $lorem:name`).DoesNotCompile(),
+		That(`{ use lorem }; put $lorem:name`).DoesNotCompile("variable $lorem:name not found"),
 
 		// prefers lib dir that appear earlier
 		That("use shadow").Puts("lib1"),
@@ -560,8 +560,8 @@ func TestUse(t *testing.T) {
 		That("use non-existent").Throws(ErrorWithMessage("no such module: non-existent")),
 
 		// Wrong uses of "use".
-		That("use").DoesNotCompile(),
-		That("use a b c").DoesNotCompile(),
+		That("use").DoesNotCompile("use requires a module name"),
+		That("use a b c").DoesNotCompile("use has superfluous argument(s)"),
 	)
 }
 

pkg/eval/compile_effect_test.go

@@ -171,7 +171,8 @@ func TestCommand_LegacyTemporaryAssignment(t *testing.T) {
 			Puts("b", "c").PrintsStderrWith("deprecated"),
 		// Using syntax of temporary assignment for non-temporary assignment no
 		// longer compiles
-		That("x=y").DoesNotCompile(),
+		That("x=y").DoesNotCompile(
+			MatchingRegexp{Pattern: "^using the syntax of temporary assignment"}),
 	)
 }
 

pkg/eval/compile_effect_unix_test.go

@@ -56,10 +56,10 @@ func TestCommand_External(t *testing.T) {
 		That("./foo 5</dev/null").Prints("foo\n"),
 
 		// Using pragma to allow or disallow implicit searched commands
-		That("pragma unknown-command = disallow", "hello").DoesNotCompile(),
+		That("pragma unknown-command = disallow", "hello").DoesNotCompile("unknown command disallowed by current pragma"),
 		That("pragma unknown-command = external", "hello").Prints("hello\n"),
 		// Pragma applies to subscope
-		That("pragma unknown-command = disallow", "{ hello }").DoesNotCompile(),
+		That("pragma unknown-command = disallow", "{ hello }").DoesNotCompile("unknown command disallowed by current pragma"),
 		// Explicit uses with e: is always allowed
 		That("pragma unknown-command = disallow", "e:hello").Prints("hello\n"),
 		// Relative external commands are always allowed

pkg/eval/compile_value.go

@@ -392,7 +392,7 @@ func (cp *compiler) lambda(n *parse.Primary) valuesOp {
 			}
 			if sigil == "@" {
 				if restArg != -1 {
-					cp.errorpf(arg, "only one argument may have @")
+					cp.errorpf(arg, "only one argument may have @ prefix")
 				}
 				restArg = i
 			}

pkg/eval/compile_value_test.go

@@ -129,7 +129,7 @@ func TestTilde_ErrorForCurrentUser(t *testing.T) {
 
 func TestWildcard(t *testing.T) {
 	Test(t,
-		That("put ***").DoesNotCompile(),
+		That("put ***").DoesNotCompile("bad wildcard: \"***\""),
 	)
 	// More tests in glob_test.go
 }
@@ -155,8 +155,8 @@ func TestVariableUse(t *testing.T) {
 	Test(t,
 		That("var x = foo", "put $x").Puts("foo"),
 		// Must exist before use
-		That("put $x").DoesNotCompile(),
-		That("put $x[0]").DoesNotCompile(),
+		That("put $x").DoesNotCompile("variable $x not found"),
+		That("put $x[0]").DoesNotCompile("variable $x not found"),
 		// Compounding
 		That("var x = SHELL", "put 'WOW, SUCH '$x', MUCH COOL'\n").
 			Puts("WOW, SUCH SHELL, MUCH COOL"),
@@ -174,7 +174,7 @@ func TestVariableUse(t *testing.T) {
 		// exists.
 		That("put $true").Puts(true),
 		// Names like $:foo are reserved for now.
-		That("var x = val; put $:x").DoesNotCompile(),
+		That("var x = val; put $:x").DoesNotCompile("variable $:x not found"),
 
 		// Pseudo-namespace E: provides read-write access to environment
 		// variables. Colons inside the name are supported.
@@ -238,7 +238,7 @@ func TestClosure(t *testing.T) {
 		// Option default value.
 		That("{|a &k=v| put $a $k } foo").Puts("foo", "v"),
 		// Option must have default value
-		That("{|&k| }").DoesNotCompile(),
+		That("{|&k| }").DoesNotCompile("option must have default value"),
 		// Exception when evaluating option default value.
 		That("{|&a=[][0]| }").Throws(ErrorWithType(errs.OutOfRange{}), "[][0]"),
 		// Option default value must be one value.
@@ -247,15 +247,15 @@ func TestClosure(t *testing.T) {
 			"(put foo bar)"),
 
 		// Argument name must be unqualified.
-		That("{|a:b| }").DoesNotCompile(),
+		That("{|a:b| }").DoesNotCompile("argument name must be unqualified"),
 		// Argument name must not be empty.
-		That("{|''| }").DoesNotCompile(),
-		That("{|@| }").DoesNotCompile(),
+		That("{|''| }").DoesNotCompile("argument name must not be empty"),
+		That("{|@| }").DoesNotCompile("argument name must not be empty"),
 		// Option name must be unqualified.
-		That("{|&a:b=1| }").DoesNotCompile(),
+		That("{|&a:b=1| }").DoesNotCompile("option name must be unqualified"),
 		// Option name must not be empty.
-		That("{|&''=b| }").DoesNotCompile(),
+		That("{|&''=b| }").DoesNotCompile("option name must not be empty"),
 		// Should not have multiple rest arguments.
-		That("{|@a @b| }").DoesNotCompile(),
+		That("{|@a @b| }").DoesNotCompile("only one argument may have @ prefix"),
 	)
 }

pkg/eval/eval_test.go

@@ -61,7 +61,7 @@ func TestMultipleEval(t *testing.T) {
 		That("fn f { put old }").Then("fn f { put new }").Then("f").
 			Puts("new"),
 		// Variable deletion. Regression test for #1213.
-		That("var x = foo").Then("del x").Then("put $x").DoesNotCompile(),
+		That("var x = foo").Then("del x").Then("put $x").DoesNotCompile("variable $x not found"),
 	)
 }
 

pkg/eval/evaltest/evaltest.go

@@ -17,6 +17,7 @@ package evaltest
 
 import (
 	"bytes"
+	"fmt"
 	"os"
 	"reflect"
 	"strings"
@@ -122,9 +123,16 @@ func (c Case) Throws(reason error, stacks ...string) Case {
 }
 
 // DoesNotCompile returns an altered Case that requires the source code to fail
-// compilation.
-func (c Case) DoesNotCompile() Case {
-	c.want.CompilationError = anyError{}
+// compilation with a specific error. It accepts a string or a MatchingRegexp{}.
+func (c Case) DoesNotCompile(ce any) Case {
+	switch v := ce.(type) {
+	case string:
+		c.want.CompilationError = stringError{isRegexp: false, str: v}
+	case MatchingRegexp:
+		c.want.CompilationError = stringError{isRegexp: true, str: v.Pattern}
+	default:
+		panic(fmt.Sprintf("unrecognized DoesNotCompile() arg type: %T", ce))
+	}
 	return c
 }
 

pkg/eval/evaltest/matchers.go

@@ -42,12 +42,33 @@ func matchRegexp(p, s string) bool {
 
 type errorMatcher interface{ matchError(error) bool }
 
-// An errorMatcher for any error.
-type anyError struct{}
+// An errorMatcher for a string literal or regexp.
+type stringError struct {
+	isRegexp bool
+	str      string
+}
 
-func (anyError) Error() string { return "any error" }
+func (se stringError) Error() string {
+	if !se.isRegexp {
+		return "literal: " + se.str
+	}
+	return "regexp: " + se.str
+}
 
-func (anyError) matchError(e error) bool { return e != nil }
+func (se stringError) matchError(e error) bool {
+	ce := eval.GetCompilationError(e)
+	if ce == nil {
+		return false
+	}
+	if !se.isRegexp {
+		return ce.Message == se.str
+	}
+	matched, err := regexp.MatchString(se.str, ce.Message)
+	if err != nil {
+		panic(err)
+	}
+	return matched
+}
 
 // An errorMatcher for exceptions.
 type exc struct {

pkg/eval/ns_test.go

@@ -25,6 +25,6 @@ func TestNs(t *testing.T) {
 
 func TestBuiltinFunctionsReadOnly(t *testing.T) {
 	Test(t,
-		That("set return~ = { }").DoesNotCompile(),
+		That("set return~ = { }").DoesNotCompile("variable $return~ is read-only"),
 	)
 }