ameliatastic / seahorse-lang

Write Anchor-compatible Solana programs in Python
Apache License 2.0
327 stars 48 forks source link

Issue with variable assignment within different scopes #73

Closed miguel-ot closed 1 year ago

miguel-ot commented 1 year ago

Variables are not correctly assigned if they are defined in a certain level and their value is changed in a different one. For example, consider the following instruction:

@instruction
def test(signer: Signer, number: f64):
   myboolean=True
   newnumber=1.0
   if number > 0.0:
      myboolean=False
      newnumber=10.0
      print(f'Your boolean is {myboolean}. Extra number: {newnumber}.')
   print(f'Your number is {number}.')
   print(f'Your boolean is {myboolean}. Extra number: {newnumber}.')

When the previous instruction is executed with the number 5.0 as an input, the output is

Program log: Instruction: Test
Program log: Your boolean is false. Extra number: 10.
Program log: Your number is 5.
Program log: Your boolean is true. Extra number: 1.

which is clearly an undesired behaviour.

miguel-ot commented 1 year ago

The previous issue does not occur if a list is used instead of a variable and a value of the list is changed within the if block. For example, if you define mylist=[1.0] before the if block and you change the value with mylist[0]=10.0 inside the if block, the change is still in place after the if block.

mcintyre94 commented 1 year ago

The example here compiles to Rust:

# program.rs
pub fn test<'info>(mut signer: SeahorseSigner<'info, '_>, mut number: f64) -> () {
    let mut myboolean = true;
    let mut newnumber = 1f64;

    if number > 0f64 {
        let mut myboolean = false;
        let mut newnumber = 10f64;

        solana_program::msg!(
            "{}",
            format!(
                "Your boolean is {}. Extra number: {}.",
                myboolean, newnumber
            )
        );
    }

    solana_program::msg!("{}", format!("Your number is {}.", number));

    solana_program::msg!(
        "{}",
        format!(
            "Your boolean is {}. Extra number: {}.",
            myboolean, newnumber
        )
    );
}

The problem is that we redefine myboolean and newnumber in the inner scope, rather than updating them. This looks kinda similar to a case described in the core readme: https://github.com/ameliatastic/seahorse-lang/tree/main/src/core#aside---uniting-the-programming-models-of-python-and-rust

My gut instinct is that we should mutate a variable that is already defined in an outer scope, rather than redefining it. This would give the Python behaviour where mutations in an inner scope also occur outside. But I'm not sure how big a change that would be or if there are cases where that isn't the right thing to do.

mcintyre94 commented 1 year ago

As a workaround this should be equivalent (in final variable values):

def test(signer: Signer, number: f64):
    myboolean = number <= 0.0
    newnumber = 10.0 if number > 0.0 else 1.0

    print(f'Your number is {number}.')
    print(f'Your boolean is {myboolean}. Extra number: {newnumber}.')
ameliatastic commented 1 year ago

I think I found the problem already, it was likely caused when I added support for tuples (which happened well before v2 was published). Here's the story if you're curious!

Basically, the logic for handling tuples is a little bit complicated - you might have program like:

x = 2
if condition:
  (x, y) = (3, 5)

There's no "simple" Rust translation for these statements. In the third line, you would need to simultaneously assign to x and declare y, which can't be done with one let statement or one assignment. So, the way that Seahorse handles it is to split it into two statements: one that declares the "undeclared" variables (y), and one that actually assigns to the tuple:

let mut x = 2;
if condition {
  let mut y;
  (x, y) = (3, 5);
}

This is how tuples work right now (and from what I can tell, this logic isn't broken). The problem came when I switched to the new system of handling assignments - I added two separate paths for handling single variables and tuples, and the single variable case wasn't updated properly. So that's how this happened. The fix is pretty simple, I'll PR in a couple minutes.