Skip to content

Don't add unknown to inferred returns of generic class instances - #3496

Open
svilenmarkov wants to merge 2 commits into
LuaLS:masterfrom
svilenmarkov:fix-inferred-return-type
Open

svilenmarkov wants to merge 2 commits into
LuaLS:masterfrom
svilenmarkov:fix-inferred-return-type

Conversation

@svilenmarkov

Copy link
Copy Markdown
---@class Box<T>
---@field value T

---@return Box<string>
function make_box()
    return { value = "x" }
end

function wrap()
    local b = make_box()
    return b
end

b is correctly showing the expected local b: Box<string>, however wrap infers its return type as:

function wrap()
 -> Box<string>|unknown

Copilot AI balanced review requested due to automatic review settings November 10, 2026 13:43

@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/vm/compiler.lua 中新增的 doc.type.sign 分支会无条件设置 hasKnownType = true 并跳出循环。问题在于:如果此时前面的迭代已经设置了 hasUnknownType = true(例如参数类型或返回类型部分未知),那么最终该函数仍会被视为“已知类型”,而未知部分被忽略。建议不要直接 break,而是先记录该类型已知,同时保留 hasUnknownType 的判定逻辑(即检查是否所有类型节点都已解析,而不是只要遇到 doc.type.sign 就认为整体已知)。请确认该分支与 hasUnknownType 的优先级和后续使用逻辑是否一致,并补充一个参数中含未知类型的测试用例。
English: The new doc.type.sign branch in script/vm/compiler.lua unconditionally sets hasKnownType = true and breaks out of the loop. The problem is that if an earlier iteration already set hasUnknownType = true (e.g. a partially unknown parameter or return type), the function will still be treated as "known type" and the unknown part is silently ignored. Instead of a bare break, record the known type while preserving the hasUnknownType check (i.e. only consider the whole thing known once all type nodes are resolved, not merely on encountering a doc.type.sign). Please verify that this branch's precedence and downstream usage are consistent with hasUnknownType, and add a test case with an unknown type among the parameters.

中文:变更日志条目描述的行为(“未标注的泛型实例返回类型不再显示 |unknown”)只验证了 Box<string> 这一条路径,建议补充一个仍然应显示 |unknown 的反例测试(例如返回类型完全无法推断的函数),以确保 doc.type.sign 分支没有把真正未知的类型误判为已知。
English: The changelog entry ("generic class instances returned from a call no longer show |unknown in the inferred return type") is only covered by the Box<string> path. Add a negative test case that should still show |unknown (e.g. a function whose return type is genuinely non-inferable) to make sure the doc.type.sign branch does not misclassify truly unknown types as known.

中文:test/type_inference/common.lua 中的多处空白行修改只是删除行尾空格,与本次修复无关,属于无关改动,建议拆到单独的格式化提交,以免干扰审阅。
English: The numerous blank-line changes in test/type_inference/common.lua only strip trailing whitespace and are unrelated to this fix. They should be moved to a separate formatting commit to avoid noise in review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the generic return-type inference fix.

No open issues 🎉

What changed in this PR

中文:此 PR 修复 Lua 类型推断中多余的 |unknown:未标注返回类型的函数转发泛型类实例时,保留准确的返回类型,例如 Box<string>。

English: This PR removes an unwanted |unknown from Lua return-type inference when an unannotated function forwards a generic class instance, preserving types such as Box<string>.

Changes:

  • 中文:将 doc.type.sign 识别为已知类型。
    English: Recognizes doc.type.sign as a known type.
  • 中文:添加回归测试,并在更新日志中记录修复。
    English: Adds a regression test and documents the fix in the changelog.
File Description
test/​type_inference/​common.lua 中文:新增泛型实例返回值回归测试,并清理尾随空白。
English: Adds a generic-instance return regression test and removes trailing whitespace.
script/​vm/​compiler.lua 中文:识别泛型类型签名,避免引入多余的 unknown。
English: Recognizes generic type signatures to avoid adding an unwanted unknown.
changelog.md 中文:记录返回类型推断修复。
English: Documents the return-type inference fix.

Review effort: Balanced

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.

2 participants