-
-
Notifications
You must be signed in to change notification settings - Fork 331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Function Overloading Overhaul (@function
annotation)
#1456
Comments
This would also fix another issue I'm having where classes that have both a static and method variant of the same function aren't narrowing, eg: ---@meta
---@class Foo
---@overload fun(): Foo
Foo = {}
---@param eventName string
---@param callback function
function Foo.Subscribe(eventName, callback) end
---@param eventName string
---@param callback function
function Foo:Subscribe(eventName, callback) end
...
local inst = Foo()
inst:Subscribe(" --> Provides autocompletion for the static method, treating the string as the callback |
Yeah, I don't know of a way where you can change the callback param given a certain first parameter, which is a very common use-case for an event system or asynchronous code. Hopefully changing how overloads work and improving the narrowing makes it possible 🙂 |
This problem has been partially solved. However, the inconvenient thing is that it is impossible to display comments for each event separately. ---Registers a callback function for an event.\
---`function(client[, topic[, message]])`. The first parameter is always the client object itself.\
---Any remaining parameters passed differ by event:
--- @class Emit
--- @field on fun(self, event: string, cb: function)
--- @field on fun(self, event: '"connect"', cb: fun(client))
--- @field on fun(self, event: '"connfail"', cb: fun(client, reason: integer)) #If the event is `"connfail"`, the 2nd parameter will be the connection failure code.
--- @field on fun(self, event: '"message"', cb: fun(client, topic: string, message: string)) @If event is `"message"`, the 2nd and 3rd parameters are received topic and message, respectively, as Lua strings.
local emit = {}
emit:on("message", function (client, topic, message) end) or ---comment**********
---@class EmitL
---@field listen fun(self, eventName: string, cb: function)
--- comment for e1
---@field listen fun(self, eventName: '"e1"', cb: fun(i:integer)):table
---@field listen fun( eventName: '"e2"', cb: fun(s:string)) @comment for e2
---@field listen fun(self, eventName: '"e3"', cb: fun(i:integer, s:string)) @comment for e3
local emitL = {}
emitL:listen("e1", function (i) end)
emitL.listen("e2", function (s) end) |
Poking my head in to add ideas here (read: personal wish list features 😉). I think a good solution/step would be to support functions as first-class objects alongside classes. The issue with function aliases (
|
I think adding a
|
Here is another issue I am experiencing with ---@param event "A"
---@param callback fun()
---@return boolean
---@overload fun(event: "B", callback: fun()): string
local function listen(event, callback) end
local result = listen("A", function() end)
local result = listen("B", function() end)
---@overload fun(event: "A", callback: fun()): boolean
---@overload fun(event: "B", callback: fun()): string
local function listen2(event) end
local result = listen2("A", function() end)
local result = listen2("B", function() end)
|
One other thing with this is that if you're trying to overload something that already exists, it will give you both definitions, which is not ideal (observed by @carsakiller and myself). On top of that, if you try to See #1834 for the discussion about this. |
|
@function
annotation)
I'm wondering if there is any status on this or plans for the |
reply to: #1456 (comment) It's unrelated to
You have to add local Table = {}
---@overload fun(consume: (fun(value, key, cnt): any), tbl: table): nil, integer
---@overload fun(consume: (fun(value, key, cnt): any)): fun(tbl: table): nil, integer
Table.ForEach = {} --[[@as function]]
local mock = { 'a', 'b', 'c', 'd', 'e', 'f' }
local each = Table.ForEach(function () end)
local res1, cnt1 = Table.ForEach(function () end, mock) -- res1: nil, cnt1: integer
local res2, cnt2 = each(mock) -- res2: nil, cnt2: integer This should work for your case 😄 @vaderkos |
I want to add that, I tested some of the incorrect type narrowing issues mentioned in the thread and they actually work in latest version of v3.10.5 🎉 (maybe get fixed in the middle, or by the recent #2765 ) I tested the following and they work, so they maybe closed 😄 |
Wow, the original example does seem to work! I still have trouble with CC:Tweaked's event system, though, as an example.
Any?---@async
---@param event? ccTweaked.os.event
---@return any ...
---@overload fun(event: "alarm"): "alarm", integer
---@overload fun(event: "char"): "char", string
---@overload fun(event: "computer_command"): "computer_command", string...
function os.pullEvent(event) end This results in all of my returns being local event, alarmID = os.pullEvent("alarm")
--> ccTweaked.os.event, any
local event, character = os.pullEvent("char")
--> ccTweaked.os.event, any
local event, arg1, arg2, arg3 = os.pullEvent("computer_command")
--> ccTweaked.os.event, any, any, any Unknown?---@async
---@param event? ccTweaked.os.event
---@return unknown ...
---@overload fun(event: "alarm"): "alarm", integer
---@overload fun(event: "char"): "char", string
---@overload fun(event: "computer_command"): "computer_command", string...
function os.pullEvent(event) end This results in everything being local event, alarmID = os.pullEvent("alarm")
--> ccTweaked.os.event, integer|unknown
local event, character = os.pullEvent("char")
--> ccTweaked.os.event, string|unknown
local event, arg1, arg2, arg3 = os.pullEvent("computer_command")
--> ccTweaked.os.event, string|unknown, unknown, unknown
local s = character:sub(0, 3)
-- no unknown warning on :sub-ing a possibly unknown value??
---@type unknown
local a -- cannot infer unknown type Try redefining?---@async
---@param event? string The event to listen for
---@return string event The name of the event that fired
---@return unknown ... Any values returned by the event
function os.pullEvent(event) end
---@param event "alarm"
---@return "alarm"
---@return integer alarmID
function os.pullEvent(event) end
---@param event "char"
---@return "char"
---@return string character
function os.pullEvent(event) end
---@param event "computer_command"
---@return "computer_command"
---@return string ...
function os.pullEvent(event) end This is slightly better and slightly worse than the above attempt to use local event, alarmID = os.pullEvent("alarm")
--> string|"alarm", integer|unknown
local event, character = os.pullEvent("char")
--> string|"char", string|unknown
local event, arg1, arg2, arg3 = os.pullEvent("computer_command")
--> string|"computer_command", string|unknown, string|unknown, string|unknown I have tried a bunch of combinations of different tricks but am still unable to get ... pull requests accepted 😆 |
Yeah, seems I think the issue you encountered here is that you have to support custom events, in which you have to add If you remove the base function signature, then everything works fine. (ofc I know this cannot be the solution, because you have to support custom events). Some immature ideaWhen I was trying to fix #2758 (comment), I leant that the logic of finding matched call resides in here: lua-language-server/script/vm/function.lua Line 359 in ddc96bd
Currently it just uses
As this is unrelated to |
reply to: #1456 (comment) I think I know why your example code doesn't work as expected. It's because how LuaLS filter out unmatched function signatures. It first check the params count: lua-language-server/script/vm/function.lua Lines 392 to 408 in ddc96bd
In your code, you have
So when you are typing the code in the middle
If the ---@meta
---@class Foo
---@overload fun(): Foo
Foo = {}
---@param eventName string
---@param callback? function
function Foo.Subscribe(eventName, callback) end
---@param eventName string
---@param callback? function # add the `?` to callback
function Foo:Subscribe(eventName, callback) end
local inst = Foo()
inst:Subscribe(" --> this will be eventName now I don't know if this type narrow logic can be optimized, but I don't think an |
Someone really smart and good-looking must've written that, @tomlau10 😉 |
After 2 days (probably more) of trying to find a solution, I think I got it! Here is what we have, the following is what people have already been doing to get type narrowing correct with the callback completion, as described by @serg3295: ---@class Emitter
---@field on fun(self: self, event: string, cb: function)
---@field on fun(self: self, event: "success", cb: fun(client))
---@field on fun(self: self, event: "fail", cb: fun(client, reason: integer)) # Cool Message 1
---@field on fun(self: self, event: "message", cb: fun(client, topic: string, message: string))
local emitter = {}
emit:on("message", function (client, topic, message) end) Problem with this as described is you won't be able to have descriptions which is what the proposed ---@alias events.success
---|"success" # Emitted on successful connections - important to have only a single entry in this alias
---@alias events.fail
---|"fail" # Emitted on failures
---@alias events.message
---|"message" # Emitted when a message is received
--- @class Emitter
--- @field on fun(self: self, event: string, cb: function)
--- @field on fun(self: self, event: events.success, cb: fun(client))
--- @field on fun(self: self, event: events.fail, cb: fun(client, reason: integer)) # Cool Message 1
--- @field on fun(self: self, event: events.message, cb: fun(client, topic: string, message: string))
local emitter = {}
emit:on("message", function (client, topic, message) end) -- now you see a description that otherwise would have no been possible to see It is tedious when you have a lot of events like this, but not like I will be the on responsible for the large annotation packages in the market place 😝 Edit: never-mind perhaps! seems like I was too excited and didn't notice callback not being narrowed correctly anymore, how sad! A different approach to this is to define a general enumeration alias that contains all events with their descriptions, but this will result in a duplicated suggest, like below: Screencast.From.2024-10-27.03-04-43.mp4 |
reply to: #1456 (comment) I tested the given code snippet, although the auto suggestion is not narrowed correctly at the beginning, it works after typing emitter:on("success", f--< retrigger completion here after typing `f` I guess this is due to different logic of type narrow between completion and match call
|
I see. What would be the suggested "fixes"? I would imagine fixing the completion step is the preferred route here, by allowing alias types / alias strings along side string values. Maybe also make it so that the match all case account for the 1 argument difference? Reminder that the alias solution is a hack around the last proposed one, which ends up duplicated |
Yes, I agree that this is the preferred route. Here is my work in progress (subject to change): tomlau10@4de84f3 |
The Problem
I think function overloading needs some changes in order for it to really function in the way that most people would find useful. This is especially problematic with event systems, as I and others have encountered.
Example
Currently, let's say I have the following function that I want to provide an overload for:
Using
@overload
The first logical option is to use
@overload
:But there is a problem, when using methods (
:
), the first parameter only gets completions for the first@param
and ignores the@overload
entirely.Ok, so for testing, let's replace the method (
:
) with a static function (.
):This still isn't great, we are still offered both callbacks even though the info we have entered only matches the
@overload
. At least the first parameter was completed this time.Multiple Definitions
So then maybe we try defining multiple functions where each
event
param has the type set to the event name we are looking for:Now, even as methods (
:
) we are receiving correct completions for the first parameter... nice! However, we are still receiving two completions for the callback - there is no narrowing happening. The completion also shows theevent
as"Destroy"
, which is incorrect for our second definition as we have only allowed"Repair"
.At least when defining the function twice, we are able to write a separate description for each of them as well as their
@param
s and@return
s. However, we receive a warning saying that we have a duplicate field.Proposed Solution
See @flrgh's idea to add a
@function
annotation to add more in-depth support for defining functions overall.The text was updated successfully, but these errors were encountered: