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?.
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 |
|
@CppCXY could you take a look? |
|
The type name "Proto" is too generic; it should be changed to |
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
d155dbd to
86c2ce5
Compare
Updated: diff --git a/crates/emmylua_code_analysis/resources/std/jit.lua b/crates/emmylua_code_analysis/resources/std/jit.lua
index 8332a74d..9b320e92 100644
--- a/crates/emmylua_code_analysis/resources/std/jit.lua
+++ b/crates/emmylua_code_analysis/resources/std/jit.lua
@@ -10,15 +10,15 @@
--- @field arch 'x86' | 'x64' | 'arm' | 'arm64' | 'arm64be' | 'ppc' | 'ppc64' | 'ppc64le' | 'mips' | 'mipsel' | 'mips32r6' | 'mips32r6el' | 'mips64' | 'mips64el' | 'mips64r6' | 'mips64r6el' | string
jit = {}
---- @param func? function|boolean
+--- @param func? function | boolean
--- @param recursive? boolean
function jit.on(func, recursive) end
---- @param func? function|boolean
+--- @param func? function | boolean
--- @param recursive? boolean
function jit.off(func, recursive) end
---- @param func? function|boolean|integer
+--- @param func? function | boolean | integer
--- @param recursive? boolean
function jit.flush(func, recursive) end
@@ -29,7 +29,7 @@ function jit.status() end
jit.opt = {}
---- @param ... string|number
+--- @param ... string | number
function jit.opt.start(...) end
--- @param mode? 'prng' | 'strhash' | 'strid' | 'mcode'
diff --git a/crates/emmylua_code_analysis/resources/std/jit/profile.lua b/crates/emmylua_code_analysis/resources/std/jit/profile.lua
index e40a4011..93b5de95 100644
--- a/crates/emmylua_code_analysis/resources/std/jit/profile.lua
+++ b/crates/emmylua_code_analysis/resources/std/jit/profile.lua
@@ -5,7 +5,7 @@
local profile = {}
--- @param mode? string
---- @param func fun(L: thread, samples: integer, vmstate: string)
+--- @param func fun(L: thread, samples: integer, vmstate: string)
function profile.start(mode, func) end
function profile.stop() end
diff --git a/crates/emmylua_code_analysis/resources/std/jit/util.lua b/crates/emmylua_code_analysis/resources/std/jit/util.lua
index e7b19a68..d1a9e49d 100644
--- a/crates/emmylua_code_analysis/resources/std/jit/util.lua
+++ b/crates/emmylua_code_analysis/resources/std/jit/util.lua
@@ -2,12 +2,12 @@
--- @meta jit.util
--- @version JIT
---- @class Proto
+--- @class JIT.Proto
local util = {}
--- @class jit.funcinfo.lua
---- @field proto Proto
+--- @field proto JIT.Proto
local funcinfo = {
linedefined = 0,
lastlinedefined = 0,
@@ -31,23 +31,23 @@ local funcinfo2 = {
upvalues = 0,
}
---- @param func function|Proto
+--- @param func function | JIT.Proto
--- @param pc? integer
---- @return jit.funcinfo.c|jit.funcinfo.lua info
+--- @return jit.funcinfo.c | jit.funcinfo.lua info
function util.funcinfo(func, pc) end
---- @param func function|Proto
+--- @param func function | JIT.Proto
--- @param pc integer
--- @return integer? ins
--- @return integer? m
function util.funcbc(func, pc) end
---- @param func function|Proto
+--- @param func function | JIT.Proto
--- @param idx integer
--- @return any? k
function util.funck(func, idx) end
---- @param func function|Proto
+--- @param func function | JIT.Proto
--- @param idx integer
--- @return string? name
function util.funcuvname(func, idx) end
@@ -83,8 +83,8 @@ function util.tracek(tr, idx) end
--- @class jit.snap: integer[]
---- @param tr integer
---- @param sn integer
+--- @param tr integer
+--- @param sn integer
--- @param getpos? boolean
--- @return jit.snap? snap
--- @return integer? pos |
|
@CppCXY ok? |
|
Great ,but to avoid conflicts, I still don't intend to merge now. |
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 and type the funcinfo proto field as a single Proto. Update the tracesnap annotation for LuaJIT commit 8e6520a7 [1]: the prototype is now tracesnap(tr, sn[, getpos]) and it returns the PC position as a second result when getpos is true. Declare getpos in the signature so the third argument is accepted. Part of tarantool/tarantool#13024 1. LuaJIT/LuaJIT@8e6520a
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.
86c2ce5 to
eb44108
Compare
Part of tarantool/tarantool#13024