Conversation
There was a problem hiding this comment.
Code Review: LuaJIT Standard Library Type Definitions
Summary
The changes improve type accuracy for LuaJIT's jit, jit.profile, and jit.util modules. Overall the direction is good, but there are several correctness issues worth addressing.
Issues & Recommendations
1. jit.lua — jit.attach return type missing
--- @param func function
--- @param event? string
function jit.attach(func, event) end- Issue:
jit.attachreturns the previous callback (ornil). The return type is undocumented, so callers lose type info. - Suggestion: Add
--- @return function?(or the appropriate callback signature).
2. jit.lua — jit.security mode type is too narrow
--- @param mode? 'prng' | 'strhash' | 'strid' | 'mcode'- Issue: LuaJIT's
jit.securityaccepts"prng","strhash","strid","mcode", and"none"(to query all). Also, whenmodeis omitted the return is a table of statuses, not aninteger. - Suggestion: Either add
'none'to the union, or document the overload:--- @overload fun(): table --- @param mode? 'prng' | 'strhash' | 'strid' | 'mcode' --- @return integer status
3. jit.lua — jit.flush overload inconsistency
--- @overload fun(tr: integer)
--- @param func? function|boolean
--- @param recursive? boolean
function jit.flush(func, recursive) end- Issue: The
@overloadsignaturefun(tr: integer)conflicts with the primary signaturefun(func?: function|boolean, recursive?: boolean). A bare integer argument will match neither cleanly (integer is notfunction|boolean). - Suggestion: Make the primary param type include
integer:or drop the overload and use a union type.--- @param func? function|boolean|integer
4. jit.lua — jit.opt.start param type too restrictive
--- @param ... string|number- Issue: LuaJIT's
jit.opt.startaccepts strings like"hotloop=10"and also numbers (e.g.jit.opt.start(3)).string|numberis fine, but note thatbooleanis also accepted in some versions. Minor — acceptable as-is, but worth confirming against the target LuaJIT version.
5. jit/profile.lua — dumpstack return type mismatch
--- @overload fun(th: thread, fmt: string, depth: integer): string
--- @param fmt string
--- @param depth integer
--- @return string dump
function profile.dumpstack(fmt, depth) end- Issue: The overload declares a
threadfirst param, but the primary signature omits it. The@return string dumpapplies to the primary signature, which is fine, but the overload's return is redundant/inconsistent. Also,profile.dumpstackcan returnnilon error. - Suggestion: Align both signatures and use
string?:--- @overload fun(th: thread, fmt: string, depth: integer): string? --- @param fmt string --- @param depth integer --- @return string? dump
6. jit/util.lua — funcinfo.lua.proto type changed incorrectly
--- @class jit.funcinfo.lua
--- @field proto Proto
...
- ---@type Proto[]
- proto = {},- Issue: In LuaJIT,
funcinfo.lua.protois a list of child prototypes (Proto[]), not a singleProto. The change fromProto[]toProtois a regression. - Suggestion: Revert to
--- @field proto Proto[]and keep the---@type Proto[]annotation on the field.
7. jit/util.lua — Trace class removed but still referenced
- Issue: The
--- @class Tracedeclaration was removed, andtrparams were changed tointeger. This is correct (traces are identified by integer IDs), but ensure no other file referencesTraceas a type. A grep forTraceacross the stdlib is recommended. - Suggestion: Confirm no dangling references remain.
8. jit/util.lua — tracesnap overload return arity
--- @overload fun(tr: integer, sn: integer, getpos: boolean): jit.snap? snap, integer? pos
--- @param tr integer
--- @param sn integer
--- @return jit.snap? snap
function util.tracesnap(tr, sn) end- Issue: The overload returns two values, but the primary signature only declares one. When
getposis true, the function returns(snap, pos). The primary signature should also allow the second return, or the overload should be the canonical form. - Suggestion: Add
--- @return integer? posto the primary signature as well, or restructure to a single signature with optionalgetpos.
9. jit/util.lua — funcinfo param union function|Proto
- Issue: This is correct for LuaJIT (accepts either a function or a proto object), but
Protois declared as an empty class. Consider adding a note or minimal fields so tooling can distinguish it fromany.
Minor / Style
jit.lua: The removed--- @overload fun(...): ...lines had malformed syntax (param funcinside the overload). Good cleanup.jit.lua:version_numchanged fromnumbertointeger— correct, since it's a bit-packed integer.jit.lua:archunion expanded withmips32r6/mips64r6variants — good, matches LuaJIT 2.1.jit/profile.lua:vmst→vmstaterename in the callback signature is a good clarity fix, but verify it matches the actual LuaJIT API docs (the parameter is positional, so the name is cosmetic).
Priority Fixes
jit.util.lua:protoshould beProto[](regression).jit.lua:jit.flushoverload conflicts with primary signature.jit.lua:jit.securitymissing'none'mode / table return overload.jit.lua:jit.attachmissing return type.jit/profile.lua:dumpstackshould returnstring?.
Fix the malformed @overload comments of jit.on/off/flush, make their function argument optional, type the jit.opt.start flags as string|number, type jit.version_num as integer, extend the jit.arch union and add the LuaJIT jit.security and jit.attach functions. Part of tarantool/tarantool#13024
Make the profile.start mode argument optional, name the callback vmstate argument after the LuaJIT implementation and document the string returned by profile.dumpstack. Part of tarantool/tarantool#13024
Trace functions take a trace number, not the unused Trace type, so type their tr arguments as integer. Accept function|Proto for the reflection functions, type the funcinfo proto field as a single Proto and add the getpos argument and position result of tracesnap. Part of tarantool/tarantool#13024
fd8419f to
d92a5b0
Compare
|
Pinned commit: 1.
|
| # | Topic | Verdict | Action |
|---|---|---|---|
| 1 | jit.attach return |
review wrong | no change |
| 2 | jit.security modes |
review wrong | no change |
| 3 | jit.flush overload |
review correct | applied: union, overload removed |
| 4 | jit.opt.start type |
review wrong | no change |
| 5 | dumpstack return |
review wrong | no change |
| 6 | funcinfo.proto |
review wrong | no change |
| 7 | Trace refs |
review correct | confirmed clean |
| 8 | tracesnap arity |
review correct | applied: single signature |
| 9 | Proto class |
minor | leave empty |
No commits made; edits remain in the working tree.
Patch with fixups
--- a/crates/emmylua_code_analysis/resources/std/jit.lua
+++ b/crates/emmylua_code_analysis/resources/std/jit.lua
@@ -18,8 +18,7 @@ function jit.on(func, recursive) end
--- @param recursive? boolean
function jit.off(func, recursive) end
---- @overload fun(tr: integer)
---- @param func? function|boolean
+--- @param func? function|boolean|integer
--- @param recursive? boolean
function jit.flush(func, recursive) end
diff --git a/crates/emmylua_code_analysis/resources/std/jit/util.lua b/crates/emmylua_code_analysis/resources/std/jit/util.lua
index 258407e8..e7b19a68 100644
--- a/crates/emmylua_code_analysis/resources/std/jit/util.lua
+++ b/crates/emmylua_code_analysis/resources/std/jit/util.lua
@@ -83,10 +83,11 @@ function util.tracek(tr, idx) end
--- @class jit.snap: integer[]
---- @overload fun(tr: integer, sn: integer, getpos: boolean): jit.snap? snap, integer? pos
---- @param tr integer
---- @param sn integer
+--- @param tr integer
+--- @param sn integer
+--- @param getpos? boolean
--- @return jit.snap? snap|
https://github.com/EmmyLuaLs/emmylua-analyzer-rust/actions/runs/35258029478/job/105326440119?pr=1252 |
traits already re-exports node::*, so the glob in syntax/mod.rs is fully shadowed and newer rustc reports it as an unused import under -D warnings.
|
@CppCXY could you take a look? |
|
The type name "Proto" is too generic; it should be changed to |
Part of tarantool/tarantool#13024