Skip to content

refactor(Lua): Spring.X -> SpringBucket.X#2799

Open
keithharvey wants to merge 13 commits intobeyond-all-reason:masterfrom
keithharvey:lua-library-types
Open

refactor(Lua): Spring.X -> SpringBucket.X#2799
keithharvey wants to merge 13 commits intobeyond-all-reason:masterfrom
keithharvey:lua-library-types

Conversation

@keithharvey
Copy link
Copy Markdown
Contributor

@keithharvey keithharvey commented Feb 14, 2026

Part of BAR type-error cleanup.

What's it do

Split the monolithic Spring table into SpringSynced, SpringShared and SpringUnsynced. This supports clearer documentation and more easily understood code. The Spring table still exists for back compatibility.

CI IS PASSING!

Also adds a LUA_DOC_EXTRACTOR_BRANCH build arg to the ci in order to allow for lua-doc-extractor forks to be used, so we can test the CI for a new version of Recoil with an experimental version of lua-doc-extractor.
Here is the passing workflow.

Related Work

Motivation

The Recoil engine exposes a single Spring table in Lua, but different contexts (synced vs unsynced) have access to different subsets of functions. These options allow generating SpringSynced and SpringUnsynced type libraries from the same C++ sources without duplicating shared helper types that already exist in the per-file output.

image

@lhog lhog requested a review from rhys-vdw February 15, 2026 01:33
@rhys-vdw
Copy link
Copy Markdown
Collaborator

rhys-vdw commented Feb 16, 2026

Makes Spring a named LuaLS class so it can be used in type annotations and inheritance. All generated function signatures automatically become methods of the Spring class.

AFAIK this is not correct, Spring is a global table, essentially a namespace. There is no way to instantiate something of type Spring.

This allows downstream inheritance for mocks or other LuaLS code comprehension

AFAIK LuaLS should be able to infer that you've extended the table simply by doing:

Spring.foo = 5

Could you tell me more about the problem you ran into? Is this for a test suite?

@keithharvey
Copy link
Copy Markdown
Contributor Author

Hey @rhys-vdw . My goal is a "spring interface", which for my purposes behaves like an EmmyLua class. I want something I can inherrit from to guarantee completeness on my SpringMock when I inherit from ISpring or whatever.

@rhys-vdw
Copy link
Copy Markdown
Collaborator

Hey @rhys-vdw . My goal is a "spring interface", which for my purposes behaves like an EmmyLua class. I want something I can inherrit from to guarantee completeness on my SpringMock when I inherit from ISpring or whatever.

Sorry I'm not trying to push back too hard, but I did consider this exact thing and decided it was not the right approach at the time. I want to be careful about the change, but I'm open to being convinced.

One immediate problem with this is it's going to change the docs to add a Spring class in addition to the global table. It might also show the Spring global table with class Spring, which could be a bit confusing/disruptive. A class implies that it's something you can instantiate, but that's not how at all how the Spring API works—it's a global table.

Does emmylua actually provide the completeness checks when inheriting? It was pretty jank when I was messing around with it.

Have you actually tried adding the annotation? If you have got your whole thing running with the annotation, have tested the docs.

