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

Convert LDoc to lua-language-server #1775

Draft
wants to merge 61 commits into
base: master
Choose a base branch
from

Conversation

rhys-vdw
Copy link
Contributor

@rhys-vdw rhys-vdw commented Nov 17, 2024

Goal

Completely convert over to lua-language-server compatible Lua type annotations that can be imported into BAR and other projects that use Recoil.

Plan has been discussed with @badosu. It's going to take a little while.

Steps

Merge steps:

  • Move repos into BAR org
    • lua-doc-extractor
    • recoil-lua-library not needed, consumers can use submodule from main repo
  • Get privileges for both.
  • Update library generator workflow
    • Get PAT with "content" write privileges enabled for recoil-lua-library
    • Set this as RECOIL_LUA_LIBRARY_GITHUB_TOKEN in Spring repo

Post-merge steps:

Stuff to follow up on

  • What types are these

    spring/rts/Lua/LuaFBOs.cpp

    Lines 412 to 415 in 38598d1

    /***
    * @table attachment
    * attachment ::= luaTex or `RBO.rbo` or nil or { luaTex [, num target [, num level ] ] }
    */
  • What table are these LuaHandle functions on?
    /*** Called when the game is (re)loaded.
    *
    * @function LoadCode
    */
  • Same as LuaMenu ☝️
  • GetSolidObjectPieceInfoHelper returns [x,y,z] and other things return { x:, y:, z: }. Come up with good names for them (currently latter is float3).
  • PieceInfo class to use float3 type
  • Same with rgb in LuaUnsyncedCtrl::SetAtmosphere, this is an array, but would it also support a table?
  • Centralize shared types into their own file.
  • Two different and conflicting definitions of cmdOpts — one claims that it can be provided as an array of values, but has different fields — confirm which is true and whether array is supported (and which params are supported in this array)
  • Same as cmdSpec ☝️ (this is the same, but has the nested cmdOpts so is it the same?
  • Possible error in LuaUnsyncedCtrl::GiveOrder — returns 1 without pushing return value?
  • Unify GLenum type name and table name (GL). Bit confusing as is!
  • Do a final search for @func x and replace with @param x function
  • Consider defining a float type?
  • LuaZip.cpp seems to be completely unused, it has some methods on Spring and VFS and others on a table called ZipFileWriter but they're not referenced anywhere in Recoil in BAR.
  • Lots of duplication in LuaConstGL.cpp
  • Docs look completely wrong for:
    • CLuaHandle::UnitCommand
    • CLuaHandle::UnitCmdDone
  • LuaOpenGL.cpp is mostly undocumented.
Manual conversion checklist
  • LuaBitOps.cpp
  • LuaConstCMD.cpp (blocked by enums)
  • LuaConstCMDTYPE.cpp (blocked by enums)
  • LuaConstCOB.cpp
  • LuaConstEngine.cpp
  • LuaConstGL.cpp
  • LuaConstGame.cpp
  • LuaConstPlatform.cpp
  • LuaFBOs.cpp
  • LuaHandle.cpp
  • LuaHandleSynced.cpp
  • LuaMathExtra.cpp
  • LuaMenu.cpp
  • LuaMetalMap.cpp
  • LuaOpenGL.cpp
  • LuaRBOs.cpp
  • LuaRules.cpp
  • LuaShaders.cpp
  • LuaSyncedCtrl.cpp
  • LuaSyncedMoveCtrl.cpp
  • LuaSyncedRead.cpp
  • LuaUnsyncedCtrl.cpp
  • LuaUnsyncedRead.cpp
  • LuaVAO.cpp
  • LuaVAOImpl.cpp
  • LuaVBO.cpp
  • LuaVBOImpl.cpp
  • LuaVFS.cpp
  • LuaZip.cpp

@rhys-vdw rhys-vdw marked this pull request as draft November 17, 2024 05:44
@rhys-vdw
Copy link
Contributor Author

Is there a way to stop CI from running while the PR is in draft? I didn't realize it was going to keep spinning up CI every time I pushed.

@sprunk
Copy link
Collaborator

sprunk commented Nov 17, 2024

In that case check the other files because sometimes you do @param type name and sometimes @param name type.

* @param a1 integer
* @param a2 integer

* @param number x
* @param number z

@rhys-vdw
Copy link
Contributor Author

rhys-vdw commented Nov 17, 2024

In that case check the other files because sometimes you do @param type name and sometimes @param name type.

Sorry it's really going to take a while, I have a full time job. I spent the entire weekend writing the code extractor, and then did a first pass with sed. I'm quite motivated to get it all done, but you'll need to wait a little bit. There's only so much that can be achieved with regex replace!

Happy to receive feedback, but pls focus on the files I've checked off in the folded up "manual conversion checklist" in the PR description.

@rhys-vdw
Copy link
Contributor Author

Hm, actually you're right. The initial sed pass did output the @param args in the wrong order... Hm! I might fix it and rebase the changes in. Probably will be a net time saving.

@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch from 9e70eb3 to aae1c98 Compare November 18, 2024 04:48
@rhys-vdw
Copy link
Contributor Author

In that case check the other files because sometimes you do @param type name and sometimes @param name type.

Good catch @sprunk. I've updated the regex and rebase the other commits on top. 9696f51

@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch 2 times, most recently from adc2667 to 54b564b Compare November 18, 2024 08:31
rts/Lua/LuaSyncedCtrl.cpp Outdated Show resolved Hide resolved
@rhys-vdw
Copy link
Contributor Author

I've opened a draft PR on BAR repo with my latest generated docs: beyond-all-reason/Beyond-All-Reason#3949

@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch 2 times, most recently from d7284c9 to f22ef88 Compare November 20, 2024 17:09
@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch from dab73e0 to c2fe27b Compare December 1, 2024 09:51
Written by badosu
@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch from 196d616 to 802c044 Compare December 1, 2024 10:36
@saurtron saurtron added the documentation Improvements or additions to documentation label Dec 2, 2024
@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch from 802c044 to 6f071d8 Compare December 3, 2024 12:04
@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch from 4626943 to 7ec427e Compare December 8, 2024 11:15
@rhys-vdw rhys-vdw force-pushed the rhys-vdw/lua-language-server branch from 7a42c8e to eb26ffe Compare December 15, 2024 10:58
---@enum CMD
CMD = {
---@type -1
FIRESTATE_NONE = nil,
Copy link
Collaborator

@badosu badosu Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can convince lua-ls to actually define the constant with the number we already know

Copy link
Contributor Author

@rhys-vdw rhys-vdw Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by my lua-doc-extractor, so we do have full control over how it is generated.

It is actually defined as that constant. Constants are types in LLS, so @type -1 is correctly defining the value, LuaLS then disregards the nil. I can't remember exactly why I did it this way, but I think it was just easier to generate and seems to work fine.

However, I noticed that this style of enum declaration (marking a table as an enum) does not export to docs properly (it ends up just knowing there is an enum called CMD, but is not aware of its members). I believe it is "correct" in terms of the Lua interface, given that there is indeed a table called CMD defined in the global table. So I'd probably prefer to take this issue up with LuaLS maintainers and have it fixed upstream rather than screw around with the generation to make it conform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants