Fix: An ArrayParam function that can run user code stays interpreted (#203)

docs/SPEC.md

@@ -577,6 +577,7 @@ All in `PM/commonTest`, so they run wherever the interpreter loads.
   | No deopting node in an impure function | not compiled; diagnostic |
   | Array access in bounds, or a runtime bounds check kept | the check stays (never an unchecked access) |
   | Array parameters only indexed, stored, `len`-ed; no aliasing (entry guard) | not compiled; diagnostic |
+  | A function holding an array parameter runs no user code: closed, and no slot read or `C(args)` whose slow path runs user code, in its whole call tree (#203) | not compiled; diagnostic |
   | OBJ references follow one ownership rule; no IR form for free, borrowed alias or NULL read | not compiled; diagnostic |
   | Integer overflow checked and promoted (deopt); an op marked `proven` must re-prove its interval fits i64 | not compiled; diagnostic `verify/proven-overflow` |
   | `Call.redo` only from an impure caller to a pure, may-deopt callee returning F64/BOOL/NONE | not compiled; diagnostic `verify/redo` |

python-multiplatform-ksp/src/main/python/typedpython/ir.py

@@ -536,9 +536,9 @@ class ArrayParam:
     At entry (guards, before any effect): the argument is exactly a `list`, every element is exactly
     `float` (F64_ARRAY) or exactly `int` within i64 (I64_ARRAY), and no two array parameters are
     the same list object (aliasing would make separate copies diverge). Then the elements are copied
-    into a C array. The function body only indexes, stores into and takes `len` of it (no calls that
-    could observe the list, no appends: the front end checks), so the list and the array cannot
-    diverge observably. On **every** exit, return, exception, or deopt, dirty elements are
+    into a C array. The function body only indexes, stores into and takes `len` of it, no appends, and
+    runs no user code at all: the verifier rejects a function holding an array that is not closed
+    (`verify/array-open`, #203), so the list and the array cannot diverge observably. On **every** exit, return, exception, or deopt, dirty elements are
     written back as new float/int objects before control leaves the function, so CPython's
     partial-update-then-raise behaviour is preserved, and a deopt redo starts from the same list
     state the interpreted function would see... which is only correct because a function with array

python-multiplatform-ksp/src/main/python/typedpython/verify.py

@@ -42,6 +42,7 @@
                               on a node `proven` does not apply to, or it is not a bool)
   verify/redo                 `Call.redo` where it is not legal (see below)
   verify/entry-global-open    a function with entry_globals is not closed
+  verify/array-open           a function with an ArrayParam runs user code (#203)
   verify/class                a class named by FieldGet/FieldSet/New/IsExact/CheckExact/Param.cls is not
                               a (valid) ClassDecl of the module, a field is not one of its fields, a
                               New names a class without trivial_init or has the wrong arity, or a
@@ -196,6 +197,7 @@
 PROVEN = "verify/proven-overflow"
 REDO = "verify/redo"
 ENTRY_OPEN = "verify/entry-global-open"
+ARRAY_OPEN = "verify/array-open"
 CLASS = "verify/class"
 FIELD = "verify/field-unproved"
 
@@ -278,6 +280,7 @@ def __init__(self, function: Function, signatures: dict[str, Function],
         self.lens: dict[str, tuple[int, int]] = {}   # single-assignment local array -> length
         self.entry: dict[str, Type] = {}     # name -> type of every entry global
         self.opens: list[str] = []           # reasons the function is not closed
+        self.slow_user: list[str] = []       # slow paths that can run user code (FieldGet, New)
 
     def error(self, rule: str, message: str) -> None:
         d = Diagnostic(self.fn.name, rule, message, self.fn.source_line)
@@ -919,9 +922,13 @@ def e_FieldGet(self, e: FieldGet, state: _State) -> ir.Expr:
         self.result(e, Type.OBJ)
         if self.known_field(e.cls, e.field, f"FieldGet .{e.field}"):
             self.proved(obj, e.cls, f"FieldGet {e.cls}.{e.field}")
-        return dataclasses.replace(e, obj=obj)        # a slot read runs no user code: not an effect
+        # a slot read runs no user code: not an effect. Its slow path after a class change (getattr)
+        # can, which matters to array functions (#203).
+        self.slow_user.append(f"FieldGet .{e.field} (generic getattr after a class change)")
+        return dataclasses.replace(e, obj=obj)
 
     def e_New(self, e: New, state: _State) -> ir.Expr:
+        self.slow_user.append(f"New {e.cls} (calls the class after a class change)")
         args = self.args(e.args, state, f"New {e.cls}")
         for k, a in enumerate(args):
             self.slot(a, Type.OBJ, f"argument {k + 1} of New {e.cls}")
@@ -1349,6 +1356,7 @@ def reject(fn_name: str, line: int, rule: str, message: str) -> None:
     checked: dict[str, Function] = {}
     callees: dict[str, set[str]] = {}
     open_why: dict[str, str] = {}          # a function that runs user code -> why
+    slow_why: dict[str, str] = {}          # a function whose slow paths can run user code -> why
     for name, g in sigs.items():
         c = _Checker(g, sigs, classes)
         rebuilt = c.run()
@@ -1358,6 +1366,8 @@ def reject(fn_name: str, line: int, rule: str, message: str) -> None:
         callees[name] = c.callees
         if c.opens:
             open_why[name] = c.opens[0]
+        if c.slow_user:
+            slow_why[name] = c.slow_user[0]
 
     # Closedness (entry_globals): a fixpoint over the call graph.
     changed = True
@@ -1374,6 +1384,26 @@ def reject(fn_name: str, line: int, rule: str, message: str) -> None:
             reject(name, g.source_line, ENTRY_OPEN,
                    f"has entry globals but is not closed: {open_why[name]}")
 
+    # ArrayParam (#203): the list is copied in and written back, so nothing may run user code while
+    # the copy is live: user code could read or change the list. Closed as above, and also no
+    # FieldGet/New, whose slow paths run user code after a class change, anywhere in the call tree.
+    array_why = dict(open_why)
+    for name, why in slow_why.items():
+        array_why.setdefault(name, why)
+    changed = True
+    while changed:
+        changed = False
+        for name, cs in callees.items():
+            if name not in array_why:
+                bad = sorted(c for c in cs if c in array_why)
+                if bad:
+                    array_why[name] = f"calls '{bad[0]}', which can run user code ({array_why[bad[0]]})"
+                    changed = True
+    for name, g in sigs.items():
+        if name in array_why and any(isinstance(p, ArrayParam) for p in g.params):
+            reject(name, g.source_line, ARRAY_OPEN,
+                   f"holds a list copy-in but can run user code: {array_why[name]}")
+
     # A call cycle (direct or mutual recursion) is legal: every compiled impl function counts its
     # frame at entry (tp_enter_call), so recursion ends in RecursionError where CPython raises it
     # and the C stack cannot overflow (issue #57). Purity and may_deopt flow through the cycle as

python-multiplatform-ksp/src/test/python/test_cgen_class_changes.py

@@ -112,9 +112,9 @@ def is_node(cb, x):
         cb()
         return type(x) is Node
 
-    def fill(a, n):
+    def fill(n):
         x = n.left
-        a[0] = SCALE
+        return SCALE
     ''')
 
 NODE = ClassDecl("Node", ("left", "right"), True)
@@ -199,12 +199,13 @@ def f_is_node():
 
 
 def f_fill():
-    # closed (no user code), impure (an array store), with an entry global
-    return fn("fill", [ArrayParam("a", Type.F64_ARRAY, True), Param("n", OBJ, cls="Node")], NONE,
+    # closed (no user code) with an entry global. Not an array function any more: a slot read's
+    # slow path can run a property, which must not run while a list copy-in is live (#203)
+    return fn("fill", [Param("n", OBJ, cls="Node")], F64,
               {"x": OBJ},
               [Assign("x", FieldGet(OBJ, _o("n"), "Node", "left")),
-               StoreIndex("a", c(0), L("SCALE", F64)), Return()],
-              pure=False, may_deopt=False, entry_globals=(Param("SCALE", F64),))
+               Return(L("SCALE", F64))],
+              pure=True, may_deopt=False, entry_globals=(Param("SCALE", F64),))
 
 
 def functions():
@@ -270,8 +271,7 @@ def sc_unchanged(M, call, P):
            call("pair", 1, 2), call("mid_get", NOOP, n), call("mid_set", NOOP, n, "y"),
            call("mid_new", NOOP, 3, 4), call("arg_new", lambda: "a", 5), call("via_pair", NOOP, 6, 7),
            call("is_node", NOOP, n), call("is_node", NOOP, M.Twin(1, 2))]
-    a = [0.0]
-    out += [call("fill", a, n), a]
+    out += [call("fill", n)]
     P("after")
     return out
 
@@ -456,16 +456,14 @@ def sc_exhaust(M, call, P):
 
 def sc_entry_global(M, call, P):
     n = M.Node(1, 2)
-    a = [0.0]
-    out = [call("fill", a, n), list(a)]
+    out = [call("fill", n)]
     P("before")
 
     def getter(s):
         M.SCALE = 5.0
         return "p"
     M.Node.left = property(getter)
-    b = [0.0]
-    out += [call("fill", b, n), list(b), M.SCALE]
+    out += [call("fill", n), M.SCALE]
     P("after")
     return out
 
@@ -601,12 +599,12 @@ def round_():
             M.via_pair(cb, shared, shared)
             M.arg_new(cb, shared)
             M.is_node(cb, n)
-            M.fill([0.0], n)
+            M.fill(n)
         A2.get_left(n2)
         A2.mid_get(cb, n2)
         A2.setl(n2, shared)
         A2.mid_set(cb, n2, shared)
-        A2.fill([0.0], n2)
+        A2.fill(n2)
         try:
             A2.check(n2)
         except AttributeError:
@@ -780,7 +778,8 @@ def test_exhausted_version_tags_take_the_slow_path(report):
 
 def test_closed_function_with_an_entry_global_sees_the_rebinding(report):
     r = same(report, "entry_global")
-    assert r["compiled"][1] == [2.0] and r["compiled"][3] == [5.0]
+    # fill returns SCALE: the old value before, the value the property getter set after
+    assert r["compiled"][0] == 2.0 and r["compiled"][1] == 5.0
 
 
 # --- behaviour: no dict watcher available ------------------------------------------------------------

python-multiplatform-ksp/src/test/python/test_pipeline.py

@@ -100,3 +100,38 @@ def test_demo_reports_identical_output(write, tmp_path):
     assert result.returncode == 0, result.stdout + result.stderr
     assert "identical output" in result.stdout and "249750.000" in result.stdout
     assert "compiled functions: total" in result.stdout
+
+
+ARRAY_CALLBACK = """\
+# typedpython: compiled
+from typing import Callable
+
+
+def first_after(xs: list[float], cb: Callable[[], None]) -> float:
+    cb()
+    return xs[0]
+
+
+def fill_then(xs: list[float], cb: Callable[[], None]) -> None:
+    xs[1] = 5.0
+    cb()
+"""
+
+
+def test_array_function_with_a_callback_matches_cpython(write, tmp_path):
+    # #203: the callback changes the list; a copy-in would read or write back stale values
+    path = write("arrcb.py", ARRAY_CALLBACK)
+    result = pipeline.compile_module(path, tmp_path / "out")
+    assert "first_after" in result.skipped and "fill_then" in result.skipped, result.skipped
+    compiled, ref = pipeline.load(result), interpreted(path)
+    for mod in (compiled, ref):
+        xs = [1.0, 2.0, 3.0]
+        def cb():
+            xs[0] = 100.0
+            xs[1] = 7.0
+        assert mod.first_after(xs, cb) == 100.0
+        ys = [1.0, 2.0, 3.0]
+        def cb2():
+            ys[1] = 9.0
+        mod.fill_then(ys, cb2)
+        assert ys == [1.0, 9.0, 3.0]

python-multiplatform-ksp/src/test/python/test_verify.py

@@ -843,14 +843,16 @@ def test_range_up_to_a_param_bounds_the_var_by_the_param_range():
 
 
 def test_len_bound_is_zero_to_two_to_the_62():
-    a = [ArrayParam("a", F64A, stored=False)]
+    # impure through a store into the array itself (an array function may not call user code, #203)
+    a = [ArrayParam("a", F64A, stored=True)]
     L = Len(I64, "a")
-    assert_accepted(fn(params=a, returns=I64, body=[IMPURE, Return(padd(L, i(2 ** 62 - 1)))]))
+    W = If(Compare(BOOL, CompareKind.GT, L, i(0)), (StoreIndex("a", i(0), f(0.0)),))
+    assert_accepted(fn(params=a, returns=I64, body=[W, Return(padd(L, i(2 ** 62 - 1)))]))
     assert_rejected(PROVEN, fn(params=a, returns=I64,
-                               body=[IMPURE, Return(padd(L, i(2 ** 62)))]))
-    assert_rejected(PROVEN, fn(params=a, returns=I64, body=[IMPURE, Return(pmul(L, i(2)))]))
-    assert_accepted(fn(params=a, returns=I64, body=[IMPURE, Return(pmul(L, i(-2)))]))  # -2**63
-    assert_rejected(PROVEN, fn(params=a, returns=I64, body=[IMPURE, Return(pmul(L, i(-3)))]))
+                               body=[W, Return(padd(L, i(2 ** 62)))]))
+    assert_rejected(PROVEN, fn(params=a, returns=I64, body=[W, Return(pmul(L, i(2)))]))
+    assert_accepted(fn(params=a, returns=I64, body=[W, Return(pmul(L, i(-2)))]))  # -2**63
+    assert_rejected(PROVEN, fn(params=a, returns=I64, body=[W, Return(pmul(L, i(-3)))]))
 
 
 def test_mod_and_floordiv_are_full_range_so_nothing_is_provable_from_them():
@@ -978,10 +980,10 @@ def test_entry_global_in_a_function_running_user_code_is_rejected(node):
 
 
 def test_the_same_open_node_without_entry_globals_is_accepted():
-    # The open node alone is fine: only entry globals need the function closed.
-    a = ArrayParam("a", F64A, stored=False)
-    assert_accepted(fn(params=[a], returns=F64, body=[
-        ExprStmt(CallObject(OBJ, Global(OBJ, "print"), (), ())), Return(Index(F64, "a", i(0)))]))
+    # The open node alone is fine: only entry globals (and array parameters, #203) need the
+    # function closed.
+    assert_accepted(fn(params=[Param("x", F64)], returns=F64, body=[
+        ExprStmt(CallObject(OBJ, Global(OBJ, "print"), (), ())), Return(lf("x"))]))
 
 
 def test_global_read_and_box_keep_a_function_closed():
@@ -1074,9 +1076,10 @@ def _base_functions():
                   Return(add(lf("s"), MathCall(F64, MathFunc.SQRT, (ToFloat(F64, li("c")),)),
                              F64)),
               ])
-    impure = fn("scale", params=[ArrayParam("a", F64A, stored=True), Param("x", F64),
-                                 Param("o", OBJ), Param("c", BOOL)],
-                locals_={"k": I64, "r": OBJ, "y": F64}, returns=F64, line=20, body=[
+    # a local array, not an ArrayParam: this function calls user code (#203)
+    impure = fn("scale", params=[Param("x", F64), Param("o", OBJ), Param("c", BOOL)],
+                locals_={"a": F64A, "k": I64, "r": OBJ, "y": F64}, returns=F64, line=20, body=[
+                    Assign("a", na(F64A, i(4), f(1.0))),
                     Assign("r", call_obj(GetAttr(OBJ, g_("Modifier"), "padding"),
                                          Box(OBJ, lf("x")), lo("o"), kw=("dp",))),
                     Assign("y", Unbox(F64, lo("r"))),
@@ -1147,6 +1150,10 @@ def _breaking_mutations(rng, g):
     if kind == 4:   # flip may_deopt
         return "flip may_deopt", dataclasses.replace(g, may_deopt=not g.may_deopt), {
             FLAG_DEOPT, DEOPT}
+    if kind == 5 and not any(isinstance(p, ArrayParam) for p in g.params):
+        kind = 3    # no ArrayParam (scale has a local array since #203): flip pure instead
+    if kind == 3:
+        return "flip pure", dataclasses.replace(g, pure=not g.pure), {FLAG_PURE, DEOPT, FLAG_DEOPT}
     if kind == 5:   # flip stored on the first array
         k = next(j for j, p in enumerate(g.params) if isinstance(p, ArrayParam))
         p = g.params[k]
@@ -1915,7 +1922,7 @@ def _array_functions():
                     StoreIndex("a", li("k"), BinOp(F64, BinOpKind.ADD, Index(F64, "z", li("k")),
                                                    f(1.0))),
                 )),
-                Assign("r", call_obj(g_("print"), Box(OBJ, Index(F64, "z", i(0))))),
+                Assign("r", Box(OBJ, Index(F64, "z", i(0)))),   # no user call: an ArrayParam (#203)
                 Return(),
             ])
     sink = fn("sink", params=[Param("x", OBJ)], returns=NONE, line=12, body=[Return()])
@@ -2021,3 +2028,40 @@ def test_each_array_mutation_kind_was_exercised():
     base = _array_functions()
     kinds = {_array_mutations(rng, base)[0] for _ in range(400)}
     assert len(kinds) == 18, sorted(kinds)     # 16 kinds; kinds 7 and 8 have two variants
+
+
+# --- #203: an ArrayParam function must be closed ---------------------------------------------
+
+ARRAY_OPEN = "verify/array-open"
+
+
+def _arr_fn(name, body, returns=F64, locals_=None, params_extra=()):
+    return fn(name, params=[ArrayParam("a", F64A, stored=False), *params_extra], returns=returns,
+              locals_=locals_, body=body)
+
+
+def test_array_function_calling_an_object_is_rejected():
+    # the callback can change the list the function copied in (#203)
+    assert_rejected(ARRAY_OPEN, _arr_fn("f", [ExprStmt(pycall("cb")), Return(Index(F64, "a", i(0)))]))
+
+
+def test_array_function_reading_an_attribute_is_rejected():
+    body = [ExprStmt(GetAttr(OBJ, lo("o"), "x")), Return(Index(F64, "a", i(0)))]
+    assert_rejected(ARRAY_OPEN, _arr_fn("f", body, params_extra=[Param("o", OBJ)]))
+
+
+def test_array_function_testing_an_object_is_rejected():
+    body = [If(Truth(BOOL, lo("o")), (Return(f(0.0)),)), Return(Index(F64, "a", i(0)))]
+    assert_rejected(ARRAY_OPEN, _arr_fn("f", body, params_extra=[Param("o", OBJ)]))
+
+
+def test_array_function_calling_an_open_function_is_rejected():
+    g = fn("g", returns=NONE, body=[ExprStmt(pycall("cb")), Return()])
+    caller = _arr_fn("f", [ExprStmt(Call(NONE, "g", ())), Return(Index(F64, "a", i(0)))])
+    assert_rejected(ARRAY_OPEN, caller, g)
+
+
+def test_closed_array_function_is_still_accepted():
+    g = fn("g", params=[Param("x", F64)], returns=F64, body=[Return(lf("x"))])
+    caller = _arr_fn("f", [Return(Call(F64, "g", (Index(F64, "a", i(0)),)))])
+    assert_accepted(caller, g)