fix: generic type resolution for class methods and iterators#3392
fix: generic type resolution for class methods and iterators#3392RomanSpector wants to merge 8 commits intoLuaLS:masterfrom
Conversation
- Fix type narrowing for `x == nil and "default" or x` pattern: the `and` handler in the tracer now propagates outNode when the right side is a truthy literal, so the `or` branch correctly infers the variable as non-nil - Fix circular dependency in calcNode for assignments inside if/for blocks: a _compilingAssigns guard prevents incomplete types and stale marks from propagating when the if-handler's getNode(lastAssign) triggers recompilation of an already-compiling setlocal - Filter no-unknown false positives when a node only contains a variable object whose base declaration has a known type Closes LuaLS#2236, LuaLS#2374, LuaLS#2494
When iterating with ipairs(self) inside a generic method where self is list<T>, the loop variable v was resolving to unknown instead of T. The fix ensures the V->T resolution chain is preserved through three layers: sign resolution finds the mapping via class generic map and extends tables, generic.lua preserves the resolved wrapper when a generic maps to another generic, and compiler.lua allows these resolved-to-generic types through the return type filter. Supports both @field [integer] T and @Class list<T>: {[integer]:T} class definition patterns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Strip outer <> from generic name views when rendering doc.type.sign parameters, since the sign already wraps them in <>. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a doc.field has no associated class, getParentClass would pass nil to getGlobalNode causing 'attempt to index a nil value' error during invisible diagnostics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements several fixes for type inference and generic resolution, specifically addressing type loss in control flow blocks due to circular dependencies and improving the inference of the x == nil and "default" or x idiom. It also enhances the handling of generic classes, arrays, and method return types, while preventing a nil crash in getParentClass and cleaning up generic type display formatting. Review feedback suggests documenting all major fixes in the changelog, extracting duplicated logic for identifying purely generic nodes into a shared utility, and refactoring deeply nested logic in script/vm/sign.lua for better maintainability.
| * `FIX` Fix type inference for `x == nil and "default" or x` idiom [#2236](https://github.com/LuaLS/lua-language-server/issues/2236) | ||
| * `FIX` Fix type loss for assignments inside `if`/`for` blocks due to circular dependency in tracer [#2374](https://github.com/LuaLS/lua-language-server/issues/2374) [#2494](https://github.com/LuaLS/lua-language-server/issues/2494) |
There was a problem hiding this comment.
The changelog entries are for secondary fixes in this PR. Please also add entries for the main fixes related to generic type resolution as described in the PR summary. For example:
FIXResolve generic class method return types.FIXCorrectly resolve nested generics indoc.type.sign.FIXResolve loop variables foripairs(self)in generic methods.FIXFix display of generic types with double angle brackets (e.g.,list<<T>>).FIXPrevent nil crash ingetParentClassfordoc.fieldwithout a class.
There was a problem hiding this comment.
Done. Added changelog entries for all main generic resolution fixes.
script/vm/compiler.lua
Outdated
| local allGeneric = true | ||
| for rn in clonedObject._resolved:eachObject() do | ||
| if rn.type ~= 'doc.generic.name' then | ||
| allGeneric = false | ||
| break | ||
| end | ||
| end |
There was a problem hiding this comment.
This logic to check if a resolved node contains only generic names is duplicated in this file (lines 1796-1801) and also in script/vm/generic.lua (lines 203-209). To improve maintainability and reduce duplication, consider extracting this into a helper function in a shared module, for example on the vm object.
For example, you could define a function in vm.vm:
function vm.isPurelyGenericNode(node)
if not node then return false end
for rn in node:eachObject() do
if rn.type ~= 'doc.generic.name' then
return false
end
end
return true
endThen you could simplify this block and the other occurrences by calling vm.isPurelyGenericNode(...).
There was a problem hiding this comment.
Done. Extracted vm.isResolvedToGeneric() helper in generic.lua to replace all 3 duplicated checks.
| vm.getClassFields(uri, classGlobal, vm.declareGlobal('type', 'integer'), function (field) | ||
| if field.extends then | ||
| guide.eachSourceType(field.extends, 'doc.generic.name', function (src) | ||
| if genericMap[src[1]] then | ||
| resolved[vKey] = genericMap[src[1]] | ||
| handled = true | ||
| end | ||
| end) | ||
| end | ||
| end) | ||
| -- Also search extends tables (for @class list<T>: {[integer]:T}) | ||
| if not handled then | ||
| for _, set in ipairs(classGlobal:getSets(uri)) do | ||
| if set.type == 'doc.class' and set.extends then | ||
| for _, ext in ipairs(set.extends) do | ||
| if ext.type == 'doc.type.table' and ext.fields then | ||
| for _, field in ipairs(ext.fields) do | ||
| if field.extends then | ||
| guide.eachSourceType(field.extends, 'doc.generic.name', function (src) | ||
| if genericMap[src[1]] then | ||
| resolved[vKey] = genericMap[src[1]] | ||
| handled = true | ||
| end | ||
| end) | ||
| end | ||
| if handled then break end | ||
| end | ||
| end | ||
| if handled then break end | ||
| end | ||
| end | ||
| if handled then break end | ||
| end | ||
| end |
There was a problem hiding this comment.
This block of code has a couple of areas for improvement:
-
Deep Nesting: The logic to search extends tables (lines 114-136) is nested up to 6 levels deep. This reduces readability and makes the code harder to maintain. Consider refactoring this into smaller helper functions to reduce the nesting depth.
-
Inefficient Loops: The callbacks to
guide.eachSourceType(lines 105-110 and 121-126) sethandled = true, but the loops continue to iterate. Ifguide.eachSourceTypecan be made to break early (e.g., by returning a value from the callback), it would improve efficiency.
There was a problem hiding this comment.
Done. Extracted findGenericInTableFields() and simplified findGenericInExtendsTable() — max nesting reduced from 6 to 3 levels.
- Extract vm.isResolvedToGeneric() helper to replace duplicated allGeneric checks in generic.lua, compiler.lua (3 locations) - Extract findGenericInTableFields() and simplify findGenericInExtendsTable() in sign.lua to reduce nesting - Add changelog entries for generic resolution fixes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f322cdc to
5c78df1
Compare
Summary
Comprehensive fixes for generic type resolution in class methods, improving type inference for generic class patterns.
Problems Fixed
1. Generic class method return types not resolved (
compiler.lua,sign.lua)When calling a method on a generic class instance, return types using class generic parameters were not resolved:
2. Nested generic resolution in
doc.type.sign(generic.lua)Generic parameters inside nested sign types (e.g.
table<uint32, list<T>>) were not being cloned/resolved correctly, causing display issues likelist<<T>>and incorrect type resolution.3.
ipairs(self)in generic class methods (sign.lua,generic.lua,compiler.lua)Loop variables from
ipairs(self)inside generic methods were not resolved:The fix works through three layers:
sign.lua: ResolveV[](from ipairs signature) against generic classes via class generic map, searching both@fieldannotations and extends tables ({[integer]:T})generic.lua: Preserve generic-to-generic resolution wrappers (V->T) inmt:resolvewhen the resolved value is another generic parametercompiler.lua: Allow resolved generic wrappers through the return type filter inbindReturnOfFunction4. Double angle brackets in display (
infer.lua)Generic type parameters were displayed with double brackets:
list<<T>>instead oflist<T>. Fixed by stripping outer<>from generic name views when renderingdoc.type.signparameters, since the sign already wraps them.5. Nil crash in invisible diagnostics (
visible.lua)getParentClasscrashed with "attempt to index nil value" when adoc.fieldhad no associated class. Added nil guard.Test Plan
ipairs(self)with@field [integer] Tpatternipairs(self)with@class list<T>: {[integer]:T}patterntable<K, V>instead oftable<<K>, <V>>