pkg/eval: Require the variable used in "set" to already exist.

This has always been the documented behavior, but up until this point, "set"
actually behaved like the legacy assignment form, which creates the variable if
it doesn't exist yet.

This fixes the discrepancy. Addresses #645.

pkg/eval/builtin_special.go

@@ -68,47 +68,30 @@ func init() {
 
 // VarForm = 'var' { VariablePrimary } [ '=' { Compound } ]
 func compileVar(cp *compiler, fn *parse.Form) effectOp {
-	lhs := lvaluesGroup{rest: -1}
+	eqIndex := -1
 	for i, cn := range fn.Args {
 		if parse.SourceText(cn) == "=" {
-			var rhs valuesOp
-			if i == len(fn.Args)-1 {
-				rhs = nopValuesOp{diag.PointRanging(fn.Range().To)}
-			} else {
-				rhs = seqValuesOp{
-					diag.MixedRanging(fn.Args[i+1], fn.Args[len(fn.Args)-1]),
-					cp.compoundOps(fn.Args[i+1:])}
-			}
-			return &assignOp{fn.Range(), lhs, rhs}
+			eqIndex = i
+			break
 		}
-		if len(cn.Indexings) != 1 {
-			cp.errorpf(cn, "variable name must be a single string literal")
-		}
-		if len(cn.Indexings[0].Indices) > 0 {
-			cp.errorpf(cn, "variable name must not have indices")
-		}
-		pn := cn.Indexings[0].Head
-		if !parse.ValidLHSVariable(pn, true) {
-			cp.errorpf(cn, "invalid variable name")
-		}
-
-		name := pn.Value
-		if !IsUnqualified(name) {
-			cp.errorpf(cn, "variable declared in var must be unqualified")
-		}
-		sigil, name := SplitSigil(name)
-		if sigil == "@" {
-			if lhs.rest != -1 {
-				cp.errorpf(cn, "multiple variable names with @ not allowed")
-			}
-			lhs.rest = i
-		}
-		slotIndex := cp.thisScope().add(name)
-		lhs.lvalues = append(lhs.lvalues,
-			lvalue{cn.Range(), &varRef{localScope, slotIndex, nil}, nil, nil})
 	}
-	// If there is no assignment, there is no work to be done at eval-time.
-	return nopOp{}
+
+	if eqIndex == -1 {
+		cp.parseCompoundLValues(fn.Args, newLValue)
+		// Just create new variables, nothing extra to do at runtime.
+		return nopOp{}
+	}
+	// Compile rhs before lhs before many potential shadowing
+	var rhs valuesOp
+	if eqIndex == len(fn.Args)-1 {
+		rhs = nopValuesOp{diag.PointRanging(fn.Range().To)}
+	} else {
+		rhs = seqValuesOp{
+			diag.MixedRanging(fn.Args[eqIndex+1], fn.Args[len(fn.Args)-1]),
+			cp.compoundOps(fn.Args[eqIndex+1:])}
+	}
+	lhs := cp.parseCompoundLValues(fn.Args[:eqIndex], newLValue)
+	return &assignOp{fn.Range(), lhs, rhs}
 }
 
 // IsUnqualified returns whether name is an unqualified variable name.
@@ -129,7 +112,7 @@ func compileSet(cp *compiler, fn *parse.Form) effectOp {
 	if eq == -1 {
 		cp.errorpf(diag.PointRanging(fn.Range().To), "need = and right-hand-side")
 	}
-	lhs := cp.parseCompoundLValues(fn.Args[:eq])
+	lhs := cp.parseCompoundLValues(fn.Args[:eq], setLValue)
 	var rhs valuesOp
 	if eq == len(fn.Args)-1 {
 		rhs = nopValuesOp{diag.PointRanging(fn.Range().To)}
@@ -608,7 +591,7 @@ func compileFor(cp *compiler, fn *parse.Form) effectOp {
 	elseNode := args.nextMustLambdaIfAfter("else")
 	args.mustEnd()
 
-	lvalue := cp.compileOneLValue(varNode)
+	lvalue := cp.compileOneLValue(varNode, setLValue|newLValue)
 
 	iterOp := cp.compoundOp(iterNode)
 	bodyOp := cp.primaryOp(bodyNode)
@@ -702,7 +685,7 @@ func compileTry(cp *compiler, fn *parse.Form) effectOp {
 	var bodyOp, exceptOp, elseOp, finallyOp valuesOp
 	bodyOp = cp.primaryOp(bodyNode)
 	if exceptVarNode != nil {
-		exceptVar = cp.compileOneLValue(exceptVarNode)
+		exceptVar = cp.compileOneLValue(exceptVarNode, setLValue|newLValue)
 	}
 	if exceptNode != nil {
 		exceptOp = cp.primaryOp(exceptNode)
@@ -798,11 +781,11 @@ func compilePragma(cp *compiler, fn *parse.Form) effectOp {
 	return nopOp{}
 }
 
-func (cp *compiler) compileOneLValue(n *parse.Compound) lvalue {
+func (cp *compiler) compileOneLValue(n *parse.Compound, f lvalueFlag) lvalue {
 	if len(n.Indexings) != 1 {
 		cp.errorpf(n, "must be valid lvalue")
 	}
-	lvalues := cp.parseIndexingLValue(n.Indexings[0])
+	lvalues := cp.parseIndexingLValue(n.Indexings[0], f)
 	if lvalues.rest != -1 {
 		cp.errorpf(lvalues.lvalues[lvalues.rest], "rest variable not allowed")
 	}

pkg/eval/builtin_special_test.go

@@ -51,13 +51,15 @@ func TestVar(t *testing.T) {
 		// Shadowing.
 		That("var x = old; fn f { put $x }", "var x = new; put $x; f").
 			Puts("new", "old"),
+		// Explicit local: is allowed
+		That("var local:x = foo", "put $x").Puts("foo"),
 
 		// Variable name that must be quoted after $ must be quoted
 		That("var a/b").DoesNotCompile(),
 		// Multiple @ not allowed
 		That("var x @y @z = a b c d").DoesNotCompile(),
-		// Namespace not allowed
-		That("var local:a").DoesNotCompile(),
+		// Non-local not allowed
+		That("var ns:a").DoesNotCompile(),
 		// Index not allowed
 		That("var a[0]").DoesNotCompile(),
 		// Composite expression not allowed
@@ -71,6 +73,8 @@ func TestSet(t *testing.T) {
 		That("var x; set x = foo", "put $x").Puts("foo"),
 		// 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(),
 		// Not duplicating tests with TestCommand_Assignment.
 		//
 		// TODO: After legacy assignment form is removed, transfer tests here.

pkg/eval/compile_effect.go

@@ -171,7 +171,7 @@ func (cp *compiler) formOp(n *parse.Form) effectOp {
 			return nopOp{}
 		}
 		for _, a := range n.Assignments {
-			lvalues := cp.parseIndexingLValue(a.Left)
+			lvalues := cp.parseIndexingLValue(a.Left, setLValue|newLValue)
 			tempLValues = append(tempLValues, lvalues.lvalues...)
 		}
 		logger.Println("temporary assignment of", len(n.Assignments), "pairs")
@@ -205,7 +205,7 @@ func (cp *compiler) formBody(n *parse.Form) formBody {
 			lhsNodes := make([]*parse.Compound, i+1)
 			lhsNodes[0] = n.Head
 			copy(lhsNodes[1:], n.Args[:i])
-			lhs := cp.parseCompoundLValues(lhsNodes)
+			lhs := cp.parseCompoundLValues(lhsNodes, setLValue|newLValue)
 
 			rhsOps := cp.compoundOps(n.Args[i+1:])
 			var rhsRange diag.Ranging
@@ -414,7 +414,7 @@ func allTrue(vs []interface{}) bool {
 }
 
 func (cp *compiler) assignmentOp(n *parse.Assignment) effectOp {
-	lhs := cp.parseIndexingLValue(n.Left)
+	lhs := cp.parseIndexingLValue(n.Left, setLValue|newLValue)
 	rhs := cp.compoundOp(n.Right)
 	return &assignOp{n.Range(), lhs, rhs}
 }

pkg/eval/compile_lvalue.go

@@ -26,13 +26,20 @@ type lvalue struct {
 	ends     []int
 }
 
-func (cp *compiler) parseCompoundLValues(ns []*parse.Compound) lvaluesGroup {
+type lvalueFlag uint
+
+const (
+	setLValue lvalueFlag = 1 << iota
+	newLValue
+)
+
+func (cp *compiler) parseCompoundLValues(ns []*parse.Compound, f lvalueFlag) lvaluesGroup {
 	g := lvaluesGroup{nil, -1}
 	for _, n := range ns {
 		if len(n.Indexings) != 1 {
 			cp.errorpf(n, "lvalue may not be composite expressions")
 		}
-		more := cp.parseIndexingLValue(n.Indexings[0])
+		more := cp.parseIndexingLValue(n.Indexings[0], f)
 		if more.rest == -1 {
 			g.lvalues = append(g.lvalues, more.lvalues...)
 		} else if g.rest != -1 {
@@ -45,13 +52,13 @@ func (cp *compiler) parseCompoundLValues(ns []*parse.Compound) lvaluesGroup {
 	return g
 }
 
-func (cp *compiler) parseIndexingLValue(n *parse.Indexing) lvaluesGroup {
+func (cp *compiler) parseIndexingLValue(n *parse.Indexing, f lvalueFlag) lvaluesGroup {
 	if n.Head.Type == parse.Braced {
 		// Braced list of lvalues may not have indices.
 		if len(n.Indices) > 0 {
 			cp.errorpf(n, "braced list may not have indices when used as lvalue")
 		}
-		return cp.parseCompoundLValues(n.Head.Braced)
+		return cp.parseCompoundLValues(n.Head.Braced, f)
 	}
 	// A basic lvalue.
 	if !parse.ValidLHSVariable(n.Head, true) {
@@ -59,27 +66,30 @@ func (cp *compiler) parseIndexingLValue(n *parse.Indexing) lvaluesGroup {
 	}
 	varUse := n.Head.Value
 	sigil, qname := SplitSigil(varUse)
+
 	var ref *varRef
-	if len(n.Indices) == 0 {
-		ref = resolveVarRef(cp, qname, nil)
-		if ref == nil {
-			segs := SplitQNameSegs(qname)
-			if len(segs) == 1 {
-				// Unqualified name - implicit local
-				ref = &varRef{localScope, cp.thisScope().addInner(segs[0]), nil}
-			} else if len(segs) == 2 && (segs[0] == "local:" || segs[0] == ":") {
-				// Qualified local name
-				ref = &varRef{localScope, cp.thisScope().addInner(segs[1]), nil}
-			} else {
-				cp.errorpf(n, "cannot create variable $%s; new variables can only be created in the local scope", qname)
-			}
-		}
-	} else {
+	if f&setLValue != 0 {
 		ref = resolveVarRef(cp, qname, n)
-		if ref == nil {
+	}
+	if ref == nil {
+		if f&newLValue == 0 {
 			cp.errorpf(n, "cannot find variable $%s", qname)
 		}
+		if len(n.Indices) > 0 {
+			cp.errorpf(n, "name for new variable must not have indices")
+		}
+		segs := SplitQNameSegs(qname)
+		if len(segs) == 1 {
+			// Unqualified name - implicit local
+			ref = &varRef{localScope, cp.thisScope().add(segs[0]), nil}
+		} else if len(segs) == 2 && (segs[0] == "local:" || segs[0] == ":") {
+			// Qualified local name
+			ref = &varRef{localScope, cp.thisScope().add(segs[1]), nil}
+		} else {
+			cp.errorpf(n, "cannot create variable $%s; new variables can only be created in the local scope", qname)
+		}
 	}
+
 	ends := make([]int, len(n.Indices)+1)
 	ends[0] = n.Head.Range().To
 	for i, idx := range n.Indices {