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

Idea: Better handling of templates in CreateFrame? #121

Open
emmericp opened this issue Dec 17, 2023 · 6 comments
Open

Idea: Better handling of templates in CreateFrame? #121

emmericp opened this issue Dec 17, 2023 · 6 comments

Comments

@emmericp
Copy link
Contributor

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 :)

  1. 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
Copy link
Contributor Author

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
Copy link
Owner

Ketho commented Dec 17, 2023

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 LuaLS/lua-language-server#1861

@emmericp
Copy link
Contributor Author

emmericp commented Dec 22, 2023

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
Copy link
Contributor Author

emmericp commented Dec 22, 2023

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
Copy link
Contributor Author

emmericp commented Feb 3, 2024

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

@emmericp emmericp reopened this Feb 3, 2024
@Ketho
Copy link
Owner

Ketho commented Feb 4, 2024

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

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

No branches or pull requests

2 participants