Skip to content

Commit 4a13868

Browse files
authored
Merge pull request #720 from Quenty/users/quenty/strict-typing
feat: Add strict typing to a lot of packages
2 parents db2a6ab + 0934c08 commit 4a13868

258 files changed

Lines changed: 8126 additions & 3377 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.claude/skills/strict-typing-luau/SKILL.md

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,23 @@ luau-lsp analyze --sourcemap=sourcemap.json --base-luaurc=.luaurc \
3737
```
3838

3939
Clean = only the `[INFO] Loading...` line. `LuauSolverV2=false` is required (repo pins the old
40-
solver). Iterate until clean, then run `npm run lint:luau` **once** as the final gate — single-file
40+
solver). **Never drop `--defs=globalTypes.d.lua` or `--base-luaurc=.luaurc`** — without `--defs`
41+
the analyzer loses the Roblox global declarations and reports *false* `Unknown global 'tick'/'time'`
42+
(and similar) errors while still resolving `game`/`Enum`, which looks like a real conversion bug but
43+
isn't. If you see unknown-global errors only on deprecated globals, you forgot `--defs`; confirm with
44+
`npm run lint:luau` (which always passes the flag) before "fixing" anything. Iterate until clean,
45+
then run `npm run lint:luau` **once** as the final gate — single-file
4146
analyze can't see files that depend on *yours*, and tightening a type ripples to subclasses and
4247
callers. Triage new downstream errors: pre-existing → leave & flag; your type is genuinely too
4348
tight for the real contract → loosen *your* type (often `T?` not `T`); a small obvious follow-on
4449
→ fix it.
4550

51+
The gate is **both** `lint:luau` (types) **and** `lint:selene` (lints), which exits non-zero.
52+
Dot-syntax conversion routinely trips selene even when analyze is clean: an `unused_variable: self`
53+
(method body ignores the now-explicit param → rename it **`_self`**) or a `shadowing` from an Rx escape
54+
`local X = X :: any` inside a function (→ cast at the source, `local X: any = require("X")`). See the
55+
"selene" section in `references/conventions.md`.
56+
4657
## Core patterns
4758

4859
**Class with a parent (the common case):**
@@ -81,6 +92,47 @@ function MyClass.GetEnabled(self: MyClass): boolean
8192
end
8293
```
8394

95+
### ⚠️ Metamethod classes — NEVER rewrite `rawget`/`rawset` or hoist `setmetatable`
96+
97+
The patterns above assume the ordinary metatable (`__index = MyClass`, default `__newindex`).
98+
Some classes define a **custom `__index` and/or `__newindex` function** that intercepts field
99+
access — often to expose computed keys (`.Value`, `.Changed`) and to **`error()` on any unknown
100+
key**. Grep for `(MyClass :: any).__index = function` / `.__newindex = function` before touching
101+
the constructor or any `self._field` access. For these classes, two edits that look like harmless
102+
cleanups are **runtime-breaking changes** — do not make them:
103+
104+
1. **Do not replace `rawget(self, "_x")` with `self._x`, or `rawset(self, "_x", v)` with
105+
`self._x = v`.** The raw calls are load-bearing: they deliberately bypass the custom metamethod.
106+
Routing the access through `self._x` fires the metamethod, which may compute a different value or
107+
`error("Bad index")` — silently for present fields, fatally for absent ones (e.g. lazy caches
108+
that start unset). Keep every `rawget`/`rawset` exactly as-is; the only allowed change is adding
109+
the receiver cast: `rawget(self, "_x")``rawget(self :: any, "_x")` (and `:: any` on the result
110+
if the checker complains about `any?`).
111+
112+
2. **Do not hoist `setmetatable` above the field assignments in the constructor.** The strict
113+
pattern `local self = setmetatable(Base.new() :: any, MyClass)` followed by `self._x = ...` is
114+
correct **only** when `__newindex` is the default (raw write). If the class has a custom
115+
`__newindex` that errors on unknown keys, that pattern makes **every** field assignment throw
116+
`"Bad index"`. Build a plain table first, then apply the metatable last — the original order:
117+
118+
```lua
119+
function MyClass.new(...): MyClass
120+
-- __newindex errors on unknown keys: assign fields BEFORE the metatable.
121+
local self = {} :: any
122+
self._serviceBag = assert(serviceBag, "No serviceBag")
123+
self._definition = assert(definition, "Bad definition")
124+
return setmetatable(self, MyClass)
125+
end
126+
```
127+
128+
(Equivalently, keep `setmetatable` first but write each field with `rawset(self :: any, "_x", v)`.)
129+
When a subclass wraps a metamethod parent — `setmetatable(Parent.new(...) :: any, MyClass)` — the
130+
fields it adds must use `rawset` too, since `MyClass.__newindex` will reject them.
131+
132+
These classes carry no test coverage during conversion, so the checker won't catch either mistake —
133+
they only surface at runtime. When in doubt, preserve the original access/order verbatim and just
134+
add casts.
135+
84136
## The export type rule — always `typeof(setmetatable(...))`, never hand-list methods
85137

