Skip to content
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

Generic typing fails on complex class #392

Open
Wunder-Wulfe opened this issue Feb 24, 2022 · 9 comments
Open

Generic typing fails on complex class #392

Wunder-Wulfe opened this issue Feb 24, 2022 · 9 comments
Assignees
Labels
bug Something isn't working icebox This is a valid request but it's currently too far off to consider

Comments

@Wunder-Wulfe
Copy link

image
image
image
If i try the other typing method, I get this issue.
image
image
image

@Wunder-Wulfe Wunder-Wulfe added the bug Something isn't working label Feb 24, 2022
@zeux
Copy link
Collaborator

zeux commented Feb 24, 2022

This is likely because you need to use explicit self to type table-based OOP code instead of using Tuple: during definition, as we can't assign a meaningful type to implicit self (which is not great, we have some plans to mitigate this both for table-based OOP and with the use of records in the future).

If that doesn't help, please post source code for this example.

@Wunder-Wulfe
Copy link
Author

Wunder-Wulfe commented Feb 24, 2022

These errors should not be occurring. Although I do use the method definition :, it should still define my function as a member of Tuple, which is used as the __index of my metatable. Also, I don't understand why I can't cast my return types, even if I am using tuples themselves. I am not too sure why it says the recursive type has different parameters either, as the parameters are identical throughout the function.

As for source code:

Tuple Class
--!strict

local Tuple = {}

local metatable = {}

local function freeze2<T>(t: T): T
    return table.freeze(t :: T) :: T
end

function Tuple.new<T>(...: T): Tuple<T>
    return freeze2(setmetatable({...} :: {T}, metatable))
end

