pkg/shell: Print both parse and compile errors when using -compileonly -json.

The new (*Evaler).Check method allows the implementation to be simpler.

Also test the JSON format feature in an integration test instead of unit test;
this makes it unnecessary for the eval package to expose NewCompilationError.

NEXT-RELEASE.md

@@ -57,6 +57,12 @@ New features in the interactive editor:
 
 -   SGR escape sequences written from the prompt callback are now supported.
 
+New features in the main program:
+
+-   When using `-compileonly` to check Elvish sources that contain parse errors,
+    Elvish will still try to compile the source code and print out compilation
+    errors.
+
 # Notable bugfixes
 
 -   Using large lists that contain `$nil` no longer crashes Elvish.

pkg/eval/compilation_error.go

@@ -1,20 +0,0 @@
-package eval
-
-import "github.com/elves/elvish/pkg/diag"
-
-const compilationErrorType = "compilation error"
-
-// NewCompilationError creates a new compilation error.
-func NewCompilationError(message string, context diag.Context) error {
-	return &diag.Error{
-		Type: compilationErrorType, Message: message, Context: context}
-}
-
-// GetCompilationError returns a *diag.Error if the given value is a compilation
-// error. Otherwise it returns nil.
-func GetCompilationError(e interface{}) *diag.Error {
-	if e, ok := e.(*diag.Error); ok && e.Type == compilationErrorType {
-		return e
-	}
-	return nil
-}

pkg/eval/compiler.go

@@ -68,12 +68,24 @@ func compile(b, g *staticNs, tree parse.Tree, w io.Writer) (op Op, err error) {
 	return Op{scopeOp, tree.Source}, nil
 }
 
+const compilationErrorType = "compilation error"
+
 func (cp *compiler) errorpf(r diag.Ranger, format string, args ...interface{}) {
 	// The panic is caught by the recover in compile above.
-	panic(NewCompilationError(fmt.Sprintf(format, args...),
-		*diag.NewContext(cp.srcMeta.Name, cp.srcMeta.Code, r)))
+	panic(&diag.Error{
+		Type:    compilationErrorType,
+		Message: fmt.Sprintf(format, args...),
+		Context: *diag.NewContext(cp.srcMeta.Name, cp.srcMeta.Code, r)})
 }
 
+// GetCompilationError returns a *diag.Error if the given value is a compilation
+// error. Otherwise it returns nil.
+func GetCompilationError(e interface{}) *diag.Error {
+	if e, ok := e.(*diag.Error); ok && e.Type == compilationErrorType {
+		return e
+	}
+	return nil
+}
 func (cp *compiler) thisScope() *staticNs {
 	return cp.scopes[len(cp.scopes)-1]
 }

pkg/shell/errors_to_json.go

@@ -1,46 +0,0 @@
-package shell
-
-import (
-	"encoding/json"
-
-	"github.com/elves/elvish/pkg/diag"
-	"github.com/elves/elvish/pkg/parse"
-)
-
-// An auxiliary struct for converting errors with diagnostics information to JSON.
-type errorInJSON struct {
-	FileName string `json:"fileName"`
-	Start    int    `json:"start"`
-	End      int    `json:"end"`
-	Message  string `json:"message"`
-}
-
-// An auxiliary struct for converting errors with only a message to JSON.
-type simpleErrorInJSON struct {
-	Message string `json:"message"`
-}
-
-// Converts the error into JSON.
-func errorToJSON(err error) []byte {
-	var e interface{}
-	switch err := err.(type) {
-	case *diag.Error:
-		e = []interface{}{
-			errorInJSON{err.Context.Name, err.Context.From, err.Context.To, err.Message},
-		}
-	case *parse.Error:
-		var errArr []errorInJSON
-		for _, v := range err.Entries {
-			errArr = append(errArr,
-				errorInJSON{v.Context.Name, v.Context.From, v.Context.To, v.Message})
-		}
-		e = errArr
-	default:
-		e = []interface{}{simpleErrorInJSON{err.Error()}}
-	}
-	jsonError, errMarshal := json.Marshal(e)
-	if errMarshal != nil {
-		return []byte(`[{"message":"Unable to convert the errors to JSON"}]`)
-	}
-	return jsonError
-}

pkg/shell/errors_to_json_test.go

