Skip to content

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

Merged
merged 7 commits into from
Jul 30, 2025

Conversation

mediremi
Copy link
Contributor

@mediremi mediremi commented Jul 28, 2025

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 but rescript-editor-analysis.exe:

$ ps aux | grep rescript
mediremi  219605 98.6  0.0 275740 12696 ?        R    20:33   0:14 /home/mediremi/projects/my_project/node_modules/@rescript/linux-x64/bin/rescript-editor-analysis.exe codeAction /home/mediremi/projects/my_project/SomeFile.res 99 9 99 9 /tmp/rescript_format_file_219371_9.res

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:

module Sqlite = {
  module Transaction = {
    type t<'a>

    external runImmediate: t<'a> => 'a = "immediate"
  }

  module Database = {
    type t

    external make: () => t = "default"

    external transaction: (t, unit => 'a) => Transaction.t<'a> = "transaction"
  }
}

let openSqliteDB = () => Sqlite.Database.make()

let transactionForProject = (fn) => {
  openSqliteDB()
  ->Sqlite.Database.transaction(fn)
  ->Sqlite.Transaction.runImmediate
  ->ignore
}
_build/default/analysis/bin/main.exe codeAction ./SomeFile.res 22 9 22 9 ./SomeFile.res

By using print_endline, I was able to pinpoint the cause of the hang to an infinite recursion in TypeUtils.resolveTypeForPipeCompletion. When TypeUtils.findReturnTypeOfFunctionAtLoc returns a type variable, resolveTypeForPipeCompletion will loop forever.

I then solved the issue by adding a depth arg to TypeUtils.resolveTypeForPipeCompletion that keeps track of the current level of recursion and makes that helper bail out after depth > 10

To prevent this issue, we just need to handle TypeUtils.findReturnTypeOfFunctionAtLoc returning a type variable by returning it rather than looping over it.

Copy link

pkg-pr-new bot commented Jul 28, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7731

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7731

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7731

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7731

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7731

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7731

commit: 702a414

@mediremi mediremi marked this pull request as ready for review July 29, 2025 08:28
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! 🎉

@nojaf
Copy link
Member

nojaf commented Jul 29, 2025

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!

mediremi added 2 commits July 30, 2025 11:55
…orces a maximum iteration count and add TODO about fixing the root of the issue
@mediremi
Copy link
Contributor Author

@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 👍

@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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.

@mediremi mediremi force-pushed the fix-code-action-hang branch from bb1c492 to 44c4030 Compare July 30, 2025 12:13
@mediremi mediremi changed the title Limit TypeUtils.resolveTypeForPipeCompletion to 10 recursive calls to prevent infinite loops Prevent infinite recursion in TypeUtils.resolveTypeForPipeCompletion by not looping over type variables Jul 30, 2025
@zth zth enabled auto-merge (squash) July 30, 2025 12:22
@zth
Copy link
Collaborator

zth commented Jul 30, 2025

@mediremi would you backport this to the VSCode repo as well? This bug should exist with v11 too.

@mediremi
Copy link
Contributor Author

I'll backport it now 👍

@zth zth merged commit 8057ded into rescript-lang:master Jul 30, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants