From 7fdaf825bb8a1a47bb6cdd695087beac3268f44b Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 22 Jan 2019 22:20:47 -0500 Subject: [PATCH] resolve: disallow augmented assignments at toplevel ...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 https://github.com/bazelbuild/starlark/issues/20#issuecomment-456647994 Most non-Bazel-like clients of Starlark set the -globalreassign flag. Change-Id: I1a21fc6871e51c201529da91dc0a46ad3e99d448 --- doc/spec.md | 26 ++++++++++++-------------- resolve/resolve.go | 2 +- resolve/testdata/resolve.star | 29 +++++++++++++++-------------- starlark/testdata/assign.star | 1 + 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/doc/spec.md b/doc/spec.md index c9cb1d49..dd4f9f22 100644 --- a/doc/spec.md +++ b/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,7 +1182,7 @@ x = 2 # static error: cannot reassign global x declared on lin ``` @@ -1190,14 +1190,13 @@ x = 2 # static error: cannot reassign global x declared on lin 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. + Implementation note: -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. Implementation note: -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. Implementation note: 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. Implementation note: -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,7 +4032,6 @@ 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()`. @@ -4041,5 +4039,5 @@ See [Starlark spec issue 20](https://github.com/bazelbuild/starlark/issues/20). * `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`). diff --git a/resolve/resolve.go b/resolve/resolve.go index 9c5a0b80..c0681f46 100644 --- a/resolve/resolve.go +++ b/resolve/resolve.go @@ -519,7 +519,7 @@ func (r *resolver) assign(lhs syntax.Expr, isAugmented bool) { switch lhs := lhs.(type) { case *syntax.Ident: // x = ... - allowRebind := isAugmented + const allowRebind = false r.bind(lhs, allowRebind) case *syntax.IndexExpr: diff --git a/resolve/testdata/resolve.star b/resolve/testdata/resolve.star index bd038ce6..8423377c 100644 --- a/resolve/testdata/resolve.star +++ b/resolve/testdata/resolve.star @@ -91,19 +91,20 @@ 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` + +--- +# It is not permitted to rebind a global using a += assignment. def f(): x += [4] # x is local to f y = 1 -y += 2 # ok (even though it is in fact a global rebinding) - -z += 3 # ok (but fails dynamically because z is undefined) +y += 2 ### `cannot reassign global y` +z += 3 --- def f(a): @@ -193,15 +194,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 diff --git a/starlark/testdata/assign.star b/starlark/testdata/assign.star index ae97bb2c..c3fb0910 100644 --- a/starlark/testdata/assign.star +++ b/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