resolve: disallow augmented assignments at toplevel (google#125)

...unless -globalreassign is set.

Formerly, the Go implementation permitted x+=y on the basis
that we couldn't statically tell whether x was a list (in which
case x+=y means x.extend(y), which is not a rebinding), or some
other type, in which case it is a forbidden rebinding.
This change makes it match the Java implementation.
See bazelbuild/starlark#20 (comment)

Most non-Bazel-like clients of Starlark set the -globalreassign flag.

doc/spec.md

@@ -1090,7 +1090,7 @@ Four Starlark constructs bind names, as illustrated in the example below:
 `load` statements (`a` and `b`),
 `def` statements (`c`),
 function parameters (`d`),
-and assignments (`e`, `h`, including the augmented assignment `e += h`).
+and assignments (`e`, `h`, including the augmented assignment `e += 1`).
 Variables may be assigned or re-assigned explicitly (`e`, `h`), or implicitly, as
 in a `for`-loop (`f`) or comprehension (`g`, `i`).
 
@@ -1182,22 +1182,21 @@ x = 2                   # static error: cannot reassign global x declared on lin
 ```
 
 <!-- The above rule, and the rule that forbids if-statements and loops at
-     toplevel, exist to ensure that there is exactly one statement
+     top level, exist to ensure that there is exactly one statement
      that binds each global variable, which makes cross-referenced
      documentation more useful, the designers assure me, but
      I am skeptical that it's worth the trouble. -->
 
 If a name was pre-bound by the application, the Starlark program may
 explicitly bind it, but only once.
 
+An augmented assignment statement such as `x += y` is considered both a
+reference to `x` and a binding use of `x`, so it may not be used at
+top level.
+
 <b>Implementation note:</b>
-An augmented assignment statement such as `x += 1` is considered a
-binding of `x`.
-However, because of the special behavior of `+=` for lists, which acts
-like a non-binding reference, the Go implementation suppresses the
-"cannot reassign" error for all augmented assigments at toplevel,
-whereas the Java implementation reports the error even when the
-statement would apply `+=` to a list.
+The Go implementation of Starlark permits augmented assignments to appear
+at top level if the `-globalreassign` flag is enabled.
 
 A function may refer to variables defined in an enclosing function.
 In this example, the inner function `f` refers to a variable `x`
@@ -2640,7 +2639,7 @@ An `if` statement is permitted only within a function definition.
 An `if` statement at top level results in a static error.
 
 <b>Implementation note:</b>
-The Go implementation of Starlark permits `if`-statements to appear at top-level
+The Go implementation of Starlark permits `if`-statements to appear at top level
 if the `-globalreassign` flag is enabled.
 
 
@@ -2667,7 +2666,7 @@ A `while` statement at top level results in a static error.
 
 <b>Implementation note:</b>
 The Go implementation of Starlark permits `while` loops only if the `-recursion` flag is enabled.
-A `while` statement is permitted at top-level if the `-globalreassign` flag is enabled.
+A `while` statement is permitted at top level if the `-globalreassign` flag is enabled.
 
 
 ### For loops
@@ -2709,7 +2708,7 @@ In Starlark, a `for` loop is permitted only within a function definition.
 A `for` loop at top level results in a static error.
 
 <b>Implementation note:</b>
-The Go implementation of Starlark permits loops to appear at top-level
+The Go implementation of Starlark permits loops to appear at top level
 if the `-globalreassign` flag is enabled.
 
 
@@ -4033,13 +4032,12 @@ See [Starlark spec issue 20](https://github.com/bazelbuild/starlark/issues/20).
 * The `chr` and `ord` built-in functions are supported.
 * The `set` built-in function is provided (option: `-set`).
 * `set & set` and `set | set` compute set intersection and union, respectively.
-* `x += y` rebindings are permitted at top level.
 * `assert` is a valid identifier.
 * The parser accepts unary `+` expressions.
 * A method call `x.f()` may be separated into two steps: `y = x.f; y()`.
 * Dot expressions may appear on the left side of an assignment: `x.f = 1`.
 * `hash` accepts operands besides strings.
 * `sorted` accepts the additional parameters `key` and `reverse`.
 * `type(x)` returns `"builtin_function_or_method"` for built-in functions.
-* `if`, `for`, and `while` are permitted at toplevel (option: `-globalreassign`).
+* `if`, `for`, and `while` are permitted at top level (option: `-globalreassign`).
 * top-level rebindings are permitted (option: `-globalreassign`).

resolve/resolve.go

@@ -293,19 +293,15 @@ type use struct {
 
 // bind creates a binding for id in the current block,
 // if there is not one already, and reports an error if
-// a global was re-bound and allowRebind is false.
+// a global was re-bound.
 // It returns whether a binding already existed.
-func (r *resolver) bind(id *syntax.Ident, allowRebind bool) bool {
+func (r *resolver) bind(id *syntax.Ident) bool {
 	// Binding outside any local (comprehension/function) block?
 	if r.env.isModule() {
 		id.Scope = uint8(Global)
 		prev, ok := r.globals[id.Name]
 		if ok {
-			// Global reassignments are permitted only if
-			// they are of the form x += y.  We can't tell
-			// statically whether it's a reassignment
-			// (e.g. int += int) or a mutation (list += list).
-			if !allowRebind && !AllowGlobalReassign {
+			if !AllowGlobalReassign {
 				r.errorf(id.NamePos, "cannot reassign global %s declared at %s", id.Name, prev.NamePos)
 			}
 			id.Index = prev.Index
@@ -446,29 +442,23 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 			}
 		}
 		r.expr(stmt.RHS)
-		// x += y may be a re-binding of a global variable,
-		// but we cannot tell without knowing the type of x.
-		// (If x is a list it's equivalent to x.extend(y).)
-		// The use is conservatively treated as binding,
-		// but we suppress the error if it's an already-bound global.
 		isAugmented := stmt.Op != syntax.EQ
 		r.assign(stmt.LHS, isAugmented)
 
 	case *syntax.DefStmt:
 		if !AllowNestedDef && r.container().function != nil {
 			r.errorf(stmt.Def, doesnt+"support nested def")
 		}
-		const allowRebind = false
-		r.bind(stmt.Name, allowRebind)
+		r.bind(stmt.Name)
 		r.function(stmt.Def, stmt.Name.Name, &stmt.Function)
 
 	case *syntax.ForStmt:
 		if !AllowGlobalReassign && r.container().function == nil {
 			r.errorf(stmt.For, "for loop not within a function")
 		}
 		r.expr(stmt.X)
-		const allowRebind = false
-		r.assign(stmt.Vars, allowRebind)
+		const isAugmented = false
+		r.assign(stmt.Vars, isAugmented)
 		r.loops++
 		r.stmts(stmt.Body)
 		r.loops--
@@ -498,7 +488,6 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 			r.errorf(stmt.Load, "load statement within a function")
 		}
 
-		const allowRebind = false
 		for i, from := range stmt.From {
 			if from.Name == "" {
 				r.errorf(from.NamePos, "load: empty identifier")
@@ -507,7 +496,7 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 			if from.Name[0] == '_' {
 				r.errorf(from.NamePos, "load: names with leading underscores are not exported: %s", from.Name)
 			}
-			r.bind(stmt.To[i], allowRebind)
+			r.bind(stmt.To[i])
 		}
 
 	default:
@@ -519,8 +508,7 @@ func (r *resolver) assign(lhs syntax.Expr, isAugmented bool) {
 	switch lhs := lhs.(type) {
 	case *syntax.Ident:
 		// x = ...
-		allowRebind := isAugmented
-		r.bind(lhs, allowRebind)
+		r.bind(lhs)
 
 	case *syntax.IndexExpr:
 		// x[i] = ...
@@ -616,15 +604,15 @@ func (r *resolver) expr(e syntax.Expr) {
 		// enclosing container (function/module) block.
 		r.push(&block{comp: e})
 
-		const allowRebind = false
-		r.assign(clause.Vars, allowRebind)
+		const isAugmented = false
+		r.assign(clause.Vars, isAugmented)
 
 		for _, clause := range e.Clauses[1:] {
 			switch clause := clause.(type) {
 			case *syntax.IfClause:
 				r.expr(clause.Cond)
 			case *syntax.ForClause:
-				r.assign(clause.Vars, allowRebind)
+				r.assign(clause.Vars, isAugmented)
 				r.expr(clause.X)
 			}
 		}
@@ -755,7 +743,6 @@ func (r *resolver) function(pos syntax.Position, name string, function *syntax.F
 	b := &block{function: function}
 	r.push(b)
 
-	const allowRebind = false
 	var seenOptional bool
 	var star, starStar *syntax.Ident // *args, **kwargs
 	for _, param := range function.Params {
@@ -769,7 +756,7 @@ func (r *resolver) function(pos syntax.Position, name string, function *syntax.F
 			} else if seenOptional {
 				r.errorf(param.NamePos, "required parameter may not follow optional")
 			}
-			if r.bind(param, allowRebind) {
+			if r.bind(param) {
 				r.errorf(param.NamePos, "duplicate parameter: %s", param.Name)
 			}
 
@@ -780,7 +767,7 @@ func (r *resolver) function(pos syntax.Position, name string, function *syntax.F
 			} else if star != nil {
 				r.errorf(param.OpPos, "optional parameter may not follow *%s", star.Name)
 			}
-			if id := param.X.(*syntax.Ident); r.bind(id, allowRebind) {
+			if id := param.X.(*syntax.Ident); r.bind(id) {
 				r.errorf(param.OpPos, "duplicate parameter: %s", id.Name)
 			}
 			seenOptional = true
@@ -803,7 +790,7 @@ func (r *resolver) function(pos syntax.Position, name string, function *syntax.F
 					starStar = id
 				}
 			}
-			if r.bind(id, allowRebind) {
+			if r.bind(id) {
 				r.errorf(id.NamePos, "duplicate parameter: %s", id.Name)
 			}
 		}

resolve/testdata/resolve.star

@@ -91,18 +91,17 @@ def g():
   f()
 
 ---
-# It's permitted to rebind a global using a += assignment.
+# It is not permitted to rebind a global using a += assignment.
 
 x = [1]
 x.extend([2]) # ok
-x += [3] # ok (a list mutation, not a global rebinding)
+x += [3] ### `cannot reassign global x`
 
 def f():
    x += [4] # x is local to f
 
 y = 1
-y += 2 # ok (even though it is in fact a global rebinding)
-
+y += 2 ### `cannot reassign global y`
 z += 3 # ok (but fails dynamically because z is undefined)
 
 ---
@@ -193,15 +192,15 @@ while U: # ok
 ---
 # The parser allows any expression on the LHS of an assignment.
 
-1 = 2 ### "can't assign to literal"
-1+2 = 3 ### "can't assign to binaryexpr"
-f() = 4 ### "can't assign to callexpr"
+1 = 0 ### "can't assign to literal"
+1+2 = 0 ### "can't assign to binaryexpr"
+f() = 0 ### "can't assign to callexpr"
 
-[a, b] = [1, 2]
-[a, b] += [3, 4] ### "can't use list expression in augmented assignment"
-(a, b) += [3, 4] ### "can't use tuple expression in augmented assignment"
-[] = [] ### "can't assign to \\[\\]"
-() = () ### "can't assign to ()"
+[a, b] = 0
+[c, d] += 0 ### "can't use list expression in augmented assignment"
+(e, f) += 0 ### "can't use tuple expression in augmented assignment"
+[] = 0 ### "can't assign to \\[\\]"
+() = 0 ### "can't assign to ()"
 
 ---
 # break and continue statements must appear within a loop

starlark/testdata/assign.star

@@ -288,6 +288,7 @@ assert.eq(g(), {"one": 1, "two": 2})
 
 ---
 # parenthesized LHS in augmented assignment (success)
+# option:globalreassign
 load("assert.star", "assert")
 
 a = 5