function Tuple.fromArray<T>(arr: {T}): Tuple<T>
    local dup: {T} = table.create(#arr)
    table.move(arr, 1, #arr, 1, dup)
    return freeze2(setmetatable(dup, metatable))
end

function Tuple:Slice<T>(from: number?, to: number?): Tuple<T>
    local rf, rt = if from==nil then 0 else from, if to==nil then #self else to
    if rf < 0 then
        rf = #self + rf + 1
    end
    if rt < 0 then
        rt = #self + rt + 1
    end
    local arr: {T} = table.create(rt - rf + 1)
    table.move(self, rf, rt, 1, arr)
    return Tuple.fromArray(arr)
end

function Tuple:Reverse<T>(): Tuple<T>
    local arr: {T} = table.create(#self)
    for i = #self, 1, -1 do
        arr[#arr + 1] = self[i] 
    end
    return Tuple.fromArray(arr)
end

function Tuple:Unpack<T>(start: number?, goal: number?): ...T
    return table.unpack(self, start, goal)
end

function Tuple:ToArray<T>(): {T}
    local arr: {T} = table.create(#self)
    table.move(self, 1, #self, 1, arr)
    return arr
end

function Tuple:Clone<T>(): Tuple<T>
    return self:Slice(1, -1)
end

function Tuple:Map<T, U>(f: (T)->(U)): Tuple<U>
    local arr: {U} = table.create(#self)
    for k: number, v: T in ipairs(self) do
        arr[k] = f(v)
    end
    return Tuple.fromArray(arr)
end

function Tuple:PairMap<T, U>(f: (number, T)->(U)): Tuple<U>
    local arr: {U} = table.create(#self)
    for k: number, v: T in ipairs(self) do
        arr[k] = f(k, v)
    end
    return Tuple.fromArray(arr)
end

function metatable:__tostring(): string
    return "( " .. table.concat(self:Map(tostring), ", ") .. " )"
end

metatable.__index = Tuple

export type Tuple<T> = typeof(freeze2(setmetatable({} :: {T}, metatable))) 

freeze2(metatable)
freeze2(Tuple)

return Tuple

I seem also to get this warning, despite autocomplete understanding this function
image

On a side note, table.freeze has a weird behavior with type parameters (which seem to convert my tables to include arrays sometimes and malform them in the return type, perhaps a generic Table type would make it copy the structure?)
image

@zeux
Copy link
Collaborator

zeux commented Feb 24, 2022

On a side note, table.freeze has a weird behavior with type parameters

Yeah :( It's the same issue we've identified in upcoming table.clone. Eventually this can be solved with constrained generics, for now we'll probably need to relax table.freeze type to simply be table.freeze<T>(t: T): T (which will not give a type error when called with a non-table, but that seems preferable to distorting types in type-safe code).

@Wunder-Wulfe
Copy link
Author

Wunder-Wulfe commented Feb 24, 2022

On a side note, table.freeze has a weird behavior with type parameters

Yeah :( It's the same issue we've identified in upcoming table.clone. Eventually this can be solved with constrained generics, for now we'll probably need to relax table.freeze type to simply be table.freeze<T>(t: T): T (which will not give a type error when called with a non-table, but that seems preferable to distorting types in type-safe code).

Is there a potential for inherited types in the future? i.e. using T, but inherting T from a Table type, or others? Essentially allowing the type T to be copied, but only if it satisfies the conditions of inherited types

i.e.

local function copy<T: {}>(tbl: T): T
    --....
end

@zeux
Copy link
Collaborator

zeux commented Feb 24, 2022

Unsure what you mean by specializing exactly - this already happens during instantiation, but the issue is that table.freeze currently has the following type signature:

table.freeze<K, V>(t: {[K]: V}): {[K]: V}

Which is to say, it accepts a dictionary that maps K to V and returns the same - which is more constrained than a fully general table type, which we don't have atm. If we had constrained generics with "table" constraint, we'd be able to say something like

table.freeze<T: table>(t: T): T

Which would mean "table.freeze returns the table with the exact same shape as was passed in, but is only valid when T is a table type".

@Wunder-Wulfe
Copy link
Author

Wunder-Wulfe commented Feb 24, 2022

Unsure what you mean by specializing exactly - this already happens during instantiation, but the issue is that table.freeze currently has the following type signature:

table.freeze<K, V>(t: {[K]: V}): {[K]: V}

Which is to say, it accepts a dictionary that maps K to V and returns the same - which is more constrained than a fully general table type, which we don't have atm. If we had constrained generics with "table" constraint, we'd be able to say something like

table.freeze<T: table>(t: T): T

Which would mean "table.freeze returns the table with the exact same shape as was passed in, but is only valid when T is a table type".

Yes this is exactly what I was referring to, and it also helps with other objects.

local function doSomething<T: Instance>(part: T): T
    --....
end

print(doSomething(workspace).CurrentCamera) --type can be resolved
-- if Instance was used, the type information would be dropped/lost

print(doSomething("hello")) --invalid type parameter
-- if plain generic templating was used, the type information would not be sufficient enough to indicate invalid arguments
local function doAnotherThing<T: (Vector3, number)>(a: T, b: T) -- faster way of defining ((Vector3, Vector3)->() | (number, number)->())
    --...
end

local function doAnotherThing<T: {X: number}>(a: T) -- anything with this component
    --...
end

@LoganDark
Copy link
Contributor

I ran into... something that may be similar to this(?) when trying to infer the type of a Signal class.

--!strict

local Signal = {}
Signal.__index = Signal

function Signal.new<A...>(): Signal<A...>
	return setmetatable({}, Signal)
end

export type Signal<A...> = typeof(Signal.new())

function Signal.Fire<A...>(self: Signal<A...>, ...: A...)
	-- ...
end

function Signal.Connect<A...>(self: Signal<A...>, handler: (A...) -> ()) -- this handler argument is what sets Luau off
	-- ...
end

local signal = Signal.new() :: Signal<number, number> -- Cannot cast 'Signal' into 'Signal' because the types are unrelated
signal:Fire(0, 0)

I thought, maybe the type definition was the issue - let's try changing it to this:

export type Signal<A...> = typeof(setmetatable({}, Signal))

Nope, doesn't work - the only thing that works is sucking it up and forward-declaring all the methods to consuming generics from a higher level (say, a SignalMeta type):

type SignalMeta<A...> = { -- This is disgusting and I hate this
	__index: SignalMeta<A...>,
	Fire: (self: Signal<A...>, A...) -> (),
	Connect: (self: Signal<A...>, handler: (A...) -> ()) -> ()
}

export type Signal<A...> = typeof(setmetatable({}, ({} :: any) :: SignalMeta<A...>))

I hate forward declaration because Luau usually lends itself to being able to infer everything automatically, giving you a really nice and DRY codebase if you just do the bare minimum and sprinkle the type syntax around when needed. However generics is a lot to ask from it.

The first code block I gave is not actually technically correct - export type Signal<A...> = typeof(Signal.new()), the type parameters A... of the call to Signal.new() are unspecified and therefore could be different from the type parameters A... of the actual Signal type being defined - in this contrived example it doesn't make much difference, but the second version export type Signal<A...> = typeof(setmetatable({}, Signal)) is more correct despite also breaking DRY (which is still slightly less annoying than having to redeclare all of Signal's methods).

I will note that, as a type system designed for a language where multiple return values is basically a first-class citizen, Luau does an extremely bad job modeling variadics (in my opinion). But that's a story for another day, this is a post about specifically generics.

@Wunder-Wulfe
Copy link
Author

Wunder-Wulfe commented Mar 4, 2022

Will probably be fixed or at least worked around now given this similar issue
#393

@andyfriesen
Copy link
Collaborator

I expect that the solution to this problem (and problems like it) will come from #863

@andyfriesen andyfriesen added the icebox This is a valid request but it's currently too far off to consider label Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working icebox This is a valid request but it's currently too far off to consider
Development

No branches or pull requests

5 participants