🪲 Fix Key Pressed adventure in level 9

[boryanagoncharenko]

Fixes #5679

There are 2 separate issues resolved with this PR:

1. Variables cannot be reassigned in the if_pressed command. This is caused by a logical error that was introduced unintentionally a couple of months ago during a refactoring. The core of the issue is that the body of the if_pressed command is placed inside a function which changes the scope. Consider the following snippet in level 9:

points = 0
if x is pressed
    points = points + 1

It gets transpiled to:

points = Value('0', num_sys='Latin')
if_pressed_mapping['x'] = 'if_pressed_x_'
def if_pressed_x_():
    points = Value(points + 1)  # this line raises an error because points is now a local variable and not a global one
    ...

With this PR, the problem is resolved via adding a global points line in the function body:

points = Value('0', num_sys='Latin')
def if_pressed_x_():
    global points
    points = Value(points + 1)

2. Variables do not work in if_pressed. To replicate the issue, execute the following program in level 9:

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

Note that no matter what keys you press (x or a you will always get the output not great. This bug was caused with the latest changes for localization.

How to test

• Test all Key Press adventures in levels 5, 7 and 9. They should be bullet-proof. They should not time out or randomly stop mid execution.
• Examine carefully the changed code in skulpt-stdlib-extensions.js. I am a bit hesitant about the way a variable value is retrieved because this solution was discovered by navigating the Sk.globals and not really confirmed with any documentation of Skulpt.
• Knowing that all variables are marked as global in the if_pressed body, think of a way to break this. While it makes the current examples work, could there be a situation in which the global keyword makes things worse?

[jpelay]

With respect to your last comment, I think I found a way to sort of break the global scope, but I think this is true in general in Hedy. If you declare a variable in the script and then use pressed in a function, the program will complain and say that the variable was not defined:

define turn
    if x is pressed
        print 'good'
    else
        print 'bad'

print 'Lets play a game of Twister!'
for i in range 1 to 10
    call turn
    x is 2
    print x

If you declare the variable within the function and then use pressed, it does work:

define turn
    x is 1

print 'Lets play a game of Twister!'
for i in range 1 to 10
    call turn
    if x is pressed
        print 'good'
    else
        print 'bad'

A last problem that I found is that if the command does not work at all within a function:

define turn
    if x is pressed
        print 'good'
    else
        print 'bad'

print 'Lets play a game of Twister!'
for i in range 1 to 10
    call turn

[jpelay]

I think this solution is great and works, my only question would be if someone assigns a multileter string, and not a single letter, and then uses it in the key press. Like this:

x is asdf
if x is pressed
    print 'great'
else
    print 'not great'

Then it will always be false!

[boryanagoncharenko]

You are right. We allow the left-hand-side of the if-pressed command to be a variable. And this variable could be assigned anything: x is 21 + 1 or x is hello world. At the same time we do not allow for multiple keys to be pressed at the same time.

What do you think is the best thing to do here? We could either silently take the first letter of the string which might be confusing. Alternatively, we could issue a warning at runtime (not sure if we have the mechanism for that?) that lets the user know that it is a multi-letter string so it will not work.

Since this issue was not introduced by the PR, I created a separate issue to discuss and track the matter: #5727

[jpelay]

I think the best way would be show an alert or an error warning the user that probably they don't want to be using a multicharacter string in a keypress! It can probably be done by checking the value of the variable and then calling the modal.alert function I think.

[boryanagoncharenko]

Thanks for all the tests!

The undefined variable problem is not specific to the if-pressed command. The issue is 2-layered. First, we make the assumption that if is-pressed is using a variable which happens to be defined after its usage, then we should give an error because it is likely a mistake:

if x is pressed
    print 'good'
else
    print 'bad'
x is 2  # Changing x to m, does not yield an error

Is this a correct assumption here? I am not sure but it feels safer to give an error and ask the user to change the x variable name if needed.

Second, the check mentioned above does not take into account that functions use a different scope. It just uses the line number for the access-before-definition error. This is definitely a bug on its own and I logged it here: #5728.

About the second issue you discovered, I fixed the transpiler, so that some if-pressed statements could be executed from a function. However, not all scenarios are covered. Consider the following snippet:

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

call turnz

You might expect that printing x would yield either great or not great but it prints a. The trouble is that the if-pressed uses a global variable x while the function uses a local one. I could easily change the function definition to use global variables, but then this will have quite an impact on functions with arguments. Since the Key Press adventure is currently broken on production, I would like to log this scenario as a separate bug. But I am not sure how to log it: we could just accept that the code above will not work as the users probably expect, or we could look into changing the functions to use global scope too, or we could disallow if_pressed statements in functions. Or we could think of getting rid of functions in the if_pressed transpilation altogether. Let me know what you think and thanks once again!

[jpelay]

The undefined variable problem is not specific to the if-pressed command. The issue is 2-layered. First, we make the assumption that if is-pressed is using a variable which happens to be defined after its usage, then we should give an error because it is likely a mistake:

if x is pressed
    print 'good'
else
    print 'bad'
x is 2  # Changing x to m, does not yield an error

Is this a correct assumption here? I am not sure but it feels safer to give an error and ask the user to change the x variable name if needed.

Yes I think so!

Second, the check mentioned above does not take into account that functions use a different scope. It just uses the line number for the access-before-definition error. This is definitely a bug on its own and I logged it here: #5728.

Thanks!

About the second issue you discovered, I fixed the transpiler, so that some if-pressed statements could be executed from a function. However, not all scenarios are covered. Consider the following snippet:

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

call turnz

You might expect that printing x would yield either great or not great but it prints a. The trouble is that the if-pressed uses a global variable x while the function uses a local one. I could easily change the function definition to use global variables, but then this will have quite an impact on functions with arguments. Since the Key Press adventure is currently broken on production, I would like to log this scenario as a separate bug. But I am not sure how to log it: we could just accept that the code above will not work as the users probably expect, or we could look into changing the functions to use global scope too, or we could disallow if_pressed statements in functions. Or we could think of getting rid of functions in the if_pressed transpilation altogether. Let me know what you think and thanks once again!

Can we somehow pass the variables used inside the if_pressed as parameters in the functions instead of using global? Although I think that wouldn't work because it wouldn't modify the variable anyway. I don't think allowing functions to use global scope would be advisable because it would break assumptions.

Or we could think of getting rid of functions in the if_pressed transpilation altogether

By this you mean forbidding the use of if_pressed inside functions?

[jpelay]

Another option would be that assignments in the if_pressed are treated as return statements:

"def turnz():
  x = Value('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 x
    return Value('great')
  global if_pressed_else_
  if_pressed_mapping['else'] = 'if_pressed_else_'
  def if_pressed_else_():
    global x
    return Value('not great')
  x = extensions.if_pressed(if_pressed_mapping)
  print(f'''{x}''')
turnz()"

Do you think this could work?

[boryanagoncharenko]

Can we somehow pass the variables used inside the if_pressed as parameters in the functions instead of using global?
Although I think that wouldn't work because it wouldn't modify the variable anyway. I don't think allowing functions to use
global scope would be advisable because it would break assumptions.

I was also thinking of a way to pass the variables to the functions as parameters but then the issue is that the changes inside the function remain in the current scope only. This is a plain python function:

def turn(x):
    x = x + 1
    print(f'inside x is {x}')
x = 1
turn(x)
print(f'outside x is {x}')

It yields the following outcome:

inside x is 2
outside x is 1

Another option would be that assignments in the if_pressed are treated as return statements

This is a perspective I have not considered. This approach definitely works for the example at hand. However, I cannot think of a way to generalize the solution to other programs. For example, if there are multiple assignments done in the if-body, this means the function should return multiple values. And if there is a print statement in between the assignments, the complexity would become crazy.

Or we could think of getting rid of functions in the if_pressed transpilation altogether

By this you mean forbidding the use of if_pressed inside functions?

I meant that before the refactoring, the if-pressed commands used pygame, so the transpiled code for the program above looked like this:

def turnz():
    x = 'a'
    pygame_end = False
    while not pygame_end:
        pygame.display.update()
        event = pygame.event.wait()
        if event.type == pygame.QUIT:
            pygame_end = True
            pygame.quit()
            break
        if event.type == pygame.KEYDOWN:
            if event.unicode == x:
                x = 'great'
                break
            # End of PyGame Event Handler
            else:
                x = 'not great'
                break
    print(f'''{x}''')

turnz()

If you look at the PR that switched from pygame to a custom implementation with functions #5117 (or the issue that this PR was closing #3966) I am not sure why this was done. Perhaps there was an issue that was just not mentioned. If not, however, we could consider just rolling back that change altogether. @Felienne may be you know why the pygame was removed?

[mergify]

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).