From 8df1356a205fb938090187c2303e819f5f354afb Mon Sep 17 00:00:00 2001 From: lab Date: Tue, 8 Sep 2026 03:52:39 +0700 Subject: [PATCH] fix(typecheck): resolve `use` before checking, under the backends' contract Closes #3412 Every `gen-*` path splices imported declarations in before compiling. `typecheck` read the raw source. So a type arriving through an import read as undeclared, and the unknown-type check added one pass ago in #3409 warned about types the spec correctly imports -- my own defect, found by measuring my own output. Measured over all 651 specs: unknown type warnings 1283 -> 1045 all warnings 1775 -> 1570 exit-code changes 0 specs/base/ternary_add.t27 alone goes from 10 warnings to 0: it writes `use base::types;` and every type it names comes from there. Counted a second way before the repair -- asking whether each warned name appears as a declaration in the RESOLVED output -- 41 distinct names across 29 of the 169 flagged files were false, against 228 true. The naive repair is wrong, and the reason is already written down. run_gen carries the contract: Safety contract ... this may only ADD declarations, never break a spec. If the spliced source stops compiling, the original is used and the spec generates exactly what it generated before. Resolving without that fallback took specs/nn/hslm.t27 from exit 0 to `Expected RParen, got Eof at line 652:1`, while all four backends still compiled it -- because they fall back and this did not. The splice can produce source the parser rejects; that is a handled condition, not a verdict about the spec. With the contract mirrored, exactly 1 of 651 specs takes the fallback and says so on stderr rather than silently checking something other than what it claims to. Mutation-checked: disabling the resolve fails the new test with the measured number, "it warned 10 times". compiler.rs is untouched, so FROZEN_HASH is unchanged. Co-Authored-By: Claude Opus 5 --- bootstrap/src/main.rs | 33 +++++++++++++- bootstrap/tests/unknown_type.rs | 45 +++++++++++++++++++ ...read-a-different-file-than-the-backends.md | 10 +++++ 3 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 docs/now/2026-09-08-the-check-read-a-different-file-than-the-backends.md diff --git a/bootstrap/src/main.rs b/bootstrap/src/main.rs index 316595d2b1..7bff110e78 100644 --- a/bootstrap/src/main.rs +++ b/bootstrap/src/main.rs @@ -6100,8 +6100,37 @@ fn run_optimize(input_path: &str, opt_level: u32) -> anyhow::Result<()> { } fn run_typecheck(input_path: &str, json: bool) -> anyhow::Result<()> { - let source = fs::read_to_string(input_path)?; - let ast = compiler::Compiler::parse_ast(&source).map_err(|e| anyhow::anyhow!("{}", e))?; + let raw = fs::read_to_string(input_path)?; + // Typecheck what the BACKENDS compile, not what the file literally holds. + // Every `gen-*` path resolves `use` first; typecheck did not, so a type + // that arrives through an import read as undeclared. Measured before this + // line existed: of 269 names the unknown-type check reported, 41 across 29 + // files were declared in the resolved output -- warnings about types the + // spec correctly imports. + // The same safety contract every `gen-*` path carries, quoted from the one + // at run_gen: "this may only ADD declarations, never break a spec. If the + // spliced source stops compiling, the original is used." Without the + // fallback, specs/nn/hslm.t27 went from exit 0 to a parse failure at + // 652:1 while all four backends still compiled it -- the splice can + // produce source the parser rejects, and that is a handled condition + // rather than a verdict about the spec. + let spliced = use_resolve::resolve(std::path::Path::new(input_path), &raw); + let (source, ast) = match compiler::Compiler::parse_ast(&spliced) { + Ok(a) => (spliced, a), + Err(splice_err) => { + let a = compiler::Compiler::parse_ast(&raw).map_err(|_| { + // The raw source failing too is a real parse error and is + // reported as the spliced one, which is the more informative. + anyhow::anyhow!("{}", splice_err) + })?; + eprintln!( + "note: spliced source did not parse, typechecking the \ +unresolved original -- imported declarations are NOT considered here" + ); + (raw.clone(), a) + } + }; + let _ = &source; let result = compiler::typecheck_ast(&ast); if json { let resp = serde_json::json!({ diff --git a/bootstrap/tests/unknown_type.rs b/bootstrap/tests/unknown_type.rs index f412c29d4b..2459b15dfb 100644 --- a/bootstrap/tests/unknown_type.rs +++ b/bootstrap/tests/unknown_type.rs @@ -110,3 +110,48 @@ fn an_array_of_a_declared_type_is_not_unknown() { ); assert!(w.is_empty(), "array wrappers must be stripped: {w:?}"); } + +/// The check must see what the BACKENDS compile, not what the file literally +/// holds. +/// +/// `typecheck` read the raw source while every `gen-*` path resolves `use` +/// first, so a type arriving through an import read as undeclared. Measured +/// over the corpus: **1283 unknown-type warnings before, 1045 after** -- 238 of +/// them were about types the spec correctly imports. `specs/base/ternary_add.t27` +/// alone went from 10 warnings to 0. +/// +/// The resolve carries the same safety contract every backend does: it may only +/// ADD declarations. When the spliced source does not parse the original is +/// used and the fallback says so -- exactly 1 of 651 specs takes that path, and +/// the exit code of `check` changes for none of them. +#[test] +fn a_type_that_arrives_through_an_import_is_not_unknown() { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("bootstrap has a parent") + .to_path_buf(); + let spec = root.join("specs/base/ternary_add.t27"); + if !spec.exists() { + // Loudly: an absent input is not a passing test. + eprintln!("SKIP: specs/base/ternary_add.t27 not in this tree"); + return; + } + let out = Command::new(env!("CARGO_BIN_EXE_t27c")) + .arg("typecheck") + .arg(&spec) + .output() + .expect("run t27c"); + let text = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + let warned: Vec<&str> = text.lines().filter(|l| l.contains("unknown type")).collect(); + assert!( + warned.is_empty(), + "this spec writes `use base::types;` and every type it names comes from \ + there; it warned {} times:\n{}", + warned.len(), + warned.join("\n") + ); +} diff --git a/docs/now/2026-09-08-the-check-read-a-different-file-than-the-backends.md b/docs/now/2026-09-08-the-check-read-a-different-file-than-the-backends.md new file mode 100644 index 0000000000..34515ac9ae --- /dev/null +++ b/docs/now/2026-09-08-the-check-read-a-different-file-than-the-backends.md @@ -0,0 +1,10 @@ +# NOW -- The check read a different file than the backends (2026-09-08) + +## The check read a different file than the backends (Closes #3412) + +- Every `gen-*` path splices imported declarations in before compiling; `typecheck` did not. So a type arriving through an import read as undeclared, and the unknown-type check shipped one pass earlier in #3409 warned about types the spec correctly imports. **This is my own defect, found by measuring my own output rather than by anyone reporting it.** +- Measured over all 651 specs: `unknown type` warnings **1283 -> 1045**, all warnings **1775 -> 1570**, exit-code changes **0**. `specs/base/ternary_add.t27` alone went **10 -> 0**: it writes `use base::types;` and every type it names comes from there. +- Counted a second way before the repair, by asking whether each warned name appears as a declaration in the RESOLVED output: **41 distinct names across 29 of the 169 flagged files** were false, against 228 true. Precision was ~85%, which is not broken -- but noise in a check whose only job is to be believed. +- The naive repair is wrong, and the reason is written in the code already. `run_gen` carries the contract: *"this may only ADD declarations, never break a spec. If the spliced source stops compiling, the original is used."* Resolving without that fallback took `specs/nn/hslm.t27` from exit 0 to a parse failure at 652:1 -- while all four backends still compiled it, because they fall back and my version did not. With the contract mirrored, exactly **1 of 651** specs takes the fallback, and it says so on stderr rather than silently typechecking something other than what it claims to. +- Mutation-checked: disabling the resolve fails the new test with the measured number, "it warned 10 times". +- Named and not done: `Trit` is declared by four specs as `pub const Trit = enum(i8)` and simply not imported by the files using it. Adding one `use` line to `specs/ar/restraint.t27` takes its emitted Rust from **30 rustc errors to 19** and brings `pub enum Trit` into the output. A spec-side repair with a measured payoff, deserving its own change.