Skip to content

Fix: Stack overflow while attempting to resolve function return value #3247

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.

Sign up for GitHub

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
sumneko merged 3 commits into from
Aug 7, 2025
+30 −0
Merged

Fix: Stack overflow while attempting to resolve function return value #3247

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b52ebf
Prevent circular resolve
Cooldude2606 Aug 6, 2025
b96159d
Update changelog
Cooldude2606 Aug 6, 2025
cf6c60e
Rename `resolving` -> `visited`
Cooldude2606 Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* `FIX` adds the `|lambda|` operator to the `Lua.runtime.nonstandardSymbol` configuration template, which allows the use of that option. Previously, support for it existed in the parser, but we could not actually use the option because it is not recognised in the configuration.
* `FIX` Typed `@field` (eg `---@field [string] boolean`) should not override other defined field [#2171](https://github.com/LuaLS/lua-language-server/issues/2171), [#2711](https://github.com/LuaLS/lua-language-server/issues/2711)
* `FIX` don't return empty hover doc when luals failed to find definition
* `FIX` Prevent stack overflow when attempting to resolve function return values. [#3246](https://github.com/LuaLS/lua-language-server/issues/3246)

## 3.15.0
`2025-6-25`
Expand Down
7 changes: 7 additions & 0 deletions script/vm/sign.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,17 @@ function mt:resolve(uri, args)

---@type table<string, vm.node>
local resolved = {}
---@type table<string, boolean>
local resolving = {}

---@param object vm.node|vm.node.object
---@param node vm.node
local function resolve(object, node)
local resolveHash = ("%s|%s"):format(object, node)
if resolving[resolveHash] then
return -- prevent circular resolve calls
end
resolving[resolveHash] = true
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

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

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.
Copy link
Copy Markdown
Contributor Author

@Cooldude2606 Cooldude2606 Aug 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@tomlau10 tomlau10 Aug 7, 2025

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Copy Markdown
Contributor Author

@Cooldude2606 Cooldude2606 Aug 7, 2025

Choose a reason for hiding this comment

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

That does read more clearly, I'll make that update 👍

Copy link
Copy Markdown
Collaborator

@sumneko sumneko Aug 7, 2025

Choose a reason for hiding this comment

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

Nice naming, much better than mark I usually using

if object.type == 'vm.node' then
for o in object:eachObject() do
resolve(o, node)
Expand Down
22 changes: 22 additions & 0 deletions test/type_inference/common.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4881,3 +4881,25 @@ end

local a, b, <?c?>, d = unpack(t)
]]

-- Test for overflow in circular resolve, only pass requirement is no overflow
TEST 'Callback<<T>>|fun():fun():fun():Success, string' [[
--- @alias Success fun(): Success
--- @alias Callback<T> fun(): Success, T

--- @return Success
local function success()
return success
end

--- @generic T
--- @param callback Callback<T>
--- @return Callback<T>
local function make_callback(callback)
return callback
end

local <?callback?> = make_callback(function()
return success, ""
end)
]]
Loading