86138
There is **one** way to write a class's export type, and it holds for generic, inherited, and

.claude/skills/strict-typing-luau/evals/README.md

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,29 @@ Per case, drive the worker (an agent running the skill) with these primitives:
2424
```bash
2525
bash evals/lib/run.sh place <case_id> # lay the nonstrict INPUT at the file's real path
2626
# ... run the skill on that path to convert it in place ...
27-
bash evals/lib/run.sh score <case_id> # -> {strict, analyze_errors, any, any_gold}
27+
bash evals/lib/run.sh score <case_id> # -> {strict, analyze_errors, selene, any, any_gold}
2828
bash evals/lib/run.sh restore <case_id> # undo: restore the package to main
2929
```
3030

31-
A conversion **passes** when `strict=true`, `analyze_errors=0`, and `any_nonrx <= any_gold_nonrx`
31+
A conversion **passes** when `strict=true`, `analyze_errors=0`, `selene=0`, `any_nonrx <= any_gold_nonrx`
3232
(no looser than the maintainer's output, *excluding* Rx-chain casts — those are deliberate policy,
33-
see `references/rx.md`). `score.sh` reports both the total `any` and the budgeted `any_nonrx`. Run
34-
`npm run lint:luau` once at the end as the downstream gate.
33+
see `references/rx.md`), and `raw >= raw_gold` (no raw-access regression, below). `selene` is the SECOND
34+
gate (CI-failing): dot-syntax conversion trips `unused_variable: self` (→ rename `_self`) and an Rx
35+
`local X = X :: any` trips `shadowing` (→ `local X: any = require`), both of which pass analyze but fail
36+
selene — so the scorer counts selene findings in the target file. `score.sh` reports both the total
37+
`any` and the budgeted `any_nonrx`. Run `npm run lint:luau` once at the end as the downstream gate.
38+
39+
**Raw-access regression gate (`raw` vs `raw_gold`) — silent runtime break, no analyze/selene signal.**
40+
Classes with a custom `__index`/`__newindex` that `error()`s on unknown keys use `rawget`/`rawset`
41+
deliberately to bypass the metamethod. A correct conversion only ever *adds* `:: any` casts to those
42+
calls; it must never remove one by rewriting `rawget(self, "_x")``self._x` (which re-fires the
43+
metamethod → wrong value, or `"Bad index"` on absent/lazy fields) or hoisting `setmetatable` above the
44+
constructor's field assignments. None of that is a type or lint error — it only breaks at runtime, and
45+
conversion cases carry no test coverage. So the scorer counts `rawget(`/`rawset(` calls in the target
46+
vs gold; `raw < raw_gold` fails the case. Gold defines the approved count, making the check
47+
false-positive-free. This guards the real regression the maintainer caught in `RogueProperty` (PR
48+
`users/quenty/strict-typing`), and the `settings-property` case (a custom-`__index` class with a
49+
`rawset`) exercises it. See the "Metamethod classes" section in SKILL.md.
3550

