DynamicScratchVar: User-defined DynamicScratchVars in @Subroutines produce opaque errors

michaeldiamant commented 2 years ago

Problem

Starting assumption:

Given: PyTeal application defines a method that meets the criteria for being annotated as @Subroutine.
When: The application developer annotates the method with @Subroutine.
- And: Re-compiles TEAL source.
Then: Generated TEAL source differs from Given TEAL source, but the application execution is unchanged.

With the assumption in mind, consider https://github.com/michaeldiamant/pyteal/pull/3/.

When retrieve_and_increment is not annotated with @Subroutine, PyTeal generates TEAL source.
When retrieve_and_increment is annotated with @Subroute, PyTeal fails TEAL source generation.
- Generated error: test_run.log

The problem might be two-fold:

Given the starting assumption, the example ought to compile.
The user-provided error might be too opaque to debug. The PyTeal app developer must understand DynamicScratchVar compiler implementation details.

Solution

Several options exist:

Attempt to enable PyTeal to complete TEAL source generation by relaxing constraints causing set_index error.

https://github.com/algorand/pyteal/blob/f2598da3dec6041051df4442cd9cc7e3c373674d/pyteal/ast/scratchvar.py#L100-L105
- My understanding is: The constraint exists to simplify the problem space and we haven't assessed implications of loosing the constraint.
Break the starting assumption and make no attempt to support successful compilation. Instead, either:
- Provide a more context aware error message to make the debugging experience easier. For example, the error message may suggest alternative implementation options to address the issue.
- Explicitly prevent any user-defined DynamicScratchVar use in @Subroutine methods. By constraining usage, it might be easier to provide a context-aware error to the user.

Dependencies

N/A

Urgency

TBD - Unclear how often users will run into the class of problem and find it challenging to debug the error message.

tzaffi commented 2 years ago

If this is a "pure research" ticket aimed mostly at seeing the viability of just removing the warning and proposing the next steps then this shouldn't be too onerous (probably 1-2 points).

tzaffi commented 2 years ago

Good news. When I loosened the restriction on using DynamicScratchVar's and made a slight tweak to the included program (there was an off-by-1 error in asserts() the program passed with flying colors. Here's the tweak that worked:

def should_it_work() -> Expr:
    xs = [
        ScratchVar(TealType.uint64),
        ScratchVar(TealType.uint64),
    ]

    def store_initial_values():
        return [s.store(Int(i + 1)) for i, s in enumerate(xs)]

    d = DynamicScratchVar(TealType.uint64)

    @Subroutine(TealType.none)
    def retrieve_and_increment(s: ScratchVar):
        return Seq(d.set_index(s), d.store(d.load() + Int(1)))

    def asserts():
        return [Assert(x.load() == Int(i + 2)) for i, x in enumerate(xs)]

    return Seq(
        Seq(store_initial_values()),
        Seq([retrieve_and_increment(x) for x in xs]),
        Seq(asserts()),
        Int(1),
    )

michaeldiamant commented 2 years ago

@tzaffi Thanks for taking a closer look. I reflected your findings into #242.

Closing - See #242 for rationale.

algorand / pyteal