LuaLS / lua-language-server

A language server that offers Lua language support - programmed in Lua
https://luals.github.io
MIT License
3.36k stars 319 forks source link

Please help me to understand generics (maybe a bug, I don't know) #450

Closed winterwolf closed 3 years ago

winterwolf commented 3 years ago

I expected here that test will be a type of Cls:

---@class Cls
local Cls = {}

---@generic T
---@param self T
---@return T
function Cls.new(self) return self end

local test = Cls:new()

return test

But it is any:

screenshot-001

What I doing wrong?

winterwolf commented 3 years ago

I get it, sorry :) I still have some problems, but I will close the issue until I can formulate them with examples.

winterwolf commented 3 years ago

Yes, I think it is the bug. Because if you say Cls.new(Cls) which is the same as Cls:new(), then everything will work fine.

screenshot-001

sumneko commented 3 years ago

Use ---@return Cls

winterwolf commented 3 years ago

@sumneko It's not a solution because classes can extend from each other. Let's say, Point can extend from Object and Rect can extend from Point and if Rect:new() will return Object, it will be wrong type and the editor will not show hints from either Point or Rect.

sumneko commented 3 years ago

How do you use the same constructor to construct different types of objects? Please provide your demo code.

winterwolf commented 3 years ago
---@class Obj
local Obj = {}

---@generic T
---@param self T
---@return T
function Obj.new(self) return self end

---@class Pnt:Obj
local Pnt = {x = 0, y = 0}

---@class Rct:Pnt
local Rct = {w = 0, h = 0}

local test = Rct.new(Rct)

-- local test = Rct:new()

return test

This is correct:

screenshot-001

Commented string should do the same.

sumneko commented 3 years ago

I can support this feature, but I think your writing style is not very good, you should set a separate construction function for each type.

winterwolf commented 3 years ago

@sumneko Thank you! Can you please explain, why do you think so? I use constructor from my OOP-library and want to just describe it once because it is the core feature of OOP itself.

sumneko commented 3 years ago
---@class Obj

---@class Pnt: Obj
---@field x number
---@field y number
local Pnt

---Create a point, XXXX
---@return Pnt
function Pnt:new() end

---@class Rct: Pnt
---@field w number
---@field h number
local Rct

---Create a Rect, XXXX
---@return Rct
function Rct:new() end

I prefer to write it like this, because usually different types of constructors require different descriptions and parameters.

winterwolf commented 3 years ago

@sumneko Oh, yes, of course you're right! But my library is a little more complicated than that. Its constructor doesn't instantly return instance of the class and can be changed as you like. The class instance is returned by another function that handles the constructor, which also keeps track of metamethods and other things. And this function is quite static so I want to describe it for all objects.

sewbacca commented 3 years ago

I think there is a pretty good reason for this use case, i stumbled on that too. I use a generalized object constructor with a specific initializer:

local class = require "class"
---@class Car : Object
local Car = class()

function Car:init()
    self.brake = false
    self.maxspeed = 100
end

return Car

And now the class file:


---@class Object
local Object = { }

--- Creates a new object from type T
---@generic T
---@param self T
---@return T
function Object:new(...)
    local object = setmetatable({}, self)
    if object.init then object:init(...) end
    return object
end

return function(parent)
    parent = parent or Object
    local class = setmetatable({ }, { __index = class })
    class.__index = class
    return class
end

So in main.lua maxspeed will should be hinted:


local Car = require "Car"
local car = Car:new()
car.maxspeed = 9000 + 1

There is one drawback though: You have to memorize all constructer variables (which maybe could be resolved, since the language server knows what self is and knows the parameters for T:init, so it could replace ... with the parameter information of init), but thats fine at least for me.

Cussa commented 3 years ago

Hello!

I think that it's really a bug. When working with OO, there are some situations that it's losing information. I created a bigger and deeper example, that I am sharing below.

---- ====================== Person class
---@class Person
---@field name string
local Person = {}
Person.__index = Person

---@return Person
function Person:new(name)
    local o = {["name"] = name}
    setmetatable(o, self)
    return o
end

--- Returns a generic that **should** inherits from Person
--- **When called with ":"** Returns a string. So, the plugin doesn't recognize the syntatic sugar for ":", expecting that the string will be the first parameter
--- **When called with "."** Returns the correct class.
---@generic T : Person
---@param self T
---@param text string
---@return T
function Person.says(self, text)
    print(self.name .. " says \"" .. text .. "\"")
    return self
end

--- Returns a generic of the **same type** that is calling. It will not care which type (could lead to problems in future)
--- **When called with ":"** Returns a string. So, the plugin doesn't recognize the syntatic sugar for ":", expecting that the string will be the first parameter
--- **When called with "."** Returns the correct class.
---@generic T
---@param self T
---@param text string
---@return T
function Person.whispers(self, text)
    print(self.name .. " whispers \"" .. text .. "\"")
    return self
end

--- It says it will return a <T>, but it returns "any"!
--- **When called with ":"** Returns a any. As it doesn't have a the input type, the plugin doesn't know which class to return
--- **When called with "."** Not possible to call.
---@generic T : Person
---@param text string
---@return T
function Person:speaks(text)
    print(self.name .. " speaks \"" .. text .. "\"")
    return self
end

---- ====================== Employee class

---@class Employee : Person
---@field company string
local Employee = {}
Employee.__index = Employee
setmetatable(Employee, Person)

---@return Employee
function Employee:new(name, company)
    ---@type Employee
    local o = Person.new(self, name)
    o.company = company
    return o
end

---@return Employee
function Employee:works()
    print(self.name .. ": I work at " .. self.company)
    return self
end

-- ===== TESTS =====
local jim = Employee:new("Jim Halpert", "Dunder Mifflin")
jim:works():says("Hello!")

-- says that it will return T, but returns string
local jim1 = jim:says("Call says with ':'")
-- says that it will return T, but returns string
local jim2 = jim:whispers("Call whispers with ':'")
-- says it will return T (even that it doesn't have the input parameter), but returns any => in theory, conceptual problem of definition from the function, as there is no way to specify that the return is a generic that is not included in the parameters
local jim3 = jim:speaks("Call speaks with ':'")

local jim30 = Employee.says(jim, "Call says with '.'")
local jim31 = Employee.whispers(jim, "Call whispers with '.'")
-- local jim32 = Employee.speaks(jim, "Says with .") => can't be called because of the way that we specified the function

-- previous calls in the fluid api/chaining method pattern
Employee:new("Jim Halpert", "Dunder Mifflin"):says("Call says with ':'")
    :whispers("Call whispers with ':'"):speaks("Call speaks with ':'")

print("finish")

In my case, I have a Person class, which has 3 methods: says, speaks and whispers. As I would like to use the Fluid API/Chaining Method approach, all of them return self. And I made some "tests" related how to specify the definition of return on it.

I have a Employee class that inherits from the Person. The idea here is to use generics, so if I call the says method from a Employee, it will still return the Employee.

When hovering the jim variable, it shows Employee, which is correct. image

When hovering the jim1 variable (returned from says method calling with syntactic sugar :), it shows string, which is an error. It seems that the plugin is understanding that the first parameter now is a string, as I don't have to pass the self. image

When hovering jim2, I see a similar situation that I have for jim1. image

When hovering jim3, it says it returns any. Most probably this is happening because today there is no way to specify the method with the syntactic sugar and the plugin understand that there will be a self "auto generated". image

Both jim30 and jim31 returns the expected result: Employee. However, this type of call doesn't allow to use the Fluent API/Chaining Method approach. image

It worth to say that sometimes, it seems that the plugin is losing some references, as just calling the reload VS Code, without changing any code, it starts to show a different return. But as we can see, even that it's saying that it's a Person, it shows the company property, as the works method, which only exists in the Employee class: image

Based on all of that, I would say that this is something that would be very interesting to fix. I even say that I can try to help, if @sumneko point me a direction to see it.

I think it worth to mention that the EmmyLua for IntelliJ has a possibility to specify the return as self, which, as far as I understood, is exactly what we would like in this case. (https://github.com/EmmyLua/IntelliJ-EmmyLua/pull/342)

I hope this extensive case shows why this is something interesting to be fixed.

sumneko commented 3 years ago

@Cussa Pull request is welcom! I just worry that my current implementation is not good enough, you may need to spend a lot of energy. Especially the examples given in the link you provided, I currently have no idea how to implement it. I am considering writing a document for others to participate in. Currently you can find the core code in script/core/guide.lua. The file has been unorganized for a long time and is constantly being patched. There are currently nearly 5000 lines. Read it It may be inconvenient.

sumneko commented 3 years ago

@Cussa Most issues will be fixed in 1.21.0, but some of them are other issues, please open a new issue about them, after you testing them in 1.21.0 . Since the subject of this issue has been resolved, I will close this issue.

Cussa commented 3 years ago

Hello @sumneko, do you have any estimation when the version 1.21.0 will be released?

Many Thnaks!