ReviewOS

stacks/stx

stx typecheck: @if conditions are unchecked, and their errors are blamed on unrelated expressions

#2013
Closed glennmichael123 opened this 9 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
9 days ago

stx typecheck does not check @if conditions as expressions. The condition's code still reaches the generated TypeScript, so a fault in it surfaces as a diagnostic attributed to an unrelated template expression inside the block, and vanishes entirely when the block has no expression to land on.

Found on 0.2.345 while bumping an app from 0.2.286. It cost about an hour, because the reported expression was provably fine and the real fault was on the line above.

Reproduction

<script client>
interface N { ok: boolean }
const note = state<N | null>(null)
const unrelated = state<string>('x')
function text(n: N | null): string { return n ? 'y' : 'n' }
</script>

@if (note() && note().ok)
  <span :text="text(note())"></span>
@endif
probe.stx:8:16  error TS2531  Object is possibly 'null'.  [template: text(note())]
Checked 1 script block(s) and 1 template expression(s) in 1 file(s) — 1 error(s)

Line 8 is the <span>. The real fault is line 7: note().ok, where note() returns N | null and TypeScript does not narrow across two calls to the same function. text(note()) is correct by its own signature, which openly accepts N | null.

Five cases that isolate it

templateresult
A<span :text="text(note())"> alone0 errors. Correct.
Bfaulty @if + <span>plain text</span>0 errors, "0 template expressions"
Cfaulty @if + <span :text="text(note())">1 error, blamed on line 8
Dfaulty @if + <span></span>1 error, blamed on unrelated()
Efaulty @if, expression after 0 errors

D is the clearest. unrelated is state<string> and can never be null, yet it is the expression named in an "Object is possibly 'null'" diagnostic.

B and E are the more serious half. The condition is counted as zero template expressions and reports nothing, so a genuinely unguarded null access in an @if passes clean whenever the block holds no interpolation.

Why this matters beyond the confusing message

The diagnostic itself is a true positive worth keeping. 0 errors1, on the condition

The column points at the offending sub-expression, so note() && note().ok squiggles the second note().

What it was

guardChainAt already emits each condition verbatim into an if (…) wrapping every expression in the block — and its own source comment claimed that a condition which does not compile "is reported where it is written — as its own template expression". That second half was never implemented. Nothing extracted conditions, so a fault in one had no line to land on: it surfaced against whichever expression was inside the block, and with no expression there was no wrapper to carry it.

Three parts

  1. Conditions are extracted as expressions (@if, @elseif, @else if, @unless), with their own line and column. Their offset is the directive's start rather than the condition's, so guardChainAt stops before the block and a condition is not narrowed by itself — while outer blocks still apply, so a nested @if still sees the enclosing guard.
  2. The guard wrapper no longer double-reports. MappedLine records the if (…) { prefix length and resolvePosition reports whether a hit landed inside it; typecheck drops those, because the condition has a line of its own now. Without this, C and D reported the same fault twice.
  3. The count is honest. Case B read 0 template expression(s) for a file that plainly had one, and "0 errors" over something uncounted is indistinguishable from a pass — your point, and it was the reason B looked clean.

On the framing

I nearly filed this as a false-positive report for exactly that reason

That is the part that made this worth doing ahead of other work. The diagnostic is a true positive — @if (x() && x().prop) reads as guarded and is not, since nothing promises two calls to x() return the same value — and it found 9 real bugs for you. But pointed at a state<string>, "Object is possibly null" reads as a broken checker, and the correct fix gets skipped. A true positive attributed to innocent code is worse than no diagnostic.

Not done

@foreach subjects. The :show / :if bindings you mention already go through DIRECTIVE_ATTR_RE and were being checked; @foreach is a statement like @if and is not, so it wants the same treatment in its own change. Happy to take it if you open one, or say so here.

One of my own fixtures was wrong in an instructive way: "a condition that really does guard" first guarded with