ScratchVar: Disallow @subroutine Recursion (#261)

* Recursive by-ref treatment: Impose more constraints - Disallow all recursion for any ScratchVar in any `@Subroutine`
* `DynamicScratchVar` treatment: Leave as is with explanatory inline comment explaining why it's like that
* Break down associated integration tests in `tests/pass_by_ref_test.py` to smaller components
* Add documentation regarding illegality of pass-by-ref recursion
* Add a simple unit test to `pyteal/compiler/subroutines_test.py` that tests the new functionality of `spillLocalSlotsDuringRecursion`

Author: tzaffi

.gitignore

@@ -132,3 +132,4 @@ dmypy.json
 
 # IDE
 .idea
+.vscode

docs/control_structures.rst

@@ -364,6 +364,27 @@ argument is even, but uses recursion to do so.
                 .Else(recursiveIsEven(i - Int(2)))
             )
 
+Recursion and `ScratchVar`'s
+----------------------------
+
+Recursion with parameters of type `ScratchVar` is disallowed. For example, the following
+subroutine is considered illegal and attempting compilation will result in a `TealInputError`:
+
+.. code-block:: python
+
+        @Subroutine(TealType.none)
+        def ILLEGAL_recursion(i: ScratchVar):
+            return (
+                If(i.load() == Int(0))
+                .Then(i.store(Int(1)))
+                .ElseIf(i.load() == Int(1))
+                .Then(i.store(Int(0)))
+                .Else(Seq(i.store(i.load() - Int(2)), ILLEGAL_recursion(i)))
+            )
+
+
+
+
 Exiting Subroutines
 -------------------
 

pyteal/ast/scratchvar.py

@@ -59,7 +59,7 @@ def index(self) -> Expr:
 class DynamicScratchVar(ScratchVar):
     """
     Example of Dynamic Scratch space whereby the slot index is picked up from the stack:
-        .. code-block:: python1
+        .. code-block:: python
 
             player_score = DynamicScratchVar(TealType.uint64)
 
@@ -97,6 +97,11 @@ def set_index(self, index_var: ScratchVar) -> Expr:
         Followup `store`, `load` and `index` operations will use the provided `index_var` until
         `set_index()` is called again to reset the referenced ScratchVar.
         """
