Skip to content

[feat] generics: capture types with backticks #3149

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jpeletier
Copy link
Contributor

This PR modifies the generics capture system to allow to capture types out of the type of the parameter passed, in addition to being able to pass the type as a string which is the way it currently works:

Currently:

---@class Vehicle
local Vehicle = {}

---@class VehicleColor
local VehicleColor = {}

---@generic T
---@param class `T`Color # the type is captured using `T`
---@return T       # T becomes class + "Color"
local function getItemColor(class) end

-- obj: is of type VehicleColor
local obj = getItemColor("Vehicle")

Now, the following is also possible:

---@class Vehicle
local Vehicle = {}

---@class VehicleColor
local VehicleColor = {}

---@generic T
---@param class `T`Color # the type is captured using `T`
---@return T       # T becomes typeof(class) + "Color"
local function getItemColor(class) end

---@type Vehicle
local someVehicle = {}

-- obj: is of type VehicleColor
local obj = getItemColor(someVehicle)

-- obj2: is of type VehicleColor
local obj2 = getItemColor("Vehicle")

Therefore, it is possible to pass a literal string that happens to be a type name like before, or otherwise pass an expression that evaluates to a type and that type will be captured with backticks. This allows to construct typings for functions that return different types that depend on the type of the object passed.

@tomlau10
Copy link
Contributor

tomlau10 commented Apr 5, 2025

I think there is a big problem here: 😕

  • if the type of the object is any / unknown / nil / or some compound type
  • the resulting type will be very weird

just using your example above

---@type Vehicle|nil
local v1 = {}
local r1 = getItemColor(v1) --> r1: VehicleColor|nilColor

PS: I am not maintainer of LuaLS, just one of the contributors.
So I cannot judge whether this PR at this state will be accepted or not.

@jpeletier
Copy link
Contributor Author

No problem, I will review that case and update the PR

@jpeletier
Copy link
Contributor Author

Hello @tomlau10 . I looked at this after your feeback.

I would leave the PR as-is. This indeed can end up creating weird types, but when you are using string you can also do weird stuff, namely, you can call getItemColor("nil") and you'll get nilColor. I think it is no big deal.

I have another variant of the code that works as follows:

---@type Vehicle|nil
local v1 = {}
local r1 = getItemColor(v1) --> r1: VehicleColor|nil

Meaning, nil, unknown, any, are propagated without string transformation.

Let me know which one would work best. I am fine either way. If not, what would you expect in such cases?

My use case is, I am writing Lua bindings for a major ECS, we need a syntax as follows:

---@class transform.Position : Component

---@class transform.PositionInstance
---@field x number
---@field y number

-- Retrieves the provided component value or nil if the component is not found
---@generic T
---@param component `T`Instance # component to retrieve
---@return T | nil
function entity:get(component) end

local pos = entity:get(transform.Position) --> pos: transform.PositionInstance|nil
if pos then
    print("Position: ", pos.x, pos.y)
else
    print("Entity does not have Position")
end

Please let me know your thoughts.

@tomlau10
Copy link
Contributor

tomlau10 commented Apr 6, 2025

but when you are using string you can also do weird stuff, namely, you can call getItemColor("nil") and you'll get nilColor. I think it is no big deal.

When using literal string, I don't think anyone would write getItemColor("nil") intentionally.
But when passing as a variable, I guess the chance of passing nil/any/unknown is much higher, either intentionally or accidentally. 🤔

Moreover, the current implementation would give inconsistent result in some of the following cases:
(still using your example above)

-- 1. just a variable with no annotation
local v1    --> v1: unknown
local r1 = getItemColor(v1) --> unknown, expected, but out of my expectation that it works correctly

-- 2. a table value
local v2 = {}   --> v2: table
local r2 = getItemColor(v2) --> unknown, expected, again out of my expectation that it works correctly

-- 3. nil from function
local function getnil() end
local v3 = getnil()  --> v3: nil
local r3 = getItemColor(v3) --> nilColor, unexpected I think

-- 4. integer const from function
local function get1() return 1 end
local v4 = get1()   --> v4: integer = 1
local r4 = getItemColor(v4) --> unknown, but I don't know what should be the behavior

-- 5. integer from function
---@return integer
local function getint() return 1 end
local v5 = getint()   --> v5: integer
local r5 = getItemColor(v5) --> integerColor, seems inconsistent with const integer case

-- 6. any from function
---@return any
local function getany() return end
local v6 = getany()
local r6 = getItemColor(v6) --> anyColor

