Ketho / vscode-wow-api

VS Code extension for World of Warcraft AddOns
https://marketplace.visualstudio.com/items?itemName=ketho.wow-api
MIT License
155 stars 33 forks source link

Idea: Better handling of templates in CreateFrame? #121

Open emmericp opened 11 months ago

emmericp commented 11 months ago

I have code like this:

frame = CreateFrame("Frame", nil, UIParent, "BackdropTemplate")
(...)
frame:ApplyBackdrop()
frame:Show()

The definition for CreateFrame makes Frame of type BackdropTemplate|Frame which means it triggers param-type-mismatch on frame:ApplyBackdrop() because BackdropTemplate|Frame != BackdropTemplate. I also can't cast frame to the template type because it doesn't inherit from Frame, so it would then fail on frame:Show() instead.

Suggestion for a fix in two steps:

1) explicitly define all templates as classes inheriting from Frame. Given that only BackdropTemplate exists this is just:

---@class BackdropTemplate: Frame

Now you can cast frame to BackdropTemplate or add an @type annotation :)

2) Ideally CreateFrame would already return the correct type, but I don't think "return type is arg4 if it exists, arg1 otherwise" can be fully modeled in the type system.

However, there is a work-around, we can duplicate the definition with different parameter numbers!

---[Documentation](https://warcraft.wiki.gg/wiki/API_CreateFrame)
---@generic T
---@param frameType `T` | FrameType
---@param name? string
---@param parent? any
---@return T frame
function CreateFrame(frameType, name, parent) end

---[Documentation](https://warcraft.wiki.gg/wiki/API_CreateFrame)
---@generic Tp
---@param frameType FrameType | string
---@param name? string
---@param parent? any
---@param template? `Tp` | TemplateType
---@param id? number
---@return Tp frame
function CreateFrame(frameType, name, parent, template, id) end

It's a bit ugly because template isn't the last parameter, but no one uses id anyways I think.

The class definition together with the duplicate CreateFrame make my example work correctly without any annotations :)

emmericp commented 11 months ago

Well, I guess the static definition is not enough if a non-base type inherits from a template, so what would really be needed is a dynamic class definition based on the generic parameters :(

Ketho commented 11 months ago

I can confirm these steps fix the param-type-mismatch warning but it's indeed a bit ugly. I have to admit I still don't really understand how luals works >.<

Dynamic class definitions would be cool, something like https://github.com/LuaLS/lua-language-server/issues/1861

emmericp commented 11 months ago

I think the "proper" solution would be a luals plugin that transforms CreateFrame calls and adds ---@class <something>: frametype, template(s). Actually shouldn't be too hard. Unfortunately I've now already done that by hand for all of our code :(

emmericp commented 11 months ago

proof of concept plugin, works for trivial cases; unfortunately LuaLS only allows for text -> text transformations, having the syntax tree available would be much better. And given how fragile this is I don't think anyone should use it, just playing around with plugins here.

function OnSetText(uri, text)
    local diffs = {}
    for start, var, arg1, arg2, arg3, arg4 in text:gmatch('\n()([^=\n]-)=%s*CreateFrame%(%s*["\']([^"\']-)["\']%s*,([^,)\n]*),([^,)\n]*),%s*["\']([^"\']-)["\']%s*,?%s*[^)]*%)') do
        local ins = ("---@diagnostic disable-next-line:undefined-doc-class\n---@class Frame%s: %s,%s\n"):format(
            uri:gsub("[^%l%u]", "") .. start, arg1, arg4
        )
        diffs[#diffs+1] = {
            start  = start,
            finish = start - 1,
            text   = ins,
        }
    end
    return diffs
end

This correctly handles cases like CreateFrame("Button", nil, nil, "BackdropTemplate")

emmericp commented 9 months ago

FWIW LuaLS added AST transformations for plugins recently and I built a plugin for Deadly Boss Mods which has a somewhat similar problem, maybe this can be an inspiration on how to resolve the template problem in a somewhat neat way: https://github.com/DeadlyBossMods/LuaLS-Config/blob/main/DBM-Plugin.lua

Ketho commented 9 months ago

Thanks, I will surely look into your luals plugin example. I have no experience with the AST, but it looks interesting.