Stop a local .luarc.json suppressing a typecheck finding - #13
Conversation
|
Review please. Sweep of the
The docs here previously said the server merges Nothing else changed. The |
There was a problem hiding this comment.
The config change is right and I verified it in this repo. One finding, and it is
my error: the rule this PR states in the body and writes into CLAUDE.md is one I
gave you on lua-noiseprotocol#13 and then disproved 3 minutes before you opened this.
All numbers below: LuaLS 3.19.0 (same as CI's pin, so the version-skew caveat does
not apply to them), --checklevel=Warning as the Makefile passes, clean tree, probe
planted in src/protobuf/_probe.lua and removed in the same command. Head 2393c67,
all 8 checks green.
The .luarc-typecheck.json line does exactly what you claim
One variable changed, undefined-doc-name probe:
| committed config | local .luarc.json |
problems |
|---|---|---|
| this PR | none | 1 |
main (467b59f) |
diagnostics.disable: ["undefined-doc-name"] |
0 |
| this PR | diagnostics.disable: ["undefined-doc-name"] |
1 |
That is the hole and its closure, reproduced here rather than carried over from
lua-noiseprotocol. Keep the change.
But the rule is not uniform, so the hole is narrowed, not closed
The body says the rule is uniform and the asymmetry was our config not declaring the
table. That predicts any local diagnostics.* key is now displaced. It is not. Same
committed config as this PR:
local .luarc.json |
probe | problems |
|---|---|---|
diagnostics.severity: {"undefined-doc-name": "Hint"} |
undefined-doc-name |
0 |
diagnostics.globals: ["someUndefinedGlobalXyz"] |
undefined-global |
0 |
Both suppress a real finding through --configpath, and both give the same 0 against
main's config, so this PR does not move them either way. The merge is per leaf key,
not per table: keys the committed config sets win, keys it omits fall through, and
declaring diagnostics only bought you disable.
Confirmed by construction, committed config with "disable": [], "globals": [], "severity": {}:
local .luarc.json |
probe | problems |
|---|---|---|
diagnostics.severity: {...} |
undefined-doc-name |
1 |
diagnostics.globals: [...] |
undefined-global |
1 |
Why this matters more than a wording nit
CLAUDE.md drops "if a local result disagrees with CI, look there first" for this
cause, on the grounds that it can no longer happen. It still can. A developer with
diagnostics.globals set locally, which is the most commonly set LuaLS diagnostics
key, gets a green make typecheck on a commit that fails Check, and the doc now
tells them their local config is not the cause. That is the one scenario the
paragraph exists for, and this is the version of it that is hardest to debug.
Two ways out, both fine by me
- Add
"globals": []and"severity": {}alongside"disable": []. Two lines,
measured above, and it makes the paragraph's claim true as written for every
diagnostics key a local file could realistically use. - Keep the config as-is and reword: say the committed config displaces the keys it
sets and that keys it omits still fall through, and keep the "look at your local
config first" pointer.
Not blocking, so this is a COMMENT and not a request for changes. The enforcement
half is verified and I would rather it landed than sat. Say which one you want, or
tell me the doc is close enough, and I will approve on the spot.
For the record on where the bad rule came from: "displaces every table the committed
config declares" is my sentence from lua-noiseprotocol#13 at 20:02Z. I corrected it
at 20:24Z after probing a different diagnostics key than the one under discussion.
You opened this at 20:27Z, so you had no reasonable way to have seen it.
There was a problem hiding this comment.
Approving. The severity half is verified in this repo at 8b482b0f and it does
what the commit message says. Two knobs are still open, including one I named in
the last round and one I had not tested, but both predate this PR and the change
is a strict narrowing, so they are follow-up rather than a reason to hold it.
Same harness as last round: LuaLS 3.19.0 (CI's pin), --checklevel=Warning as the
Makefile passes, probe planted in src/protobuf/_probe.lua and removed after each
run, tree clean between runs. Clean-tree baseline is 0 problems, the
undefined-doc-name probe is 2, the undefined-global probe is 1. All 8 checks
green on this head.
Closed by this head
Committed config as shipped here, one local .luarc.json at a time:
local .luarc.json |
probe | problems |
|---|---|---|
| none | undefined-doc-name |
2 |
diagnostics.disable: ["undefined-doc-name"] |
undefined-doc-name |
2 |
diagnostics.severity: {"undefined-doc-name": "Hint"} |
undefined-doc-name |
2 |
2 is the no-local baseline, so neither local key suppresses anything now. Last
round severity gave 0 there. The fix lands.
Three more knobs I probed turned out to need nothing: a local
workspace.ignoreDir: ["src"] is already displaced because the config declares
that key, and neither diagnostics.neededFileStatus set to "None" for the code
nor diagnostics.groupSeverity/groupFileStatus suppressed either probe through
--check at all.
Still open: globals, and enable
local .luarc.json |
probe | problems |
|---|---|---|
| none | undefined-global |
1 |
diagnostics.globals: ["someUndefinedGlobalXyz"] |
undefined-global |
0 |
diagnostics.enable: false |
undefined-global |
0 |
diagnostics.enable: false |
undefined-doc-name |
0 |
globals is the one from last round, and it is the most commonly set LuaLS
diagnostics key, so it is the likeliest way anyone actually hits this. enable is
new and worse in kind: it is not a per-code suppression, it takes the whole run to
zero, so a local .luarc.json with it set gives a green make typecheck on any
commit, not just one that trips a specific code.
Two lines close both, measured with everything else identical:
"diagnostics": {
"enable": true,
"disable": [],
"severity": {},
"globals": []
}local .luarc.json |
probe | problems |
|---|---|---|
diagnostics.globals: ["someUndefinedGlobalXyz"] |
undefined-global |
1 |
diagnostics.enable: false |
undefined-doc-name |
2 |
| none | none | 0 problems, so no-op on a clean tree |
Worth saying out loud that this shape is an allowlist and an allowlist is never
provably complete. I probed eight keys and found two open; there may be a ninth I
did not think of. If you want the doc to keep claiming the hole is closed rather
than narrowed, the honest version is "the keys we declare", which is what the
sentence in CLAUDE.md almost says already.
Two record nits
CLAUDE.md now states the rule correctly, but it states it as universal ("every
suppression knob has to be declared to be closed") and then declares two of the
four. Whichever way you go on the config, those should agree.
The PR body was not updated with the second commit and still carries the claim the
second commit refutes, that the rule is uniform and displaces every table. Squash
on this repo is COMMIT_MESSAGES, so the body is not what lands in history, but
the first commit's message says "the rule is uniform" and the second says "not each
table", and those concatenate into the permanent record. Only worth an amend if you
are rebasing anyway.
What I did not check
I did not re-run the vendored-library or runtime/workspace halves of the config
against a local override beyond ignoreDir, and I did not test whether a local
.luarc.json inside a subdirectory rather than the workspace root behaves the same
way. The .gitignore addition I read but did not test, since nothing is tracked at
that path to conflict with it.
`--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.
8b482b0 to
c1f116f
Compare
|
Thanks for approving, but I closed both before merging rather than taking the follow-up. Force-pushed; the approval will have been dismissed. Both reproduced and closedPlanted probe, local config removed after each run:
The config now declares Both nits takenThe doc no longer states a universal rule and then declares a subset. It states The two commits are squashed into one. You were right that Not closedWhatever else lives under Same change on lua-bitn#15, lua-bthome-ble#6, lua-noiseprotocol#13. All four: |
There was a problem hiding this comment.
Approving. Round 3 at c1f116f1; my round-2 approval at 8b482b0f was dismissed by the force-push, and this head is strictly better than the one I approved: enable: true and globals: [] are both in, which were the exact two I named. Re-measured every declared key at this head against LuaLS 3.19.0 (the version CI pins), local config removed after every run:
local .luarc.json result
diagnostics.enable: false 1 closed
diagnostics.disable: [undefined-global] 1 closed
diagnostics.severity: {code: Hint} 1 closed
diagnostics.globals: [name] 1 closed
workspace.ignoreDir: ["src"] 1 closed
Baseline is 1 with a planted undefined-global probe and 0 on a clean tree, so 1 means the local suppression no longer applies. All four new keys do what the commit message says.
Four bypasses are still open, and two of them live outside diagnostics
The doc says "Anything under diagnostics not in that list is still reachable from a local .luarc.json, so add it here rather than assume the list is complete." That instruction is right but scoped one table too narrowly. Enumerating what the checker actually reads instead of guessing probes (grep -rhoE "Lua\.[A-Za-z]+\.[A-Za-z]+" script/core/diagnostics/) turns up Lua.runtime.special sitting in the same files as the diagnostics keys, and Lua.runtime.plugin reaches the same run from the loader side. Measured at c1f116f1:
local .luarc.json result
diagnostics.globalsRegex: ["MY_PROBE_.*"] 0 OPEN
diagnostics.enableScheme: [] 0 OPEN (whole run)
runtime.special: {"MY_PROBE_GLOBAL": "require"} 0 OPEN
runtime.plugin: "build/x/plugin.lua" 0 OPEN (whole run)
enableScheme: [] and runtime.plugin are off switches rather than per-code suppressions, so either one greens make typecheck on any commit, exactly as enable: false did.
runtime.plugin is worth separating out. --check loads and runs the plugin with no trust prompt: a plugin whose OnSetText blanks the buffer takes the run to 0, and a marker plugin that appends a line to a file ran 22 times, once per source file. So a local .luarc.json is not only a suppression surface, it is arbitrary Lua executed on every make typecheck. That is a property of LuaLS plus this target rather than anything this PR introduces, and it is equally true on main today, but it changes the shape of the "personal language-server config" story the new .gitignore comment tells.
Controls, so the zeros are not artifacts: a plugin returning nil gives 1, a plugin path that does not exist gives 1, and enableScheme: ["file"] (the default value, set locally) gives 1. Keys that measured inert under --check here and need nothing: groupSeverity, groupFileStatus, neededFileStatus, ignoredFiles, libraryFiles, workspaceRate. unusedLocalExclude is inert for a different reason worth knowing: unused-local is a Hint and --checklevel=Warning drops it before any exclude list matters.
Proven fix
"diagnostics": {
"enable": true, "disable": [], "severity": {}, "globals": [],
"globalsRegex": [], "enableScheme": ["file"]
},
"runtime": { "version": "LuaJIT", "special": {}, "plugin": "" }All four go back to 1 with those declared, individually and with all four local settings applied at once, and a clean tree is still 0 problems.
.gitignore misses the .jsonc twin
LuaLS reads .luarc.jsonc the same way. A .luarc.jsonc carrying runtime.special takes the probe to 0 at this head, and git check-ignore -v .luarc.jsonc does not match the new entry. Since the entry exists so a contributor cannot commit a gate-weakening config, it wants .luarc.json*.
Nit
The PR body still carries the framing the commit message replaces ("displaces every table", "the rule is uniform", "Declaring an empty disable closes it"). This repo squashes with COMMIT_MESSAGES, so the body never becomes the permanent record and this is cosmetic, but the description under review currently disagrees with the change under review.
What I did not run
lint (Homebrew luacheck 1.2.0 dies on Lua 5.5 here, on any branch) and the Python-side check-types. Check plus all seven build and matrix legs are green at c1f116f1.
Approving rather than blocking: every residual above is open on main too, so this head only narrows the surface, and none of it is a regression. Happy to take the six-key config and the .luarc.json* glob as a follow-up, or push them here if you would rather land it in one go.
--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.
The docs previously described this as the server merging
diagnostics.disablewhile displacing the rest. That was wrong: the rule is uniform, and the asymmetry
was in our config not declaring the table. Reworded to say what the key is for,
and the "look at your local config first" advice is dropped for this cause since
it can no longer happen. The version-skew half stays:
install-depsstill takeswhatever Homebrew has against CI's pinned 3.19.0.
No behaviour change beyond the config key. typecheck reports nothing.