Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,8 @@ pub struct Parameter {
pub name: String,
pub type_annotation: Option<TypeExpr>,
pub default: Option<Expression>,
#[serde(default)]
pub pattern: Option<Pattern>,
}

/// Import item for selective imports: `import { foo as bar } from "module"`
Expand Down
72 changes: 71 additions & 1 deletion src/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6339,7 +6339,12 @@ impl Interpreter {
// Should not reach here due to arity check above
Value::Unit
};
func_env.borrow_mut().define(param.name.clone(), value);
func_env
.borrow_mut()
.define(param.name.clone(), value.clone());
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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());
}

Copilot uses AI. Check for mistakes.
if let Some(ref pat) = param.pattern {
self.bind_pattern(pat, &value)?;
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Suggested change
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);

Copilot uses AI. Check for mistakes.
}

// Environment is already set to func_env for contract checking and body execution
Expand Down Expand Up @@ -12017,4 +12022,69 @@ page
other => panic!("expected String('ab'), got {:?}", other),
}
}

#[test]
fn test_destructured_map_param() {
let result = eval(
r#"
fn greet({ name, email }) {
return name + " <" + email + ">"
}
greet(map { "name": "Alice", "email": "a@b.com" })
"#,
)
.unwrap();
match result {
Value::String(s) => assert_eq!(s, "Alice <a@b.com>"),
other => panic!("expected String, got {:?}", other),
}
}

#[test]
fn test_destructured_map_param_with_type() {
let result = eval(
r#"
fn greet({ name, email }: Map) -> String {
return name + " <" + email + ">"
}
greet(map { "name": "Bob", "email": "b@b.com" })
"#,
)
.unwrap();
match result {
Value::String(s) => assert_eq!(s, "Bob <b@b.com>"),
other => panic!("expected String, got {:?}", other),
}
}

#[test]
fn test_destructured_array_param() {
let result = eval(
r#"
fn first_two([a, b, ...rest]) {
return a + b
}
first_two([10, 20, 30])
"#,
)
.unwrap();
assert!(matches!(result, Value::Int(30)));
}

#[test]
fn test_destructured_param_with_regular_params() {
let result = eval(
r#"
fn process(id, { name }) {
return str(id) + ": " + name
}
process(42, map { "name": "Alice" })
"#,
)
.unwrap();
match result {
Value::String(s) => assert_eq!(s, "42: Alice"),
other => panic!("expected String, got {:?}", other),
}
}
}
12 changes: 11 additions & 1 deletion src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,15 @@ impl Parser {

if !self.check(&TokenKind::RightParen) {
loop {
let name = self.consume_identifier("Expected parameter name")?;
let (name, pattern) =
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)
Comment on lines +311 to +317
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
};

let type_annotation = if self.match_token(&[TokenKind::Colon]) {
Some(self.parse_type()?)
Expand All @@ -325,6 +333,7 @@ impl Parser {
name,
type_annotation,
default,
pattern,
});

if !self.match_token(&[TokenKind::Comma]) {
Expand Down Expand Up @@ -522,6 +531,7 @@ impl Parser {
name: param_name,
type_annotation,
default: None,
pattern: None,
});
}

