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

Typing a table as an interface makes it abstract #825

Closed
Hedwig7s opened this issue Oct 15, 2024 · 11 comments · Fixed by #832
Closed

Typing a table as an interface makes it abstract #825

Hedwig7s opened this issue Oct 15, 2024 · 11 comments · Fixed by #832
Labels
semantics Unexpected or unsound behaviors

Comments

@Hedwig7s
Copy link

Hedwig7s commented Oct 15, 2024

Minimal test case:

--mod1
local interface test1
    a: string
end

local mod1 = {} as test1
mod1.a = "foo"

return mod1
--mod2
local mod1 = require("mod1") -- interfaces are abstract; consider using a concrete record

In this example there's no reason to use an interface but in a situation where you need to be able to use an interface both for subtyping and typing (e.g. define a class's type and also use it for subtypes in inheritance, but also getting classes from a function call (replacing mod1 the table)) you'd have to have a seperate record which would be messy and bloated

Unless there's another way to do this (like function generics or something) could this be looked into? Thanks

@Hedwig7s
Copy link
Author

Note: The reason I can't use local mod1: test1 is because in my setup I need to call a function to get a class (using middleclass) and the return is the base Class type so at some point I'd have to typecast anyway

@hishamhm hishamhm added the semantics Unexpected or unsound behaviors label Oct 15, 2024
@hishamhm
Copy link
Member

The bit that was similar to #829 is indeed solvable with local type as I commented there, but there might be something more in your example here indeed.

When I envisioned modules returning interfaces, I assumed those files would contain the returned interface only (e.g. what we did in luarocks/luarocks#1705 with the luarocks.types.* module hierarchy).

Returning an interface and yet having side-effecting operations in the module that would warrant a concrete require() in the generated Lua (the abstract local type = require(...) construct erases that line) is not something I had considered.

I am trying to understand your use-case better to see if there's a way to support this, or if organizing the code some other way would lead to a possible workaround. But at first glance I think a separate record would be needed, so that the module returns something concrete that can be required by the other module.

Given your example in #821 (reply in thread), wouldn't you already have to declare both an interface and a record to get the middleclass behavior?

@Hedwig7s
Copy link
Author

Hedwig7s commented Oct 16, 2024

Given your example in #821 (reply in thread), wouldn't you already have to declare both an interface and a record to get the middleclass behavior?

The issue is by declaring the instance as record, it would make it impossible to subtype later for inheritence without yet another interface which would get messy (having 3 types is already excessive for 1 class I feel)
And in this example {} is actually a class in my code (IDE didn't show the issue earlier)

The main issue is typing something as an interface marks it abstract
Also returning a table containing the class resolves it ({class=class}.class is shown as type "type Class = Class" rather than "Class" by vscode-teal)

@svermeulen
Copy link
Contributor

I currently use interface tables to do some runtime logic as well, so would be sad if interface tables didn't actually exist in the generated lua anymore. I'm using latest version now and I see that they do still exist in generate lua, but can't be used anymore due to the change in #830.

To elaborate more on my specific use case, I'm doing a kind of dependency injection in lua and using interface tables as the 'id', kind of like this:

local interface IFoo
end

local record Foo is IFoo
end

local function register(_id:any, _value:any)
   -- ...
end

local foo:Foo

register(IFoo, foo)

With the change in #830, this is no longer allowed. Is there any way to do a cast or something to suppress this error and get access to an "any" reference to interface tables?

BTW - I noticed today that enums that are obtained via an include now require the "local type" + "require" syntax as well, like this:

local enum Fruits
   "apple"
   "banana"
   "orange"
end

return Fruits
local type Fruits <const> = require("fruits")

It makes sense but didn't see this explicitly called out anywhere so mentioning in case this wasn't intentional

@Hedwig7s
Copy link
Author

First quickly with the enums that makes sense (typescript also doesn't allow abstract types to be required via standard import)
Second with the interfaces as parameters calling directly with the interface doesn't really make sense since the whole idea is interfaces don't exist at runtime in exchange for the ability to extend them (again same as typescript)
You should use an enum or string in the place of that (or record if the type isn't extended)

@Hedwig7s
Copy link
Author

Speaking of typescript interfaces they do not have this issue, aka an object can be cast to an interface without suddenly being abstract

@Hedwig7s
Copy link
Author

One fix could be #724 as then you can pass the type without it actually existing

@hishamhm
Copy link
Member

hishamhm commented Oct 18, 2024

I'm using latest version now and I see that they do still exist in generate lua

@svermeulen That's a bug, they should be fully abstract.

I'm doing a kind of dependency injection in lua and using interface tables as the 'id', kind of like this:

That is not a lot more typesafe than using a string/enum and doing register("IFoo", value), right? @Hedwig7s suggested the same above.

Your particular use-case looks tame enough, but exposing interfaces as concrete objects can have a bunch of other side-effects. I'd rather not go there now; interactions of purely abstract interfaces in the presence of polymorphism is already complex enough as is.

It makes sense but didn't see this explicitly called out anywhere so mentioning in case this wasn't intentional

Yes, that was intentional. All abstract types should require type annotation.

@hishamhm
Copy link
Member

hishamhm commented Oct 18, 2024

Speaking of typescript interfaces they do not have this issue, aka an object can be cast to an interface without suddenly being abstract

@Hedwig7s In general a record can be cast to an interface in Teal without becoming abstract.

I think I'm finally understanding the issue here now. In your original example, the compiler is treating local mod1 = require("mod1") as if mod1 had ended with return test1 (which should be abstract and require local type), but return mod1 should allow a concrete require.

@svermeulen
Copy link
Contributor

That is not a lot more typesafe than using a string/enum and doing register("IFoo", value), right? @Hedwig7s suggested the same above.

Your particular use-case looks tame enough, but exposing interfaces as concrete objects can have a bunch of other side-effects. I'd rather not go there now; interactions of purely abstract interfaces in the presence of polymorphism is already complex enough as is.

Makes sense. I'll just use a separate concept of ID when I upgrade like you suggest

@hishamhm
Copy link
Member

@Hedwig7s @svermeulen please give this PR a try (some of the new testcases include examples of the expected behavior, you can take a look).
#832

The original example from this issue now works. @Hedwig7s, I'm not sure if this will allow you to "use an interface both for subtyping and typing" as you originally intended; if this doesn't address your use-case, I would suggest declaring the interface in a separate file that can be required separately from the interface instance.

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

Successfully merging a pull request may close this issue.

3 participants