-
-
Notifications
You must be signed in to change notification settings - Fork 339
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
Overriding methods #2569
Comments
We don't have a way to do this currently. You may be interested in writing a short plugin that would do that for you. |
I looked into plugins and I can e.g. replace |
Oh, looks like the Wiki docs aren't up to date anymore. There is more info on plugins on the project's GitHub pages: https://luals.github.io/wiki/plugins/. I can't really help you from here, I haven't used plugin api. |
Alright thank you for the links, I made a plugin which injects However I struggle to understand another concept, maybe you can help me here :) ---@class (exact) A
---@field ox number
local a = {}
function a:init()
--This is fine
self.ox = 0
end
---@class (exact) B : A
local b = {}
function b:init()
--This is an injection and thus invalid
self.ox = 0
end
---@type B
local x
--But this is fine again?
x.ox = 0 Why does it want to inject a new field into B in the B's init? How can I specify that its fine to inject when its already inherited? Another viewpoint would be: I don't want inheritance, but composition. Right now I have |
In my opinion, inject field diagnostic isn't smart enough yet and has its false positives. I suggest opening an issue or looking if an issue about this was already reported. |
Could you please consider sharing your plug-in? I'm stuck with then same issue. Appologise if you've already done publishing it. I've tried and failed to find it. |
Here: https://pastebin.com/fmWLcjjw Annotate overridden functions with ---@class (exact) A
local a = {}
---Test Comment
---@param test string Some argument
---@return string @ Some return
---@alias def.A.Method fun(self: A, test : string) : string
function a:Method(test) return "" end
---@class (exact) B : A
local b = {}
---@see A.Method
---@type def.A.Method
function b:Method(test)
return "hi"
end
---@type B
local test
test:Method("") -- Annotations are there! |
Thanks so much! |
已经有相同的issue了 |
Partially implemented in 7c481f5 now, arguments are inferred. |
If I understand the recent change correctly, it's for child class overriding parent class's method. ---@class skill
local skill = {}
---@param casterID integer
---@param targetID integer
function skill:onBegin(casterID, targetID) end
---@param casterID integer
---@param targetID integer
function skill:onEnd(casterID, targetID) end
---@return table<string, skill>
local generate_skill_objs = function ()
return {}
end
local list = generate_skill_objs()
function list.skill1:onBegin(casterID, targetID) end --< duplicate-set-field
function list.skill1:onEnd(casterID, targetID) end --< duplicate-set-field In order to make use of the recent change, you have to create new class for each of your skill, and extending the base skill class, like the following -- same code to generate `list` as above
---@class skill1: skill
local skill1 = list.skill1
function skill1:onBegin(casterID, targetID)
print(casterID, targetID) --< integer, integer
end
function skill1:onEnd(casterID, targetID)
print(casterID, targetID) --< integer, integer
end It can be interpreted as edit: ---@class skills1: skill
local skill1 = list.skill1
|
Thanks for comment. The recent change does cover part of my need, but not all of it. I know that I can do it this way. but there's still reason for the need of a more powerful implement. In my case, there are tens of skill objs in every character's script, and the number of character script is more. It will be considerable lines of redundant code and makes it easier to cause name conflict. Thus, I'm still hoping that we can just skip all those repeat annotations. Hopefully this enhancement could solve all the same related issues in #477, #1779 and #1757. |
btw your link for Actually you can just write |
Thank you! |
Strange, if I define the interface using ---@class skill
local skill = {}
---@type fun(self: skill, casterID: integer, targetID: integer)
function skill:onBegin(casterID, targetID) end
---@type fun(self: skill, casterID: integer, targetID: integer)
function skill:onEnd(casterID, targetID) end
---@return table<string, skill>
local generate_skill_objs = function ()
return {}
end
local list = generate_skill_objs()
function list.skill1:onBegin(casterID, targetID) end --< integer, integer, but with `duplicate-set-field` warning
function list.skill1:onEnd(casterID, targetID) end --< integer, integer, but with `duplicate-set-field` warning |
I think I have found a way!! 😄 @NicameOurister
---@class skill
---@field onBegin fun(self: skill, casterID: integer, targetID: integer)
---@field onEnd fun(self: skill, casterID: integer, targetID: integer)
local skill = {}
function skill:onBegin(casterID, targetID)
print(casterID, targetID) -- integer, integer
-- your default implementation
end
function skill:onEnd(casterID, targetID)
print(casterID, targetID) -- integer, integer
-- your default implementation
end
---@return table<string, skill>
local generate_skill_objs = function ()
return {}
end
local list = generate_skill_objs()
function list.skill1:onBegin(casterID, targetID)
print(casterID, targetID) -- integer, integer
-- no duplicate-set-field
end
function list.skill2:onBegin(casterID, targetID)
print(casterID, targetID) -- integer, integer
-- no duplicate-set-field
end I'm still trying to find out why |
Thanks for the solution! It works as needed and does really solved the biggest problem, but it produces some little new problems. I can totally accept those extra ---field definitions . But I'm still kind of bothered with the definition jump mask problem. After all, the whole purpose of mine is to make my code library as accessible as possible for others. I'm going to settle with the current solution while keep looking for a better way. Thanks again for the help. |
I think I have successfully made it working without breaking anything 🎉 (at least all existing tests passed). Root causeI believe the function param infer logic resides in here: lua-language-server/script/vm/compiler.lua Lines 1110 to 1124 in b9639f6
Fixing side effects
Fixing duplicate set fieldWe still have to deal with the lua-language-server/script/core/diagnostics/duplicate-set-field.lua Lines 54 to 70 in b9639f6
So we have to find a way to skip the warning of this overriding class function / method use case.
if vm.getDefinedClass(guide.getUri(def), def.node)
and not vm.getDefinedClass(guide.getUri(src), src.node)
then
-- allow type variable to override function defined in class variable
goto CONTINUE
end ConclusionWith the above proposed change, now we can have param type inference in your original annotation style, and without ---@class skill
local skill = {}
---@param casterID integer
---@param targetID integer
function skill:onBegin(casterID, targetID) end
---@type table<string, skill>
local list = {}
function list.skill1:onBegin(casterID, targetID)
print(casterID, targetID) -- integer, integer
-- no `duplicate-set-field` as well
end
function list.skill2:onBegin(casterID, targetID)
print(casterID, targetID) -- integer, integer
-- no `duplicate-set-field` as well
end I will need some time to do a more in-depth test before I create a PR. Meanwhile you may try to patch this in your local copy of luals to see the actual effect 😄@NicameOurister |
Roger that. Thanks for all these work. |
My related PR is merged recently, so this use case: #2569 (comment) will be supported in the next release 👀 |
I think it is no need to check |
For example: ---@class Player
local Player = {}
function Player:setName(name)
self.name = name
end
function Player:setName(name)
self.playerName = name
end
This had helped me successfully found a bug in my team's project🤦♂️ where the method wrongly defined using another namespace (instead of the namespace used in that file) local Parent = require "Parent"
-- this file should define methods of `Child` class
---@class Child: Parent
local Child = {}
function Child:f1() end
function Child:f2() end
function Parent:f3() end --> don't know why suddenly used wrong namespace here ...
-- and `Parent` does already have a `:f3()`
--> duplicate-set-field triggered
function Child:f4() end |
If you mean "we just accidentally used the same function name for completely unrelated logic, instead of doing so intentionally," and think that’s important for Lua, then why don’t other linters, like Robocop and PEP8 linters, focus on this issue? And here are some additional comments: Accidental function redefinition: Git merge conflict issues: Why it is inappropriate for a public linter to solve private issues: Impact on development flexibility: Over-generalization of checks: Code review and process issues: |
LuaLS (the language server) is to do type inferencing and provide type hints / completions. Now that each diagnostics can be turned on or off, if you think this diagnostic is useless, then you can just turn it off 😅 // ignore follow warning as code base is not fully annotated yet
"diagnostics.neededFileStatus": {
"need-check-nil": "None!",
"param-type-mismatch": "None!",
"assign-type-mismatch": "None!",
"unbalanced-assignments": "None!",
"cast-local-type": "None!",
"invisible": "None!"
},
I agree with you 👍 that every mistakes are made by human, not the language or the linter tool. ---@enum MyEnum
local MyEnum = {
a = 1,
b = 2,
}
print(MyEnum.c) --> warning: Undefined field `c`.
Code reviewers are also human, they can certainly reduce the chance of a coding error being merged to upstream, but they cannot catch all coding errors 😂.
I don't quite understand this statement 😕 .
LuaLS doesn't enforce PS: I am not maintainer of this project, I am just a user and a contributor. |
Okay, good! Thats alright 👍 Robocop and Pep8 have millions of contributors and a large community that LuaLS can't match. I just give my opinion and hope it Thank you for your recognition ❤. |
It would be great if I can override fields, not just shadow them, to retain their signature and doc.
The signature wont change for overridden methods, and thus I would like to avoid copy pasting the whole doc for each method. How would I resolve this efficiently?
The text was updated successfully, but these errors were encountered: