Skip to content

fix: return single definition for anonymous-function field assignment - #3450

Open
tomlau10 wants to merge 1 commit into
LuaLS:masterfrom
tomlau10:fix/goto-definition-single-candidate
Open

tomlau10 wants to merge 1 commit into
LuaLS:masterfrom
tomlau10:fix/goto-definition-single-candidate

Conversation

@tomlau10

Copy link
Copy Markdown
Contributor

Fixes #2451

Summary

Go to definition on a field assigned an anonymous function returned two candidates on the same line — the field name and the function value.

A = {}
A.c = function() end
A.c()   -- goto definition on `c` previously returned 2 candidates: `c` and `function`

local f = function() end behaves the same way. After this PR, only the field name is returned, consistent with how literal values already behave (e.g. X.y = 1 jumps to y).

Root cause (click to expand)

vm.getDefs produces two independent defs through two paths:

  1. Name candidate (c): compileByNodeChain resolves the setfield LHS, and the definition provider unwraps it via src.field.

  2. Function-value candidate (function): searchByNodecompileNode merges the field's value into the compiled type. The function node survives the literal filter because function literals are explicitly exempted there — an exemption meant for generic type inference (e.g. v2 = f(function() end)), not for direct assignments.

Neither existing dedup catches it:

  • node-identity dedup in getDefs treats them as different nodes;

  • the nested-range dedup in sortResults only removes a target whose range contains another — these two are same-line siblings.

Fix (click to expand)

In script/core/definition.lua, after collecting targets, drop a function-value target when its owning assignment's name node is also a target:

local targetMark = {}
for _, res in ipairs(results) do
    targetMark[res.target] = true
end
for i = #results, 1, -1 do
    local target = results[i].target
    if target.type == 'function' then
        local parent = target.parent
        if parent and guide.isAssign(parent) then
            local owner = parent.field or parent.method or parent.index or parent.variable or parent
            if targetMark[owner] then
                table.remove(results, i)
            end
        end
    end
end

Why this shape:

  • Conditioned on the owner also being a target, so generic-inference defs (where the function literal is the only candidate) are left untouched.

  • Scoped to the definition provider only. type-definition and implementation already return a single candidate through their own filters (a type whitelist and getRefs + isAssign respectively), so they are unaffected.

Tests

  • Updated test/definition/function.lua: local f = function() end now expects a single definition at f.

  • Added test/definition/field.lua regression cases for both global and local tables (A.c = function() end).

  • Full test suite passes.


中文摘要

A.c = function() end 的寫法,goto definition 原本會回傳兩個結果(欄位名稱 c 和 function 值)。此 PR 在 definition.lua 加了一段處理:當欄位名稱也在結果中時,移除冗餘的 function 值,只保留名稱節點,與 X.y = 1 跳到 y 的慣例一致。type-definition / implementation 兩個 provider 因各自已有 filter,不受影響;測試全部通過。

此 bug 的根源,是 function 字面量豁免在 2022 年某次 global-manager/infer 大型 refactor (0e159ee037) 中順手加入的,屬 refactor 的副作用、並非刻意設計;回傳 2 個 candidate 的測試也是同期的批量 update commit (9451329b33) 帶入的。

本 PR 由 Claude Code(deepseek-v4-flash)協助分析與撰寫。

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Notice: It looks like you're using hustcer/deepseek-review, but the CHAT_TOKEN hasn't been configured in your repo's Variables/Secrets. Please ensure this token is set for proper functionality. For step-by-step guidance, refer to the CHAT_TOKEN Config section of README.

@sumneko

sumneko commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

@github-actions review一下

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

中文:该改动在 script/core/definition.lua 中新增的去重逻辑存在潜在的空指针风险:target.type 在未先判断 target 是否为 nil 的情况下被访问。虽然 results 中的元素通常都有 target,但若某个结果缺少该字段(例如来自其他分支的构造结果),此处会抛出 attempt to index a nil value 错误。建议改为 if target and target.type == 'function' then

English: The new deduplication logic in script/core/definition.lua has a potential nil-dereference risk: target.type is accessed without first checking whether target is nil. Although elements in results usually have a target, if any result lacks that field (e.g. constructed by another branch), this will throw an attempt to index a nil value error. Suggest changing it to if target and target.type == 'function' then.

