Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix If type bug #329

Merged
merged 2 commits into from
May 9, 2022
Merged

Fix If type bug #329

merged 2 commits into from
May 9, 2022

Conversation

jasonpaulos
Copy link
Member

@jasonpaulos jasonpaulos commented May 6, 2022

PyTeal If statements have the following rules:

  1. If a then branch (what executes when the condition is true) and an else branch (what executes when the condition is false) exist, then both must return the same TealType.
  2. If a then branch exists and an else branch does not, then the then branch must return TealType.none, since in this case the else branch is essentially Seq() (a noop expression that returns nothing).

These rules are properly enforced for the original If syntax (If(<condition>, <then>, <else>)), but the new syntax (If(<condition>).Then(<then>).Else(<else>)) unfortunately does not enforce these rules. This PR fixes that.

@jasonpaulos jasonpaulos marked this pull request as ready for review May 6, 2022 20:57
@@ -37,7 +37,7 @@ def __test_single_conditional(
ifExpr = (
pt.If(expr.output_slots[1].load())
.Then(expr.output_slots[0].load())
.Else(pt.Bytes("None"))
.Else(pt.App.globalGet(pt.Bytes("None")))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unit test was previously passing despite its then and else branches returning different types for some iterations.

To fix it, I've made the else branch return App.globalGet, which is a TealType.any, so it will be correct for all then branches unless the then branch is TealType.none.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I believe @algoidurovic wrote this test originally, so I'm curious if he has any thoughts about changing this test.

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one comment

pyteal/ast/if_.py Show resolved Hide resolved
@jasonpaulos jasonpaulos merged commit 22deba5 into master May 9, 2022
@jasonpaulos jasonpaulos deleted the fix-if-bug branch May 9, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants