pkg/eval: Delay initialization of variable slots to declaration time.

Initialization of variable slots were "hoisted" to the beginning of each scope.
This commit moves that initialization to the command that declares the variable,
which make it easier to resolve #1257 and #1235.

Also properly handle variables that are created as part of a temporary
assignment, addressing #532.

There are some failing tests, which will be fixed in upcoming commits.

pkg/eval/builtin_special.go

@@ -81,7 +81,7 @@ func compileVar(cp *compiler, fn *parse.Form) effectOp {
 	if eqIndex == -1 {
 		cp.parseCompoundLValues(fn.Args, newLValue)
 		// Just create new variables, nothing extra to do at runtime.
-		return nopOp{}
+		return &newLocalsOp{fn.Range(), cp.parseCompoundLValues(fn.Args, newLValue)}
 	}
 	// Compile rhs before lhs before many potential shadowing
 	var rhs valuesOp
@@ -255,14 +255,16 @@ func (op fnOp) exec(fm *Frame) Exception {
 	// Initialize the function variable with the builtin nop function. This step
 	// allows the definition of recursive functions; the actual function will
 	// never be called.
-	fm.local.slots[op.varIndex].Set(NewGoFn("<shouldn't be called>", nop))
+	val := NewGoFn("", nop)
+	fm.local.slots[op.varIndex] = vars.FromPtr(&val)
 	values, exc := op.lambdaOp.exec(fm)
 	if exc != nil {
 		return exc
 	}
 	c := values[0].(*closure)
 	c.Op = fnWrap{c.Op}
-	return fm.errorp(op.keywordRange, fm.local.slots[op.varIndex].Set(c))
+	fm.local.slots[op.varIndex].Set(c)
+	return nil
 }
 
 type fnWrap struct{ effectOp }
@@ -314,7 +316,7 @@ func (op useOp) exec(fm *Frame) Exception {
 	if err != nil {
 		return fm.errorp(op, err)
 	}
-	fm.local.slots[op.varIndex].Set(ns)
+	fm.local.slots[op.varIndex] = vars.FromPtr(&ns)
 	return nil
 }
 

pkg/eval/builtin_special_test.go

@@ -322,9 +322,13 @@ func TestUse_SetsVariableCorrectlyIfModuleCallsExtendGlobal(t *testing.T) {
 	}
 
 	g := ev.Global()
-	if g.IndexString("a:").Get().(*Ns) == nil {
+
+	if a := g.IndexString("a:"); a == nil {
+		t.Errorf("slot $a: is nil")
+	} else if a.Get() == nil {
 		t.Errorf("$a: is nil")
 	}
+
 	if g.IndexString("b").Get().(string) != "foo" {
 		t.Errorf(`$b is not "foo"`)
 	}

pkg/eval/compile_effect.go

@@ -162,25 +162,29 @@ func isReaderGone(exc Exception) bool {
 }
 
 func (cp *compiler) formOp(n *parse.Form) effectOp {
-	var tempLValues []lvalue
 	var assignmentOps []effectOp
 	if len(n.Assignments) > 0 {
 		assignmentOps = cp.assignmentOps(n.Assignments)
 		if n.Head == nil {
 			cp.errorpf(n, `using the syntax of temporary assignment for non-temporary assignment is no longer supported; use "var" or "set" instead`)
 			return nopOp{}
 		}
-		for _, a := range n.Assignments {
-			lvalues := cp.parseIndexingLValue(a.Left, setLValue|newLValue)
-			tempLValues = append(tempLValues, lvalues.lvalues...)
-		}
-		logger.Println("temporary assignment of", len(n.Assignments), "pairs")
 	}
 
 	redirOps := cp.redirOps(n.Redirs)
 	body := cp.formBody(n)
 
-	return &formOp{n.Range(), tempLValues, assignmentOps, redirOps, body}
+	// Delete new variables created from temporary assignments.
+	for _, subop := range assignmentOps {
+		for _, lv := range subop.(*assignOp).lhs.lvalues {
+			if !lv.newLocal {
+				continue
+			}
+			cp.thisScope().infos[lv.ref.index].deleted = true
+		}
+	}
+
+	return &formOp{n.Range(), assignmentOps, redirOps, body}
 }
 
 func (cp *compiler) formBody(n *parse.Form) formBody {
@@ -252,7 +256,6 @@ func (cp *compiler) formOps(ns []*parse.Form) []effectOp {
 
 type formOp struct {
 	diag.Ranging
-	tempLValues   []lvalue
 	tempAssignOps []effectOp
 	redirOps      []effectOp
 	body          formBody
@@ -275,29 +278,28 @@ func (op *formOp) exec(fm *Frame) (errRet Exception) {
 	// fm here is always a sub-frame created in compiler.pipeline, so it can
 	// be safely modified.
 
-	// Temporary assignment.
-	if len(op.tempLValues) > 0 {
-		// There is a temporary assignment.
-		// Save variables.
+	if len(op.tempAssignOps) > 0 {
+		// There are temporary assignments. Save the original value of the variables.
+		var newLocals []int
 		var saveVars []vars.Var
 		var saveVals []interface{}
-		for _, lv := range op.tempLValues {
-			variable, err := derefLValue(fm, lv)
-			if err != nil {
-				return fm.errorp(op, err)
-			}
-			saveVars = append(saveVars, variable)
-		}
-		for i, v := range saveVars {
-			// TODO(xiaq): If the variable to save is a elemVariable, save
-			// the outermost variable instead.
-			if u := vars.HeadOfElement(v); u != nil {
-				v = u
-				saveVars[i] = v
+		for _, subop := range op.tempAssignOps {
+			for _, lv := range subop.(*assignOp).lhs.lvalues {
+				if lv.newLocal {
+					newLocals = append(newLocals, lv.ref.index)
+					continue
+				}
+				variable, err := derefLValue(fm, lv)
+				if err != nil {
+					return fm.errorp(op, err)
+				}
+				if u := vars.HeadOfElement(variable); u != nil {
+					variable = u
+				}
+				val := variable.Get()
+				saveVars = append(saveVars, variable)
+				saveVals = append(saveVals, val)
 			}
-			val := v.Get()
-			saveVals = append(saveVals, val)
-			logger.Printf("saved %s = %s", v, val)
 		}
 		// Do assignment.
 		for _, subop := range op.tempAssignOps {
@@ -310,18 +312,13 @@ func (op *formOp) exec(fm *Frame) (errRet Exception) {
 		// occurs when evaling other part of the form.
 		defer func() {
 			for i, v := range saveVars {
-				val := saveVals[i]
-				if val == nil {
-					// TODO(xiaq): Old value is nonexistent. We should delete
-					// the variable. However, since the compiler now doesn't
-					// delete it, we don't delete it in the evaler either.
-					val = ""
-				}
-				err := v.Set(val)
+				err := v.Set(saveVals[i])
 				if err != nil {
 					errRet = fm.errorp(op, err)
 				}
-				logger.Printf("restored %s = %s", v, val)
+			}
+			for _, local := range newLocals {
+				fm.local.slots[local] = nil
 			}
 		}()
 	}

pkg/eval/compile_effect_test.go

@@ -150,6 +150,12 @@ func TestCommand_Special(t *testing.T) {
 	)
 }
 
+func TestFoo(t *testing.T) {
+	Test(t,
+		That("a=foo put $a").Puts("foo"),
+	)
+}
+
 func TestCommand_Assignment(t *testing.T) {
 	// NOTE: TestClosure has more tests for the interaction between assignment
 	// and variable scoping.

pkg/eval/compile_lvalue.go

@@ -22,6 +22,7 @@ type lvaluesGroup struct {
 type lvalue struct {
 	diag.Ranging
 	ref      *varRef
+	newLocal bool
 	indexOps []valuesOp
 	ends     []int
 }
@@ -68,6 +69,7 @@ func (cp *compiler) parseIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalues
 	sigil, qname := SplitSigil(varUse)
 
 	var ref *varRef
+	var newLocal bool
 	if f&setLValue != 0 {
 		ref = resolveVarRef(cp, qname, n)
 		if ref != nil && len(ref.subNames) == 0 && ref.info.readOnly {
@@ -87,6 +89,7 @@ func (cp *compiler) parseIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalues
 			name := segs[0]
 			ref = &varRef{localScope,
 				staticVarInfo{name, false, false}, cp.thisScope().add(name), nil}
+			newLocal = true
 		} else if len(segs) == 2 && (segs[0] == "local:" || segs[0] == ":") {
 			if segs[0] == "local:" {
 				cp.deprecate(n, "the local: special namespace is deprecated; use the variable directly", 17)
@@ -97,6 +100,7 @@ func (cp *compiler) parseIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalues
 			// Qualified local name
 			ref = &varRef{localScope,
 				staticVarInfo{name, false, false}, cp.thisScope().add(name), nil}
+			newLocal = true
 		} else {
 			cp.errorpf(n, "cannot create variable $%s; new variables can only be created in the local scope", qname)
 		}
@@ -107,7 +111,7 @@ func (cp *compiler) parseIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalues
 	for i, idx := range n.Indices {
 		ends[i+1] = idx.Range().To
 	}
-	lv := lvalue{n.Range(), ref, cp.arrayOps(n.Indices), ends}
+	lv := lvalue{n.Range(), ref, newLocal, cp.arrayOps(n.Indices), ends}
 	restIndex := -1
 	if sigil == "@" {
 		restIndex = 0
@@ -116,6 +120,20 @@ func (cp *compiler) parseIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalues
 	return lvaluesGroup{[]lvalue{lv}, restIndex}
 }
 
+type newLocalsOp struct {
+	diag.Ranging
+	lhs lvaluesGroup
+}
+
+func (op *newLocalsOp) exec(fm *Frame) Exception {
+	for _, lv := range op.lhs.lvalues {
+		if lv.newLocal {
+			newLocal(fm, lv.ref)
+		}
+	}
+	return nil
+}
+
 type assignOp struct {
 	diag.Ranging
 	lhs lvaluesGroup
@@ -185,6 +203,9 @@ type noSuchVariableError struct{ name string }
 func (er noSuchVariableError) Error() string { return "no variable $" + er.name }
 
 func derefLValue(fm *Frame, lv lvalue) (vars.Var, error) {
+	if lv.newLocal {
+		return newLocal(fm, lv.ref), nil
+	}
 	variable := deref(fm, lv.ref)
 	if variable == nil {
 		return nil, NoSuchVariable(fm.srcMeta.Code[lv.From:lv.To])
@@ -214,3 +235,9 @@ func derefLValue(fm *Frame, lv lvalue) (vars.Var, error) {
 	}
 	return elemVar, nil
 }
+
+func newLocal(fm *Frame, ref *varRef) vars.Var {
+	variable := MakeVarFromName(ref.info.name)
+	fm.local.slots[ref.index] = variable
+	return variable
+}

pkg/eval/compiler.go

@@ -68,10 +68,6 @@ func (op nsOp) prepare(fm *Frame) (*Ns, func() Exception) {
 		n := len(op.template.infos)
 		newLocal := &Ns{make([]vars.Var, n), op.template.infos}
 		copy(newLocal.slots, fm.local.slots)
-		for i := len(fm.local.infos); i < n; i++ {
-			// TODO: Take readOnly into account too
-			newLocal.slots[i] = MakeVarFromName(newLocal.infos[i].name)
-		}
 		fm.local = newLocal
 	} else {
 		// If no new variable has been created, there might still be some