+        # Explanatory comment per Issue #242: Preliminary evidence shows that letting users
+        # pass in any ScratchVar subtype (i.e. DynamicScratchVar) may in fact work.
+        # However, we are leaving this guard in place pending further investigation.
+        # TODO: gain confidence that DynamicScratchVar can be used here and
+        # modify the below strict type equality to `isinstance(index_var, ScratchVar)`
         if type(index_var) is not ScratchVar:
             raise TealInputError(
                 "Only allowed to use ScratchVar objects for setting indices, but was given a {}".format(

pyteal/compiler/subroutines.py

@@ -3,6 +3,7 @@
 from collections import OrderedDict
 from itertools import chain
 
+from ..errors import TealInputError
 from ..types import TealType
 from ..ast import SubroutineDefinition
 from ..ir import TealComponent, TealOp, Op
@@ -11,7 +12,7 @@
 Node = TypeVar("Node")
 
 
-def depthFirstSearch(graph: Dict[Node, Set[Node]], start: Node, end: Node) -> bool:
+def graph_search(graph: Dict[Node, Set[Node]], start: Node, end: Node) -> bool:
     """Check whether a path between start and end exists in the graph.
 
     This works even if start == end, in which case True is only returned if the
@@ -56,12 +57,38 @@ def findRecursionPoints(
         reentryPoints[subroutine] = set(
             callee
             for callee in subroutineGraph[subroutine]
-            if depthFirstSearch(subroutineGraph, callee, subroutine)
+            if graph_search(subroutineGraph, callee, subroutine)
         )
 
     return reentryPoints
 
 
+def find_recursive_path(
+    subroutine_graph: Dict[SubroutineDefinition, Set[SubroutineDefinition]],
+    subroutine: SubroutineDefinition,
+) -> List[SubroutineDefinition]:
+    visited = set()
+    loop = []
+
+    def dfs(x):
+        if x in visited:
+            return False
+
+        visited.add(x)
+        loop.append(x)
+        for y in subroutine_graph[x]:
+            if y == subroutine:
+                loop.append(y)
+                return True
+            if dfs(y):
+                return True
+        loop.pop()
+        return False
+
+    found = dfs(subroutine)
+    return loop if found else []
+
+
 def spillLocalSlotsDuringRecursion(
     version: int,
     subroutineMapping: Dict[Optional[SubroutineDefinition], List[TealComponent]],
@@ -90,6 +117,23 @@ def spillLocalSlotsDuringRecursion(
     """
     recursivePoints = findRecursionPoints(subroutineGraph)
 
+    recursive_byref = None
+    for k, v in recursivePoints.items():
+        if v and k.by_ref_args:
+            recursive_byref = k
+            break
+
+    if recursive_byref:
+        msg = "ScratchVar arguments not allowed in recursive subroutines, but a recursive call-path was detected: {}()"
+        raise TealInputError(
+            msg.format(
+                "()-->".join(
+                    f.name()
+                    for f in find_recursive_path(subroutineGraph, recursive_byref)
+                )
+            )
+        )
+
     coverAvailable = version >= Op.cover.min_version
 
     for subroutine, reentryPoints in recursivePoints.items():

pyteal/compiler/subroutines_test.py

@@ -1,5 +1,7 @@
 from collections import OrderedDict
 
+import pytest
+
 from .. import *
 
 from .subroutines import (
@@ -1277,6 +1279,94 @@ def subImpl(a1, a2, a3):
     }
 
 
+def test_spillLocalSlotsDuringRecursion_recursive_with_scratchvar():
+    # modifying test_spillLocalSlotsDuringRecursion_multiple_subroutines_no_recursion()
+    # to be recursive and fail due to by-ref args
+    def sub1Impl(a1):
+        return None
+
+    def sub2Impl(a1, a2: ScratchVar):
+        return None
+
+    def sub3Impl(a1, a2, a3):
+        return None
+
+    subroutine1 = SubroutineDefinition(sub1Impl, TealType.uint64)
+    subroutine2 = SubroutineDefinition(sub2Impl, TealType.uint64)
+    subroutine3 = SubroutineDefinition(sub3Impl, TealType.none)
+
+    subroutine1L1Label = LabelReference("l1")
+    subroutine1Ops = [
+        TealOp(None, Op.store, 0),
+        TealOp(None, Op.load, 0),
+        TealOp(None, Op.int, 0),
+        TealOp(None, Op.eq),
+        TealOp(None, Op.bnz, subroutine1L1Label),
+        TealOp(None, Op.err),
+        TealLabel(None, subroutine1L1Label),
+        TealOp(None, Op.int, 1),
+        TealOp(None, Op.callsub, subroutine3),
+        TealOp(None, Op.retsub),
+    ]
+
+    subroutine2L1Label = LabelReference("l1")
+    subroutine2Ops = [
+        TealOp(None, Op.store, 1),
+        TealOp(None, Op.load, 1),
+        TealOp(None, Op.int, 0),
+        TealOp(None, Op.eq),
+        TealOp(None, Op.bnz, subroutine2L1Label),
+        TealOp(None, Op.err),
+        TealLabel(None, subroutine2L1Label),
+        TealOp(None, Op.int, 1),
+        TealOp(None, Op.retsub),
+    ]
+
+    subroutine3Ops = [
+        TealOp(None, Op.retsub),
+    ]
+
+    l1Label = LabelReference("l1")
+    mainOps = [
+        TealOp(None, Op.txn, "Fee"),
+        TealOp(None, Op.int, 0),
+        TealOp(None, Op.eq),
+        TealOp(None, Op.bz, l1Label),
+        TealOp(None, Op.int, 100),
+        TealOp(None, Op.callsub, subroutine1),
+        TealOp(None, Op.return_),
+        TealLabel(None, l1Label),
+        TealOp(None, Op.int, 101),
+        TealOp(None, Op.callsub, subroutine2),
+        TealOp(None, Op.return_),
+    ]
+
+    subroutineMapping = {
+        None: mainOps,
+        subroutine1: subroutine1Ops,
+        subroutine2: subroutine2Ops,
+        subroutine3: subroutine3Ops,
+    }
+
+    subroutineGraph = {
+        subroutine1: {subroutine2},
+        subroutine2: {subroutine1},
+        subroutine3: set(),
+    }
+
+    localSlots = {None: set(), subroutine1: {0}, subroutine2: {1}, subroutine3: {}}
+
+    with pytest.raises(TealInputError) as tie:
+        spillLocalSlotsDuringRecursion(
+            5, subroutineMapping, subroutineGraph, localSlots
+        )
+
+    assert (
+        "ScratchVar arguments not allowed in recursive subroutines, but a recursive call-path was detected: sub2Impl()-->sub1Impl()-->sub2Impl()"
+        in str(tie)
+    )
+
+
 def test_resolveSubroutines():
     def sub1Impl(a1):
         return None

tests/compile_asserts.py

@@ -4,9 +4,9 @@
 from pyteal.ir import Mode
 
 
-def compile_and_save(approval):
+def compile_and_save(approval, version):
     teal = Path.cwd() / "tests" / "teal"
-    compiled = compileTeal(approval(), mode=Mode.Application, version=6)
+    compiled = compileTeal(approval(), mode=Mode.Application, version=version)
     name = approval.__name__
     with open(teal / (name + ".teal"), "w") as f:
         f.write(compiled)
@@ -38,8 +38,7 @@ def assert_teal_as_expected(path2actual, path2expected):
 
         assert len(elines) == len(
             alines
-        ), f"""EXPECTED {len(elines)} lines for {path2expected}
-but ACTUALLY got {len(alines)} lines in {path2actual}"""
+        ), f"""EXPECTED {len(elines)} lines for {path2expected} but ACTUALLY got {len(alines)} lines in {path2actual}"""
 
         for i, actual in enumerate(alines):
             expected = elines[i]
@@ -56,18 +55,15 @@ def assert_teal_as_expected(path2actual, path2expected):
 """
 
 
-def assert_new_v_old(approve_func):
-    try:
-        teal_dir, name, compiled = compile_and_save(approve_func)
+def assert_new_v_old(approve_func, version):
+    teal_dir, name, compiled = compile_and_save(approve_func, version)
 
-        print(
-            f"""Compilation resulted in TEAL program of length {len(compiled)}. 
-    To view output SEE <{name}.teal> in ({teal_dir})
-    --------------"""
-        )
+    print(
+        f"""Compilation resulted in TEAL program of length {len(compiled)}. 
+To view output SEE <{name}.teal> in ({teal_dir})
+--------------"""
+    )
 
-        path2actual = teal_dir / (name + ".teal")
-        path2expected = teal_dir / (name + "_expected.teal")
-        assert_teal_as_expected(path2actual, path2expected)
-    except Exception as e:
-        assert not e, f"failed to ASSERT NEW v OLD for {approve_func.__name__}: {e}"
+    path2actual = teal_dir / (name + ".teal")
+    path2expected = teal_dir / (name + "_expected.teal")
+    assert_teal_as_expected(path2actual, path2expected)