ReviewOS

stacks/stx

typecheck: --lib is documented repeatable but only the last one is honoured — a second file silently discards the first

#1925
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

--lib is documented as Extra ambient declaration file (repeatable), but only the last occurrence is honoured. Passing a second one silently discards the first, so an app that splits its ambient declarations across files gets the diagnostics of whichever file it happened to list last.

Measured on 0.2.176, same project and same glob every time, counting error TS lines:

invocationerrors
(no --lib)59
--lib A28
--lib B59
--lib A --lib B59  ← same as B alone
--lib B --lib A28  ← same as A alone

A is types/app.d.ts (the app's domain globals — AuthUser, ProjectRow, MeUser, …), B is types/stx-runtime.d.ts. The last two rows are the whole report: order decides the result, and the flag that is meant to accumulate replaces instead.

Why this is easy to lose an afternoon to

The failure is silent and it points the wrong way. Listing every declaration file — the obvious thing to do, and what "repeatable" invites — lands on the unhelped baseline of 59, which reads as "the flag does nothing" rather than "the flag is dropping all but one of your files". I concluded --lib was broken outright and only found the real rule by bisecting one file at a time.

It also masks real errors rather than merely hiding types. With the app's globals loaded, 15 TS2304 Cannot find name collapse into 12 genuine, actionable errors that were previously reported as missing names — including a null-dereference in a SQL-interpolating path. Getting the invocation wrong does not just under-report; it reports the wrong kind of problem.

Suggested fix

Collect the values instead of overwriting — accumulate --lib into an array and pass all of them to the program's root file set. If accepting only one is deliberate, the help text should say (last wins) rather than (repeatable), and passing more than one should warn.

Note on adjacent history

stx#1906 (--lib silently suppresses every diagnostic, closed) covered a different symptom of the same flag — that one reported 0 errors, this one reports the baseline. Also fixed since I last measured: a relative --lib path used to be ignored silently and now works, so --lib types/app.d.ts and --lib "$PWD/types/app.d.ts" both give 28. This is the residual.

Environment

  • stx 0.2.176 (built from source), bun 1.3.14, macOS 15
  • 38-view Stacks app used as the e2e surface for stx
14 days ago

Fixed in 65c070c. Your bisection was right, and the mechanism is one level below where it looked.

The CLI parser collapses a repeated option to its last value — and not just for <path>: I checked <path...> and [path...] too, and all three return the string "b" for --lib a --lib b. So there is no option spec that expresses "repeatable", which means the Array.isArray(options.lib) guard already in the command could never run. It was defensive code for a shape the parser never produces.

Values are now read from argv, where all of them still are. Your table, on the fix:

invocationresult
--lib AA's names resolve
--lib BB's names resolve
--lib A --lib Bboth resolve
--lib B --lib Aboth resolve
--lib=A --lib=Bboth resolve

Two details worth naming:

  • --lib=path works as well as --lib path. Someone who has been told a flag repeats will reasonably try either spelling, and the parser handled neither.
  • A missing value no longer swallows the next option. --lib --no-client would otherwise have consumed --no-client as a path and silently turned client checking off — the same class of silent failure, introduced by the fix for it.

Your framing that it reports the wrong kind of problem rather than merely under-reporting is the part I put in the commit message. 15 unresolved-name errors standing in for 12 actionable ones, including a null dereference on a SQL-interpolating path, is a much worse outcome than a wrong count, and it is why this was worth doing properly rather than just documenting (last wins).

On the adjacent point — reading ambients from tsconfig

You are right that it would make this class of bug much harder to hit, and I want to be straight that I have not done it here.

The checker deliberately builds its own program: it generates a tsconfig in a temp directory listing only the virtual buffers, because the buffers are synthetic files that the app's own include globs would never match. It does already read the project's tsconfig — readProjectPathAliases walks extends, strips JSONC correctly, and rewrites paths to absolute targets — so the plumbing for reading include/files is there, and it is maybe twenty lines.

I stopped short because it changes every existing project's results in one step: names that were unresolved start resolving (good), but a project whose .d.ts files have errors of their own would newly trip the --lib-is-broken guard from #1906, and a broad include glob would pull real cost into every run. That is a behaviour change that deserves its own issue, its own measurement, and its own release note rather than riding along with a flag fix.

If you want it, file it and I will do it — the case for it is good, and with --lib now accumulating you at least have a correct workaround in the meantime rather than a misleading one.

Verification: 8 tests, sabotage-checked by restoring last-wins semantics (3 fail). Full suite 11902 pass; the 2 failures are the craft binary resolution pair, which fail only on machines with craft installed and pass on CI.

Sign in to comment on this issue.