Stop a local .luarc.json suppressing a typecheck finding - #15
Conversation
|
Review please. Sweep of the The one-line part: The docs part is bigger here, because #12 merged before I understood the claims were per-repo and this repo still carried the originals. Measured rather than assumed:
That also resolves why the same key behaved differently in each repo across the last three rounds, which is now written down rather than left as scattered observations: a library carrying 5.1-era compat shims fails the deprecation check once the server assumes a newer language. lua-bitn and lua-protobuf have them, lua-noiseprotocol and lua-bthome-ble do not and measure identically pinned, unset, or on 5.1. The version-divergence justification is dropped; 3.18.2 and 3.19.0 agree here, and pinning CI stands on its own. CI green, 3/3 modules, typecheck clean. |
There was a problem hiding this comment.
Measured in this repo rather than carried over from lua-noiseprotocol#13, since the two are not the same case: lua-bitn carries the 5.1 compat shims and lua-noiseprotocol does not.
The config change does what the title says. The paragraph explaining why it works does not survive a second probe, and the same measurement says the config is two leaf keys short of the guarantee the paragraph makes.
The fix works
LuaLS 3.19.0, probe return TotallyUndefinedGlobalXyz planted in src/bitn/_probe.lua, --checklevel=Warning, one variable moved at a time:
| committed config | local .luarc.json |
findings |
|---|---|---|
main (ee6406c) |
none | 1 |
| main | diagnostics.disable: ["undefined-global"] |
0 |
c5acc18 |
none | 1 |
c5acc18 |
diagnostics.disable: ["undefined-global"] |
1 |
Row 2 is the hole, row 4 is it closed. That is exactly the claim in the description and it holds.
But --configpath merges per leaf key, not per table
Same committed config from this branch, same probe, three different keys inside the one diagnostics table:
local .luarc.json key |
findings under c5acc18 |
|---|---|
diagnostics.disable |
1 (displaced) |
diagnostics.globals: ["TotallyUndefinedGlobalXyz"] |
0 (fell through) |
diagnostics.severity: {"undefined-global": "Hint"} |
0 (fell through) |
Same table, same file, opposite behaviour, so declaring diagnostics.disable does not displace diagnostics. Keys the committed config declares win, keys it omits fall through. workspace.ignoreDir is the control: it is declared, and a local ignoreDir pointing at the probe file is ignored.
Two consequences.
The doc sentence is wrong as written. Worse than wrong in the abstract, it is wrong in the direction that costs a reader time: someone chasing a green local run against a red Check reads "displaces every table the committed config declares" and crosses their .luarc.json off the list, when it is still the answer two thirds of the time.
And "without that key a local .luarc.json could still suppress a finding" is true, but the converse the sentence implies is not. Under this branch's config, a local diagnostics.severity downgrading a rule to Hint is invisible at --checklevel=Warning: green make typecheck, red Check, which is the exact failure the PR exists to stop. diagnostics.globals gets there too.
What closes it, measured
"diagnostics": {
"disable": [],
"globals": [],
"severity": {}
}Re-ran the five local variants against that: disable, globals, severity, ignoredFiles, and no local file at all, all 1 finding. Every suppression path I could find is displaced. I am proposing this having measured it rather than reasoned it, because the sentence being corrected here is one I handed you in lua-noiseprotocol#13 round 3 without measuring, and you committed it on my say-so. It is on that branch too, at 2951842, and neither branch has merged, so both are still cheap to fix.
Suggested replacement for the paragraph:
--configpathmerges per leaf key, not per table: keys the committed config declares win, keys it omits fall through to a local.luarc.json. That is why it declares emptydisable,globalsandseverity. Leave any one of the three undeclared and a local file can still give a green run on a commit that failsCheck.
The rest of the docs change checks out
runtime.version is right, and right for the reason now given. Unset, 4 findings, all deprecated: _compat.lua:13 (math.pow or function(x, y)), bit16.lua:195, bit32.lua:269, bit64.lua:570 (unpack or table.unpack). Pinned to LuaJIT, 0. So the old "reports a different set of findings rather than fewer" framing was the thing that needed correcting, and the new text names the mechanism and the count correctly.
"3.18.2 and 3.19.0 agree here" holds: pulled 3.18.2 alongside and ran both. Clean at head under both, 4 findings under both with the key unset. Dropping "the findings do move between versions" is fair, and the replacement is accurate: install-deps shells out to brew install lua-language-server with no pin, build.yml sets LUALS: 3.19.0, and the typecheck target already echoes the version it is about to run.
make typecheck clean, make test 3/3 modules, all 8 checks green on c5acc18.
One aside on runtime.version
The description cites the sibling repos measuring the same "pinned, unset, or set to 5.1". That third cell is not measuring what it looks like. runtime.version only accepts the canonical strings, so "5.1" is silently ignored and the server falls back to Lua 5.4. Here: "5.1" gives the same 4 deprecation findings as unset, while "Lua 5.1" gives 8. The conclusion about those repos is probably still fine, they have no shims to trip on, but that cell is a duplicate of the unset cell rather than independent evidence.
Artifact
Comment, not approve. Nothing here is broken and CI is green, but the paragraph would land on main stating a rule I have now disproved in two repos, and the config is two keys short of what it promises. Push those and I will approve, or tell me the partial fix is deliberate and I will approve as is.
Not checked: the lua-protobuf math.frexp claim. The repo exists, I did not measure it.
There was a problem hiding this comment.
Re-reviewed c5acc18 -> b6482473 (3 files, +10/-4). 8/8 checks green on the new head.
Both findings from the last round landed: diagnostics.severity is declared, and the
--configpath paragraph now says "each individual setting", not "each table". I re-measured
the severity claim against a config with that key removed and it reproduces cleanly, so the
new key is doing real work: no-severity config plus a local severity override gives 0
findings, and declaring it gives 1.
The doc now states a rule the config is three knobs short of
CLAUDE.md:74 says "every suppression knob has to be declared to be closed". That is the
right rule. .luarc-typecheck.json declares two of them. I planted an undefined-global in
src/bitn/init.lua and ran every local .luarc.json variant against the committed config at
this exact head, LuaLS 3.19.0, --checklevel=Warning:
local .luarc.json |
findings | status |
|---|---|---|
| none (baseline) | 1 | |
diagnostics.disable |
1 | closed |
diagnostics.severity |
1 | closed by this push |
diagnostics.globals |
0 | open |
diagnostics.globalsRegex |
0 | open |
runtime.special |
0 | open |
globalsRegex and runtime.special are new since my last round, and runtime.special is the
one I would not have guessed: mapping any name onto a builtin makes the checker treat it as
defined, so {"runtime": {"special": {"MY_GLOBAL": "require"}}} in a personal config buys the
same green run that disable used to.
I chased the enumeration into the server rather than stopping at three probes, because "closes
the hole" has now been short twice. In --check mode the severity layer reads exactly two
config keys, Lua.diagnostics.disable and Lua.diagnostics.severity
(script/cli/check_worker.lua:148-160); groupSeverity never reaches it and
neededFileStatus is overwritten wholesale by downgrade_checks_to_opened just below. So
anything else has to be read by a checker directly, and script/core/diagnostics/*.lua
references only globals, globalsRegex, severity, neededFileStatus, groupSeverity,
groupFileStatus, disable, unusedLocalExclude, workspaceRate, runtime.version and
runtime.special. I measured all of them. The four not in the table above
(groupSeverity, groupFileStatus, neededFileStatus, unusedLocalExclude) are inert here,
and workspace.library, workspace.ignoreDir, workspace.useGitIgnore, runtime.version and
diagnostics.disableScheme are inert or already displaced.
Measured fix, all five local variants back to 1 finding, including all five applied at once:
"runtime": {
"version": "LuaJIT",
"special": {}
},
"diagnostics": {
"disable": [],
"globals": [],
"globalsRegex": [],
"severity": {}
}Clean tree with that config still reports nothing, same as the config at this head, so it is
not trading the hole for noise.
Two smaller notes
The .gitignore entry is hygiene, not part of the fix, and I would not want it read as closing
anything: the hole is an untracked personal file, which .gitignore does not prevent from
existing. It is also inert for typecheck scope, since workspace.useGitIgnore is declared
false and therefore displaces a local override. I confirmed both by gitignoring src/ and
re-running with and without a local useGitIgnore: true: 1 finding either way. No tracked
.luarc.json to break. Worth keeping, just for a different reason than the rest of the diff.
The PR body still carries the "displaces every table the committed config declares" sentence
that CLAUDE.md just corrected. Cosmetic only: this repo is COMMIT_MESSAGES /
COMMIT_OR_PR_TITLE for squash, so the body does not become the commit message, and neither
of the two commit messages on this branch repeats the claim.
Where this leaves it
Not blocking, and I am not requesting changes: this head is strictly better than main and
better than c5acc18. But the config and its own documentation disagree right now, so either
fold in the three keys above, or soften CLAUDE.md:74 so it does not claim completeness. Say
which and I will approve on the next push; if you would rather ship as-is I will approve this
head instead, just tell me.
Did not re-test: the runtime.version deprecation findings and the 3.18.2 versus 3.19.0
comparison, both unchanged by this diff and both verified last round. All measurements above
are LuaLS 3.19.0 on macOS against the --check CLI path only, not the editor language server,
where groupSeverity and neededFileStatus do apply and my "inert" results would not hold.
`--configpath` displaces each individual setting the committed config declares,
not each table, so a suppression knob under `diagnostics` is only closed if the
committed config names it. `.luarc-typecheck.json` named none, and `make
typecheck` reads a local `.luarc.json` for everything it does not name, so a
developer could get a green run on a commit that fails `Check`.
Four live bypasses, each measured with a planted probe and the local config
removed after every run:
local .luarc.json probe before after
diagnostics.enable: false any 0 2
diagnostics.disable: [code] code 1 2
diagnostics.severity: {code: Hint} code 1 2
diagnostics.globals: [name] global 1 2
`enable: false` silences the check entirely; the rest suppress individual codes.
The config now declares all four and each is a no-op on a clean tree. The doc
states the rule rather than the list, and says to add any further key found
reachable rather than treat four as complete.
Also adds `.luarc.json` to `.gitignore`. It is covered by a global ignore on this
machine but not by the repo, so a contributor without that entry could commit one,
and a committed `diagnostics` setting would weaken the gate for everyone rather
than just its author.
b648247 to
2f74871
Compare
|
Re-review please. This head is past the one I first asked about: it now closes four
The docs correction specific to this repo is unchanged from my earlier comment: CI green, 3/3 modules, typecheck clean. |
There was a problem hiding this comment.
Re-reviewed b6482473 -> 2f748710. The force-push squashed the branch's three commits
into one on the same base ee6406c, so the real delta is small: .luarc-typecheck.json
gains enable and globals, and the --configpath paragraph in CLAUDE.md is rewritten.
8/8 green on the new head, make typecheck clean, make test 3/3.
Approving. In my last round I said I would approve on the next push if you either folded in
the remaining keys or softened the doc so it stopped claiming completeness, and you did some
of both. Everything below is follow-up, not a condition.
Your claims check out, including the one I missed
CLAUDE.md says all four declared keys were measured as live bypasses. They were. I
control-tested each one by stripping it from the committed config and re-applying the same
local override, which separates "declaring it closed the hole" from "my override was a typo
that never did anything":
| key | local override vs head config | vs config with that key removed |
|---|---|---|
diagnostics.disable |
1 (closed) | 0 |
diagnostics.severity |
1 (closed) | 0 |
diagnostics.globals |
1 (closed) | 0 |
diagnostics.enable |
1 (closed) | 0 |
enable: false is a genuine bypass and it is one I did not name last round, so that key is
yours, not mine. Clean tree with this config still reports nothing, so none of the four adds
noise.
My last enumeration was incomplete, and the corrected one finds five more
I claimed last round that I had chased this into the server and measured the complete
candidate set. That was wrong: I grepped script/core/diagnostics/*.lua, which only finds
keys the individual checkers read, and diagnostics.enable is not read there. You finding a
key my "complete" list missed is what sent me back.
The authoritative list is the settings registry, script/config/template.lua, which declares
every setting the server accepts: 15 under Lua.diagnostics.* and 12 under Lua.runtime.*.
I measured all 27 at this head against a planted undefined-global, LuaLS 3.19.0,
--checklevel=Warning, baseline 1 finding. Five still suppress it to 0:
local .luarc.json |
findings | shape |
|---|---|---|
diagnostics.globalsRegex |
0 | targeted |
runtime.special |
0 | targeted |
diagnostics.enableScheme: [] |
0 | blanket |
runtime.fileEncoding: "utf16le" |
0 | blanket |
runtime.plugin |
0 | blanket |
Worth separating the two shapes. globalsRegex and runtime.special reproduce the exact
failure this PR exists to stop: a targeted local suppression of one real finding, green
make typecheck, red Check. The other three do not suppress a finding so much as make the
run vacuous, which is arguably worse, because a green typecheck over zero analysed files
looks identical to a green typecheck over a clean tree. runtime.plugin also executes
arbitrary Lua from the personal config; the one I tested just blanks every source file before
analysis.
The rest are inert here: groupSeverity, groupFileStatus, neededFileStatus,
ignoredFiles, libraryFiles, unusedLocalExclude, workspaceRate, workspaceDelay,
workspaceEvent, runtime.nonstandardSymbol, unicodeName, path, pathStrict, meta,
enableLuaJITExtensions. Two increase findings rather than hide them, so they are noise
rather than a hole: local runtime.builtin disabling basic gives 183, and
workspace.library pointing at src gives 73.
The doc hedge is scoped one table too narrowly
The new closing sentence is the right instinct, but it says "anything under diagnostics not
in that list". Three of the five open vectors are under runtime, not diagnostics, so as
written it reads as though the runtime table is accounted for when special, fileEncoding
and plugin are all live. Widening it to "anything the server accepts, in any table" costs a
few words and matches what the mechanism actually does, since --configpath displacement is
per leaf setting and has nothing to do with which table the leaf sits in.
Measured config, if you want to close the rest
"runtime": {
"version": "LuaJIT",
"special": {},
"fileEncoding": "utf8",
"plugin": "",
"pluginArgs": []
},
"diagnostics": {
"enable": true,
"disable": [],
"severity": {},
"globals": [],
"globalsRegex": [],
"enableScheme": ["file"]
}enableScheme and fileEncoding are declared at the server's own defaults, ["file"] and
"utf8", rather than empty, since an empty enableScheme is itself the bypass. Against that
config all nine overrides above go back to 1 finding, including all nine applied at once;
against the config at this head all nine applied at once give 0. Clean tree with it still
reports nothing, so it is not trading the hole for noise.
I would still not call that list closed. It is 27 settings measured against one
undefined-global probe at one checklevel, and a suppression that only bites a different
diagnostic would not show up in it. The general point in your last sentence is the durable
part; the specific list is just where the measurement got to today.
--configpathdisplaces every table the committed config declares and leaves therest to fall through to a local
.luarc.json..luarc-typecheck.jsondid notdeclare
diagnostics, so a developer withdiagnostics.disableset locally got agreen
make typecheckon a commit that failsCheck. Declaring an emptydisablecloses it.Measured in lua-noiseprotocol against a planted finding and a local disable for
it: 1 problem without the key, 2 with it, so the local suppression stops applying.
Also corrects the typecheck docs, which carried two claims from #12 that were
written for another repo and never re-tested here.
runtime.versionis load-bearing, as the docs said, but not for the statedreason. Unset, the server assumes Lua 5.4 and flags
math.powin_compat.luaand the
unpackfallbacks in bit16/32/64 as deprecated: 0 findings pinned, 4unset. That is the general shape, here and in lua-protobuf, where the same key
decides whether the
math.frexppolyfills trip the gate: a library carrying5.1-era compat shims fails the deprecation check once the server assumes a newer
language. Libraries without them, lua-noiseprotocol and lua-bthome-ble, measure
identically with the key pinned, unset, or set to 5.1.
The version pin was justified as "the findings do move between versions".
3.18.2 and 3.19.0 agree here. CI pins the server so an upstream release cannot
move the count, which stands without the claim.
No behaviour change beyond the config key. typecheck reports nothing, 3/3 test
modules pass.