hedyorg / hedy

Hedy is a gradual programming language to teach children programming. Gradual languages use different language levels, where each level adds new concepts and syntactic complexity. At the end of the Hedy level sequence, kids master a subset of syntactically valid Python.
https://www.hedy.org
European Union Public License 1.2
1.3k stars 284 forks source link

🪲 If-pressed assignment does not work in functions #5749

Open boryanagoncharenko opened 2 weeks ago

boryanagoncharenko commented 2 weeks ago

Describe the bug In level 12, the following program outputs a instead of great or not great.

define turnz
    x is 'a'
    if x is pressed
        x is 'great'
    else
        x is 'not great'
    print x

call turnz

The trouble is that the if-pressed uses a global variable x while the function uses a local one. We could:

boryanagoncharenko commented 1 week ago

Confirmed in the contributors meeting that functions should not use a global scope. This means that we are left with 2 options:

boryanagoncharenko commented 1 week ago

Perhaps there is another way to support if-pressed statements in functions. We could put all variables in the current scope (global if not in a function or local if in a function) in a dictionary. Every variable definition, assignment or reference will just be substituted with access to the dictionary. In this way we will be able to bridge the gap between if-pressed definitions and their outer scope and still leave the local scope of functions intact. This is an example of using if-pressed not in a function:

__gs = dict()
__gs['var1'] = 1
__gs['var2'] = 10
def if_pressed_handler():
    __gs['var1'] = 2
    __gs['var2'] = __gs['var2'] + 1
    __gs['var3'] = 15
if_pressed_handler()
print(f'var1 is reassigned in if-pressed. Expected value is 2. Actual value is {__gs["var1"]}.')
print(f'var2 is accessed and reassigned in if-pressed. Expected value is 11. Actual value is {__gs["var2"]}.')
print(f'var3 is defined in if-pressed. Expected value is 15. Actual value is {__gs["var3"]}.')

And this is how if-pressed would appear in a function:

def turnz():
    __ls = dict()
    __ls['x'] = 'a'
    def if_pressed_x():
        __ls['x'] = 'great'
    def if_pressed_else():
        __ls['x'] = 'not great'
    if_pressed_x() # simulates pressing x
    print(__ls['x'])
turnz()

We could only use this approach if the program contains an if-pressed command. In this way we will limit the scope of the change. @jpelay what do you think? Do you see issues with this approach?

jpelay commented 1 week ago

Hi @boryanagoncharenko I think your idea is great, you're right with something like that I think we can sort the global scope situation. Although I tried this code on Skulpt and it didn't really work:

def turnz():
  __ls = dict()
  __ls['x'] = 'a'
  if_pressed_mapping = {"else": "if_pressed_default_else"}
  global if_pressed_x_
  if_pressed_mapping['x'] = 'if_pressed_x_'
  def if_pressed_x_():
    __ls['x'] = 'great'    
  global if_pressed_else_
  if_pressed_mapping['else'] = 'if_pressed_else_'
  def if_pressed_else_():    
    __ls['x'] = 'not great'      
  extensions.if_pressed(if_pressed_mapping)
  print(f'''{__ls['x']}''')
turnz()

Hopefully you can get it to work!

jpelay commented 1 week ago

The above snippet didn't work, because Skulpt doesn't like variables that start with double underscores, I think. Anyways, changing the snippet to this manages to get it working:

if_pressed_mapping = {"else": "if_pressed_default_else"}
def turnz():
  local_variables_xxxx = dict()
  local_variables_xxxx['x'] = 'a'
  if_pressed_mapping = {"else": "if_pressed_default_else"}
  global if_pressed_x_
  if_pressed_mapping['x'] = 'if_pressed_x_'
  def if_pressed_x_():
    local_variables_xxxx['x'] = 'great'
  global if_pressed_else_
  if_pressed_mapping['else'] = 'if_pressed_else_'
  def if_pressed_else_():    
    local_variables_xxxx['x'] = 'not great'      
  extensions.if_pressed(if_pressed_mapping)
  print(f'''{local_variables_xxxx['x']}''')
turnz()

Here's a video:

https://github.com/user-attachments/assets/e47fc43e-5f46-4d57-9a98-594a130fc443

I think the idea of isolating the dictionary to functions when using ifpressed is a good idea. How would it work with parameters and such? The same?

boryanagoncharenko commented 1 week ago

Thanks so much for pointing this out!

I have not thought about the double underscore prefix. My assumption is that Skulpt maintains everything in the global scope but it does not do much for local scopes. I tried looking into implementing support for the nonlocal keyword but it is hard because only the global scope seems to be accessible and correct.

My guess is that defining the __ls variable within the turnz function made it a local variable, so it was never added to the Sk.globals list and, in turn, could not be used in the if-pressed functions. The problem seems to be gone if (do not laugh please!) the local scope is defined globally.

global __ls
__ls = dict()
def turnz():
  __ls['x'] = 'a'
  if_pressed_mapping = {"else": "if_pressed_default_else"}
  global if_pressed_x_
  if_pressed_mapping['x'] = 'if_pressed_x_'
  def if_pressed_x_():
    global __ls
    __ls['x'] = 'great'
  global if_pressed_else_
  if_pressed_mapping['else'] = 'if_pressed_else_'
  def if_pressed_else_():
    global __ls
    __ls['x'] = 'not great'
  extensions.if_pressed(if_pressed_mapping)
  print(f'''{__ls['x']}''')
turnz()

Now the output becomes great / not great which is the correct behavior.

boryanagoncharenko commented 1 week ago

You are right, starting with double underscore basically Skulpt fail to create a closure from one local scope to another. Thanks, once again!

jpelay commented 1 week ago

You are right, starting with double underscore basically Skulpt fail to create a closure from one local scope to another. Thanks, once again!

Yes, it was a crazy bug!!!