interp: make REPL stricter about parsing errors

So far the REPL loop was treating any parsing error coming from
go/parser to generate the AST, as having occurred because the source
code was not yet complete (unfinished block). And it was therefore
ignoring all of them.

However, some of these errors are legitimate, and must be caught as soon
as they occur, otherwise the REPL cycle would stay in an errored state
forever (even when the block terminates), without the user getting any
feedback about it.

Therefore, this change adds an extra check when a parsing error occurs,
i.e. it verifies that it looks like an "EOF" error (unfinished block)
before it ignores it (as the user is supposed to terminate the block
eventually). Otherwise the error is treated just like a "non-parsing"
(cfg, gta, ...) error and printed out.

Fixes #637

Author: mpl

interp/interp.go

@@ -14,6 +14,7 @@ import (
 	"runtime"
 	"runtime/debug"
 	"strconv"
+	"strings"
 	"sync"
 	"sync/atomic"
 )
@@ -492,6 +493,15 @@ func (interp *Interpreter) Use(values Exports) {
 	}
 }
 
+type scannerErrors scanner.ErrorList
+
+func (sce scannerErrors) isEOF() bool {
+	for _, v := range sce {
+		return strings.HasSuffix(v.Msg, `, found 'EOF'`)
+	}
+	return true
+}
+
 // REPL performs a Read-Eval-Print-Loop on input reader.
 // Results are printed on output writer.
 func (interp *Interpreter) REPL(in io.Reader, out io.Writer) {
@@ -527,10 +537,13 @@ func (interp *Interpreter) REPL(in io.Reader, out io.Writer) {
 		if err != nil {
 			switch e := err.(type) {
 			case scanner.ErrorList:
-				// Early failure in the scanner: the source is incomplete
-				// and no AST could be produced, neither compiled / run.
-				// Get one more line, and retry
-				continue
+				if scannerErrors(e).isEOF() {
+					// Early failure in the scanner: the source is incomplete
+					// and no AST could be produced, neither compiled / run.
+					// Get one more line, and retry
+					continue
+				}
+				fmt.Fprintln(out, err)
 			case Panic:
 				fmt.Fprintln(out, e.Value)
 				fmt.Fprintln(out, string(e.Stack))
@@ -541,6 +554,7 @@ func (interp *Interpreter) REPL(in io.Reader, out io.Writer) {
 		src = ""
 		prompt(v)
 	}
+	// TODO(mpl): log s.Err() if not nil?
 }
 
 // Repl performs a Read-Eval-Print-Loop on input file descriptor.

interp/interp_eval_test.go

@@ -1,12 +1,17 @@
 package interp_test
 
 import (
+	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"log"
 	"net/http"
+	"os"
 	"reflect"
+	"strconv"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -611,3 +616,117 @@ func assertEval(t *testing.T, i *interp.Interpreter, src, expectedError, expecte
 		t.Fatalf("got %v, want %s", res, expectedRes)
 	}
 }
+
+func TestEvalEOF(t *testing.T) {
+	tests := []struct {
+		desc      string
+		src       []string
+		errorLine int
+	}{
+		{
+			desc: "no error",
+			src: []string{
+				`func main() {`,
+				`println("foo")`,
+				`}`,
+			},
+			errorLine: -1,
+		},
+		{
+			desc: "no parsing error, but block error",
+			src: []string{
+				`func main() {`,
+				`println(foo)`,
+				`}`,
+			},
+			errorLine: 2,
+		},
+		{
+			desc: "parsing error",
+			src: []string{
+				`func main() {`,
+				`println(/foo)`,
+				`}`,
+			},
+			errorLine: 1,
+		},
+	}
+
+	for it, test := range tests {
+		i := interp.New(interp.Options{})
+		var stderr bytes.Buffer
+		safeStderr := &safeBuffer{buf: &stderr}
+		pin, pout := io.Pipe()
+		defer func() {
+			// Closing the pipe also takes care of making i.REPL terminate,
+			// hence freeing its goroutine.
+			_ = pin.Close()
+			_ = pout.Close()
+		}()
+
+		go func() {
+			i.REPL(pin, safeStderr)
+		}()
+		for k, v := range test.src {
+			if _, err := pout.Write([]byte(v + "\n")); err != nil {
+				t.Error(err)
+			}
+			Sleep(100 * time.Millisecond)
+
+			errMsg := safeStderr.String()
+			if k == test.errorLine {
+				if errMsg == "" {
+					t.Fatalf("%d: statement %q should have produced an error", it, v)
+				}
+				break
+			}
+			if errMsg != "" {
+				t.Fatalf("%d: unexpected error: %v", it, errMsg)
+			}
+		}
+	}
+}
+
+type safeBuffer struct {
+	mu  sync.RWMutex
+	buf *bytes.Buffer
+}
+
+func (sb *safeBuffer) Read(p []byte) (int, error) {
+	return sb.buf.Read(p)
+}
+
+func (sb *safeBuffer) String() string {
+	sb.mu.RLock()
+	defer sb.mu.RUnlock()
+	return sb.buf.String()
+}
+
+func (sb *safeBuffer) Write(p []byte) (int, error) {
+	sb.mu.Lock()
+	defer sb.mu.Unlock()
+	return sb.buf.Write(p)
+}
+
+const (
+	// CITimeoutMultiplier is the multiplier for all timeouts in the CI.
+	CITimeoutMultiplier = 3
+)
+
+// Sleep pauses the current goroutine for at least the duration d.
+func Sleep(d time.Duration) {
+	d = applyCIMultiplier(d)
+	time.Sleep(d)
+}
+
+func applyCIMultiplier(timeout time.Duration) time.Duration {
+	ci := os.Getenv("CI")
+	if ci == "" {
+		return timeout
+	}
+	b, err := strconv.ParseBool(ci)
+	if err != nil || !b {
+		return timeout
+	}
+	return time.Duration(float64(timeout) * CITimeoutMultiplier)
+}