loop #95

shiqizng · 2021-08-10T17:28:56Z

This PR adds While and For loops to Pyteal.

…o feature/loop

This reverts commit a0d67cc.

jasonpaulos

Nice work, nested loops and continue/break seem to be working as expected! My primary concern is about the sorting function, which I explain more below.

jasonpaulos · 2021-08-18T18:27:23Z

pyteal/ast/while_.py

+        return "(While {} {})".format(self.cond, self.doBlock)
+
+    def type_of(self):
+        if self.doBlock is None:


type_of is meant to indicate which type, if any, the expression pushes to the stack when it finishes, so I think this should return TealType.none instead.

jasonpaulos · 2021-08-18T18:29:01Z

pyteal/ast/while_.py

+        return self.doBlock.type_of()
+
+    def Do(self, doBlock: Seq):
+        self.doBlock = doBlock


We don't want the doBlock to end with additional things on the stack (e.g. While(cond).Do(Int(1)) should be invalid), so could you add the check require_type(doBlock.type_of(), TealType.none) here?

Also, could you change the type annotation for doBlock from Seq to Expr? I expect most of the time a Seq will be used, but other expressions can be used as well.

jasonpaulos · 2021-08-18T23:33:07Z

pyteal/compiler/sort.py

-                if i == 0:
-                    S.insert(0, m)
-                else:
+        if type(n) == TealConditionalBlock:


Is there a reason for the extra complexity here? I tested locally and this simpler version worked just as well, plus it is nearly identical to the old code generation when loops are not present:

S = [start] order = [] visited = set() # I changed visited to a set to be more efficient while len(S) != 0: n = S.pop() if id(n) in visited: continue S += n.getOutgoing() order.append(n) visited.add(id(n))

^ The previous version should be good here, even if For, While introduce cycles to the graph...

jasonpaulos · 2021-08-18T23:53:38Z

pyteal/compiler/sort.py

+
+        order.append(n)
+        visited.append(n)
+


Also, I realized there is a potential bug when a non-topological sort is used for this function. If there is a "diamond" graph structure, i.e. a diverging branch which later converges, like so:

start / \ true false \ / end

then one potential ordering is [start, true, end, false], which translates to the TEAL code:

start: ... bz false true: ... end: ... false: ... b end

This is a problem because if end does not contain a return op, the ops from false will be executed after end finishes.

To fix this, sortBlocks should take an additional argument, end, which is the ending node of the graph. Then, after sorting the blocks into the list order, we need to make the end block the last element in the list. This guarantees no code can accidentally execute after it.

Adding this code before the return statement should do it:

endIndex = -1 for i, block in enumerate(order): if end is block: endIndex = i break if endIndex == -1: raise TealInternalError("End block not present") order.pop(endIndex) order.append(end)

you're correct. I simplified the algorithm as suggested.

jasonpaulos · 2021-08-19T00:31:24Z

pyteal/ir/tealsimpleblock.py


    def __eq__(self, other: object) -> bool:
+        # check for loop
+        if self in self.blocks:


I don't love that these functions need to modify the object in order to work, but I can't think of a better solution right now -- I might make a follow up PR if I can think of something better.

In any case, self.blocks and self.visited do not need to be lists, since they only ever contain self or nothing. A cleaner solution would be to make a single self.visited boolean attribute, and the functions can set this to true/false to indicate whether self has been visited.

algochoi

Looking good, just had some small questions and comments - I think we might want to run some formatter (e.g. black) before pushing to make spacing/line breaks more consistent and delete any commented out code blocks.

algochoi · 2021-08-19T15:38:51Z

pyteal/ast/for_.py

+
+    def type_of(self):
+        if str(self.doBlock) == str(Seq([Int(0)])):
+            raise TealCompileError("For expression must have a thenBranch", self) 


Is there a test for this branch? Also is this just checking if the doBlock is False or 0 (and if so do we just want to check for None here)?

yes, it should be checking for None.

pyteal/ast/for_test.py

pyteal/compiler/compiler_test.py