中文:local owner = parent.field or parent.method or parent.index or parent.variable or parent 这一回退到 parent 本身的做法过于宽泛。当赋值语句的左侧不是字段/变量节点(例如 t[k] = function() endparent.index 为 nil 的某些形态,或解构/多值赋值)时,owner 会退化为赋值节点本身,而该节点几乎不可能出现在 targetMark 中,因此去重会静默失效。更稳妥的做法是显式枚举可接受的左侧节点类型,或在无法确定 owner 时跳过删除。

English: The fallback local owner = parent.field or parent.method or parent.index or parent.variable or parent is too broad. When the left-hand side of the assignment is not a field/variable node (e.g. certain forms of t[k] = function() end where parent.index is nil, or destructuring/multi-value assignments), owner degrades to the assignment node itself, which will almost never be present in targetMark, so the deduplication silently fails. A more robust approach is to explicitly enumerate the acceptable left-hand node types, or skip removal when the owner cannot be determined.

中文:去重逻辑依赖 target.parent 直接指向赋值节点。若 AST 中函数节点与赋值节点之间还存在中间包装节点(例如括号表达式 A.c = (function() end)),parent 将不是赋值节点,去重不会生效,导致行为不一致。建议确认 AST 结构或改为向上遍历查找最近的赋值祖先。

English: The deduplication relies on target.parent pointing directly to the assignment node. If the AST contains an intermediate wrapper node between the function node and the assignment node (e.g. a parenthesized expression A.c = (function() end)), parent will not be the assignment node, the deduplication will not take effect, and behavior becomes inconsistent. Suggest confirming the AST structure or walking upward to find the nearest assignment ancestor.

中文:测试覆盖不完整。新增用例只覆盖了 A.c = function() endlocal A = {} 两种形式,未覆盖 A.c = (function() end)A["c"] = function() end、多值赋值 A.c, A.d = function() end, function() end 以及方法定义 A:c = function() end 等边界情况。建议补充这些用例,以验证去重逻辑在各类赋值形态下均正确。

English: Test coverage is incomplete. The new cases only cover A.c = function() end and local A = {}; they do not cover edge cases such as A.c = (function() end), A["c"] = function() end, multi-value assignment A.c, A.d = function() end, function() end, or method definitions A:c = function() end. Suggest adding these cases to verify the deduplication works correctly across assignment forms.

中文:test/definition/function.lua 中把 local <!f!> = <!function () end!> 改为 local <!f!> = function () end,删除了对函数值本身作为定义目标的断言。这实际上弱化了原有测试,掩盖了“函数值不再作为候选”这一行为变化。若该行为变更是有意为之,应在 changelog 中明确说明;否则应保留原断言并新增针对新行为的用例。

English: In test/definition/function.lua, changing local <!f!> = <!function () end!> to local <!f!> = function () end removes the assertion that the function value itself is a definition target. This effectively weakens the existing test and hides the behavioral change that the function value is no longer a candidate. If this change is intentional, it should be explicitly documented in the changelog; otherwise, keep the original assertion and add a new case for the new behavior.

中文:性能方面,新增逻辑对 results 做了两次遍历(一次建 targetMark,一次反向删除),在结果集较大时会产生额外开销。虽然通常 results 很小,但可考虑合并为一次遍历或仅在存在函数类型目标时才构建 targetMark,以减少不必要的分配。

English: Regarding performance, the new logic iterates over results twice (once to build targetMark, once for reverse removal), adding overhead when the result set is large. Although results is usually small, consider merging into a single pass or only building targetMark when a function-type target exists, to reduce unnecessary allocation.

中文:targetMark 以节点对象为键,依赖节点对象的引用相等性。若 results 中同一逻辑定义存在多个不同的节点实例(例如来自不同解析路径的副本),去重将失效。建议确认 results 中的节点是否保证为同一引用。

English: targetMark uses node objects as keys, relying on reference equality of node objects. If results contains multiple distinct node instances for the same logical definition (e.g. copies from different resolution paths), deduplication will fail. Suggest confirming that nodes in results are guaranteed to be the same reference.

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.

Two definitions for local function variables

2 participants