@@ -1,34 +0,0 @@
-package shell
-
-import (
-	"errors"
-	"testing"
-
-	"github.com/elves/elvish/pkg/diag"
-	"github.com/elves/elvish/pkg/eval"
-	"github.com/elves/elvish/pkg/parse"
-	"github.com/elves/elvish/pkg/tt"
-)
-
-func TestErrorsToJSON(t *testing.T) {
-	tt.Test(t, tt.Fn("errorToJSON", errorToJSON), tt.Table{
-		tt.Args(eval.NewCompilationError(
-			"ERR",
-			diag.Context{Name: "file", Ranging: diag.Ranging{From: 5, To: 7}}),
-		).Rets(
-			[]byte(`[{"fileName":"file","start":5,"end":7,"message":"ERR"}]`),
-		),
-		tt.Args(&parse.Error{Entries: []*diag.Error{
-			{
-				Message: "ERR1",
-				Context: diag.Context{Name: "file1", Ranging: diag.Ranging{From: 5, To: 7}}},
-			{
-				Message: "ERR2",
-				Context: diag.Context{Name: "file2", Ranging: diag.Ranging{From: 15, To: 16}}},
-		}}).Rets(
-			[]byte(`[{"fileName":"file1","start":5,"end":7,"message":"ERR1"},` +
-				`{"fileName":"file2","start":15,"end":16,"message":"ERR2"}]`),
-		),
-		tt.Args(errors.New("ERR")).Rets([]byte(`[{"message":"ERR"}]`)),
-	})
-}

pkg/shell/script.go

