ReviewOS

stacks/stx

stx typecheck: --lib is documented as repeatable but the last one wins — earlier ambient files are dropped silently

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

--help calls --lib repeatable:

--lib <path>    Extra ambient declaration file (repeatable)

It is not. The last one wins and the earlier ones are dropped silently, so a project that splits its ambient declarations across two files gets one of them and no indication the other was ignored.

Repro

Two ambient files. types/domain.d.ts declares the app's domain types (declare type IssueLevel = …), types/globals.d.ts declares one SSR global (declare const requestContext: …). Measured on stx 0.2.176, .stx/cache cleared before each run, counting unresolved-name errors across the same 38 files:

invocationrequestContext unresolvedIssueLevel unresolvedtotal
--lib types/domain.d.ts780407
--lib types/globals.d.ts011323
--lib types/domain.d.ts --lib types/globals.d.ts011323

The third row is identical to the second. Passing both is exactly equivalent to passing only the last, and the 11 IssueLevel errors are the proof that the first file was discarded.

Concatenating the two into one file resolves everything both declared, which is the workaround — but it forces unrelated declarations to share a file purely to satisfy the flag.

Why it is easy to miss

The failure is silent and looks like a project problem. Adding a second --lib to fix one set of unresolved names appears to work, because the names you were chasing resolve; the names the first file was providing quietly break at the same moment, and if you were not counting per-name you would read the new total as progress. The total here went 407 → 323 while a category of error was being introduced.

Adjacent, possibly the same root cause

The checker does not appear to read ambient declarations from the project's tsconfig.json at all. This repo has "include": ["types/**/*.d.ts", …] and tsc --noEmit resolves every one of these names, but a typecheck run with no --lib reports all 407 including the ones types/domain.d.ts declares. If the checker built its program from tsconfig, --lib would rarely be needed and this bug would be much harder to hit.

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.


Filed twice (this and #1925) — closing both against the same commit.

Sign in to comment on this issue.