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] Upvalues aren't closed correctly after continue and break statements #608

Closed
1 task done
RevengerWizard opened this issue Mar 6, 2023 · 4 comments · Fixed by #609
Closed
1 task done

[BUG] Upvalues aren't closed correctly after continue and break statements #608

RevengerWizard opened this issue Mar 6, 2023 · 4 comments · Fixed by #609
Labels
bug Something isn't working

Comments

@RevengerWizard
Copy link
Contributor

RevengerWizard commented Mar 6, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Currently, it seems that continue and break statements are broken when inside of a loop body there is a closure. This is because currently both statements when closing all locals don't check if the current local popped is an upvalue.

// Discard any locals created inside the loop.
for (int i = compiler->localCount - 1;
     i >= 0 && compiler->locals[i].depth > compiler->loop->scopeDepth;
     i--) {
    emitByte(compiler, OP_POP);
}

This piece of code is present in both continueStatement and breakStatement functions.
I'd just open a pull request, but I'm sadly I'm unable to correctly compile Dictu under Windows with MinGW because of some weird compiler error, so I'll just provide the fix here:

// Discard any locals created inside the loop.
for (int i = compiler->localCount - 1;
     i >= 0 && compiler->locals[i].depth > compiler->loop->scopeDepth;
     i--) {
    if(compiler->locals[i].isUpvalue)
    {
        emitByte(compiler, OP_CLOSE_UPVALUE);
    }
    else
    {
        emitByte(compiler, OP_POP);
    }
}

Expected Behavior

No response

Steps To Reproduce

No response

Anything else?

No response

@RevengerWizard RevengerWizard added the bug Something isn't working label Mar 6, 2023
@Jason2605
Copy link
Member

Thanks for this!

Hmmm would be interesting to see an example script where this is causing issues - also what issues are you getting on windows?

@RevengerWizard
Copy link
Contributor Author

RevengerWizard commented Mar 7, 2023

Sorry, I didn't provide any examples, here are some:
With continue:

var f;
for(var i = 1; i <= 3; i += 1) 
{
    var j = 4;
    
    def g() { print(i + j); }
    f = g;
    continue;
}

f();
var f;
while(f == null) 
{
    var i = "i";
    def g() { print(i); }
    f = g;
    continue;
}

f();

With break:

var f;
while(true) 
{
    var i = "i";
    def g() { print(i); }
    f = g;
    break;
}

f();

As for the compile issues, right now with the current last commit I only get uninitialized errors in some files in the datatypes folder (dicts.c, lists.c, result.c, etc). I had more errors before, but it seems that most of those just disappeared, weird.

C:\...\Dictu-develop\src\vm\datatypes\dicts\dicts.c: In function 'declareDictMethods':
C:\...\Dictu-develop\src\vm\datatypes\dicts\dicts.c:175:29: error: 'Dict' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  175 |     ObjModule *DictModule = AS_MODULE(Dict);
      |                             ^
C:\...\Dictu-develop\src\vm\datatypes\dicts\dicts.c:172:11: note: 'Dict' was declared here
  172 |     Value Dict;
      |           ^

Edit: also many linker undefined reference errors to various functions (argparse_init, argparse_parse, dictuInitVM, dictuFreeVM, dictuInterpret, argparse_help_cb)

@Jason2605
Copy link
Member

Ahhh very good find, thank you for providing the issue and the reproduction steps!! I'm more than happy to create the PR but if you want to get your name on the commits and into the repo open up that PR (even if it's just via the web portal and we can go from there!) - just let me know

@RevengerWizard
Copy link
Contributor Author

I'll try to open a pull request with the fix.

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

Successfully merging a pull request may close this issue.

2 participants