It's annoying that they don't want to implement typeof over there, it would sidestep this whole issue. :-(

Anyway, not completely opposed so long as you've actually tried it and it's working. I would at least want a non-doc comment there explaining why there is a class. Also, is your mock part of Spring or an external project?

@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Feb 26, 2026

nah I need to generate recoil-lua-library and do a post-Recoil-publication version, what I have gotten to work with some decent/surprising levels of edit-time type comprehension, if I jump through the correct hoops, is my own EmmyLua class as ISpring in types/spring.lua over in BAR. This "I" prefix naming might solve our problems around ambiguity here too? Make two independent "interface" EmmyLua classes, ISpringSynced and ISpringUnsynced, and have Spring itself inherit from both? Then we can reflect on the LuaSynced/Unsynced programmatically as part of our build step somehow? Either way, I do think publishing the explicit types is worth it, we could at least verify the mock completeness at test time, even if we're still building it with duck typing (my current mock situation). I'm really just trying to put guard rails in place to make sure this mock gets more useful over time, instead of less useful.

But if we can get away with just telling EmmyLua to express that table as a type with one line, that'd be ideal until we figure out the more complicated and bifurcated pure-interface solution? I'll try to actually generate this and link you the commit diffing the class diff over in BAR and hopefully some red underlines soon.

@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Feb 26, 2026

image

After thinking about this a bit more -- what if instead of one monolithic Spring class, the generator output to SpringSynced/SpringUnsynced based on which source file the functions come from? Spring would inherit from both:

---@class SpringSynced
---@class SpringUnsynced
---@class Spring : SpringSynced, SpringUnsynced
Spring = {}

That way LuaLS could actually catch "calling an unsynced function in a synced context" at edit time, which is a real class of bugs. The file-to-table mapping is already implicit (LuaSyncedCtrl.cpp -> synced, LuaUnsyncedCtrl.cpp -> unsynced), the generator just doesn't express it.

Implementation-wise, the cleanest version would be a CLI option on lua-doc-extractor like --table-mapping "LuaSyncedCtrl.cpp:SpringSynced,..." so the C++ annotations don't need to change. The alternative would be renaming all the @function Spring.X annotations in the C++ to @function SpringSynced.X etc., but that's ~460 lines for what's essentially a config change. I can do the latter in this PR or the former in a lua-doc-extractor PR, open to ideas.

@rhys-vdw
Copy link
Copy Markdown
Collaborator

I tried to keep it as simple as possible so that it's easy to maintain. The only reason that pattern was used for the various callback classes was because it was necessarily to mix and match them.

Since there is no way to type Spring based on file, there is little benefit to that approach. Trust me I've spent a lot of time thinking about this and discussing it. Err on the side of simplicity until features come out that actually solve the problems of the code base.

Adding extra classes is just going to make the docs not make sense, confuse people who are trying to document it, and discourage the many contributors who hate the idea of static analysis.

That said, the Spring class might be fine. I need to read the rest of what you said. Unfortunately you've caught me in the middle of an insanely busy week.

@rhys-vdw
Copy link
Copy Markdown
Collaborator

Actually I've changed my mind, you seem to have your head around it. Just try to keep things simple and consistent, add comments where there might be confusion. Maybe speak to @badosu too.

I'm still pretty sure there's no way to change the type of Spring based on context, but that is the holy grail as you've identified.

@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Feb 26, 2026

--- DELETED because it's now just noise. Basically I used the CI to spit out the buckets I wanted from the source files by just coding the buckets into the CI, as described above. rhys-vdw preferred a more self-expressive decorator pattern similar to what we were already doing, so we came up with the context idea. --

@lhog lhog requested a review from badosu February 26, 2026 12:57
@rhys-vdw
Copy link
Copy Markdown
Collaborator

Sorry haven't had a chance to have a full look at this, but if we're taking this route then it would be better to defined SpringSynced and SpringUnsynced all the way through the codebase and then do this, no?

---@class Spring : SpringSynced, SpringUnsynced
Spring = {}

@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Feb 28, 2026

Yeah agreed. I pushed a commit doing exactly that -- Spring now inherits from SpringSynced and SpringUnsynced directly, with a SpringShared base for the 425 functions that exist in both contexts.

The per-file output still uses Spring.* for now. Going fully "all the way through" means the per-file output would also use the constituent names, which breaks the 1:1 mapping from source files to docs pages. Two options: do it now as a follow-up, or ship this and let consumers migrate to the explicit types first, then graduate them to the primary output in a breaking version. Everyone accesses through Spring anyway so the migration path is just inheritance.

I can see value in keeping the source file categories on the Recoil side, too. Maybe the ideal is forming the input categories (source files) into output categories (Shared/Synced/Unsynced) at the docs level?

@keithharvey keithharvey changed the title Add ---@class Spring annotation to Lua library Synced and Unsynced EmmyLua Classes Mar 1, 2026
@keithharvey keithharvey mentioned this pull request Mar 1, 2026
@keithharvey keithharvey changed the title Synced and Unsynced EmmyLua Classes EmmyLua Synced-Unsynced-Shared Types Mar 1, 2026
@rhys-vdw
Copy link
Copy Markdown
Collaborator

I'm not sure why the library build is failing, it looks as though there is a bug in the parser unfortuanately.

@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Mar 14, 2026

I'm not sure why the library build is failing, it looks as though there is a bug in the parser unfortuanately.

Nah I think I just need to swap out the version of lua-doc-extractor as a separate docker image. The CI pulls from master currently and I didn't really sweat it. Works fine locally in my little test script so ostensibly it would work once https://github.com/keithharvey/lua-doc-extractor/tree/contexts hits main, just need to add a build_arg to the CI/docker over here [probably as a separate tiny PR]

@rhys-vdw
Copy link
Copy Markdown
Collaborator

rhys-vdw commented Mar 14, 2026

The CI pulls from master currently and I didn't really sweat it.

Oh that's bad. I have been releasing it on npm. I have some vague memory that there was a problem installing it from there, but now I'm not sure.

Edit: or do you mean CI is pulling Recoil from master instead of the PR branch? No it's not

@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Mar 15, 2026

Still haven't fixed the CI, but I did finish the @context refactor! @rhys-vdw check it out. I like the docs but let me know what you think (added a screenshot to the description)!

Comment thread rts/Lua/LuaSyncedRead.cpp
@keithharvey
Copy link
Copy Markdown
Contributor Author

Rebased on #2868 -- can always revert if that doesn't go but i need it for a unified workflow right now.

@keithharvey keithharvey force-pushed the lua-library-types branch 4 times, most recently from 8bc2619 to 7d9b121 Compare March 17, 2026 06:07
@keithharvey keithharvey force-pushed the lua-library-types branch 5 times, most recently from 98893c0 to a884e58 Compare March 23, 2026 22:48
@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Mar 30, 2026

The AST transform (keithharvey/bar#28) correctly maps every function to exactly one type

keithharvey and others added 8 commits April 12, 2026 10:44
- IsReplay always returns boolean, not boolean?
- GetUnitMoveDefID returns false|nil not boolean|nil, string|nil not string?
- VBO Download returns number[] not [number, ...][]
Use lua-doc-extractor's @context attribute to partition Lua API
documentation into synced, unsynced, and shared output files.
Tables appearing in multiple contexts (e.g. Spring) are remapped
to context-specific classes (SpringSynced, SpringUnsynced) that
inherit from a shared base (SpringShared).

Also adds CI support for overriding lua-doc-extractor repo/branch
via workflow_dispatch inputs and repository variables.

Depends on: rhys-vdw/lua-doc-extractor#77
…hared

SpringSynced and SpringUnsynced no longer inherit from SpringShared.
All three are independent classes; Spring inherits from all three
directly.

This prevents calling the same function via multiple types
(e.g. SpringSynced.GetGameFrame vs SpringShared.GetGameFrame).
Each function maps to exactly one type -- the language server
enforces the correct one, so developers don't have to memorize it.

Also removes remove_inherited_members from the doc site generator,
which was only needed to deduplicate inherited members.
Add LuaSpringContext::SetupAliases(lua_State*), a small inline helper
that creates `SpringShared`, `SpringSynced`, and `SpringUnsynced` as
references to the existing `Spring` global. Lua tables are reference
types, so any function added to `Spring` later (e.g. by the def-parser
extensions in Game.cpp::LoadDefs) is automatically visible through all
three aliases without re-population.

Call sites — every handler that builds a `Spring` table:
  * LuaParser.cpp        — def-parser sandbox (units/weapons/features/sounds)
  * LuaHandleSynced.cpp  — gadgets, both synced and unsynced halves
  * LuaUI.cpp            — widgets
  * LuaIntro.cpp         — intro screen
  * LuaMenu.cpp          — main menu

The alias names live in a single constexpr array in the new header
LuaSpringContext.h, with a doc-comment block cross-referencing the
type stubs in rts/Lua/library/Spring.lua and the lua-doc-extractor
@context tags that drive the type-system split. Spring.lua gets a
reciprocal comment pointing back at the header so the runtime/type
relationship is discoverable from either side.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tighten LuaSpringContext::SetupAliases to only expose the aliases each
sandbox actually needs, instead of registering all three everywhere.
Calling SpringUnsynced.X from a synced gadget context now fails at
runtime with the same "attempt to index nil" error EmmyLua already
reports statically -- runtime now enforces the type-system split.

Per-handler context:
  Synced   -> SpringShared + SpringSynced
  Unsynced -> SpringShared + SpringUnsynced
  Both     -> SpringShared + SpringSynced + SpringUnsynced

Call sites:
  * LuaParser.cpp       -- isSyncedCtxt picks Synced or Unsynced
  * LuaHandleSynced.cpp -- Unsynced for the unsynced half;
                           Both for the synced half (CSyncedLuaHandle
                           registers LuaUnsyncedCtrl in addition to
                           LuaSyncedCtrl, so synced gadget code can
                           call unsynced control functions by design)
  * LuaUI.cpp           -- Unsynced (widgets)
  * LuaIntro.cpp        -- Unsynced
  * LuaMenu.cpp         -- Unsynced

SpringShared is always exposed since it is the read-only intersection
valid in every sandbox.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The system.lua sandbox tables (used by unitdefs parsing and the base
gadget framework) only exposed `Spring`, not the context aliases
(`SpringShared`, `SpringSynced`, `SpringUnsynced`). Files loaded
through these sandboxes (unit definitions, base gadgets) could not
access the aliases set by SetupAliases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bruno-DaSilva and others added 3 commits April 12, 2026 13:57
Inverts the per-fenv Spring table model from aliases-to-Spring to
Spring-merged-from-split-tables, per review feedback on rhys's PR
discussion. The three @context buckets (`SpringShared`,
`SpringSynced`, `SpringUnsynced`) become the real tables; `Spring`
is a back-compat view merged from the appropriate subset at handler
init time.

Previously: `AddEntriesToTable(L, "Spring", X::PushEntries)` built a
unified Spring, then `SetupAliases` pointed all three split names at
it — meaning `SpringSynced.GetMouseState()` (wrong bucket) worked at
runtime despite being a type error statically.

Now: each handler's 3–4 Spring-feeding `AddEntriesToTable` calls are
retargeted to the correct split table (driven by each `Lua*` source
file's `@context` annotation), then `BuildSpringFromSplitTables`
merges the context-appropriate subset into `Spring`. Out-of-bucket
access fails at runtime with "attempt to index a nil value" —
matches what EmmyLua reports statically.

Per-file bucketing (source of truth: file-level @context tags):
  LuaSyncedRead.cpp      -> SpringShared   (@context synced, unsynced)
  LuaSyncedCtrl.cpp      -> SpringSynced   (@context synced)
  LuaUnsyncedCtrl.cpp    -> SpringUnsynced (@context unsynced)
  LuaUnsyncedRead.cpp    -> SpringUnsynced (@context unsynced)
  LuaUICommand.cpp       -> SpringShared   (shared UI helpers)
  CLuaMenu Load*Funcs    -> SpringUnsynced (menu fenv is unsynced)
  LuaUtils Echo/Log in
  LuaParser inline       -> SpringShared   (universal utilities)

Carve-out preserved: synced gadgets still get LuaUnsyncedCtrl
methods in their SpringUnsynced table (historical allowance — synced
gadgets can call unsynced-debug functions). Context::Both merges
all three split tables into Spring for that fenv.

BuildSpringFromSplitTables warns via LOG_L(L_WARNING, ...) on key
collisions across split tables — indicates an engine authoring bug
(a function landed in more than one @context bucket). First-merged
value wins so the warning is actionable rather than masked.

Handlers updated:
  LuaHandleSynced (unsynced + synced fenv)
  LuaUI
  LuaIntro
  LuaMenu
  LuaParser (inline Echo/Log/TimeCheck)
Matches the split-tables-as-primary engine model: every `Spring.X`
call site in Recoil's own Lua (cont/, AI/, tools/, test/) is now
routed to the correct `SpringSynced` / `SpringUnsynced` /
`SpringShared` bucket per the engine's `@context` annotations.

  SpringShared   : reads valid in both synced and unsynced contexts
                   (most of LuaSyncedRead, Echo, Log, TimeCheck)
  SpringSynced   : synced-only control (LuaSyncedCtrl)
  SpringUnsynced : widgets, IO, UI, local-player queries
                   (LuaUnsynced{Ctrl,Read}, GetMy{Team,AllyTeam,Player}ID)

Bucketing driven by the stubs under `rts/Lua/library/generated/`,
which are the output of `just lua::library` over `@context`-annotated
`Lua*.cpp` files. The same source of truth BAR's `spring-split`
codemod consumes, so Recoil's in-tree Lua and BAR's game content
stay classified identically.

Also cleans up three pre-existing bugs surfaced by the rewrite:
  * widgets.lua:484 `widget.SendToUnsynced = Spring.SendToUnsynced`
    was nil-deref — `SendToUnsynced` is a bare global set at line
    504 of LuaHandleSynced.cpp, never lived under `Spring.`.
    Fixed to `widget.SendToUnsynced = SendToUnsynced`.
  * widgets.lua:683 called `Spring.LOG(...)` which didn't exist;
    `LOG` is a top-level constants table (LOG.INFO etc.), and the
    function wanted was `Spring.Log` (now `SpringShared.Log`).
  * rts/Lua/LuaParser.cpp: added the missing @function doc block
    for `Spring.TimeCheck`. Without it, lua-doc-extractor never
    emitted a stub for the function, which broke IDE completion +
    hid it from any library-driven tooling.

Three call sites reference functions that have no C++ binding at all
(`Spring.IsUnitIconic`, `Spring.MakeFont`, `Spring.GetGroupAIName`).
They were always nil at runtime — the rewrite preserves that (they
now nil-deref through SpringUnsynced instead of Spring). Dead code
cleanup can happen in a separate pass.

Submodule AI/Skirmish/{BARb,CircuitAI} Lua not touched here —
upstream-maintained, needs its own PRs.
keithharvey and others added 2 commits April 15, 2026 12:00
… of truth

Up until now each binding declared @function Spring.X with the table
prefix Spring, and lua-doc-extractor's remapDocTableNames() rewrote
that at emit time to SpringShared/Synced/Unsynced.X based on a
file-level @context tag. Author friction (per rhys_vdw, verbatim):
"you add a new method, copy the doc from the one next to it, it ends
up being Spring.Foo, but then it's magically SpringSynced.Foo in
Lua. Or... It's both, and ppl don't use the new preferred tables."

Now that every consumer (BAR + Recoil in-tree Lua) references
SpringBucket.X directly, the remap has no reason to exist. Each
@function declaration names the final bucket directly; what you see
in the .cpp is what you get in the stub.

Rewrote 834 @function declarations across 10 files:

  LuaSyncedCtrl.cpp        SpringSynced   (was @context synced)      210
  LuaSyncedRead.cpp        SpringShared   (was @context synced,unsync) 266
  LuaUnsyncedCtrl.cpp      SpringUnsynced (was @context unsynced)    168
  LuaUnsyncedRead.cpp      SpringUnsynced (was @context unsynced)    173
  LuaPathFinder.cpp        SpringShared   (no prior @context)          7
  LuaMetalMap.cpp          SpringShared   (no prior @context)          4
  LuaUtils.cpp             SpringShared   (Echo, Log — no prior)       2
  LuaHandleSynced.cpp      SpringShared   (CallAsTeam, loadstring)     2
  LuaUI.cpp                SpringShared   (SetShockFrontFactors)       1
  LuaParser.cpp            SpringShared   (TimeCheck)                  1

Removed 6 file-level `@context` tags — no longer parsed.

Consolidated the 6 GiveOrder{Array,}To{Unit,UnitArray,UnitMap}
functions that were previously declared in both LuaSyncedCtrl and
LuaUnsyncedCtrl, and auto-promoted to SpringShared by the extractor
via promoteMultiContextDocs(). Now explicitly declared once as
@function SpringShared.GiveOrder* in LuaSyncedCtrl.cpp; dropped the
duplicate @function line from LuaUnsyncedCtrl.cpp (the prose
docblock there is kept for greppability but won't be extracted).

Updated LuaSpringContext.{h,cpp} comments that referenced the old
@context mechanism.

No consumer changes needed — BAR's and Recoil's in-tree Lua already
reference SpringBucket.X and will resolve identically against the
regenerated stubs. Verifier: `just lua::library` output should be
byte-identical to the pinned copy.

Prereq for the lua-doc-extractor simplification that deletes the
remap / partition / promote machinery.
Two corrections to 3c51c9c that surfaced during stub verification:

1. @field Spring.X decls (missed in the @function rewrite): 8 members
   on the back-compat Spring table that previously got renamed to
   SpringBucket.X by the extractor's remapDocTableNames() pass. Now
   authored with the bucket prefix directly:

     LuaSyncedCtrl.cpp  Spring.MoveCtrl       -> SpringSynced.MoveCtrl
                        Spring.UnitScript     -> SpringSynced.UnitScript
     LuaSyncedRead.cpp  Spring.{ALL,MY,ALLY,ENEMY}_UNITS -> SpringShared.*
     LuaRules.cpp       Spring.UnitRendering    -> SpringUnsynced.UnitRendering
                        Spring.FeatureRendering -> SpringUnsynced.FeatureRendering

2. Restored `@context unsynced` at the top of LuaObjectRendering.cpp.
   Non-Spring tables (ObjectRenderingTable, UnitScriptTable) don't carry
   a bucket prefix in their @function decls — their output placement
   relies on a file-level @context hint, the one surviving use of
   @context in the simplified extractor. LuaUnitScript.cpp already
   retained its @context synced.

After these, regenerated stubs match the pinned copy byte-identical
modulo the one intentional SpringShared.TimeCheck doc addition.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@keithharvey
Copy link
Copy Markdown
Contributor Author

keithharvey commented Apr 15, 2026

The build is failing because the submodule commit does not exist on origin, it works if you wire up the upstream.

Quick update but I went ahead and just changed the docs to be SpringBucket instead of Spring for consistency after talking with @rhys-vdw . This greatly simplified the lua-doc-extractor side of things.

@keithharvey keithharvey changed the title EmmyLua Synced-Unsynced-Shared Types refactor(Lua): Spring.X -> SpringBucket.X Apr 15, 2026
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

Successfully merging this pull request may close these issues.

3 participants