Meaning, nil, unknown, any, are propagated without string transformation.

Let me know which one would work best. I am fine either way. If not, what would you expect in such cases?

I personally think skipping nil / unknown / any would be better 👍
But TBH I seldom use backtick in my projects, so my opinions may not be representative 😂 .

Again I am not author of LuaLS, so this may need opinions from maintainers. 🤔
But I do think that skipping those special types when processing would be good enough for this feature at the moment.
Since Completion is more important than perfection as author said: #3110 (comment) 😄

@Frityet
Copy link
Contributor

Frityet commented Apr 8, 2025

Hello @tomlau10 . I looked at this after your feeback.

I would leave the PR as-is. This indeed can end up creating weird types, but when you are using string you can also do weird stuff, namely, you can call getItemColor("nil") and you'll get nilColor. I think it is no big deal.

I have another variant of the code that works as follows:

---@type Vehicle|nil
local v1 = {}
local r1 = getItemColor(v1) --> r1: VehicleColor|nil

Meaning, nil, unknown, any, are propagated without string transformation.

Let me know which one would work best. I am fine either way. If not, what would you expect in such cases?

My use case is, I am writing Lua bindings for a major ECS, we need a syntax as follows:

---@class transform.Position : Component

---@class transform.PositionInstance
---@field x number
---@field y number

-- Retrieves the provided component value or nil if the component is not found
---@generic T
---@param component `T`Instance # component to retrieve
---@return T | nil
function entity:get(component) end

local pos = entity:get(transform.Position) --> pos: transform.PositionInstance|nil
if pos then
    print("Position: ", pos.x, pos.y)
else
    print("Entity does not have Position")
end

Please let me know your thoughts.

Maybe we could get something like:

---@generic T
---@param x `T`
---@return `T`Colour
local function f(x)
    return ...
end

@tomlau10
Copy link
Contributor

tomlau10 commented Apr 9, 2025

Since there is a v4.0 rewrite ongoing, I think it's better to first ask opinions from maintainers 🤔 @sumneko @CppCXY
Otherwise when v4.0 is released, this new syntax / feature won't be supported on day one.

And if v4.0 already planned this feature in another syntax, then we should take an implementation which is compatible with that new syntax, so to minimize any future migration overhead.

@CppCXY
Copy link
Collaborator

CppCXY commented Apr 9, 2025

I believe that too many peculiar syntaxes have been added to sumneko for various workarounds. This is neither consistent nor does it add cognitive burden. I think a better syntax would be to use built-in special generic classes based on supporting generics.

---@generic T
---@param a T
---@return CompositeType<T, "CCC">
function f(a)

end

Of course, this is just a suggestion. I understand that implementing this in LuaLS might be somewhat difficult.

@tomlau10
Copy link
Contributor

tomlau10 commented Apr 9, 2025

If there is no exact planning on the syntax of how v4.0 will handle this feature, then I think the current best option is to just keep the existing syntax as is 🤔

@jpeletier
Copy link
Contributor Author

@Frityet

Maybe we could get something like:

---@generic T
---@param x `T`
---@return `T`Colour
local function f(x)
   return ...
end

I agree this is the syntax that would make sense, but it is not how the backticks capture works right now unfortunately.

@CppCXY @tomlau10
This PR only changes where 'T' is read from. As of today, 'T' comes from a string. I am just adding that it may come from the type of the passed-in expression also, in a manner that is coherent with what exists today.
The discussion of what to do for a future version is beyond the scope of this PR.

So I agree with @tomlau10 :

If there is no exact planning on the syntax of how v4.0 will handle this feature, then I think the current best option is to just keep the existing syntax as is 🤔

... but adding the extra that 'T' may come from the type of the passed-in parameter, instead of exclusively deal with literal strings, otherwise I can't write the bindings to the ECS:

---@class transform.Position : Component

---@class transform.PositionInstance
---@field x number
---@field y number

-- Retrieves the provided component value or nil if the component is not found
---@generic T
---@param component `T`Instance # component to retrieve
---@return T | nil  # T is actually `T`Instance
function entity:get(component) end

local pos = entity:get(transform.Position) --> pos: transform.PositionInstance|nil
if pos then
    print("Position: ", pos.x, pos.y)
else
    print("Entity does not have Position")
end

@tomlau10
Copy link
Contributor

... but adding the extra that 'T' may come from the type of the passed-in parameter, instead of exclusively deal with literal strings

Yes, that's what I mean 😄

  • adding the new feature in this PR, while keeping the existing capturing syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants