-
Notifications
You must be signed in to change notification settings - Fork 470
Prevent infinite recursion in TypeUtils.resolveTypeForPipeCompletion
by not looping over type variables
#7731
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
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! 🎉
Could you please provide more details here? This fix seems to be just a temporary solution, and it doesn’t address the underlying issue. What is causing the repeated recursion? It would be helpful if you could add a clear comment for anyone who might encounter this later. At the very least, please explain what you're working around. I don’t want to hold things up, but we shouldn’t merge this without more context about the structural implications. Thank you! |
…orces a maximum iteration count and add TODO about fixing the root of the issue
@nojaf you're right, this only fixes the symptom and not the root issue, and I should have left a TODO regarding that. I've now added a comment to the code about this 👍 |
analysis/src/TypeUtils.ml
Outdated
@@ -540,7 +540,15 @@ let rec resolveTypeForPipeCompletion ~env ~package ~lhsLoc ~full | |||
in | |||
match typFromLoc with | |||
| Some typFromLoc -> | |||
typFromLoc |> resolveTypeForPipeCompletion ~lhsLoc ~env ~package ~full | |||
(* Prevent infinite loops when `typFromLoc` is a type variable by bailing out after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is typFromLoc
in this case?
That is still not clear to me, which part of the example is passed here over and over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure - I don't understand this code very well, I only tracked down this issue with a combination of adding log statements and binary search.
…by not looping over type variables
bb1c492
to
44c4030
Compare
TypeUtils.resolveTypeForPipeCompletion
to 10 recursive calls to prevent infinite loopsTypeUtils.resolveTypeForPipeCompletion
by not looping over type variables
@mediremi would you backport this to the VSCode repo as well? This bug should exist with v11 too. |
I'll backport it now 👍 |
Sometimes when attempting saving a ReScript file in VS Code, the formatter will appear to hang and this prevents the file from being saved:
By running
ps aux | grep rescript
, I discovered that the source of the problem was not the formatter butrescript-editor-analysis.exe
:Using the file from my codebase that had triggered this issue as a starting point, I was able to create the following ReScript file and bash script to reproduce the issue:
By using
print_endline
, I was able to pinpoint the cause of the hang to an infinite recursion inTypeUtils.resolveTypeForPipeCompletion
. WhenTypeUtils.findReturnTypeOfFunctionAtLoc
returns a type variable,resolveTypeForPipeCompletion
will loop forever.I then solved the issue by adding adepth
arg toTypeUtils.resolveTypeForPipeCompletion
that keeps track of the current level of recursion and makes that helper bail out afterdepth > 10
To prevent this issue, we just need to handle
TypeUtils.findReturnTypeOfFunctionAtLoc
returning a type variable by returning it rather than looping over it.