algochoi · 2021-08-19T16:15:05Z

examples/signature/split.teal

-&&
-b l3
-l2:
+bz l2


Are these auto-generated from the pyteal file?

Yep, it seems Shiqi updated split.teal and dutch_auction.teal with new generated version.

ahangsu

Looks good, I left some minor comments here and there for now.

pyteal/ast/for_.py

pyteal/ast/while_.py

pyteal/ast/for_.py

ahangsu · 2021-08-19T17:27:31Z

pyteal/compiler/sort.py

-                if i == 0:
-                    S.insert(0, m)
-                else:
+        if type(n) == TealConditionalBlock:


^ The previous version should be good here, even if For, While introduce cycles to the graph...

algochoi

The new formatting is much easier to read -- just one question about passing variables within CompileOptions

pyteal/ast/break_test.py

pyteal/compiler/compiler.py

jasonpaulos

After these are addressed, this looks good to merge

jasonpaulos · 2021-08-20T16:47:38Z

pyteal/ast/break_test.py

+    expr = While(items[0]).Do(items[1])
+    actual, _ = expr.__teal__(options)
+
+    options.currentLoop = expr


The code from this line downward does not seem to be needed, since it doesn't check anything new.

A more appropriate test would probably be to set options.currentLoop = expr, then do start, _ = items[1].__teal__(options) and verify options.breakBlocks contains only start

jasonpaulos · 2021-08-20T16:48:32Z

pyteal/ast/continue_test.py

+    expr = While(items[0]).Do(items[1])
+    actual, _ = expr.__teal__(options)
+
+    options.currentLoop = expr


The above comment applies here too.

jasonpaulos · 2021-08-20T16:51:10Z

pyteal/ast/for_.py

+
+        for block in options.continueBlocks:
+            block.setNextBlock(stepStart)
+            doEnd.setNextBlock(block)


I dont't think this line should be here?

jasonpaulos · 2021-08-20T16:54:55Z

pyteal/ast/for_.py

+        condEnd.setNextBlock(branchBlock)
+        condStart.addIncoming(doStart)
+
+        startEnd.nextBlock = condStart


condStart.addIncoming(doStart) seems unnecessary here, since compileTeal invokes addIncoming on the start block of the entire AST.

Also, can you use setNextBlock() instead of directly setting startEnd.nextBlock?

jasonpaulos · 2021-08-20T16:57:18Z

pyteal/ast/for_.py

+
+    def type_of(self):
+        if self.doBlock is None:
+            raise TealCompileError("For expression must have a thenBranch", self)


nit: some places refer to the doBlock as thenBranch, can you update these?

jasonpaulos · 2021-08-20T17:21:15Z

pyteal/ast/while_test.py

+    expr.__teal__(options)
+
+
+def test_while():


Again, this test is really good, and this file could use 2 more versions of it which test continue and break.

jasonpaulos · 2021-08-20T17:43:47Z

pyteal/compiler/compiler_test.py

