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

[BUG] Local variable in optional argument assignment #742

Open
1 task done
RevengerWizard opened this issue Jul 29, 2024 · 3 comments
Open
1 task done

[BUG] Local variable in optional argument assignment #742

RevengerWizard opened this issue Jul 29, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@RevengerWizard
Copy link
Contributor

RevengerWizard commented Jul 29, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Given this function declaration with various optional arguments:

def call(text, x=0, y=0, r=0, sx=15, sy=sx, ox=0, oy=0)
{
    print(text, x, y, r, sx, sy, ox, oy);
}

call(1, 2);

We can see that the optional argument sy changed unexpectedly:

1  <-  text ok
2  <-  x ok
0  <-  y ok
0  <-  r ok
1  <-  sx ok
0  <-  sy wrong, it changed to 0?
0  <-  ok
0  <-  ok

As far as I can tell it could have something to do to with the assignment to a local in the optional sy=sx.
Calling with just 1 as argument doesn't modify the other parameters, the problem seems to start with more arguments given.

Expected Behavior

It should probably let us use safely locals.

The same example in Python directly throws an error telling us name 'sx' is not defined, but optional arguments in Python are already weird to deal with.

On the other hand, Javascript gives us the expected values:

function call(text, x=0, y=0, r=0, sx=1, sy=sx, ox=0, oy=0) {
     console.log(text, x, y, r, sx, sy, ox, oy);
}

call(1, 2, 3)
// 1 2 3 0 1 1 0 0

call(1, 2, 3, 4, 5)
// 1 2 3 4 5 5 0 0

Steps To Reproduce

No response

Anything else?

The handling for optional arguments happens in the opcode DEFINE_OPTIONAL in the function declaration, not sure if it can help or if there might be a fix

@RevengerWizard RevengerWizard added the bug Something isn't working label Jul 29, 2024
@Jason2605
Copy link
Member

This is an interesting one.. Yeah I think if you'd asked me what I'd expect to happen I would say the Python one would be correct in that sx hasn't been defined - this is the route I think we should probably go here too

@RevengerWizard
Copy link
Contributor Author

RevengerWizard commented Dec 11, 2024

I gave the example of Python as a comparison of what it looks like in other languages.

Python's optional arguments behave very weirdly overall if you provide as value a mutable type, like a list. They retain state, which is not ideal to say the least.
Javascript in this case gives us what we'd expect.

Though, looking at the bytecode DEFINE_OPTIONAL itself, it'll need some big rework, because it breaks immediately if the value of the optional arguments isn't just a constant/number/string.

@Jason2605
Copy link
Member

Jason2605 commented Dec 11, 2024

Python's optional arguments behave very weirdly overall if you provide as value a mutable type, like a list. They retain state, which is not ideal to say the least.

I guess it makes sense as it's reference to the same list object each call, so if you mutate, it's gonna mutate 🤷

Though, looking at the bytecode DEFINE_OPTIONAL itself, it'll need some big rework, because it breaks immediately if the value of the optional arguments isn't just a constant/number/string.

I'm not entirely sure what you mean here? In dictu you can pass any value as an optional param not just those types

def test(myList=[]) {
    print(myList);
}

test(); // []
test([1, 2, 3]); // [1, 2, 3]



var mylist = [];

def test(a=mylist) {
    a.push(1);
    print(a);
}

test(); // [1]
test(); // [1, 1]
test(); // [1, 1, 1]
test(); // [1, 1, 1, 1]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants