Skip to content

Recursive function with no parameters generate while loop which contains assignment to undeclared _param variable #6719

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

Closed
3 of 5 tasks
hackwaly opened this issue Apr 7, 2024 · 9 comments · Fixed by #6907
Labels
Milestone

Comments

@hackwaly
Copy link

hackwaly commented Apr 7, 2024

Thank you for filing! Check list:

  • Is it a bug? Usage questions should often be asked in the forum instead.
  • Concise, focused, friendly issue title & description.

Recursive function with no parameters generate while loop which contains assignment to undeclared _param variable

let test = () => {
  let rec loop = () => {
    switch () {
    | _ => loop()
    }
  }
  loop()
}
// Generated by ReScript, PLEASE EDIT WITH CARE


function test() {
  var loop = function () {
    while(true) {
      _param = undefined;
      continue ;
    };
  };
  return loop();
}

export {
  test ,
}
/* No side effect */
@hackwaly
Copy link
Author

hackwaly commented May 6, 2024

Any plan to fix this bug?

@tx46
Copy link

tx46 commented May 21, 2024

The biggest problem I see with this is that it will silently freeze instead of overflow the stack...

@TheSpyder
Copy link
Contributor

TheSpyder commented Jul 15, 2024

I came across this while upgrading my project to ReScript 11 and considering a swap to uncurried mode. The specific issue is with code that takes a JS generator and turns it into a "lazy sequence".

In my case, the project still has uncurried: false with the code in question manually uncurried, so it is blocking my upgrade to ReScript 11. I could switch it to curried code, but then it would be slow (and block my attempts to use uncurried mode).
https://rescript-lang.org/try?version=v11.1.2&code=DYUwLgBAdiAekF4IGcQEcIIHwvQCgEoAoI0SAMwEtgwQAnTCPVNAGggD9yDMcBvIhAhkIdEAGMIACxDAADvUaFegoSgDulMOKm40AWiwx4EAWogAfCAGUA9gFsQeWD2wRyzngH4bDpy4gALmlZBTpCVSErADlbGF4IWJhIiABfEiEZeXoidKA

@cristianoc cristianoc added the bug label Jul 24, 2024
@cristianoc cristianoc added this to the v12 milestone Jul 24, 2024
@cristianoc
Copy link
Collaborator

Likely place to start from: #6131

@cristianoc
Copy link
Collaborator

@hackwaly @TheSpyder how does this look? #6907

@rescript-lang rescript-lang deleted a comment from hackwaly Jul 25, 2024
@TheSpyder
Copy link
Contributor

The JS certainly looks good! I didn't realise there was broken code in the expected test output 😂

@cristianoc
Copy link
Collaborator

Improved the PR, now it simply omits the problematic assignment without reverting to using a spurious parameter in the case of recursive functions.

@cristianoc
Copy link
Collaborator

cristianoc commented Jul 25, 2024

The JS certainly looks good! I didn't realise there was broken code in the expected test output 😂

That happens often. There are so many old and forgotten tests, that chances are the repro is already right there.

@TheSpyder
Copy link
Contributor

Yes, I have worked on many old codebases where that's true 🙃

cknitt pushed a commit to cknitt/rescript that referenced this issue Jul 26, 2024
cknitt pushed a commit to cknitt/rescript that referenced this issue Jul 26, 2024
cknitt pushed a commit to cknitt/rescript that referenced this issue Jul 26, 2024
cknitt pushed a commit that referenced this issue Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants