fix(semantic): report undefined types before the type checker can mask them (RTOP-286)

[JoaoGSP]

Closes RTOP-286. Upstream half of DOPE-553 (openplc-web #748 / openplc-editor #1099).

The defect

analyze() runs three passes and gates both the second and the third on this.errors.length === 0. Undefined type 'X' is raised by the third. Because the language server merges every open document into a single AST, one type error in any document suppressed undefined-type reporting for the entire project.

The OpenPLC editor synthesizes all user data types into one TYPE … END_TYPE document. Deleting a data type that a POU variable still used produced a type error in that POU, which suppressed pass 3, which meant the datatypes document received an empty diagnostics array. Every data type lost its markers, including unrelated errors that were being reported correctly a moment earlier. Recreating the deleted type brought them all back, which is what identified the dangling reference as the trigger.

The change

validateTypeReferences reads only the symbol tables built by pass 1. It has no dependency on the type checker having run, and it is idempotent. Hoisting it ahead of the gate reports the undefined type itself instead of the cascade it causes.

That is also the better diagnostic: a reference to a type that does not exist is the root cause, and Condition must be a boolean or bit type, got FOO is a symptom of it.

The hoisted findings are deliberately excluded from both gates. Without that, they would close the gates themselves and cost far more than the cascading error they explain — the entire type checker plus all thirteen validators in validateSemantics, project-wide, for every project containing one undefined type. That was the first version of this change and review caught it: x : Foo alongside an unrelated ok := neverDeclared reported the undefined type and lost Undeclared variable 'NEVERDECLARED', which development reports. With the exclusion, every later pass runs exactly as it does today and the undefined type is always reported alongside whatever else they find, the cascading error included. Suppressing that cascade is not separable from suppressing everything else those passes report, so it is kept.

No requirements gathering document and no cybersecurity risk assessment are owed here: this is a bug fix to an existing diagnostic path, with no new attack surface and no behavioural scope beyond which diagnostics are reported.

One risk worth naming: the check now runs even when pass 1 itself recorded errors. That cannot invent a false undefined type, because the symbol table pass records errors and keeps registering declarations rather than bailing out. Verified against a located-variable-in-VAR_INPUT error with a valid enum reference in the same source.

Verification

CI does not run on pull requests to development, so this is the whole evidence base.

Gate	Result
Root suite	96 files, 2398 passed, 7 skipped (baseline 2395; delta is the three new tests)
Extension suite	20 files, 282 passed
tsc root and extension	both exit 0
format:check	clean
eslint src/semantic/analyzer.ts	30 warnings before and after, zero delta

Three tests, all of which fail on stock and pass with the fix, confirmed by reverting the analyzer and re-running: the multi-source case this ticket is about, its single-source control, and — added after review — an undefined type alongside an unrelated later-pass error, which pins the gate exclusion described above.

End to end in the OpenPLC editor, with this build's browser-server.js swapped in: a project with a struct field of an undefined type, plus a POU whose body type-errors against a different user type, shows one error squiggle with this build and zero with the pinned v0.6.6 — same project, same view.

Release note

This needs a v0.6.7 cut and a binary-versions.json bump in openplc-web and openplc-editor before it reaches users. development is currently behind main as well as ahead, which wants reconciling at the cut.

🤖 Generated with Claude Code

[dcoutinho1328]

Reviewed with a compiled development build and a compiled build of this branch side by side, running the same sources through compile() and diffing the reported errors.

The fix does what the ticket asks, but the hoist has a side effect worth closing before merge: because validateTypeReferences writes into this.errors, its own findings now close the two this.errors.length === 0 gates. A single undefined type anywhere in the merged AST skips the whole type checker and all of pass 3. Details inline on src/semantic/analyzer.ts.

Two notes on the description rather than the code:

1. "the symptom is suppressed" understates it. What is suppressed is every pass 2 diagnostic and all thirteen pass 3 validators, project-wide, not only the cascading got FOO error. Someone reading this PR later to understand the trade-off that was accepted will not get that from the current wording.

2. Documentation link. The body cites Closes RTOP-286 but does not link it. This is a bug fix, so no Requirements Gathering document and no cybersecurity risk assessment are owed, but that should be one explicit line in the body rather than left implicit, so the next reader does not have to work out whether they are missing or simply not required.

On the release note: agreed that this needs a v0.6.7 cut and the binary-versions.json bumps in openplc-web and openplc-editor, and that development being both behind and ahead of main wants reconciling at the cut.

[dcoutinho1328]

This hoist trades one silence for another, and the new one is broader than the old.

validateTypeReferences reports through addError, so its findings land in this.errors before both gates below are evaluated. One undefined type anywhere in the merged AST therefore makes this.errors.length === 0 false at line 341 and again at line 348: the type checker never runs, and neither do the thirteen validators in validateSemantics.

Same source compiled against development and against this branch:

PROGRAM Main VAR x : Foo; ok : BOOL; END_VAR  ok := neverDeclared; END_PROGRAM

development: ["Undefined type 'FOO' in PROGRAM 'MAIN'", "Undeclared variable 'NEVERDECLARED'"]
this branch: ["Undefined type 'FOO' in PROGRAM 'MAIN'"]

No additionalSources needed to hit it, and the same loss shows with Cannot assign to CONSTANT variable 'K' and the rest of pass 3.

In the editor this reproduces the ticket's own symptom from the other side: delete a data type a POU still references, and every remaining squiggle in every POU goes dark instead of the datatypes document going blank.

Excluding the hoisted pass's errors from the two gates fixes it:

const errorsBeforeTypeRefs = this.errors.length;
this.validateTypeReferences(ast);
const typeRefErrors = this.errors.length - errorsBeforeTypeRefs;

// Pass 2: Type checking
if (errorsBeforeTypeRefs === 0) {
  ...
}

// Pass 3: Semantic validation
if (this.errors.length - typeRefErrors === 0) {
  this.validateSemantics(ast);
}

I applied that and ran the suite: 96 files, 2397 passed, 7 skipped, identical to the numbers in your table. Every diagnostic development produced comes back, the undefined type is still reported as the root cause, and the case from the ticket now reports both Undefined type 'FOO' in STRUCT 'MYSTRUCT' and Condition must be a boolean or bit type, got INT rather than either one alone.

It also does not reintroduce the ordering invariant you removed from validateSemantics. Pass 3 already ran with undefined types present on development, since validateSemantics reported them and carried on to the other validators rather than bailing out.

Separately, the hoist itself checks out: isKnownType reads only symbolTables.globalScope, so there is no dependency on the type checker having run, and I saw no false positives from interfaces, EXTENDS/IMPLEMENTS, or a pass 1 error sitting next to a valid enum reference.

[JoaoGSP]

Confirmed and fixed in 3e691ad. You were right, and your repro reproduces exactly as written — I ran it against a compiled development and a compiled branch build side by side before changing anything:

development:  Undefined type 'FOO' in PROGRAM 'MAIN'  +  Undeclared variable 'NEVERDECLARED'
first version: Undefined type 'FOO' in PROGRAM 'MAIN'

The gates now subtract the errors the hoisted pass contributed, so they test what they always tested and the hoisted findings cannot close them:

const errorsBeforeTypeReferences = this.errors.length;
this.validateTypeReferences(ast);
const typeReferenceErrors = this.errors.length - errorsBeforeTypeReferences;

if (this.errors.length - typeReferenceErrors === 0) { /* pass 2 */ }
if (this.errors.length - typeReferenceErrors === 0) { /* pass 3 */ }

Your case now reports both errors, and the case this ticket is about still reports Undefined type 'FOO' in STRUCT 'MYSTRUCT' against the datatypes document. The cascading got FOO returns with pass 2, which I have stopped trying to avoid: it is not separable from everything else those passes report, and losing the rest is clearly the worse trade. The description said "the symptom is suppressed" and now says what was actually at stake, along with the explicit line that a bug fix owes no requirements document or risk assessment.

[dcoutinho1328]

Both new tests assert only that at least one Undefined type error is present, so both pass whether or not the rest of the diagnostics survived. The full suite passes identically with and without the suppression described on analyzer.ts, which is why it did not surface here.

Worth adding a case that pins the thing that actually regressed: an undefined type plus an unrelated error that a later pass owns, asserting both are reported.

it("should not let an undefined type suppress unrelated diagnostics", () => {
  const mainST = `
    PROGRAM Main
      VAR
        x : Foo;
        ok : BOOL;
      END_VAR
      ok := neverDeclared;
    END_PROGRAM
  `;

  const result = compile(mainST, {});

  expect(result.errors.some((e) => /Undefined type/i.test(e.message))).toBe(true);
  expect(result.errors.some((e) => /Undeclared variable/i.test(e.message))).toBe(true);
});

That one fails on the current branch and passes on development, which is the opposite direction from the two tests added here and is what makes the pair meaningful.

[JoaoGSP]

Fair, and added as you wrote it. You are right about why it did not surface: my two cases asserted only that some Undefined type error was present, so they were blind to whatever else the gates had swallowed, and the suite passed identically either way.

The new case fails on the first version of this branch and passes now, which I checked by reverting the analyzer and re-running rather than assuming it. Root suite is 2398 with it.