v0.4.1: Destructuring Params, Circular Import Diagnostics, Otherwise Lint#21
Conversation
Allow map and array destructuring patterns directly in function parameter
lists, consistent with existing destructuring in let bindings and for loops.
fn greet({ name, email }) { ... }
fn first_two([a, b, ...rest]) { ... }
fn process(id, { name }: Map) { ... }
- ast.rs: add optional `pattern` field to Parameter struct
- parser.rs: parse_parameters() detects `{`/`[` and calls parse_pattern()
to build a synthetic parameter name with an attached pattern
- interpreter.rs: call_user_function() calls bind_pattern() after binding
the synthetic name, expanding destructured variables into scope
- typechecker.rs: bind_pattern() called for param patterns; strict-lint
annotation warnings skip synthetic destructured params
- typechecker.rs: update test expectations for otherwise-block-must-diverge
behaviour change (warning → error) already present on this branch
- tests: 4 unit tests in interpreter.rs + 4 integration tests in
language_features_tests.rs
All 984 tests pass.
7979985 to
151d592
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the NTNT language implementation to support pattern destructuring directly in function parameters, and upgrades non-diverging otherwise blocks from a lint warning to a lint error to prevent runtime crashes.
Changes:
- Add function-parameter destructuring (map/array patterns) via existing
parse_pattern()/bind_pattern()plumbing. - Make non-diverging
otherwiseblocks a typechecker error (previously warning) and update diagnostics tests accordingly. - Add new interpreter + language feature tests for destructured parameters.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/ast.rs |
Adds pattern: Option<Pattern> to Parameter for destructuring-capable params. |
src/parser.rs |
Parses {...} / [...] patterns as function params using synthetic parameter names. |
src/interpreter.rs |
Binds destructuring patterns at function call time and adds runtime tests. |
src/typechecker.rs |
Enforces otherwise divergence as an error; binds pattern variables for params; updates tests. |
tests/language_features_tests.rs |
Adds end-to-end tests for destructured function parameters. |
Comments suppressed due to low confidence (1)
src/typechecker.rs:856
- In strict lint mode, destructured parameters without a type annotation no longer emit MissingParamAnnotation diagnostics because of the
&& param.pattern.is_none()guard. This makes strict lint less effective for the new feature (pattern-bound variables will default toAnysilently). Consider emitting a dedicated diagnostic for “destructured parameter missing type annotation” (or reusing MissingParamAnnotation but with a clearer message) whenparam.pattern.is_some() && param.type_annotation.is_none().
if self.strict_lint {
let fn_line = self.find_line_near(&format!("fn {}", name));
for param in params {
if param.type_annotation.is_none() && param.pattern.is_none() {
self.emit_with_kind(
Severity::Warning,
DiagnosticKind::MissingParamAnnotation,
format!(
"Parameter '{}' in function '{}' has no type annotation",
param.name, name
),
fn_line,
Some(format!("Add a type: {}: Type", param.name)),
);
}
src/interpreter.rs
Outdated
| func_env | ||
| .borrow_mut() | ||
| .define(param.name.clone(), value.clone()); |
There was a problem hiding this comment.
Function-call parameter binding defines the synthetic "_destructure_N" parameter in the function environment even when the parameter is a destructuring pattern. This makes an internal implementation detail user-visible (it can be referenced from the function body) and can also collide with user-chosen parameter names (e.g., a user explicitly naming a param _destructure_0). Consider skipping define(param.name, ...) when param.pattern.is_some() (bind only the pattern vars), or use a non-lexable internal name and ensure it’s never bound in the runtime environment.
| func_env | |
| .borrow_mut() | |
| .define(param.name.clone(), value.clone()); | |
| // Only bind the named parameter when there is no destructuring pattern. | |
| // For destructuring params, bind only the pattern variables to avoid | |
| // exposing synthetic names (e.g., "_destructure_0") in the environment. | |
| if param.pattern.is_none() { | |
| func_env | |
| .borrow_mut() | |
| .define(param.name.clone(), value.clone()); | |
| } |
src/typechecker.rs
Outdated
| .map(|t| self.resolve_type_expr(t)) | ||
| .unwrap_or(Type::Any); | ||
| self.bind(¶m.name, typ); | ||
| self.bind(¶m.name, typ.clone()); |
There was a problem hiding this comment.
Type binding for destructured parameters currently also binds the synthetic parameter name (e.g., _destructure_0) into the function scope via self.bind(¶m.name, ...). This leaks an internal name into user-visible type checking (and allows referencing it in code if the interpreter also binds it), and can conflict with user-defined names. Consider not binding param.name when param.pattern.is_some() (only bind the variables introduced by the pattern).
| self.bind(¶m.name, typ.clone()); | |
| // For destructured parameters, only bind the pattern variables, not the | |
| // synthetic parameter name (e.g., `_destructure_0`), to avoid leaking | |
| // internal names into the function scope. | |
| if param.pattern.is_none() { | |
| self.bind(¶m.name, typ.clone()); | |
| } |
src/typechecker.rs
Outdated
| @@ -5121,21 +5131,30 @@ mod tests { | |||
|
|
|||
| #[test] | |||
| fn test_otherwise_without_return_warns() { | |||
There was a problem hiding this comment.
Test name is now misleading: test_otherwise_without_return_warns asserts an Error diagnostic (not a warning). Renaming the test to reflect the new behavior will make failures easier to interpret.
| fn test_otherwise_without_return_warns() { | |
| fn test_otherwise_without_return_errors() { |
src/typechecker.rs
Outdated
| // Gradual typing: untyped code produces no type errors. | ||
| // Note: a non-diverging otherwise block is now an error (not a type error), | ||
| // so we use a properly diverging otherwise to isolate the gradual typing check. | ||
| let diags = check( | ||
| r#" | ||
| let val = Some(42) otherwise { | ||
| let x = 1 | ||
| } | ||
| fn foo(a) { | ||
| let b = fn(x) { x + 1 } | ||
| } |
There was a problem hiding this comment.
The comment says this test uses a “properly diverging otherwise” to isolate gradual typing, but the sample code no longer includes an otherwise at all. Update or remove the comment to match the current fixture so the test intent stays clear.
When files form an import cycle (a.tnt → b.tnt → a.tnt), the type
checker now emits a warning showing the exact cycle chain instead of
silently returning empty exports.
warning: Circular import detected: a.tnt → b.tnt → a.tnt
Hint: break one of these imports to resolve the cycle
Implementation:
- typechecker.rs: detect cycles before the module cache check (Pass 1
cache was masking cycles), accumulate cycle messages in a new
detected_cycles field threaded through nested contexts via mem::take
- Deduplicate cycle warnings at the root check_program_with_options
entry point and attempt to locate the import line number
- Two new tests: 2-file cycle and 3-file cycle, both verifying the
warning message includes file names and the → chain
Inspired by Zig's dependency loop diagnostics (devlog 2026-03-10).
| fn test_circular_import_three_file_cycle() { | ||
| use std::io::Write; | ||
| // Create a three-file cycle: a → b → c → a | ||
| let dir = std::env::temp_dir().join("ntnt_test_circular_three"); |
There was a problem hiding this comment.
These circular-import tests create directories under std::env::temp_dir() with fixed names (e.g. ntnt_test_circular_three). Rust tests run in parallel by default, so this can race/flap if multiple test processes (or repeated runs) hit the same paths. Use a unique temp directory per test run (e.g. tempfile::tempdir() or include PID + random suffix), and rely on RAII cleanup where possible.
| let dir = std::env::temp_dir().join("ntnt_test_circular_three"); | |
| let dir = std::env::temp_dir().join(format!( | |
| "ntnt_test_circular_three_{}_{}", | |
| std::process::id(), | |
| std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap() | |
| .as_nanos() | |
| )); |
| // Should emit a warning about the circular import with the cycle chain | ||
| let warnings: Vec<_> = diags | ||
| .iter() | ||
| .filter(|d| { | ||
| d.severity == Severity::Warning && d.message.contains("Circular import detected") | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
The circular-import warning assertions added here are good coverage, but the test still uses a fixed temp directory name (ntnt_test_circular earlier in this test). Because tests run concurrently by default, fixed names can cause intermittent failures. Prefer creating a unique temp dir per test run (e.g. tempfile::tempdir() or a randomized suffix) so parallel test execution is safe.
src/interpreter.rs
Outdated
| func_env | ||
| .borrow_mut() | ||
| .define(param.name.clone(), value.clone()); | ||
| if let Some(ref pat) = param.pattern { | ||
| self.bind_pattern(pat, &value)?; | ||
| } |
There was a problem hiding this comment.
call_user_function now clones every argument value to bind it into the function environment (define(..., value.clone())). Since Value::clone() is deep for arrays/maps/structs, this adds potentially significant overhead to every function call even when the parameter is not destructured. You can avoid the clone by binding the destructuring pattern before moving value into the environment, or only cloning when param.pattern.is_some().
| func_env | |
| .borrow_mut() | |
| .define(param.name.clone(), value.clone()); | |
| if let Some(ref pat) = param.pattern { | |
| self.bind_pattern(pat, &value)?; | |
| } | |
| // Bind any destructuring pattern before moving the value into the environment | |
| if let Some(ref pat) = param.pattern { | |
| self.bind_pattern(pat, &value)?; | |
| } | |
| func_env | |
| .borrow_mut() | |
| .define(param.name.clone(), value); |
| if self.check(&TokenKind::LeftBrace) || self.check(&TokenKind::LeftBracket) { | ||
| let pat = self.parse_pattern()?; | ||
| let synth_name = format!("_destructure_{}", params.len()); | ||
| (synth_name, Some(pat)) | ||
| } else { | ||
| let name = self.consume_identifier("Expected parameter name")?; | ||
| (name, None) |
There was a problem hiding this comment.
The synthetic parameter name (_destructure_{n}) is used as Parameter.name for destructured parameters. This can leak into user-facing diagnostics (e.g., the “required parameter cannot follow a parameter with a default” error will report _destructure_1, which doesn't exist in source code). Consider storing a separate display name (derived from the pattern) for diagnostics, or enhancing error formatting to refer to the pattern/first bound identifier instead of the synthetic name.
src/typechecker.rs
Outdated
| // Gradual typing: untyped code produces no type errors. | ||
| // Note: a non-diverging otherwise block is now an error (not a type error), | ||
| // so we use a properly diverging otherwise to isolate the gradual typing check. |
There was a problem hiding this comment.
This test comment says it uses a “properly diverging otherwise” to isolate gradual typing, but the test body no longer contains an otherwise at all. Update the comment to match the current test intent (or add the diverging otherwise back if that was the intended coverage).
| // Gradual typing: untyped code produces no type errors. | |
| // Note: a non-diverging otherwise block is now an error (not a type error), | |
| // so we use a properly diverging otherwise to isolate the gradual typing check. | |
| // Gradual typing: untyped code (including unannotated lambdas) produces no type errors. |
- Don't leak synthetic _destructure_N names into scope; only bind pattern variables for destructured params (interpreter + typechecker) - Avoid unnecessary value.clone() on non-destructured params - Rename test_otherwise_without_return_warns → _errors to match behavior - Fix stale test comment about diverging otherwise block
- Mark destructuring in function parameters as complete - Mark otherwise block divergence as lint error (upgraded from warning) - Mark circular import diagnostics as complete - Add function parameter destructuring examples to AI_AGENT_GUIDE - Add circular import detection note to type checker docs - Sync CLAUDE.md and copilot-instructions.md
v0.4.1 Release
Destructuring in Function Parameters
Pattern destructuring now works directly in function parameter lists, consistent with existing destructuring in
letbindings,match, andforloops.Implementation:
ast.rs: optionalpatternfield onParameterparser.rs:parse_parameters()detects{/[and callsparse_pattern()to build a synthetic parameter with an attached patterninterpreter.rs:call_user_function()callsbind_pattern()for destructured params; only binds pattern variables (not the synthetic name)typechecker.rs: binds pattern variables into scope; skips synthetic_destructure_NnamesCircular Import Diagnostics
When files form an import cycle, the type checker now emits a warning showing the exact cycle chain instead of silently returning empty exports.
Inspired by Zig's dependency loop diagnostics.
Implementation:
detected_cyclesfield onTypeContext, threaded through nested contexts viamem::take/restoreresolving_filesat detection point, using filenamesOtherwise Block Divergence → Lint Error
Non-diverging
otherwiseblocks are now a lint error (previously a warning). Anotherwiseblock that doesn't end withreturn,break, orcontinuewill fail type checking, preventing runtime crashes.Review Feedback Addressed
_destructure_Nparameter names no longer leak into user scopevalue.clone()removed for non-destructured paramsTest Coverage
Files Changed
src/ast.rspattern: Option<Pattern>onParametersrc/parser.rs{}/[]patterns as function paramssrc/interpreter.rssrc/typechecker.rstests/language_features_tests.rsROADMAP.mddocs/AI_AGENT_GUIDE.mdCLAUDE.md+copilot-instructions.md