3651
**Whole-package runs.** Cases sharing an `input` commit belong to one package conversion — e.g.
3752
the six `settings-*` cases (all `input: b1bc1512aa`, `src/settings`, the `PlayerSettings` package
@@ -77,6 +92,19 @@ one sub-agent per file within a dependency layer, for a package > ~3 files) or `
7792
file / handful / error-fix). Guards the real-use regression where the agent converted a whole package
7893
serially because the skill never told it to parallelize.
7994

95+
### 6. Routing test (~instant, no LLM) — model routing heuristic
96+
97+
```bash
98+
bash evals/lib/run.sh routing
99+
```
100+
101+
Mechanical assert on `plan.js`'s model routing: a **ServiceBag service** (`.ServiceName`, a plain
102+
table with no `setmetatable`) must route to **opus**, a plain util to **sonnet**. It reads `plan.js`'s
103+
real `isClass` predicate (single source of truth, so it can't drift) and applies the same model rule
104+
to inline samples — no fixtures, no git, no judge call. Guards the real-use regression where a service
105+
mis-routed to sonnet got a shallow conversion (colon syntax, no `export type`) that broke a consumer.
106+
Add a case = one entry in `routing.js`'s `cases`.
107+
80108
## Design notes
81109

82110
- **Files are placed at package granularity** (`src/<pkg>`): a package is the unit of

.claude/skills/strict-typing-luau/evals/lib/plan.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,10 @@ for (const f of files) {
4242
const reqs = [...src.matchAll(/require\("([^"]+)"\)/g)].map((m) => m[1]);
4343
adj[f] = [...new Set(reqs.filter((r) => nameToFile[r] && nameToFile[r] !== f).map((r) => nameToFile[r]))];
4444
traits[f] = {
45-
isClass: /setmetatable\(\{\}/.test(src),
45+
// class-grade for typing = needs an `export type` block + dot-syntax methods. Covers true
46+
// metatable classes AND ServiceBag services (plain table + `.ServiceName`, no setmetatable) —
47+
// the latter still need the full class treatment, so route them to opus, not sonnet.
48+
isClass: /setmetatable\(\{\}/.test(src) || /\.ServiceName\s*=/.test(src),
4649
usesRx: reqs.some((r) => /^(Rx|RxSignal|Observable|Brio)$/.test(r)),
4750
};
4851
}
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
#!/usr/bin/env node
2+
// Mechanical (no-LLM) eval: plan.js's MODEL ROUTING heuristic. Guards the rule that a node needing
3+
// the full class treatment (export type + dot-syntax methods) goes to opus, a plain util to sonnet.
4+
//
5+
// The regression this catches: a ServiceBag service is a PLAIN table (`.ServiceName`, no
6+
// `setmetatable`), so a naive `isClass = /setmetatable/` check mis-routes it to sonnet — which
7+
// produced a shallow conversion (colon syntax, no export type) that broke a downstream consumer.
8+
// plan.js therefore also treats `.ServiceName` as class-grade. This test asserts that, by reading
9+
// plan.js's ACTUAL `isClass` predicate (so it can't silently drift) and applying plan.js's own
10+
// model rule. No fixtures, no git, no model call — pure and fast.
11+
const fs = require("fs");
12+
const path = require("path");
13+
14+
const planPath = path.join(__dirname, "plan.js");
15+
const src = fs.readFileSync(planPath, "utf8");
16+
17+
// Pull the real `isClass:` expression straight out of plan.js (single source of truth).
18+
const m = src.match(/isClass:\s*([^\n]+?),\s*\n/);
19+
if (!m) {
20+
console.error("FAIL: could not locate the `isClass:` predicate in plan.js — did its shape change?");
21+
process.exit(1);
22+
}
23+
const isClass = new Function("src", `return (${m[1]});`);
24+
// plan.js model rule (kept in sync with modelFor): a unit is opus if any target isClass/usesRx, or
25+
// the unit is a cyclic cluster (>1 file). For a single-file, non-Rx node, model is decided by isClass.
26+
const modelOf = (text, usesRx = false, clusterSize = 1) =>
27+
isClass(text) || usesRx || clusterSize > 1 ? "opus" : "sonnet";
28+
29+
const cases = [
30+
{
31+
name: "ServiceBag service (.ServiceName, no setmetatable)",
32+
src: 'local X = {}\nX.ServiceName = "X"\nfunction X.Init(self, serviceBag) end',
33+
isClass: true,
34+
model: "opus",
35+
},
36+
{
37+
name: "ServiceBag service, tabs/extra spaces before =",
38+
src: 'local X = {}\nX.ServiceName\t= "X"\n',
39+
isClass: true,
40+
model: "opus",
41+
},
42+
{
43+
name: "metatable class",
44+
src: "local C = setmetatable({}, Base)\nC.__index = C",
45+
isClass: true,
46+
model: "opus",
47+
},
48+
{
49+
name: "plain util / data module",
50+
src: "local Utils = {}\nfunction Utils.foo(x: number): number\n\treturn x\nend\nreturn Utils",
51+
isClass: false,
52+
model: "sonnet",
53+
},
54+
{
55+
name: "util that merely mentions ServiceName in a string/comment (not an assignment)",
56+
src: 'local Utils = {}\n-- returns the ServiceName for a tag\nfunction Utils.nameFor() return "ServiceName" end',
57+
isClass: false,
58+
model: "sonnet",
59+
},
60+
];
61+
62+
let fails = 0;
63+
for (const c of cases) {
64+
const gotClass = isClass(c.src);
65+
const gotModel = modelOf(c.src);
66+
const ok = gotClass === c.isClass && gotModel === c.model;
67+
if (!ok) fails++;
68+
console.log(`${ok ? "PASS" : "FAIL"} ${c.name}`);
69+
if (!ok) console.log(` isClass: got ${gotClass} want ${c.isClass} | model: got ${gotModel} want ${c.model}`);
70+
}
71+
console.log("");
72+
if (fails) {
73+
console.log(`ROUTING TEST: ${fails}/${cases.length} FAILED`);
74+
process.exit(1);
75+
}
76+
console.log(`ROUTING TEST: all ${cases.length} cases passed`);

.claude/skills/strict-typing-luau/evals/lib/run.sh

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
# run.sh place <id> Worker loop primitive: put the pre-conversion (nonstrict) INPUT at
99
# the file's real path so an agent can convert it in place.
1010
# run.sh score <id> Score whatever is at the file's path now, vs gold.
11-
# run.sh restore <id> Restore the file's package back to main (undo place/convert).
11+
# run.sh restore <id> Restore the file's package back to its committed state (HEAD, undo place/convert).
1212
# run.sh plan <pkg> Print the INTRA-PACKAGE conversion order (dependency-first, cyclic
1313
# clusters collapsed). The orchestration artifact a driver walks. Add
1414
# `json` for machine-readable output. Intra-package only — see plan.js.
@@ -27,6 +27,9 @@
2727
# single-file workers over-orchestrating). One claude -p call.
2828
# run.sh parallelism Judge the execution STRATEGY — fan out parallel sub-agents for a package
2929
# (> ~3 files) vs sequential for a single file/handful. One claude -p call.
30+
# run.sh routing Mechanical (no-LLM, ~instant): assert plan.js's model heuristic routes a
31+
# ServiceBag service (.ServiceName, no setmetatable) to opus, a plain util to
32+
# sonnet. Reads plan.js's real predicate so it can't drift. See routing.js.
3033
#
3134
# Files are laid down at PACKAGE granularity (src/<pkg>) because a package is the unit of
3235
# type-consistency: a cyclic file scored against nonstrict siblings would error falsely.
@@ -42,8 +45,10 @@ pkg_of(){ echo "$1" | cut -d/ -f1-3 | sed -E 's#(src/[^/]+)/.*#\1#'; }
4245
place_gold(){ local p; p="$(field "$1" path)"; git checkout "$(field "$1" gold)" -- "$(pkg_of "$p")"; }
4346
place_in() { local p; p="$(field "$1" path)"; git checkout "$(field "$1" input)" -- "$(pkg_of "$p")"; }
4447
restore() { local p pk; p="$(field "$1" path)"; pk="$(pkg_of "$p")";
45-
# back to main, unstage, and remove gold-only NEW files (e.g. a new *Types.lua)
46-
git checkout main -- "$pk" 2>/dev/null || true
48+
# back to the current branch's committed state (HEAD), unstage, and remove gold-only
49+
# NEW files (e.g. a new *Types.lua). HEAD not main: on main they're identical, but on a
50+
# feature branch `git checkout main` would revert the branch's own work in this package.
51+
git checkout HEAD -- "$pk" 2>/dev/null || true
4752
git reset -q HEAD -- "$pk"; git clean -fdq -- "$pk"; }
4853
score() { bash "$HERE/score.sh" "$(field "$1" path)" "$(field "$1" gold)"; }
4954

@@ -57,8 +62,9 @@ case "${1:-}" in
5762
triggers) bash "$HERE/triggers.sh" ;;
5863
tooling) bash "$HERE/tooling.sh" ;;
5964
parallelism) bash "$HERE/parallelism.sh" ;;
65+
routing) node "$HERE/routing.js" ;;
6066
gold)
61-
printf '%-22s %-8s %-6s %-8s %-5s %s\n' CASE POLARITY STRICT ANALYZE ANY VERDICT
67+
printf '%-22s %-8s %-6s %-8s %-7s %-5s %-7s %s\n' CASE POLARITY STRICT ANALYZE SELENE ANY RAW VERDICT
6268
fails=0
6369
for id in $(ids); do
6470
pol="$(field "$id" polarity)"
@@ -67,15 +73,20 @@ case "${1:-}" in
6773
restore "$id"
6874
strict="$(node -e "console.log(JSON.parse(process.argv[1]).strict)" "$row")"
6975
errs="$(node -e "console.log(JSON.parse(process.argv[1]).analyze_errors)" "$row")"
76+
selene="$(node -e "console.log(JSON.parse(process.argv[1]).selene)" "$row")"
7077
any="$(node -e "console.log(JSON.parse(process.argv[1]).any)" "$row")"
71-
# gold expectation: positive => strict & 0 errors; negative => nonstrict (reverted)
78+
raw="$(node -e "console.log(JSON.parse(process.argv[1]).raw)" "$row")"
79+
raw_gold="$(node -e "console.log(JSON.parse(process.argv[1]).raw_gold)" "$row")"
80+
# gold expectation: positive => strict & 0 analyze errors & 0 selene findings & no raw-access
81+
# regression (raw >= raw_gold, tautological on gold — proves the check doesn't false-positive);
82+
# negative => nonstrict (reverted)
7283
verdict=PASS
7384
if [ "$pol" = positive ]; then
74-
{ [ "$strict" = true ] && [ "$errs" -eq 0 ]; } || { verdict=FAIL; fails=$((fails+1)); }
85+
{ [ "$strict" = true ] && [ "$errs" -eq 0 ] && [ "$selene" -eq 0 ] && [ "$raw" -ge "$raw_gold" ]; } || { verdict=FAIL; fails=$((fails+1)); }
7586
else
7687
[ "$strict" = false ] || { verdict=FAIL; fails=$((fails+1)); }
7788
fi
78-
printf '%-22s %-8s %-6s %-8s %-5s %s\n' "$id" "$pol" "$strict" "$errs" "$any" "$verdict"
89+
printf '%-22s %-8s %-6s %-8s %-7s %-5s %-7s %s\n' "$id" "$pol" "$strict" "$errs" "$selene" "$any" "$raw/$raw_gold" "$verdict"
7990
done
8091
echo
8192
[ "$fails" -eq 0 ] && echo "GOLD SMOKE TEST: all cases passed" || { echo "GOLD SMOKE TEST: $fails FAILED"; exit 1; }

.claude/skills/strict-typing-luau/evals/lib/score.sh

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,5 +36,21 @@ GOLD_SRC="$(git show "$GOLD_REF:$FILE" 2>/dev/null || true)"
3636
ANY_GOLD="$(printf '%s' "$GOLD_SRC" | count_any)"
3737
ANY_GOLD_NONRX="$(printf '%s' "$GOLD_SRC" | count_any_nonrx)"
3838

39-
printf '{"file":"%s","strict":%s,"analyze_errors":%s,"any":%s,"any_gold":%s,"any_nonrx":%s,"any_gold_nonrx":%s}\n' \
40-
"$FILE" "$STRICT" "$ERRORS" "$ANY_FILE" "$ANY_GOLD" "$ANY_FILE_NONRX" "$ANY_GOLD_NONRX"
39+
# --- raw-access regression: metamethod classes (custom __index/__newindex that errors on unknown
40+
# keys) use rawget/rawset DELIBERATELY to bypass the metamethod. A correct conversion only ever ADDS
41+
# `:: any` casts to those calls — it must never remove one by rewriting `rawget(self,"_x")` -> `self._x`
42+
# (which re-fires the metamethod: wrong value or a "Bad index" runtime error). Gold is the approved
43+
# count, so `raw < raw_gold` is a definitive, false-positive-free regression. awk is pipefail-safe.
44+
count_raw() { awk '{n+=gsub(/rawget\(|rawset\(/,"")} END{print n+0}'; }
45+
RAW_FILE="$(count_raw < "$FILE")"
46+
RAW_GOLD="$(printf '%s' "$GOLD_SRC" | count_raw)"
47+
48+
# --- selene: the SECOND gate. Dot-syntax conversion trips `unused_variable: self` (→ rename `_self`)
49+
# and Rx `local X = X :: any` trips `shadowing` — both pass analyze but FAIL lint:selene (CI-failing).
50+
# Count selene findings in the target file only. Best-effort: needs the roblox std (generate once).
51+
[ -f roblox.yml ] || selene generate-roblox-std >/dev/null 2>&1 || true
52+
SELENE_OUT="$(selene --display-style=Json --config=selene.toml "$FILE" 2>/dev/null || true)"
53+
SELENE="$(printf '%s\n' "$SELENE_OUT" | grep -c '"severity"' || true)"
54+
55+
printf '{"file":"%s","strict":%s,"analyze_errors":%s,"selene":%s,"any":%s,"any_gold":%s,"any_nonrx":%s,"any_gold_nonrx":%s,"raw":%s,"raw_gold":%s}\n' \
56+
"$FILE" "$STRICT" "$ERRORS" "$SELENE" "$ANY_FILE" "$ANY_GOLD" "$ANY_FILE_NONRX" "$ANY_GOLD_NONRX" "$RAW_FILE" "$RAW_GOLD"

0 commit comments

Comments
 (0)