Skip to content

fix: resolve fields of generic T: Class via its constraint - #3465

Open
JasperSurmont wants to merge 2 commits into
LuaLS:masterfrom
JasperSurmont:master
Open

JasperSurmont wants to merge 2 commits into
LuaLS:masterfrom
JasperSurmont:master

Conversation

@JasperSurmont

Copy link
Copy Markdown

Inside a method where self is a constrained generic, field access on self gives undefined-field:

---@class SomeClass
---@field someVar integer
local SomeClass = {}

---@generic T: SomeClass
---@param self T
---@return T
function SomeClass:xyz()
    print(self.someVar) -- Undefined field `someVar`
    return self
end

Cause

searchFieldSwitch had no case for doc.generic.name. The generic constraint (T: SomeClass) was ignored.

Fix

Added a doc.generic.name case that keeps the default lookup and also searches the fields of the constraint type (source.generic.extends). searchFieldSwitch is now declared before it's assigned, so the new case can call it recursively.

Tests

Added a case to test/diagnostics/undefined-field.lua

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

中文:审查摘要:该改动为 doc.generic.name 新增了一个 searchFieldSwitch 分支,用于在泛型约束(T: SomeClass)上查找字段。整体方向合理,但存在若干需要确认的问题:递归/无限循环风险、source.generic 字段来源的假设、以及 searchFieldSwitch 由 local 改为前向声明后可能引入的初始化顺序问题。建议在合并前补充针对嵌套泛型与自引用约束的测试。

English: Review summary: This change adds a new doc.generic.name branch to searchFieldSwitch so that fields are looked up on a generic's constraint (T: SomeClass). The direction is reasonable, but there are several points to confirm: recursion/infinite-loop risk, the assumption about where source.generic comes from, and the potential initialization-order issue introduced by changing searchFieldSwitch from a local to a forward declaration. Adding tests for nested generics and self-referential constraints before merging is recommended.


中文:问题 1(潜在无限递归):新分支中对 constraint 调用 vm.compileNode(constraint),然后对每个结果对象再次调用 searchFieldSwitch(n.type, ...)。如果约束本身解析为另一个 doc.generic.name(例如 ---@generic T: U、---@generic U: T,或约束指向自身),就会形成 doc.generic.name → doc.generic.name 的循环,导致栈溢出或死循环。建议加入已访问集合(visited set)或深度上限来防止循环。

English: Issue 1 (potential infinite recursion): In the new branch, vm.compileNode(constraint) is called and then searchFieldSwitch(n.type, ...) is invoked for each resulting object. If the constraint itself resolves to another doc.generic.name (e.g. ---@generic T: U with ---@generic U: T, or a self-referential constraint), this forms a doc.generic.name → doc.generic.name cycle, causing a stack overflow or infinite loop. Add a visited set or a depth limit to break cycles.


中文:问题 2(source.generic 的可用性假设):代码假设 doc.generic.name 类型的节点一定带有 source.generic 且其 extends 字段存在。若某些路径下 source.generic 为 nil(例如节点由其他方式构造),source.generic and source.generic.extends 会安全返回 nil 并提前 return,行为上可接受;但这也意味着该分支在无约束泛型下会静默跳过约束查找,需确认这是预期行为,并考虑是否应回退到 default 分支的本地/全局 ID 查找(当前已调用,故影响有限)。

English: Issue 2 (assumption about source.generic availability): The code assumes a doc.generic.name node always carries source.generic with an extends field. If source.generic is nil on some paths (e.g. nodes constructed differently), source.generic and source.generic.extends safely yields nil and returns early, which is acceptable behavior; however, it also means the branch silently skips constraint lookup for unconstrained generics. Confirm this is intended, and consider whether it should fall back to the default branch's local/global ID lookup (already called here, so impact is limited).


中文:问题 3(前向声明与初始化顺序):将 local searchFieldSwitch = util.switch() 改为 local searchFieldSwitch 加后续赋值,是为了让新分支内的递归调用能引用到变量本身。这本身可行,但需确认在 searchFieldSwitch 完成赋值之前没有任何代码路径会调用它(例如 util.switch() 的 :case/:call 注册过程中是否触发求值)。若存在提前调用,会得到 nil 调用错误。建议在赋值完成后加断言或注释说明该约束。

English: Issue 3 (forward declaration and initialization order): Changing local searchFieldSwitch = util.switch() to a forward declaration plus later assignment is needed so the recursive call inside the new branch can reference the variable itself. This works, but confirm that no code path invokes searchFieldSwitch before the assignment completes (e.g. whether util.switch()'s :case/:call registration triggers evaluation). If an early call exists, it will fail with a nil call error. Consider adding an assertion or a comment documenting this constraint after the assignment.


中文:问题 4(测试覆盖不足):新增测试仅覆盖了单层约束 T: SomeClass 的字段访问与未定义字段报错,未覆盖:约束为另一个泛型、约束为联合类型、约束为 nil/无约束、以及约束链上字段继承等场景。建议补充这些用例,尤其是问题 1 中提到的循环约束,以验证不会栈溢出。

English: Issue 4 (insufficient test coverage): The new test only covers field access and undefined-field reporting for a single-level constraint T: SomeClass. It does not cover: constraint being another generic, constraint being a union type, constraint being nil/unconstrained, or field inheritance along the constraint chain. Add these cases, especially the cyclic constraint mentioned in Issue 1, to verify no stack overflow occurs.


中文:问题 5(changelog 描述与实现一致性):changelog 中写“fields are now looked up on the constraint”,但实现中约束查找是在本地/全局 ID 查找之后追加的,且仅在 constraint 存在时生效。描述基本准确,但建议明确说明“仅在泛型约束存在时”以及“未定义字段仍会报错”,避免用户误解为所有泛型字段访问都不再报错。

English: Issue 5 (changelog wording vs. implementation): The changelog says "fields are now looked up on the constraint", but the implementation appends constraint lookup after local/global ID lookup and only applies when constraint exists. The description is broadly accurate, but consider clarifying "only when a generic constraint exists" and "undefined fields are still reported", to avoid users misreading it as all generic field accesses no longer reporting errors.

@JasperSurmont

Copy link
Copy Markdown
Author

Going through the review:

  1. Recursion: I don't believe this can cycle. The parser only turns names into generic references inside @param/@return/@type/@field etc., never inside @Generic itself. So the constraint is always a plain type, never another doc.generic.name.. I added a T: T test to verify this.
  2. Unconstrained generics: A bare T carries no field information, and the default local/global lookups still run, so I reckon this is intended
  3. Init order: util.switch() and :case/:call only register handlers, and nothing runs until the chain is assigned so I don't see an issue here
  4. Tests: added more tests. A constraint that refers to another generic (U: A, T: U) doesn't resolve, but that's because constraints cannot be another generic, so I believe it's out of scope.
  5. Changelog: reworded to say unknown fields still warn.

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