Lua: Prevent "duplicate method" error on gadget/widget callins

[rhys-vdw]

Work done

LLS currently believes all widget/gadget instances are the same object, and complains about the same callin being twice (even though it's actually two different objects).

This introduces a type for Widget and Gadget and forces LLS to believe each is a unique instance.

The trick is adding this to the top of each widget:

local widget = widget ---@type Widget

This shadows the "global" widget and tricks LLS into thinking it's a new instance of Widget.

Note

This isn't the most perfect solution, but about as good as LLS can get for this codebase (at present) and it's a fairly unintrusive. It is trivial to remove with a find and replace if neeeded.

Test steps

• Open branch in VSCode.
• Observe lack of this particular warning in widget/gadget.

Screenshots:

If you're making visible changes, add before/after screenshots or videos of the major
changes so it's easier for reviewers to see what is different in this PR

BEFORE:

AFTER:

[sprunk]

I didn't go through all 500 files to verify they're assigned correctly but the general idea looks good. A minor caveat would be to make sure some of the more convoluted wupgets/functions aren't at the 200 locals / 60 upvalues limit (I assume this would immediately show up at load though).

[rhys-vdw]

I didn't go through all 500 files to verify

If it helps, they were mostly achieved with:

find: function widget:GetInfo()
replace: local widget = widget ---@type Widget\n\nfunction widget:GetInfo()

make sure some of the more convoluted wupgets/functions aren't at the 200 locals / 60 upvalues limit (I assume this would immediately show up at load though).

Is that a limitation of Lua files in general? I was not aware of it. What is an upvalue?

[sprunk]

If you've got

local x
local function f()
    x = 1 -- upvalue: local to some scope outside ours
    g = 123 -- actual global (i.e. not local to any scope directly above)
end

then x is an upvalue of function f.

Max number of locals and upvalues per function is a limitation of the Lua VM and is defined here https://github.com/beyond-all-reason/spring/blob/00aa01002e8295f5f0dfd85b220eab5befcf9b38/rts/lib/lua/include/luaconf.h#L482-L489

So:

-- file.lua
local v1
local v2
...
local v200
-- local v201 -- error when running the file (a file scope is one big anonymous function)!

function f()
  v1 = v1 + 1 -- upvalue: local to the outside scope
  v2 = v2 + 1
  ...
  v60 = v60 + 1
  -- v61 = v61 + 1 -- also error
end

[WatchTheFort]

Some widgets and gadgets do have code before the GetInfo callin, but I haven't seen any that don't have it as the first callin.

[sprunk]

You could in theory also have widgets that do function GetInfo() or function foo() bla end; widget["GetInfo"] = foo etc., but this is one of the cases where everything being blindly copypasted works in your favour because AFAICT nothing strays from the boilerplate version.

[rhys-vdw]

Some widgets and gadgets do have code before the GetInfo callin, but I haven't seen any that don't have it as the first callin.

Yeah, it seems pretty consistent. In any case it removes a few hundred errors, if a few need to be manually fixed later it's not a huge problem.

[rhys-vdw]

Max number of locals and upvalues per function is a limitation of the Lua VM and is defined here

Ha. Well, I didn't check that... Um... Is there a way to?

[WatchTheFort]

Max number of locals and upvalues per function is a limitation of the Lua VM and is defined here

Ha. Well, I didn't check that... Um... Is there a way to?

You'll see errors in the log

[rhys-vdw]

Cool, I'll report back after some testing. Hopefully it reports these errors when files are loaded and not just when some codepath is hit.

[WatchTheFort]

Pulled it locally and there no more additional errors in the log, so that's a good sign.

If adding one more upvalue to a file would break it because it was at exactly 60, then that file has major architecture issues that need to be resolved.

[saurtron]

Don't really like this solution, at least if it doesn't have any benefits for the code itself.

It could be argued that making the widget/gadget local has some lua benefit tho. I'm talking from a perspective of purely avoiding LLS errors.

I think there should be some way around this that doesn't require modifying every file and also seems a bit fragile and cumbersome since it seems easy to forget. Also the upget and local limit in lua is no joke.

Not a big deal but I think it would better be avoided, specially since one thing is documenting code and code analysis and another is when we need to convert the code into a kind of typescript.

I believe it should be possible to just filter out the errors we don't want or instruct the LLS to assume every file has an independent declaration there. Otherwise maybe the files could be preprocessed before being parsed by LLS XD, to avoid having to modify all of them like that.

[rhys-vdw]

Don't really like this solution, at least if it doesn't have any benefits for the code itself.

Yeah, it's not my dream solution, but as mentioned it's the least intrusive. Ultimately though code exists to be maintained by humans and whittling down the error list to actual errors (there are many!) is a great benefit to maintenance, and therefore the code.

I think there should be some way around this

I mean... I've spent the last few months experimenting and asking LLS users. There are "better" ways of expressing it but they are much more complicated. Long term I might find a cleaner solution, but as I said above, this is as good as I can come up with.

seems a bit fragile and cumbersome since it seems easy to forget.

When you hover the error, you also hover widget/gadget so you see this:

https://github.com/rhys-vdw/Beyond-All-Reason/blob/db0c9ab527b78ce2a9cb8eada3784f45771bd863/types/Widget.lua#L7-L17

---**Attention:** To prevent complaints from Lua Language Server, e.g.
---
---> ```md
---> Duplicate field `CommandNotify` (duplicate-set-field)
---> ```
---
---Add this line at the top of your widget script:
---
---```lua
---local widget = widget ---@type Widget
---```

But yeah, ideally the API could be changed so that it can be marked up naturally. For example if a new widget was created like:

local widget = Widget.Create()

Also the upget and local limit in lua is no joke.

Just so I understand—every time someone adds a local variable to a file this a pretty big deal?

I believe it should be possible to just filter out the errors we don't want or instruct the LLS to assume every file has an independent declaration there. Otherwise maybe the files could be preprocessed before being parsed by LLS XD, to avoid having to modify all of them like that.

I don't think it's possible to solve this specific problem, because the situation is that this does appear as a global variable and tehre's no way to tell LLS that it's not. The best solution would be to create a function like Widget.Create() or some alternative API that doesn't depend on globals.

You're more than welcome to experiement, but I am quite confident this is the right step for now (even if we get to revert it later)

[saurtron]

local widget = Widget.Create()

That looks better to me, not sure what others think. Only problem is not sure if we can do that in a way that LLS likes, but I suppose we can. If other devs like that solution better and we can't find a way to simply not add anything, we can try to make it work.

Just so I understand—every time someone adds a local variable to a file this a pretty big deal?

Yes, also even tho it can be worked around there is the possibility it can trigger errors in widgets/gadgets close to the limit, not sure if it can trigger till the affected funcions hit too. It's not a very big probability but I think it could happen.

I don't think it's possible to solve this specific problem, because the situation is that this does appear as a global variable and tehre's no way to tell LLS that it's not. The best solution would be to create a function like Widget.Create() or some alternative API that doesn't depend on globals.

You're more than welcome to experiement, but I am quite confident this is the right step for now (even if we get to revert it later)

Ok, as I said it's not the end of the world so not really a blocker, but I'll pm you to see if we can come up with anything.

[rhys-vdw]

local widget = Widget.Create()

That looks better to me, not sure what others think. Only problem is not sure if we can do that in a way that LLS likes, but I suppose we can. If other devs like that solution better and we can't find a way to simply not add anything, we can try to make it work.

This is my preference actually, I thought it would be more controversial so I didn't do it. It's very easily achieved.

[sprunk]

Just so I understand—every time someone adds a local variable to a file this a pretty big deal?

No, the limits are actually very high, just despite this existing BAR wupgets tend to be old shitcode and a handful used to be near the limit. Ideally somebody would measure because the situation may have improved in the last 10 years.

also seems a bit fragile and cumbersome since it seems easy to forget.

This is an argument for keeping local widget = widget over local widget = Widget.Create() or similar, because the former is almost a no-op so nothing bad actually happens to widget logic if you forget it, just that you get a warning if you happen to develop in vscode. If Widget.Create() just returns the same table as global widget so widget:SomeCallin() still works if you forget it then it sounds fine though.

[rhys-vdw]

@sprunk thanks for the feedback. I had a chat with @saurtron last night regarding this issue. His position is that we should introduce a function like this:

function Widget.Create()
  return _G.widget ---@type Widget
end

I also suggested we could even go down this path:

local widget = Widget.Create({
  name = "My cool widget",
  author = "Me!",
  layer = 888
})

function widget:SomethingHappened()
  Spring.Echo "And i love it!"
end

---@param info WidgetInfo
---@return Widget widget
function Widget.Create(info)
  local widget = _G.widget ---@type Widget
  function widget:GetInfo()
    return info
  end
  return widget
end

However...

Any change like this is a change to the official API of BAR, which means that gadgets (including third-party gadgets) may start to use this API. If added, we should honour this Widget.Create function forever.

After consideration, I am against this change.

While the above is much nicer, I believe we should spend some time formulating the best possible API for widgets/gadgets before we make any changes to actual logic.

This is an argument for keeping local widget = widget over local widget = Widget.Create() or similar, because the former is almost a no-op so nothing bad actually happens to widget logic if you forget it

I agree. I want to move cautiously and respect the importance of public API and back-compatibility. If we want to make a change to how widgets/gadgets are defined we should not do it flippantly just to satisfy the type behaviour of LLS right now. Bear in mind that the tooling itself is evolving and we might yet find a better way to type what we have now.

In summary: I would prefer to stay with the current little local widget = widget ---@type Widget hack for now, and keep in mind the possibility of making something better in future.

[...] just that you get a warning if you happen to develop in vscode.

Side note: This is not a VSCode thing, it's lua-language-server which is supported in any IDE that has language server support (NeoVim etc is supported, but probably any modern code editor). It's also possible to run checks in CLI.

[rhys-vdw]

Alright, I ran the branch locally. Did a 1v1 and a raptors... Didn't see anything unusual. Attached log in case someone else knows what to look for.
infolog.txt

[WatchTheFort]

Is it possible to inject this inside the widget handler itself, so it only needs to be done in one place?

[rhys-vdw]

Is it possible to inject this inside the widget handler itself, so it only needs to be done in one place?

Which?

[WatchTheFort]

The code that makes LLS think each widget has its own instance of widget.

[rhys-vdw]

The code that makes LLS think each widget has its own instance of widget.

We could write a function that returns the instance as discussed above. Locals are local to a file so they can't be declared in a different file.

[rhys-vdw]

The code that makes LLS think each widget has its own instance of widget.

The other option which would allow us to use globals that are local to one file (which we have now) without them being interpreted as the same global, would be to each widget and gadget in its own folder with its own .luarc.json. That's somewhat "correct" but would be a pretty big and horrible change.

I've been thinking of some alternative widget APIs that use a more idiomatic Lua pattern. When I'm done on this larger type project I'd like to present them. There are many ways to do this without a global.

[WatchTheFort]

I'm happy to merge this as-is. However, it is going to add a level confusion because each widget will have a cryptic local widget = widget ---@type Widget line, without an explanation of why it's needed.

Perhaps we could a short -- Workaround for LLS duplicate callin warnings comment before the line.

[rhys-vdw]

Perhaps we could a short -- Workaround for LLS duplicate callin warnings comment before the line.

When you hover the error, you also hover widget/gadget so you see this:

https://github.com/rhys-vdw/Beyond-All-Reason/blob/db0c9ab527b78ce2a9cb8eada3784f45771bd863/types/Widget.lua#L7-L17

---**Attention:** To prevent complaints from Lua Language Server, e.g.
---
---> ```md
---> Duplicate field `CommandNotify` (duplicate-set-field)
---> ```
---
---Add this line at the top of your widget script:
---
---```lua
---local widget = widget ---@type Widget
---```

I felt this was sufficient until we can update the API. If you remove the line, the warnings appear, when you hover widget you see this explanation.

[WatchTheFort]

Good enough for me

[rhys-vdw]

Great. Rebased onto master. Think it should be good to merge at your convenience.