Conversation
There was a problem hiding this comment.
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.
|
@github-actions review一下 |
There was a problem hiding this comment.
中文:该改动在 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() end 中 parent.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() end 与 local 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.
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.
local f = function() endbehaves the same way. After this PR, only the field name is returned, consistent with how literal values already behave (e.g.X.y = 1jumps toy).Root cause (click to expand)
vm.getDefsproduces two independent defs through two paths:Name candidate (
c):compileByNodeChainresolves thesetfieldLHS, and the definition provider unwraps it viasrc.field.Function-value candidate (
function):searchByNode→compileNodemerges the field's value into the compiled type. Thefunctionnode 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
getDefstreats them as different nodes;the nested-range dedup in
sortResultsonly 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: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-definitionandimplementationalready return a single candidate through their own filters (a type whitelist andgetRefs+isAssignrespectively), so they are unaffected.Tests
Updated
test/definition/function.lua:local f = function() endnow expects a single definition atf.Added
test/definition/field.luaregression 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 的測試也是同期的批量updatecommit (9451329b33) 帶入的。本 PR 由 Claude Code(deepseek-v4-flash)協助分析與撰寫。