Skip to content

[Types] LLM env layer (emmylua config, type stubs, manual fixes) - #7447

Closed
keithharvey wants to merge 4 commits into
beyond-all-reason:migfrom
keithharvey:fmt-llm-source
Closed

[Types] LLM env layer (emmylua config, type stubs, manual fixes)#7447
keithharvey wants to merge 4 commits into
beyond-all-reason:migfrom
keithharvey:fmt-llm-source

Conversation

@keithharvey

@keithharvey keithharvey commented Apr 16, 2026

Copy link
Copy Markdown
Collaborator

Warning

Superseded — this PR was an earlier mint of the stack. The type-error cleanup
now lands via the current stack tip fmt-llm #8398.
This slice's current counterpart is #8397.
Kept closed for review history; nothing here merges.

Part of BAR type-error cleanup. Human-curated env layer that prepares the codebase for the LLM type-fix pass.

This branch carries:

  • .emmyrc.json globals and analyzer config
  • types/* stubs for vendored/generated declarations
  • Explicit type ignores for known dead code
  • CI gate configuration
  • Manual source fixes that require human judgement

The fix recipes the subsequent LLM pass (#8235) uses are catalogued in SKILL.md — same rulebook that guides the subagents.

Branch Topology

All branches in the BAR type-error cleanup stack — see Bulk Migrations for the migration log and how to run just bar::migrate::stylua-cleanup. Regenerated deterministically by just bar::migrate::stylua-cleanup-generate. Generated 2026-07-22 19:34:36 UTC.

Leaves — each isolates one transform's diff vs fmt:

Branch Command Diff vs parent Units
fmt stylua 1425 files, +196955 −195449 ✅ pass
mig-bracket bar-lua-codemod bracket-to-dot 351 files, +7779 −7779 ✅ pass
mig-rename-aliases bar-lua-codemod rename-aliases 177 files, +376 −376 ✅ pass
mig-detach-bar-modules bar-lua-codemod detach-bar-modules 181 files, +1702 −1573 ✅ pass
mig-integration-tests <hand curated> 20 files, +134 −81 ✅ pass
mig-busted-types <hand curated> 12 files, +1501 −0 ✅ pass

Rollups — composite branches stacking the leaves and (for fmt-llm) the env + LLM layers:

Branch Diff vs master Diff vs parent Units
mig 1449 files, +201211 −198022 528 files, +11446 −9763 ✅ pass
👉 fmt-llm-source — you are here 1454 files, +201680 −198185 61 files, +523 −217 ✅ pass
fmt-llm 1456 files, +201733 −198206 32 files, +70 −38 ✅ pass

@github-actions

github-actions Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Integration Test Results

14 tests   6 ✅  3s ⏱️
 1 suites  8 💤
 1 files    0 ❌

Results for commit 673a85b.

♻️ This comment has been updated with latest results.

@keithharvey
keithharvey changed the base branch from master to mig April 22, 2026 17:19
@keithharvey
keithharvey changed the base branch from mig to master April 22, 2026 17:35
@keithharvey
keithharvey changed the base branch from master to mig April 22, 2026 17:44
@keithharvey
keithharvey changed the base branch from mig to master April 22, 2026 18:12
@keithharvey
keithharvey changed the base branch from master to mig April 24, 2026 15:45
@keithharvey
keithharvey changed the base branch from mig to master April 24, 2026 16:54
@keithharvey
keithharvey changed the base branch from master to mig April 24, 2026 16:54
@keithharvey
keithharvey force-pushed the fmt-llm-source branch 6 times, most recently from deb455c to d4149b0 Compare April 24, 2026 19:35

@keithharvey keithharvey left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Rationale for each env-layer decision, per file — every undefined-global suppression justified inline below. (Comments requested off-source so the code stays terse.)

Comment thread .busted
"spec/?.lua", "spec/?/init.lua",
}, ";")
}, ";"),
helper = "spec/spec_helper.lua",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

spec_helper installs the Spring/SpringShared/SpringSynced/SpringUnsynced shims that unit-def loading (Builders.Spring:WithRealUnitDefs) relies on. Previously only the _all and coverage tasks set helper, so the default task (just bar::units) silently swallowed ~20 unit-def load failures per run — any spring-split unit def touching SpringShared.GetModOptions() nil-dereffed at require time.

Comment thread .emmyrc.json
"widgetHandler",
"actionHandler",
"gadgetHandler",
"CALLIN_MAP",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

CALLIN_MAP is a real global in the LuaRules environment: luarules/gadgets.lua does VFS.Include(HANDLER_DIR .. "callins.lua"), which resolves to the engine base content's LuaGadgets/callins.lua (CALLIN_MAP = {} at top level). The analyzer can't see engine base content, so it's declared here. (An earlier revision of this branch suppressed it at the use site with a "likely dead code" TODO — wrong: gadgetHandler:RegisterGlobal is called by live gadgets and would crash if CALLIN_MAP were nil.)

Comment thread init.lua
-- This file includes common functionality that should be available globally

-- shim for engines without RecoilEngine LuaSpringContext::SetupAliases
SpringShared = SpringShared or Spring

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Backwards-compat shim for the Spring API type split. The RecoilEngine PR (LuaSpringContext::SetupAliases) creates these globals in every Lua sandbox; engines without that commit only expose Spring. This lets the spring-split codemod output run on both. Once the engine PR reaches mainline this is a harmless no-op (or Spring never fires).

-- Icon Generator Config File
--
-- info/scheme/faction are injected into this include's fenv by the caller
---@diagnostic disable: undefined-global

@keithharvey keithharvey Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

File-level suppression: this config is VFS.Included by the icon-generator gadget, which injects info, scheme, and faction into the include's environment as template parameters. The analyzer can't follow fenv injection, so every read of those three looks undefined. Scoped to this file only.

A refactor of how these globals are DI'd and accessed is probably the answer here, but outside the scope of this PR.

@@ -1,3 +1,6 @@
-- decompiled BOS: bare globals are per-unit script state
---@diagnostic disable: undefined-global

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

File-level suppression: this is the only .lua unit script in scripts/Units/ (everything else is .bos/.cob), decompiled from BOS. Unit scripts run one environment per unit instance, and the decompiled code uses dozens of bare globals as per-unit mutable state (bMoving, bAiming, rightArm, animSpeed, …) alongside framework-injected ones (unitID). Declaring them in .emmyrc.json would leak them workspace-wide; a file-scoped disable is the correct blast radius.

Comment thread common/luaUtilities/json.lua Outdated
local encodeString
local isArray
local isEncodable
local null -- assigned below

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

null was used by encode/decode before its function null() definition, resolving to a global at runtime (worked by accident: assignment ran before first call). Forward-declared as a local; function null() now assigns the upvalue.

Comment thread modules/lava.lua
-- Get lava config for map.
-- mapConfig has preference over gameConfig, unless game sets 'overrideMap'
local gameConfig, mapConfig
local mapNameNoVersion = mapName and trimMapVersion(mapName) or mapName

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hoisted out of the if mapName block because the MAP_CONFIG_PATH log below reads it even when mapName == nil (previously a nil-global read on that path).


--- Integration-test DSL that the `dbg_test_runner` widget injects into each
--- test file's env, plus extras from `common/testing/test_extra_utils.lua`.
---@class Test

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@meta stub for the integration-test DSL that dbg_test_runner injects into each test file's environment via setfenv (plus the flat-merged assertion/util globals from common/testing/). @meta keeps it analysis-only — nothing here exists as a runtime global outside test files. Named IntegrationTests.lua (not Test.lua after the class) to make the file's scope obvious when browsing types/.

Comment thread .github/workflows/type_check.yml Outdated
- name: Install emmylua_check
env:
# keep in lockstep with BAR-Devtools docker/dev.Containerfile EMMYLUA_VERSION
EMMYLUA_VERSION: 0.22.0

@keithharvey keithharvey Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Pinned to the same version as the BAR-Devtools dev container (docker/dev.Containerfile, ARG EMMYLUA_VERSION) so editor results match CI. Bump both together.

Reviewer note: these comments were all ported from looooong comments that used to live in the code, but full disclosure: these were ported here and improved by fable. This PR seems legit and surgical to me. Most of the emmylua globals exceptions we add are hanky VFS file loading and probably something we should refactor, but this PR is not the time.


local raptorStartBoxTexture = "LuaUI/Images/rapt-tileable_v002_small.tga"

local noRushTime = 0 -- was a bare read that always resolved nil; 0 matches runtime behavior

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

noRushTime is only ever a local in gfx_norush_timer_gl4.lua; these widgets have always read it as a bare identifier at the shader-uniform call site, i.e. nil → 0 at runtime (master behavior, pre-dating this branch). Declaring = 0 locally matches runtime exactly and lets the analyzer check the reference instead of suppressing it.

keithharvey and others added 4 commits July 22, 2026 13:25
…ixes

Human-curated environment that, together with the LLM type-triage pass
(fmt-llm), drives emmylua_check to zero on the migrated tree. Per-change
rationale lives in PR beyond-all-reason#7447 review comments.

- .emmyrc.json globals + diagnostics; types/* stubs; busted mock; CI gate
- forward-decl / assertEqual declarations; reverted orphaned kikito loader
- rationale-comment strip; types/IntegrationTests rename
- deterministic pins for type-triage leftovers the LLM mishandles on big
  files (multi_attack opts, HighlightUnit forward-decl, ripairs suppress,
  gui_pip gameFrame use-before-declare)
- json.lua forward-decl tidy (relocate null, drop dead decode_scan*)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
IsDevModeCached (upstream beyond-all-reason#6918) references `utilities` inside its own
table constructor, where the local is not yet in scope — Lua resolves
those reads as GLOBALS, so the first real call would index nil. Dormant
today only because nothing calls it. Forward-declare the local so the
closure captures it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0.22.0's analyzer resolves a local inside its own table constructor, so
constructor-self-reference globals — a real dormant-crash class (see the
springFunctions.lua fix) — passed CI silently. 0.24.0 catches them; the
whole workspace surfaces exactly the four occurrences of that one bug,
fixed in the previous commit, so the stricter gate lands green. Also
tracks the upstream release asset rename (arm64 -> aarch64).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gui_flowui.lua: Draw.Button uses `opaque` — Draw.Element's 17th
parameter, which Button never had; the global read was always nil.
Pinned false (behavior-identical) until upstream decides whether Button
should expose an opaque mode.

snd_notifications.lua: `customNotifications` is persisted by
GetConfigData but never declared or assigned anywhere. Declared nil so
the round-trip is explicit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@keithharvey

Copy link
Copy Markdown
Collaborator Author

Superseded by #8397 — recreated as a same-repo PR (head on beyond-all-reason instead of the fork) so the fmt → mig → fmt-llm-source → fmt-llm chain can use GitHub's native stacked pull requests. Branch content is identical; review history stays readable here.

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.

1 participant