@@ -1,6 +1,7 @@
 package shell
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -52,18 +53,17 @@ func Script(fds [3]*os.File, args []string, cfg *ScriptConfig) int {
 	src := parse.Source{Name: name, Code: code, IsFile: true}
 	if cfg.CompileOnly {
 		parseErr, compileErr := ev.Check(src, fds[2])
-		var err error
-		if parseErr != nil {
-			err = parseErr
-		} else if compileErr != nil {
-			err = compileErr
-		}
-		if err != nil {
-			if cfg.JSON {
-				fmt.Fprintf(fds[1], "%s\n", errorToJSON(err))
-			} else {
-				diag.ShowError(fds[2], err)
+		if cfg.JSON {
+			fmt.Fprintf(fds[1], "%s\n", errorsToJSON(parseErr, compileErr))
+		} else {
+			if parseErr != nil {
+				diag.ShowError(fds[2], parseErr)
 			}
+			if compileErr != nil {
+				diag.ShowError(fds[2], compileErr)
+			}
+		}
+		if parseErr != nil || compileErr != nil {
 			return 2
 		}
 	} else {
@@ -89,3 +89,33 @@ func readFileUTF8(fname string) (string, error) {
 	}
 	return string(bytes), nil
 }
+
+// An auxiliary struct for converting errors with diagnostics information to JSON.
+type errorInJSON struct {
+	FileName string `json:"fileName"`
+	Start    int    `json:"start"`
+	End      int    `json:"end"`
+	Message  string `json:"message"`
+}
+
+// Converts parse and compilation errors into JSON.
+func errorsToJSON(parseErr *parse.Error, compileErr *diag.Error) []byte {
+	var converted []errorInJSON
+	if parseErr != nil {
+		for _, e := range parseErr.Entries {
+			converted = append(converted,
+				errorInJSON{e.Context.Name, e.Context.From, e.Context.To, e.Message})
+		}
+	}
+	if compileErr != nil {
+		converted = append(converted,
+			errorInJSON{compileErr.Context.Name,
+				compileErr.Context.From, compileErr.Context.To, compileErr.Message})
+	}
+
+	jsonError, errMarshal := json.Marshal(converted)
+	if errMarshal != nil {
+		return []byte(`[{"message":"Unable to convert the errors to JSON"}]`)
+	}
+	return jsonError
+}

pkg/shell/script_test.go

@@ -40,52 +40,69 @@ func TestScript_Cmd(t *testing.T) {
 	f.TestOut(t, 2, "")
 }
 
-func TestScript_DoesNotCompile(t *testing.T) {
-	f := Setup()
-	defer f.Cleanup()
-
-	ret := Script(f.Fds(), []string{"echo $a"}, &ScriptConfig{Cmd: true})
-
-	if ret != 2 {
-		t.Errorf("got ret %v, want 2", ret)
-	}
-	f.TestOutSnippet(t, 2, "compilation error")
-	f.TestOut(t, 1, "")
+var scriptErrorTests = []struct {
+	name        string
+	code        string
+	compileOnly bool
+	json        bool
+	wantExit    int
+	wantOut     string
+	wantErr     string
+}{
+	{name: "parse error",
+		code:     "echo [",
+		wantExit: 2,
+		wantErr:  "parse error"},
+	{name: "parse error with -compileonly and -json",
+		code: "echo [", compileOnly: true, json: true,
+		wantExit: 2,
+		wantOut:  `[{"fileName":"code from -c","start":6,"end":6,"message":"should be ']'"}]` + "\n"},
+	{name: "multiple parse errors with -compileonly and -json",
+		code: "echo [{", compileOnly: true, json: true,
+		wantExit: 2,
+		wantOut:  `[{"fileName":"code from -c","start":7,"end":7,"message":"should be ',' or '}'"},{"fileName":"code from -c","start":7,"end":7,"message":"should be ']'"}]` + "\n"},
+	{name: "compile error",
+		code:     "echo $a",
+		wantExit: 2,
+		wantErr:  "compilation error"},
+	{name: "compile error with -compileonly and -json",
+		code: "echo $a", compileOnly: true, json: true,
+		wantExit: 2,
+		wantOut:  `[{"fileName":"code from -c","start":5,"end":7,"message":"variable $a not found"}]` + "\n"},
+	{name: "parse error and compile error with -compileonly and -json",
+		code: "echo [$a", compileOnly: true, json: true,
+		wantExit: 2,
+		wantOut:  `[{"fileName":"code from -c","start":8,"end":8,"message":"should be ']'"},{"fileName":"code from -c","start":6,"end":8,"message":"variable $a not found"}]` + "\n"},
+	{name: "exception",
+		code:     "fail failure",
+		wantExit: 2,
+		wantOut:  "",
+		wantErr:  "fail failure"},
+	{name: "exception with -compileonly",
+		code: "fail failure", compileOnly: true,
+		wantExit: 0},
 }
 
-func TestScript_DoesNotCompile_JSON(t *testing.T) {
-	f := Setup()
-	defer f.Cleanup()
-
-	ret := Script(f.Fds(), []string{"echo $a"},
-		&ScriptConfig{Cmd: true, CompileOnly: true, JSON: true})
-
-	if ret != 2 {
-		t.Errorf("got ret %v, want 2", ret)
-	}
-	f.TestOutSnippet(t, 1, "variable $a not found")
-	f.TestOut(t, 2, "")
-}
-
-func TestScript_Exception(t *testing.T) {
-	f := Setup()
-	defer f.Cleanup()
-
-	ret := Script(f.Fds(), []string{"fail failure"}, &ScriptConfig{Cmd: true})
-	if ret != 2 {
-		t.Errorf("got ret %v, want 2", ret)
-	}
-	f.TestOutSnippet(t, 2, "fail failure")
-	f.TestOut(t, 1, "")
-}
-
-func TestScript_Exception_CompileOnly(t *testing.T) {
-	f := Setup()
-	defer f.Cleanup()
-
-	ret := Script(f.Fds(), []string{"fail failure"}, &ScriptConfig{
-		Cmd: true, CompileOnly: true})
-	if ret != 0 {
-		t.Errorf("got ret %v, want 0", ret)
+func TestScript_Error(t *testing.T) {
+	for _, test := range scriptErrorTests {
+		t.Run(test.name, func(t *testing.T) {
+			f := Setup()
+			defer f.Cleanup()
+			exit := Script(f.Fds(), []string{test.code}, &ScriptConfig{
+				Cmd: true, CompileOnly: test.compileOnly, JSON: test.json})
+			if exit != test.wantExit {
+				t.Errorf("got exit code %v, want 2", test.wantExit)
+			}
+			f.TestOut(t, 1, test.wantOut)
+			// When testing stderr output, we either only test that there is no
+			// output at all, or that the output contains a string; we never
+			// test it in full, as it is intended for human consumption and may
+			// change.
+			if test.wantErr == "" {
+				f.TestOut(t, 2, "")
+			} else {
+				f.TestOutSnippet(t, 2, test.wantErr)
+			}
+		})
 	}
 }