Improve typing

[vaisest]

This PR contains type definitions for the SimpleGraphic API, which gets loaded to _G but makes LSPs scream, because it doesn't know about them. This also helps with development by containing type hints. The file is strictly a definition file and doesn't get loaded at runtime.

I'd also like to have a look at solving the dynamic class loading somehow. It's quite weird and is seemingly completely incompatible with type hints, as anything that new() produces has the any type. If anyone has ideas, please help.

Modules can be typed more easily, with the following annotation:

---@module "Common/SomeThing"
local import = LoadModule("Common/SomeThing")

require would work similarly, but doesn't require the annotation to retain type information.

There would also be code quality benefits. As far as I can see, currently it is not really feasible to have linting in this project as there are way too many false errors. If these were fixed, problems such as dead code could probably be detected easily. I've noticed there have been a few code paths where code accidentally accesses an undefined global variable and which would crash. These would probably be easier to notice with linting.

[Wires77]

I wonder if HeadlessWrapper.lua can be used instead of this file? That one is meant to mirror SimpleGraphic's API for tools that use PoB on the command line.

[Nightblade]

it could

[vaisest]

Could it? I don't really know how the headless wrapper works, but doesn't this file override some functions so that the program can run headless?

The meta file doesn't do anything. It's just there for LSP info

[Wires77]

It does override some functions, but if the annotations can be in there instead of this .def.lua file it can pull double duty. PoB running normally also doesn't load HeadlessWrapper.lua, and those annotations could help out people running headless as well. If the LSP just sees these globally defined functions it could probably do the same for HeadlessWrapper.lua

[vaisest]

But HeadlessWrapper.lua is meant to actually be executed if you want to run in headless mode, right? The meta file isn't meant to be run at all.

At least with my current setup, I'd recommend explicitly ignoring the file in the LSP, exactly because it's not meant to be loaded with PoB, and the LSP reading it adds confusing things to the global namespace which then aren't actually present in PoB.

I'd also recommend ignoring the spec folder for the same reason. I think it makes sense to keep the def file separate

[Wires77]

HeadlessWrapper should have definitions of every global function from SimpleGraphic in there, so calling code doesn't break CLI usage with a missing global. Is there even a way to see things in the global namespace that it adds extra? I'm just trying to avoid having to add two functions every time we add one in SimpleGraphic's API. Here's the difference, for reference:

In HeadlessWrapper.lua but missing from _SimpleGraphic.def.lua:

runCallback
GetVirtualScreenSize
require
newBuild
loadBuildFromXML
loadBuildFromJSON

In _SimpleGraphic.def.lua but missing from HeadlessWrapper.lua:

NewArtHandle
GetDrawLayer
SetBlendMode
GetDrawColor
print
SetForeground

[Nightblade]

There are some annotation examples in emmylua_ls's files that might help, for example:
..\AppData\Local\emmylua_ls\resources\std\table\new.lua

If you're getting Claude (or w/e) to help, make sure it's only considering the latest (rust) version of EmmyLua, not the old .NET or Java ones.

[vaisest]

The latest PR changes the way classes are constructed. Instead of the constructor being in the varargs of newClass, it's now a separately defined method, which is called by new().

Basically, class creation is now:

---@class Example
local ExampleClass = newClass("Example", "Parent1", "Parent2")
---@param whatever: WhatTypeItIs it's a good idea to type these
function ExampleClass:Example(whatever)
    self.whatever = whatever
end

Which means the class type has information that self.whatever exists. In most cases this isn't super useful as everything is any, but at least now typing these isn't completely useless. It also works well for discovering fields and methods, since now you can use the IDE "go to definition" button to go to, e.g. a method added by the parent class.

It's also possible to add more fields to the class with ---@field under it, which is a good idea if a field is only supposed to be initialised outside the class file.

At least with Sumneko's LSP, there were also warnings about making a class with a typo in the class name.

This linter PR also seems relevant, as these changes make linters much more useful: https://github.com/PathOfBuildingCommunity/PathOfBuilding-PoE2/pull/1796/changes

Using parent class constructors still looks a bit weird. For example self.Control() has a warning, because the LSP doesn't see that the function gets self supplied to it by new() via some sort of Lua magic. I'll look into how that can be fixed.

LoadModule() might also be possible with generics in a similar way. require for example already works properly with no annotations.

[vaisest]

The last commit makes the proxy metatable (or whatever it's called) not supply self to the parent constructor. This means that you now use : to call it and type checking works correctly, as the LSP sees the self variable.

[mcagnion]

A local Codex-assisted analysis harness, followed by a Claude review of the findings, was used to sanity-check the current PR head (41d0976e1a353e20c2ea2ae741afedc8486a917e) against origin/dev.

Checks covered git diff --check, LuaJIT syntax loading for changed Lua files, normal headless startup, an Export startup smoke with SimpleGraphic stubs, focused luacheck on HeadlessWrapper.lua + _SimpleGraphic.def.lua, LuaLS diagnostics against origin/dev, and the Windows busted suite.

The pass found two runtime issues.

1. Export startup currently fails.

newClass now treats every vararg as a parent class name, but these Export classes still use the old trailing constructor function form:

src/Export/Classes/Dat64File.lua
src/Export/Classes/DatFile.lua
src/Export/Classes/DatListControl.lua
src/Export/Classes/GGPKData.lua
src/Export/Classes/GGPKSourceListControl.lua
src/Export/Classes/RowListControl.lua
src/Export/Classes/ScriptListControl.lua
src/Export/Classes/SpecColListControl.lua

The Export smoke fails with:

../Modules/Common.lua:72: attempt to concatenate local 'className' (a function value)

The minimal fix that passed locally was to convert those classes to the named constructor style already used by the migrated src/Classes/ files:

local XClass = newClass("X", "Parent")

function XClass:X(...)
    ...
end

2. _SimpleGraphic.def.lua is currently executed by HeadlessWrapper.lua, but its callback globals do not match the wrapper.

If _SimpleGraphic.def.lua is meant to stay definition-only, then HeadlessWrapper.lua probably should not dofile() it. If it is meant to provide the executable headless stubs, then SetCallback / GetCallback need to use the same callback table as the wrapper. The minimal executable-stub fix that passed locally was:

function SetCallback(name, func)
    __callbackTable__[name] = func
end

function GetCallback(name)
    return __callbackTable__[name]
end

In the current smoke this only matters if anything calls SetCallback or GetCallback after the headless wrapper runs; the symbol is undefined as written, so this looks like a latent bug rather than a guaranteed startup crash.

After applying those two local changes, these checks passed:

syntax check: pass
normal headless smoke: pass
export smoke: pass
luacheck smoke: pass
Windows busted suite: 224 successes / 0 failures / 0 errors / 0 pending

LuaLS also reports new diagnostics on existing dynamic patterns that the annotations currently model too narrowly, such as function-valued control shown / width / height, and Export data classes accessing fields without ---@field declarations.

Manual UI testing was not done.

[vaisest]

Commit c0a223e changes new() to not call the constructor. This is motivated by the lack of type checking on varargs. It wraps the constructor to check parent initialisation. The construction syntax would now be different:

new("Control"):Control(nil, {0, 4, 0, 0})

The codebase was updated using a regex to do this. This is useful, because the LSP can now see what you should be putting into the constructor, instead of only seeing any-typed varargs:

But it does change the code style more than the previous changesm as those were strictly limited to class definition. Thoughts?