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

rawset does not support ---@class annotations #2862

Closed
LJSonik opened this issue Sep 20, 2024 · 10 comments · Fixed by #2867
Closed

rawset does not support ---@class annotations #2862

LJSonik opened this issue Sep 20, 2024 · 10 comments · Fixed by #2867

Comments

@LJSonik
Copy link

LJSonik commented Sep 20, 2024

How are you using the lua-language-server?

Visual Studio Code Extension (sumneko.lua)

Which OS are you using?

Windows

What is the issue affecting?

Annotations

Expected Behaviour

I expect EntitySerializer.serializePlayer to autocomplete properly in main.lua.

Actual Behaviour

It does not autocomplete because the ---@class annotation is apparently ignored with rawset.
It works if I do EntitySerializer = {} instead, but I cannot do that in the environment I work with.

Reproduction steps

EntitySerializer.lua:

---@class EntitySerializer
rawset(_G, "EntitySerializer", {})

Player.lua:

---@class EntitySerializer
local m = EntitySerializer

function m.serializePlayer(player)
end

main.lua:

EntitySerializer. -- No autocompletion here

Additional Notes

No response

Log File

No response

@tomlau10
Copy link
Contributor

tomlau10 commented Sep 21, 2024

---@class annotation is apparently ignored with rawset.

I think currently it is not supported, because a ---@class needs to be attached to a variable assignment. And rawset(...) is parsed as an function call not variable assignment, so luals don't know you want to annotate _G.EntitySerializer as ---@class EntitySerializer

but I cannot do that in the environment I work with.

Can you write it as _G.EntitySerializer = {} in your environment?

---@class EntitySerializer
_G.EntitySerializer = {}

workaround 1

If it's not possible either, another approach is to fake the language server by writing another dummy assignment statement:

rawset(_G, "EntitySerializer", {})

---@class EntitySerializer
EntitySerializer = EntitySerializer
-- or even _G.EntitySerializer = _G.EntitySerializer

workaround 2

