Berry fix walrus to not create variable (#25090)

.doc_for_ai/BERRY_LANGUAGE_REFERENCE.md

@@ -43,7 +43,7 @@ Relational:   < <= == != >= >
 Logical:      && || !  (short-circuit)
 Bitwise:      ~ & | ^ << >>
 Assignment:   = += -= *= /= %= &= |= ^= <<= >>=
-Walrus:       :=  (assign + return value in expressions)
+Walrus:       :=  (assign + return value in expressions; the target must already exist, := never creates a variable)
 Other:        . [] ?: ..
 ```
 `..` = range (int..int), string concat (auto-converts RHS), or list/bytes append-in-place.

lib/libesp32/berry/berry_port/be_parser.py

@@ -1830,17 +1830,34 @@ def walrus_expr(parser, e):
     sub_expr(parser, e, ASSIGN_OP_PRIO)
     op = next_type(parser)
     if op == OptWalrus:
+        finfo = parser.finfo
         check_symbol(parser, e)
+        # ':=' only assigns an existing variable, it never creates one (neither
+        # local nor global, nor a local shadowing a builtin). A new local would
+        # take the register right above the other locals, which may still hold
+        # a temporary of the enclosing expression, and an implicit global is an
+        # easy way to hide a typo.
+        if e.type == ETVOID or \
+                (e.type == ETGLOBAL and e.v.idx < be_builtin_count(parser.vm)):
+            name = e.v.s if e.type == ETVOID else be_builtin_name(parser.vm, e.v.idx)
+            parser.lexer.linenumber = line
+            push_error(parser,
+                "cannot create variable '%s' with ':=', "
+                "declare it with 'var' first", be_str2cstr(name))
         e1 = _clone_expdesc(e)
         parser.finfo.binfo.sideeffect = 1
         scan_next_token(parser)  # skip ':='
+        base = finfo.freereg  # registers below are still in use by the enclosing expression
         expr(parser, e)
         check_var(parser, e)
-        if check_newvar(parser, e1):
-            new_var(parser, e1.v.s, e1)
         if be_code_setvar(parser.finfo, e1, e, True):
             parser.lexer.linenumber = line
             parser_error(parser, "try to assign constant expressions.")
+        if e1.type == ETLOCAL and e.type == ETLOCAL and finfo.freereg > base:
+            # the value is now held by the local variable: release the temporary
+            # registers used by the right side, e.g. the object of `l[i]`.
+            # `:=` never creates a local, so `base` is never below the locals
+            finfo.freereg = base
 
 
 # static void expr(bparser *parser, bexpdesc *e)

lib/libesp32/berry/src/be_parser.c

@@ -1160,20 +1160,38 @@ static void walrus_expr(bparser *parser, bexpdesc *e)
     sub_expr(parser, e, ASSIGN_OP_PRIO);    /* left expression */
     btokentype op = next_type(parser);
     if (op == OptWalrus) {
+        bfuncinfo *finfo = parser->finfo;
+        int base;
         check_symbol(parser, e);
+        /* ':=' only assigns an existing variable, it never creates one (neither
+         * local nor global, nor a local shadowing a builtin). A new local would
+         * take the register right above the other locals, which may still hold
+         * a temporary of the enclosing expression, and an implicit global is an
+         * easy way to hide a typo. */
+        if (e->type == ETVOID ||
+            (e->type == ETGLOBAL && e->v.idx < be_builtin_count(parser->vm))) {
+            bstring *name = (e->type == ETVOID) ? e->v.s : be_builtin_name(parser->vm, e->v.idx);
+            parser->lexer.linenumber = line;
+            push_error(parser, "cannot create variable '%s' with ':=', "
+                "declare it with 'var' first", str(name));
+        }
         bexpdesc e1 = *e;           /* copy var to e1, e will get the result of expression */
         parser->finfo->binfo->sideeffect = 1;   /* has side effect */
         scan_next_token(parser);    /* skip ':=' */
+        base = finfo->freereg;      /* registers below are still in use by the enclosing expression */
         expr(parser, e);
         check_var(parser, e);
-        if (check_newvar(parser, &e1)) { /* new variable */
-            new_var(parser, e1.v.s, &e1);
-        }
         if (be_code_setvar(parser->finfo, &e1, e, btrue /* do not release register */ )) {
             parser->lexer.linenumber = line;
             parser_error(parser,
                 "try to assign constant expressions.");
         }
+        if (e1.type == ETLOCAL && e->type == ETLOCAL && finfo->freereg > base) {
+            /* the value is now held by the local variable: release the temporary
+             * registers used by the right side, e.g. the object of `l[i]`.
+             * `:=` never creates a local, so `base` is never below the locals */
+            finfo->freereg = (bbyte)base;
+        }
     }
 }
 

lib/libesp32/berry/tests/compiler.be

@@ -154,3 +154,78 @@ assert(nk_h() == [10, 'x', 2])
 nk_n = nil
 compile("nk_n = [0, 0, 0, 0, 0]  nk_n[nk_n[4]] = 1  nk_n[1] = nk_n[0] + 1")()
 assert(nk_n == [1, 2, 0, 0, 0])
+
+# Walrus into a local, with an index or member expression on the right, as a
+# call argument. The right side is compiled straight into the register of the
+# local, but the registers of its object and index were not released, so the
+# value was passed one slot too high:
+#
+# ```berry
+# def g()
+#   var p = 0
+#   return f(p := l[3], 4)    # passed l and p instead of p and 4
+# end
+# ```
+def wl_f(a, b) return [a, b] end
+wl_l = [10, 11, 12, 13, 14]
+def wl_g() var p = 0 return wl_f(p := wl_l[3], 4) end
+assert(wl_g() == [13, 4])       # was [wl_l, 13]
+def wl_h() var p = 0 return wl_f(4, p := wl_l[3]) end
+assert(wl_h() == [4, 13])       # was [4, wl_l]
+def wl_i(k) var p = 0 return str(p := wl_l[k + 1]) end
+assert(wl_i(2) == '13')         # was '3', the index
+import global
+def wl_j() var p = 0 return wl_f(p := global.wl_l, 4) end
+assert(wl_j() == [wl_l, 4])     # was [<module: global>, wl_l]
+
+# Walrus never creates a variable, neither a local, nor a global, nor a local
+# shadowing a builtin: the target must already exist. A new local created in
+# the middle of an expression took the register right above the other locals,
+# which could still hold a temporary, so its value was lost:
+#
+# ```berry
+# def t(a)
+#   var r = a * 2 + (n := a + 1)    # computed n + n
+#   return [r, n]                   # [22, 11] instead of [31, 11]
+# end
+# ```
+#
+# `wl_new` is never defined, as a local or as a global.
+import string
+def wl_error(code, name)
+  try
+    compile(code)
+  except 'syntax_error' as e, m
+    return string.find(m, "cannot create variable '" + name + "' with ':='") >= 0
+  end
+  return false
+end
+# new local over a pending temporary, was [22, 11]
+assert(wl_error("def t(a) var r = a * 2 + (wl_new := a + 1) return [r, wl_new] end", 'wl_new'))
+# new local as a call argument, was type_error: 'int' value is not callable
+assert(wl_error("def t(a) return wl_f(a, wl_new := a + 1) end", 'wl_new'))
+# new local in a list literal in a block, was 11, the list was overwritten
+assert(wl_error("do var a = 10 var r = [a, (wl_new := a + 1)] print(r) end", 'wl_new'))
+# new local in the else branch of `?:`, t(true) returned nil
+assert(wl_error("def t(c) var r = c ? 1 : (wl_new := 2) return r end", 'wl_new'))
+# new local with nothing pending, used to work, now refused as well
+assert(wl_error("def t(a) var r = (wl_new := a + 1) + a * 2 return [r, wl_new] end", 'wl_new'))
+assert(wl_error("def t(a) if (wl_new := a + 1) > 5 return wl_new end return 0 end", 'wl_new'))
+assert(wl_error("def t(k) var p = 0 p = wl_l[k + 1] wl_new := 5 return wl_new end", 'wl_new'))
+# new global at module level
+assert(wl_error("wl_new := 4", 'wl_new'))
+assert(wl_error("wl_r = 2 * 3 + (wl_new := 4)", 'wl_new'))
+# local shadowing a builtin
+assert(wl_error("def t() print := 1 end", 'print'))
+# an existing variable is still assigned: local, global, from a function too
+def wl_k(a) var n = 0 var r = a * 2 + (n := a + 1) return [r, n] end
+assert(wl_k(10) == [31, 11])
+def wl_m(a) var n var r = (n := a + 1) + a * 2 return [r, n] end
+assert(wl_m(10) == [31, 11])
+def wl_n(a) var n if (n := a + 1) > 5 return n end return 0 end
+assert(wl_n(10) == 11)
+wl_s = 0
+wl_r = 2 * 3 + (wl_s := 4)
+assert(wl_r == 10 && wl_s == 4)
+def wl_o() return wl_s := 5 end
+assert(wl_o() == 5 && wl_s == 5)

lib/libesp32/berry/tests/walrus.be

@@ -72,8 +72,21 @@ var ins = confused_walrus()
 assert(ins.f() == ins)
 
 # Check overwriting a builtin (https://github.com/berry-lang/berry/issues/416)
+# `:=` never creates a variable, so it cannot create a local shadowing a builtin
+import string
+def walrus_create_error(c)
+    try
+        compile(c)
+    except 'syntax_error' as e, m
+        return string.find(m, "cannot create variable 'print' with ':='") >= 0
+    end
+    return false
+end
+assert(walrus_create_error("def f() print := 1 end"))
 
+# a local declared with `var` can still shadow the builtin
 def check_overwrite_builtin()
+    var print
     print := 1
     assert(print == 1)
 end