Expand Down
48 changes: 33 additions & 15 deletions src/typechecker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -744,10 +744,14 @@ impl TypeContext {

// When otherwise is present, unwrap Result<T,E> -> T or Option<T> -> T
let inferred = if let Some(otherwise_block) = otherwise {
// Check that the otherwise block diverges
// Check that the otherwise block diverges.
// This is an error, not a warning: non-diverging otherwise blocks
// always crash at runtime ("otherwise block must diverge"), so
// catching this at lint time prevents production outages.
// See Finding #76: production outage from silent runtime error.
if !self.block_diverges(otherwise_block) {
let line = self.find_line_near("otherwise");
self.warning(
self.error(
"otherwise block does not diverge — it must end with return, break, or continue".to_string(),
line,
Some("Add a return, break, or continue statement".to_string()),
Expand Down Expand Up @@ -838,7 +842,7 @@ impl TypeContext {
if self.strict_lint {
let fn_line = self.find_line_near(&format!("fn {}", name));
for param in params {
if param.type_annotation.is_none() {
if param.type_annotation.is_none() && param.pattern.is_none() {
self.emit_with_kind(
Severity::Warning,
DiagnosticKind::MissingParamAnnotation,
Expand Down Expand Up @@ -869,7 +873,10 @@ impl TypeContext {
.as_ref()
.map(|t| self.resolve_type_expr(t))
.unwrap_or(Type::Any);
self.bind(&param.name, typ);
self.bind(&param.name, typ.clone());
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type binding for destructured parameters currently also binds the synthetic parameter name (e.g., _destructure_0) into the function scope via self.bind(&param.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).

Suggested change
self.bind(&param.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(&param.name, typ.clone());
}

Copilot uses AI. Check for mistakes.
if let Some(ref pat) = param.pattern {
self.bind_pattern(pat, &typ);
}
}

// Set expected return type
Expand Down Expand Up @@ -1941,7 +1948,7 @@ impl TypeContext {
// Strict lint: warn about untyped lambda parameters
if self.strict_lint {
for param in params {
if param.type_annotation.is_none() {
if param.type_annotation.is_none() && param.pattern.is_none() {
let line = self.find_line_near(&param.name);
self.emit_with_kind(
Severity::Warning,
Expand All @@ -1966,6 +1973,9 @@ impl TypeContext {
.map(|t| self.resolve_type_expr(t))
.unwrap_or(Type::Any);
self.bind(&p.name, typ.clone());
if let Some(ref pat) = p.pattern {
self.bind_pattern(pat, &typ);
}
typ
})
.collect();
Expand Down Expand Up @@ -5121,21 +5131,30 @@ mod tests {

#[test]
fn test_otherwise_without_return_warns() {
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
fn test_otherwise_without_return_warns() {
fn test_otherwise_without_return_errors() {

Copilot uses AI. Check for mistakes.
let warnings = check_warnings(
// Non-diverging otherwise blocks are now errors (not warnings) since they
// always crash at runtime — catching this at lint time prevents outages.
let diags = check(
r#"
let x = Some(42) otherwise {
let y = 1
}
"#,
);
let otherwise_warnings: Vec<_> = warnings
let otherwise_diags: Vec<_> = diags
.iter()
.filter(|w| w.message.contains("otherwise"))
.filter(|d| d.message.contains("otherwise"))
.collect();
assert!(
!otherwise_warnings.is_empty(),
"otherwise block without return should warn: {:?}",
warnings
!otherwise_diags.is_empty(),
"otherwise block without return should produce a diagnostic: {:?}",
diags
);
assert!(
otherwise_diags
.iter()
.any(|d| d.severity == Severity::Error),
"otherwise block without return should be an error: {:?}",
otherwise_diags
);
}

Expand Down Expand Up @@ -5226,17 +5245,16 @@ mod tests {

#[test]
fn test_phase2_gradual_preserved() {
// 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.
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
let diags = check(
r#"
let val = Some(42) otherwise {
let x = 1
}
fn foo(a) {
let b = fn(x) { x + 1 }
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
"#,
);
// Only the otherwise warning, no type errors
let errors: Vec<_> = diags
.iter()
.filter(|d| d.severity == Severity::Error)
Expand Down
59 changes: 59 additions & 0 deletions tests/language_features_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1698,6 +1698,65 @@ for [num, word] in pairs {
assert_eq!(lines[2], "3: three");
}

// ============================================================================
// Destructuring in Function Parameters
// ============================================================================

#[test]
fn test_destructured_map_param() {
let code = r#"
fn greet({ name, email }) {
return name + " <" + email + ">"
}
print(greet(map { "name": "Alice", "email": "a@b.com" }))
"#;
let (stdout, _, exit_code) = run_ntnt_code(code);
assert_eq!(exit_code, 0, "Destructured map param should work");
assert!(stdout.trim().contains("Alice <a@b.com>"));
}

#[test]
fn test_destructured_map_param_with_type() {
let code = r#"
fn greet({ name, email }: Map) -> String {
return name + " <" + email + ">"
}
print(greet(map { "name": "Bob", "email": "b@b.com" }))
"#;
let (stdout, _, exit_code) = run_ntnt_code(code);
assert_eq!(exit_code, 0, "Destructured map param with type should work");
assert!(stdout.trim().contains("Bob <b@b.com>"));
}

#[test]
fn test_destructured_array_param() {
let code = r#"
fn sum_first_two([a, b, ...rest]) {
return a + b
}
print(sum_first_two([10, 20, 30]))
"#;
let (stdout, _, exit_code) = run_ntnt_code(code);
assert_eq!(exit_code, 0, "Destructured array param should work");
assert!(stdout.trim().contains("30"));
}

#[test]
fn test_destructured_param_with_regular_params() {
let code = r#"
fn process(id, { name }) {
return str(id) + ": " + name
}
print(process(42, map { "name": "Alice" }))
"#;
let (stdout, _, exit_code) = run_ntnt_code(code);
assert_eq!(
exit_code, 0,
"Destructured param mixed with regular params should work"
);
assert!(stdout.trim().contains("42: Alice"));
}

// ============================================================================
// Higher-Order Functions: filter, transform
// ============================================================================
Expand Down
Loading