Fix: Stack overflow while attempting to resolve function return value #3247
Conversation
Fixes: LuaLS#3246 Co-authored-by: Tom Lau <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a stack overflow issue that occurs when resolving function return values in circular type dependencies. The solution prevents infinite recursion by tracking resolving object-node pairs in a hash table.
- Adds circular dependency detection to the
resolvefunction inscript/vm/sign.lua - Includes a regression test case for circular function type aliases
- Updates the changelog to document the fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| script/vm/sign.lua | Implements circular resolve call detection using a hash table to prevent stack overflow |
| test/type_inference/common.lua | Adds test case with circular function type aliases to verify the fix |
| changelog.md | Documents the stack overflow fix in the changelog |
script/vm/sign.lua
Outdated
| if resolving[resolveHash] then | ||
| return -- prevent circular resolve calls | ||
| end | ||
| resolving[resolveHash] = true |
There was a problem hiding this comment.
The resolving hash entry is never cleared after the resolve call completes, which could lead to memory accumulation and potentially incorrect behavior if the same object-node pair needs to be resolved again in a different context. Consider clearing the entry after the resolve call finishes or using a different approach like a call stack tracker.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Due to the structure of this function, it would be required to maintain 6 cleanup points for each of the 6 returns. This reduces the maintainability in favour of techical correctness. But as it is shown by the passing of all tests, there is no incorrect behaviour by not performing the cleanup; additionally, memory accumulation is minimal as each entry in the hash table is small and the table is gc'd after resolution is complete. Therefore, I do not see a reason to perform manual cleanup despite it's techical correctness, although happy to add it if others disagree.
There was a problem hiding this comment.
maybe the variable name resolving is a bit misleading 😕
actually it acts like a visited hash table to store all pairs that have triggered a resolve()
=> no need to resolve the same pair more than once (which is my original intention)
maybe we should pick a better variable name? 🤔
There was a problem hiding this comment.
That does read more clearly, I'll make that update 👍
There was a problem hiding this comment.
Nice naming, much better than mark I usually using
Prevents circular resolve calls and avoids potential stack overflows by tracking all resolving object-node pairs in a hash table. Solution suggested by @tomlau10
Fixes #3246