If you found the above too hacky (or it doesn't work well due to your environment), then you may want to write a meta definition file which is only read by luals, to define this global variable class:

-- meta.lua
-- whatever filename is ok, but make sure it is in your workspace for luals to preload it
-- or add this file in your `workspace.library` setting
-- say if you put all meta files under `meta/` then you can just add `meta/` in `workspace.library[]`

---@meta _

---@class EntitySerializer
EntitySerializer = {}

workaround 3

this is not good... newly defined fields seems get injected into `table` class instead of `EntitySerializer` ... (you may still toggle to see this old suggestion)

edit: just another workaround 🤔

-- EntitySerializer.lua
---@class EntitySerializer
local EntitySerializer = {}
rawset(_G, "EntitySerializer", EntitySerializer)

but in this way EntitySerializer will have an union type of table|EntitySerializer when accessing in another file 😕

@LJSonik
Copy link
Author

LJSonik commented Sep 21, 2024

And rawset(...) is parsed as an function call not variable assignment
This is true, but a quick look at the source code gives me the impression that rawset is detected directly at the lexing level, and various parts of the code treat it as a code assignment, so I wouldn't be surprised if it is easy to support annotating it.

_G.XXX = is essentially the same as XXX = , so it doesn't work either.

Workaround 1 is too hacky.

Workaround 3 is too unreliable.

Workaround 2 is what I am currently using, but it is still far from ideal because you need to remember to do it for every new module, and because less-experienced modders in the community will be either confused or avoid the extension if they have to rely on hacks for basic functionality.

That said, workaround 2 is probably what I will continue using if I have no better option.

@tomlau10
Copy link
Contributor

May I ask what's special in your environment? Does it have special metatable set to _G which disallows injecting global by users? 🤔

@tomlau10
Copy link
Contributor

This is true, but a quick look at the source code gives me the impression that rawset is detected directly at the lexing level, and various parts of the code treat it as a code assignment, so I wouldn't be surprised if it is easy to support annotating it.

As a recent contributor having fixed a few issues, I would say many features of luals are just patching here and there, especially for those very specific use cases / features. Sometimes new features added in luals might break / not be compatible with existing features.

Unless someone can create a simple PR to fix a specific issue, I would never say that supporting / fixing that specific thing is easy. 😅

@LJSonik
Copy link
Author

LJSonik commented Sep 21, 2024

May I ask what's special in your environment? Does it have special metatable set to _G which disallows injecting global by users? 🤔

Yes, this Lua API has a protection against users accidentally creating globals, as it is a terrible practice 99% of the time and new users commonly forget to use the local keyword.
The occasional cases where one actually needs to do that, rawsetting into the _G table is the standard way to do it.

With that said, the idea of using a plugin to replace rawset(_G, "xxx", yyy) with xxx = yyy just came to mind, which could be a decent enough solution if it does turn out to be difficult or undesirable to support directly.

@tomlau10
Copy link
Contributor

tomlau10 commented Sep 22, 2024

if it does turn out to be difficult or undesirable to support directly.

Yeah though I am not 100% sure, luals may not plan to support using @class to annotate rawset. It's just not matching how those annotation works, those @type / @class needs to be binded to a variable assignment statement, and are not expected to bind with a function call.

The limited support for rawset, and a bug?

In addition, the current support for rawset seems very limited. It only works if the first argument is _G, where it will create a global variable for it. But if this is a local table, seems this statement is ignored:

local t1 = {}
rawset(t1, "a", 1)
print(t1.a)  -- unknown

local t2 = {}
t2.a = 1
print(t2.a) -- integer: 1

Moreover even for the case of _G, it always add a table type for the value, and I think it's a bug and should open another issue to track it: 😳

rawset(_G, "a", 1)
print(a)    -- integer|table
print(_G.a) -- integer|table

Further ideas

The occasional cases where one actually needs to do that, rawsetting into the _G table is the standard way to do it.

I don't know if you can do anything to your mod community, maybe you can propose to have a global utility function for creating global namespace

  • an api provided by your mod engine:
-- create a global variable so its name can be used later
function global(k, v)
    rawset(_G, k, v or false)
    return rawget(_G, k)
end
  • EntitySerializer.lua:
---@class EntitySerializer
EntitySerializer = global("EntitySerializer", {})

function EntitySerializer:xxx() end
function EntitySerializer.yyy() end
  • this uses an assignment statement which makes luals happy 😄
  • and whenever modders want define global variables they have to use this global() which makes things clear, without touching rawset or _G

With that said, the idea of using a plugin to replace rawset(_G, "xxx", yyy) with xxx = yyy just came to mind, which could be a decent enough solution

This will be indeed a good solution, as it doesn't require changing anything in you mod community👍

@tomlau10
Copy link
Contributor

After testing a bit, I think I am able to make it work (support @class in rawset()) 🤔

  • currently when binding the docs, the logic won't consider a call statement
    we have to add this in here:
    local bindDocAccept = {
    'local' , 'setlocal' , 'setglobal',
    'setfield' , 'setmethod' , 'setindex' ,
    'tablefield', 'tableindex', 'self' ,
    'function' , 'return' , '...' ,
    }
    local function bindDocs(state)
    local text = state.lua
    local sources = {}
    guide.eachSourceTypes(state.ast, bindDocAccept, function (src)
    sources[#sources+1] = src
    end)
local bindDocAccept = {
    'local'     , 'setlocal'  , 'setglobal',
    'setfield'  , 'setmethod' , 'setindex' ,
    'tablefield', 'tableindex', 'self'     ,
    'function'  , 'return'     , '...'      ,
    'call', -- add `call` type
}

local function bindDocs(state)
    local text = state.lua
    local sources = {}
    guide.eachSourceTypes(state.ast, bindDocAccept, function (src)
        -- allow binding docs with rawset(_G, ...)
        if src.type == 'call' then
            if src.node.special ~= 'rawset' or not src.args then
                return
            end
            local g, key = src.args[1], src.args[2]
            if not g or not key or g.special ~= '_G' then
                return
            end
        end
        sources[#sources+1] = src
    end)
  • and also here:
    if src.start >= start then
    if src.type == 'local'
    or src.type == 'self'
    or src.type == 'setlocal'
    or src.type == 'setglobal'
    or src.type == 'tablefield'
    or src.type == 'tableindex'
    or src.type == 'setfield'
    or src.type == 'setindex'
    or src.type == 'setmethod'
    or src.type == 'function'
    or src.type == 'return'
    or src.type == '...' then
    if bindDoc(src, binded) then
    ok = true
    end
...
     or src.type == 'return' 
     or src.type == '...'
     or src.type == 'call' -- for `rawset`
     then
        ...
  • then the followings will work 👀
---@class A
---@field id integer
rawset(_G, "A", {})

print(A.  <-- here will suggest `id` as integer field

But ...

the bug (?) that I mentioned in the previous comment still persists: A will be of a union type table|A instead of purely A ... 😅
Still need further debugging, maybe you or others can try to start from here to finish this feature request 😄

@LJSonik
Copy link
Author

LJSonik commented Sep 22, 2024

Thank you for your insightful comments!

I think I'll look into opening a separate issue for the table|A bug as you suggested.

The global() suggestion is also interesting and may be useful to other people, but it would not affect mods made in the past, so I will probably go with the plugin solution, or just keep it as-is if proper editor support ends up being reliably implemented.

@tomlau10
Copy link
Contributor

The related PR and bugfix is merged, so this feature will be in the next release of LuaLS 😄 @LJSonik

@LJSonik
Copy link
Author

LJSonik commented Sep 25, 2024

That was fast! Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants