Skip to content

Fix generic classes with generic methods - #3489

Open
NathanSnail wants to merge 2 commits into
LuaLS:masterfrom
NathanSnail:generic_classes
Open

NathanSnail wants to merge 2 commits into
LuaLS:masterfrom
NathanSnail:generic_classes

Conversation

@NathanSnail

Copy link
Copy Markdown
Contributor

Currently if you have a generic class with a generic method one of the generic substitution maps will override the other, resulting in values being typed as <T> when they have a knowable type. This fixes this for both @generic and @field.

@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.

中文:在 compiler.lua 中,resolveArgs 在新增代码里被使用,但该 diff 片段中未显示其定义;请确认它在 bindReturnOfFunction 作用域内已定义且非 nil,否则 rnode:resolve(..., resolveArgs) 与 sign:resolve(..., resolveArgs) 可能行为异常或报错。
English: In compiler.lua, the new code uses resolveArgs, but its definition is not shown in this diff; please confirm it is defined and non-nil within the bindReturnOfFunction scope, otherwise rnode:resolve(..., resolveArgs) and sign:resolve(..., resolveArgs) may misbehave or error.

中文:compiler.lua 中新增的 sign:resolve(...) 二次解析逻辑会对 returnNode 的每个对象再次调用 vm.cloneObject(rnode, resolved) 并 vm.compileNode,存在重复编译与潜在性能开销;建议确认此路径不会在热路径上被频繁触发,或缓存结果。
English: The newly added secondary resolution via sign:resolve(...) in compiler.lua calls vm.cloneObject(rnode, resolved) and vm.compileNode again for every object in returnNode, which may cause redundant compilation and performance overhead; please verify this path is not hit frequently on hot paths, or cache the result.

中文:generic.lua 中 cloneObject 对 doc.generic.name 新增了 _resolved 分支,当 anyResolved 为 false 时直接把 source._resolved 共享给 newName(vm.setNode(newName, source._resolved, true)),这会让克隆对象与原对象共享同一节点,后续对克隆的修改可能污染原对象;建议确认是否需要深拷贝或至少不共享可变节点。
English: In generic.lua, cloneObject adds a _resolved branch for doc.generic.name; when anyResolved is false it directly shares source._resolved with newName (vm.setNode(newName, source._resolved, true)), so the clone and the original share the same node, and later mutations to the clone may pollute the original; please confirm whether a deep copy is needed or at least avoid sharing mutable nodes.

中文:generic.lua 中 doc.type.function 克隆新增了 async = source.async 与 signs = source.signs,其中 signs 是直接引用而非克隆,若后续对克隆的 signs 做修改会影响原对象;建议确认 signs 是否需要克隆或只读使用。
English: In generic.lua, the doc.type.function clone now copies async = source.async and signs = source.signs, where signs is a direct reference rather than a clone; if the clone's signs is later mutated it will affect the original; please confirm whether signs needs cloning or is used read-only.

中文:compiler.lua 中 hasClassGenerics 的检测逻辑:先检查 doc.generic.name,若没有才检查 doc.type.name。若返回类型同时包含类泛型与方法级泛型(如 Parser<[T, U]> 中 T 为类泛型、U 为方法泛型),hasClassGenerics 为 true 会走克隆分支,此时 U 依赖 sign:resolve 的二次解析来补全;请补充测试覆盖“类泛型与方法泛型混合且方法泛型需从参数推断”的场景,确保 U 被正确解析。
English: In compiler.lua, the hasClassGenerics detection first checks doc.generic.name and only checks doc.type.name if none found. If the return type contains both class generics and method-level generics (e.g. T is a class generic and U is a method generic in Parser<[T, U]>), hasClassGenerics is true and the clone branch runs, where U relies on the secondary sign:resolve to be filled in; please add tests covering the case where class and method generics are mixed and the method generic must be inferred from arguments, ensuring U resolves correctly.

中文:compiler.lua 中 guide.eachSourceType(rtn, 'doc.type.name', checkGeneric) 仅在 doc.generic.name 未命中时执行,但 checkGeneric 只判断 genericMap[src[1]];若 doc.type.name 的 src[1] 是已解析的泛型名(非原始泛型),可能误判。建议确认 doc.type.name 的 [1] 语义与 genericMap 的键一致。
English: In compiler.lua, guide.eachSourceType(rtn, 'doc.type.name', checkGeneric) only runs when doc.generic.name does not match, but checkGeneric only checks genericMap[src[1]]; if src[1] of doc.type.name is an already-resolved generic name (not the original generic), it may be misjudged. Please confirm the semantics of [1] for doc.type.name match the keys of genericMap.

中文:generic.lua 中 doc.type.table 克隆新增 isTuple = source.isTuple,这是合理的字段保留;但请确认 isTuple 在克隆后与 fields 的一致性,避免元组/表类型混淆。
English: In generic.lua, the doc.type.table clone now copies isTuple = source.isTuple, which is a reasonable field to preserve; but please confirm isTuple remains consistent with fields after cloning to avoid tuple/table type confusion.

中文:测试用例中 Parser<[integer, string]> 等期望值依赖元组类型推断,建议同时增加负向测试(如 x:add(y) 后误用为 Parser<integer> 应报错),以验证类型检查而非仅推断。
English: The test cases expecting Parser<[integer, string]> etc. rely on tuple type inference; consider adding negative tests (e.g. misusing the result of x:add(y) as Parser<integer> should error) to validate type checking rather than inference alone.

中文:compiler.lua 中新增的 sign:resolve 二次解析在 resolved 非空时重建 returnNode,但若 returnNode 中某些节点无法被 vm.cloneObject 克隆(返回 nil),则回退为 newReturnNode:merge(rnode),可能混入未解析的泛型节点;建议确认这种混合结果不会导致下游类型推断错误。
English: The new secondary resolution via sign:resolve in compiler.lua rebuilds returnNode when resolved is non-empty, but if some nodes in returnNode cannot be cloned by vm.cloneObject (returns nil), it falls back to newReturnNode:merge(rnode), potentially mixing in unresolved generic nodes; please confirm this mixed result does not cause downstream type inference errors.

中文:generic.lua 中 _resolved 分支遍历 source._resolved:eachObject() 时,对 rn.type == 'doc.generic.name' 且 resolved[rn[1]] 存在的节点合并解析结果,否则原样合并;若 rn 本身也是带 _resolved 的泛型名,未递归处理,可能遗漏嵌套解析。建议确认是否需要递归处理嵌套 _resolved。
English: In the _resolved branch of generic.lua, when iterating source._resolved:eachObject(), nodes with rn.type == 'doc.generic.name' and resolved[rn[1]] present are merged with the resolution, otherwise merged as-is; if rn itself is a generic name with _resolved, it is not handled recursively, potentially missing nested resolution. Please confirm whether nested _resolved needs recursive handling.

中文:generic.lua 中 needsClone 的新增判断遍历 src._resolved 并检查 hasConcreteResolution(rn[1]),但未检查 rn 是否为 doc.generic.name 之外的类型;虽然已有 rn.type == 'doc.generic.name' 条件,建议确认 _resolved 节点集合中是否可能包含非泛型节点导致逻辑遗漏。
English: The new needsClone check in generic.lua iterates src._resolved and checks hasConcreteResolution(rn[1]), but does not check whether rn is a type other than doc.generic.name; although the rn.type == 'doc.generic.name' condition exists, please confirm whether the _resolved node set may contain non-generic nodes that would be missed.

中文:整体上,本次改动修复了泛型类上泛型方法(通过 @field 与 ---@generic 两种写法)的返回类型推断,测试覆盖较充分;主要风险集中在 resolveArgs 的作用域/取值、_resolved 共享引用导致的别名污染,以及二次解析的性能与混合节点问题。建议补充上述边界测试并确认共享引用是否安全。
English: Overall, this change fixes return type inference for generic methods on generic classes (both via @field and ---@generic), with reasonably thorough test coverage; the main risks are the scope/value of resolveArgs, aliasing pollution from shared _resolved references, and performance/mixed-node issues in the secondary resolution. Recommend adding the boundary tests above and confirming whether shared references are safe.

@NathanSnail

Copy link
Copy Markdown
Contributor Author

The existing tests already handle the doc.type.name not being in genericMap as otherwise the explicitly typed values wouldn't work. The diagnostic test isn't possible to implement because currently all mismatched assignment diagnostics for generic classes don't work, but this is a separate issue and not addressed here.

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.

1 participant