Add a typecheck target and clear its findings - #12
Conversation
There was a problem hiding this comment.
Reviewed at 8ed79af, having also read 4d17d82 and 7cf59d0 as they landed. Cloned and ran things rather than reading the diff alone.
Approving. The typecheck target is sound, it is a genuine gate, and the two annotation fixes are behaviour-neutral. One caveat that is not yours, at the bottom.
What I verified
make typecheckat this head: clean, 7/7 files, under both 3.19.0 and 3.18.2 (downloaded 3.18.2 to check rather than take the CLAUDE.md claim on trust).- Non-vacuous. The same command on base (
ea30c87) reports exactly 2 problems,_compat.lua:139 cast-local-typeandbit64.lua:969 param-type-mismatch, which are precisely the two this PR fixes. - A real gate, not decorative. lua-language-server exits
1on findings, somake typecheckfails rather than printing and passing. Now thatcheckdepends on it, CI enforces it. - The CI install step is correct: the 3.19.0 linux-x64 tarball extracts
bin/at its root, sotar -xz -C "$RUNNER_TEMP"plus$RUNNER_TEMP/binonGITHUB_PATHresolves. Verified against the actual archive. _compat.luais behaviour-neutral:okandresultfrom the native probe are read only at lines 37-38, and nothing after line 141 touches them, so dropping the reassignment changes nothing.- Tests pass 3/3 locally, including under LuaJIT.
- Tree stays clean:
--logpathwrites intobuild/, already ignored at.gitignore:43. test.expectedstill appears atbit64.lua:1280and:1285, but that loop has notype()branch so narrowing never applies there. Not an unfixed instance of the same thing.
On 8ed79af: I had this written up as a blocking finding before you pushed it. I reproduced the 7cf59d0 failure locally by planting a single file under .luarocks/ (7 files became 8, 146-problem shape, make check red), and .luarocks is not in .gitignore, so useGitIgnore: true would not have saved it either. Your fix is the right one and more complete than mine: I only had .luarocks, and the leafo actions also drop .lua and .install into the workspace. I re-verified by planting a bad file in all three at this head: still 7/7 and clean.
One claim I could not verify: luacheck 1.2.0 is broken against Lua 5.5 on this machine (attempt to assign to const variable 'field_name'), and it fails identically on base, so that is my environment rather than your change. CI's Check job passes, so treat the luacheck half as confirmed by CI, not by me.
Not yours, but it will keep the branch red: LuaJIT 2.1
Exactly one job genuinely fails, Lua luajit-2.1, and it does so consistently across both runs. Everything else that renders as red is fail-fast: true cancelling siblings: by job conclusion, run 31272663088 had luajit-2.0 cancelled and 5.1-5.4 green, and run 31273038146 had 5.1 and 5.4 cancelled and luajit-2.0 green. gh pr checks prints cancelled as fail, which had me believing both LuaJIT jobs were broken until I read the conclusions from the API. Worth knowing when reading this matrix.
The real failure is roughly 24 tests across the 32-bit and 64-bit modules:
FAIL: mask(0xFFFFFFFF)
Expected: 0xFFFFFFFF
Got: 0xFFFFFFFFFFFFFFFF
The CI log names the cause: Using: native operators (Lua 5.3+). On the runner's LuaJIT (2.1.1785763465) the probe at _compat.lua:36 now compiles a & b successfully, so _compat picks the native 64-bit backend instead of the bit library and the 32-bit masking assumptions break.
Why it is provably not this PR:
- The probe at lines 36-38 is byte-identical between base and head.
- The native block returns at line 127, so line 141, the only
_compatcode you touched, never executes on that path. src/bitn/bit32.luais the same blob on both revisions (ffa8469) and it is one of the failing modules.
My local LuaJIT is slightly older (2.1.1783773675), still fails the probe, selects bit library, and both base and head pass 3/3. luajit-2.0 on the runner passes for the same reason. So this is toolchain drift on the 2.1 rolling build, not a regression. I could not re-run base's January CI to demonstrate it directly (Resource not accessible by integration), so that last step is inference from the identical blobs rather than a measurement.
This is a real library bug independent of the typecheck work: bitn is wrong on current LuaJIT builds, which is the runtime the drivers actually care about. Worth its own ticket, and I would rather fix it separately than tangle it with this. Say the word and I will file it.
One small note
The description on the first commit said "under 5.4 it reports nothing ... so an absent version looks like a clean run." That is not what it does. With runtime.version unset I measure 5 problems and exit 1 (four deprecated plus the param-type-mismatch); 5.4 gives 5, 5.1 gives 9, LuaJIT gives 2. Your in-tree wording ("a different set of findings rather than fewer") is the accurate one, and this repo squashes with COMMIT_MESSAGES, so the PR body will not become the commit message. Noting it only so the reasoning does not get quoted back later as "unset looks clean."
`install-deps` already installed lua-language-server but nothing ran it, so the LuaCATS annotations were only checked by whoever had it wired into an editor. Adds a `typecheck` target pinned to a committed config, and fixes the two things it found. `runtime.version` in the config is load-bearing rather than boilerplate. Unset, the server defaults to Lua 5.4 and checks this library as the wrong language: under 5.4 it reports nothing, under LuaJIT two problems, so an absent version looks like a clean run. Pinned to LuaJIT, and `--configpath` keeps the result off whatever `.luarc.json` a developer happens to have. The two findings: - `_compat.lua` reused the `ok, result` locals from the native-operator probe for `pcall(require, "bit")` a hundred lines later. One holds a compiled chunk, the other a library table, so the reuse was misleading as well as untypeable. Separate locals. - `bit64.lua`'s selftest branched on `type(test.expected)` but kept indexing through the field, which the server does not narrow. Held in a local, which also drops the repetition. Both are annotation-level; no behaviour changes. 3/3 test modules pass, luacheck is clean across 7 files, and typecheck now reports nothing.
The target from the previous commit was runnable but nothing ran it, which is the state it was added to fix. Adds it to `check`, which CI already invokes, and installs lua-language-server in the Check job since CI had luacheck and stylua but not the server. The version is pinned to 3.19.0 rather than floating. Findings genuinely move between versions, so an unpinned server would make CI red on an upstream release rather than on a change to this repo. Verified clean under 3.18.2-dev and 3.19.0.
The whole-repo scope means CI type-checks its own toolchain: leafo/gh-actions-lua installs into .lua and gh-actions-luarocks into .luarocks, both inside the workspace, so the first gated run reported 146 problems across 43 files, almost all of them in luacheck and luafilesystem sources rather than this repo.
8ed79af to
b0e0d26
Compare
install-depsalready installed lua-language-server but nothing ran it, so theLuaCATS annotations were only checked by whoever had it wired into an editor.
Adds a
typechecktarget pinned to a committed config, and fixes the two thingsit found.
runtime.versionin the config is load-bearing rather than boilerplate. Unset,the server defaults to Lua 5.4 and checks this library as the wrong language:
under 5.4 it reports nothing, under LuaJIT two problems, so an absent version
looks like a clean run. Pinned to LuaJIT, and
--configpathkeeps the result offwhatever
.luarc.jsona developer happens to have.The two findings:
_compat.luareused theok, resultlocals from the native-operator probe forpcall(require, "bit")a hundred lines later. One holds a compiled chunk, theother a library table, so the reuse was misleading as well as untypeable.
Separate locals.
bit64.lua's selftest branched ontype(test.expected)but kept indexingthrough the field, which the server does not narrow. Held in a local, which
also drops the repetition.
Both are annotation-level; no behaviour changes. 3/3 test modules pass, luacheck
is clean across 7 files, and typecheck now reports nothing.