+                Seq(
+                    [
+                        For(
+                            i.store(Int(0)),


This should be j.store(Int(0))

jasonpaulos · 2021-08-20T17:47:42Z

pyteal/compiler/compiler_test.py

+                            i.store(Int(0)),
+                            j.load() < Int(4),
+                            j.store(j.load() + Int(2)),
+                        ).Do(Seq([j.store(Int(1))]))


Could you change the body to App.globalPut(Itob(j.load()), j.load() * Int(2)) or similar so this would not be an infinite loop?

jasonpaulos · 2021-08-20T18:07:11Z

pyteal/compiler/sort.py

+
+    endIndex = -1
+    for i, block in enumerate(order):
+        if block == end:


Can you check if block is end instead? This is a slightly stronger check, since we want to make sure we find the end block exactly, not a different block which happens to have the same ops as the end block, which is what == checks.

jasonpaulos · 2021-08-20T18:08:37Z

pyteal/ir/tealsimpleblock.py

@@ -10,6 +10,8 @@ class TealSimpleBlock(TealBlock):
    def __init__(self, ops: List[TealOp]) -> None:
        super().__init__(ops)
        self.nextBlock: Optional[TealBlock] = None
+        self.blocks: List[TealBlock] = []


This variable isn't needed anymore

jasonpaulos

Looks good!

ahangsu

Should be good to go

shiqizng added 11 commits August 6, 2021 11:44

adding while loop

080e9ee

basic while loop working

3278f9f

split failing

e32090a

compile tests work

4a504d1

fix sort tests

ddf4596

test fix and added for

f220493

rm lines

a228bc5

Merge branch 'feature/loop' of https://github.com/algorand/pyteal int…

9300d08

…o feature/loop

revert teal simple block change

b35e40f

update if check

f981278

test updates

b240d45

jasonpaulos linked an issue Aug 11, 2021 that may be closed by this pull request

Add looping #72

Closed

shiqizng added 13 commits August 11, 2021 16:43

added tests, fixed nested loop

8509293

disable slot validation

572d6d7

fix tests

a0d67cc

index on loop: a0d67cc fix tests

1e67815

WIP on loop: a0d67cc fix tests

459cb87

Revert "fix tests"

ac83095

This reverts commit a0d67cc.

sort test fix

cdd08a9

fix mypy tests

03d0c69

fix unit tests

7365703

fix unit tests

e24c8c9

add continue,break

62439ec

add break, continue

d6fc42c

add new lines

6eb6388

shiqizng changed the title ~~[WIP] loop~~ loop Aug 18, 2021

shiqizng requested review from jasonpaulos, algochoi and ahangsu August 18, 2021 15:41

shiqizng marked this pull request as ready for review August 18, 2021 15:41

jasonpaulos suggested changes Aug 19, 2021

View reviewed changes

algochoi reviewed Aug 19, 2021

View reviewed changes

ahangsu suggested changes Aug 19, 2021

View reviewed changes

shiqizng added 6 commits August 20, 2021 09:27

addressing reviews

247f6d5

Merge commit 'formatting' into feature/loop

105374c

formatting

13e0f0d

Merge branch 'review' into feature/loop

6d17e63

error fix

e05581e

format

23c93b0

algochoi reviewed Aug 20, 2021

View reviewed changes

pyteal/ast/break_test.py Show resolved Hide resolved

pyteal/compiler/compiler.py Show resolved Hide resolved

jasonpaulos suggested changes Aug 20, 2021

View reviewed changes

addressing reviews

48e46f8

jasonpaulos approved these changes Aug 23, 2021

View reviewed changes

ahangsu approved these changes Aug 23, 2021

View reviewed changes

shiqizng merged commit 8ad44a4 into master Aug 23, 2021

jasonpaulos mentioned this pull request Aug 31, 2021

v0.8.0 #105

Merged

ahangsu deleted the feature/loop branch March 2, 2022 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loop #95

loop #95

shiqizng commented Aug 10, 2021 •

edited

Loading

jasonpaulos left a comment

jasonpaulos Aug 18, 2021

jasonpaulos Aug 18, 2021

jasonpaulos Aug 18, 2021

ahangsu Aug 19, 2021

jasonpaulos Aug 18, 2021

shiqizng Aug 20, 2021

jasonpaulos Aug 19, 2021

algochoi left a comment

algochoi Aug 19, 2021

shiqizng Aug 20, 2021

algochoi Aug 19, 2021

ahangsu Aug 19, 2021

ahangsu left a comment

ahangsu Aug 19, 2021

algochoi left a comment

jasonpaulos left a comment

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos Aug 20, 2021

jasonpaulos left a comment

ahangsu left a comment

loop #95

loop #95

Conversation

shiqizng commented Aug 10, 2021 • edited Loading

jasonpaulos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algochoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahangsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algochoi left a comment

Choose a reason for hiding this comment

jasonpaulos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonpaulos left a comment

Choose a reason for hiding this comment

ahangsu left a comment

Choose a reason for hiding this comment

shiqizng commented Aug 10, 2021 •

edited

Loading