Move all variable creations to start of lexical scope.

Commit 8c71635ca3 moved the creation to the start
of pipelines; the approach works for simplistic cases like "x = 1 | nop", but
fails in more complex cases, such as "nop (x = 1) | nop".

The correct place to hoist variable creations is the lexical scope, and this
commit implements this approach.

This fixes #623.

pkg/eval/compile_effect.go

@@ -16,6 +16,35 @@ import (
 	"github.com/xiaq/persistent/hashmap"
 )
 
+// An effectOpBody that creates all variables in a scope before executing the
+// body.
+type scopeOp struct {
+	inner  effectOpBody
+	locals []string
+}
+
+func wrapScopeOp(op effectOp, locals []string) effectOp {
+	return effectOp{scopeOp{op.body, locals}, op.Ranging}
+}
+
+func (op scopeOp) invoke(fm *Frame) error {
+	for _, name := range op.locals {
+		var variable vars.Var
+		if strings.HasSuffix(name, FnSuffix) {
+			val := Callable(nil)
+			variable = vars.FromPtr(&val)
+		} else if strings.HasSuffix(name, NsSuffix) {
+			val := Ns(nil)
+			variable = vars.FromPtr(&val)
+		} else {
+			variable = vars.FromInit(nil)
+		}
+		fm.local[name] = variable
+	}
+
+	return op.inner.invoke(fm)
+}
+
 func (cp *compiler) chunkOp(n *parse.Chunk) effectOp {
 	return makeEffectOp(n, chunkOp{cp.pipelineOps(n.Pipelines)})
 }
@@ -41,16 +70,10 @@ func (op chunkOp) invoke(fm *Frame) error {
 }
 
 func (cp *compiler) pipelineOp(n *parse.Pipeline) effectOp {
-	saveNewLocals := cp.newLocals
-	cp.newLocals = nil
-
 	formOps := cp.formOps(n.Forms)
 
-	newLocals := cp.newLocals
-	cp.newLocals = saveNewLocals
-
 	return makeEffectOp(n,
-		&pipelineOp{n.Background, parse.SourceText(n), formOps, newLocals})
+		&pipelineOp{n.Background, parse.SourceText(n), formOps})
 }
 
 func (cp *compiler) pipelineOps(ns []*parse.Pipeline) []effectOp {
@@ -62,10 +85,9 @@ func (cp *compiler) pipelineOps(ns []*parse.Pipeline) []effectOp {
 }
 
 type pipelineOp struct {
-	bg        bool
+	bg     bool
-	source    string
+	source string
-	subops    []effectOp
+	subops []effectOp
-	newLocals []string
 }
 
 const pipelineChanBufferSize = 32
@@ -75,20 +97,6 @@ func (op *pipelineOp) invoke(fm *Frame) error {
 		return ErrInterrupted
 	}
 
-	for _, name := range op.newLocals {
-		var variable vars.Var
-		if strings.HasSuffix(name, FnSuffix) {
-			val := Callable(nil)
-			variable = vars.FromPtr(&val)
-		} else if strings.HasSuffix(name, NsSuffix) {
-			val := Ns(nil)
-			variable = vars.FromPtr(&val)
-		} else {
-			variable = vars.FromInit(nil)
-		}
-		fm.local[name] = variable
-	}
-
 	if op.bg {
 		fm = fm.fork("background job" + op.source)
 		fm.intCh = nil

pkg/eval/compile_effect_test.go

@@ -108,6 +108,7 @@ func TestCompileEffect(t *testing.T) {
 		// Concurrently creating a new variable and accessing existing variable.
 		// Run with "go test -race".
 		That("x = 1", "put $x | y = (all)").DoesNothing(),
+		That("nop (x = 1) | nop").DoesNothing(),
 
 		// Assignment errors when the RHS errors.
 		That("x = [][1]").Throws(errWithType{errs.OutOfRange{}}, "[][1]"),

pkg/eval/compile_value.go

@@ -471,8 +471,10 @@ func (cp *compiler) lambda(n *parse.Primary) valuesOpBody {
 	for _, optName := range optNames {
 		thisScope.set(optName)
 	}
-
+	savedLocals := cp.pushNewLocals()
-	subop := cp.chunkOp(n.Chunk)
+	chunkOp := cp.chunkOp(n.Chunk)
+	scopeOp := wrapScopeOp(chunkOp, cp.newLocals)
+	cp.newLocals = savedLocals
 
 	// XXX The fiddlings with cp.capture is error-prone.
 	capture := cp.capture
@@ -483,7 +485,7 @@ func (cp *compiler) lambda(n *parse.Primary) valuesOpBody {
 		cp.registerVariableGet(name, nil)
 	}
 
-	return &lambdaOp{argNames, restArgName, optNames, optDefaultOps, capture, subop, cp.srcMeta, n.Range().From, n.Range().To}
+	return &lambdaOp{argNames, restArgName, optNames, optDefaultOps, capture, scopeOp, cp.srcMeta, n.Range().From, n.Range().To}
 }
 
 type lambdaOp struct {

pkg/eval/compiler.go

@@ -18,7 +18,7 @@ type compiler struct {
 	scopes []staticNs
 	// Variables captured from outer scopes.
 	capture staticNs
-	// New variables created within a pipeline.
+	// New variables created in a lexical scope.
 	newLocals []string
 	// Destination of warning messages. This is currently only used for
 	// deprecation messages.
@@ -45,7 +45,18 @@ func compile(b, g staticNs, tree parse.Tree, w io.Writer) (op Op, err error) {
 			panic(r)
 		}
 	}()
-	return Op{cp.chunkOp(tree.Root), tree.Source}, nil
+	savedLocals := cp.pushNewLocals()
+	chunkOp := cp.chunkOp(tree.Root)
+	scopeOp := wrapScopeOp(chunkOp, cp.newLocals)
+	cp.newLocals = savedLocals
+
+	return Op{scopeOp, tree.Source}, nil
+}
+
+func (cp *compiler) pushNewLocals() []string {
+	saved := cp.newLocals
+	cp.newLocals = nil
+	return saved
 }
 
 func (cp *compiler) errorpf(r diag.Ranger, format string, args ...interface{}) {