Overload intellisense issue with type of parameters

Rathoz commented 3 months ago

How are you using the lua-language-server?

Visual Studio Code Extension (sumneko.lua)

Which OS are you using?

Windows

What is the issue affecting?

Type Checking

Expected Behaviour

a is string|number

Actual Behaviour

a is string

Reproduction steps

---@overload fun(a: string): string
---@overload fun(a: number): table
function Foo.Bar(a)
    -- `a` is incorrectly assumed to be a `string`
    return a:lower() -- This does not error while it should!
end

Additional Notes

No response

Log File

No response

tomlau10 commented 1 month ago

I think I've found the cause of this issue, in the compileFunctionParam() of here: https://github.com/LuaLS/lua-language-server/blob/ba8f90eb0fab18ce8aee2bdbf7007dc63050381d/script/vm/compiler.lua#L1090-L1107

It returns true immediately after using the first function type's params' inferred view to set into the function object itself
Thus the param in the actual function only has the first type defined in the overload, instead of the combined type of all overloads

I tried to remove this early break, and it can solve this issue:

local function compileFunctionParam(func, source)
    -- local call ---@type fun(f: fun(x: number));call(function (x) end) --> x -> number
    local funcNode = vm.compileNode(func)
    local found = false
    for n in funcNode:eachObject() do
        ...
        found = true -- replace the original `return true`
        ...
    end
    if found then
        return true
    end

However this fix cause a few new problems...

When the function is a callargs type (in another word callback) and the type narrow function fail to find the exact signature, this will cause the params in the callback function have combined types, and this is undesirable in many cases.
- An example that I can think of is in the comment of an already closed issue: https://github.com/LuaLS/lua-language-server/issues/2758#issuecomment-2227686601
- with this suggested patch, the following code will be problematic:
```
local e = defines.events
script.on_event(e.on_ai_command_completed, function (event)
-- before patch: `event` = `EventData.on_ai_command_completed`
-- after patch: `event` = `EventData|EventData.on_ai_command_completed`
end)
```
- this is because the type narrow did not perform well in this case, and 2 signatures matches, one with the event as EventData.on_ai_command_completed, the other with type EventData
- and this patch will combine the 2...
Another problematic case is in this issue: https://github.com/LuaLS/lua-language-server/issues/2509 It actually works partially as of LuaLS v3.10.5. It only fails when pass rvalue as the type, because the base function is inferred as:
```
function expect(x: ffi.cdata*, type: "rvalue")
-> gccjit.LValue*|gccjit.RValue*|gccjit.Type*
```
- the reason is the same as described in this issue, because only the first overload param types are used when inferring the function params' type.
- with this fix, it will make things worse...
```
function expect(x: ffi.cdata*, type: "lvalue"|"rvalue"|"type")
-> gccjit.LValue*|gccjit.RValue*|gccjit.Type*
```
- with whichever type value: "lvalue"|"rvalue"|"type", all will match this base function, and no type narrow can be performed 😇
```
local lval = expect(data, "lvalue")
-> gccjit.LValue*|gccjit.RValue*|gccjit.Type*
-- because "lvalue" now matches the base function signature
```

PS: I am working on PR #2822 to try to fix that #2509 🤔

tomlau10 commented 3 weeks ago

I just found out that even with my #2838, now although a in Foo.Bar() can be correctly inferred as string|number, a:lower() will not trigger warning 😅 . I think that's because the string.lower actually accepts number as the 1st param: https://github.com/LuaLS/lua-language-server/blob/d1320ae5e41086fd9f569c2504a88130447444b8/meta/template/string.lua#L85-L88

string.lower indeed accepts string|number as 1st param, just that it doesn't allow number:lower() call style, and it is not addressed in this issue nor in my PR #2838.

Maybe @Rathoz you have to open another issue for that 😂

Rathoz commented 3 weeks ago

Correct on string.lower() accpect string|number.

number doesn't have any metatable, unlike string. aString:lower() is just syntactic sugar for aString.lower(aString) which thanks the metatable turns into string.lower(aString). But do the same on a number aNumber:lower() is tries to call the function aNumber.lower(aNumber), which doesn't exist.

I can open a new issue on it if you like

LuaLS / lua-language-server