Conversation
…r handling in type checker
There was a problem hiding this comment.
Pull request overview
This PR enhances the mq-check type checker's handling of union types and tuple types, with several correctness improvements. It also fixes a bug in section.mq where range was called with a single argument (which doesn't match any registered overload) instead of the correct two-argument form.
Changes:
- Type inference improvements: Union types with unresolved
Varmembers are now properly deferred;UnionandTupletypes are now deeply resolved inresolve_type; index accesses onUnion(Array, None)variables now extract the element type correctly;if-without-elsein loops now producesType::Noneinstead of a fresh type var for the false-branch. - Builtin type signature updates:
!andnotnow acceptString,Number,Array, andDictoperands (matching mq's truthy/falsy semantics); unary minus is now registered as"-"instead of"unary-". - Module and CLI fixes:
section.mq'stocfunction fixes arangecall from single-arg to two-arg form; a blank-line separator between files is removed frommq-check's CLI output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
crates/mq-lang/modules/section.mq |
Fixes range(lvl - 1) to range(0, lvl - 1) — only 2/3-arg forms exist |
crates/mq-check/src/main.rs |
Removes blank-line separator between files in multi-file output |
crates/mq-check/src/lib.rs |
Adds Union/Var cases to resolve_deferred_tuple_accesses; changes union_members_consistent_return to return Option<Type> and skip None-propagation; adds second deferred-tuple-access pass after overload resolution; defers Union binary ops |
crates/mq-check/src/infer.rs |
Adds Union and Tuple recursive resolution to resolve_type |
crates/mq-check/src/constraint.rs |
Changes if-without-else fallthrough to Type::None; defers Union binary ops instead of erroring immediately; always uses Tuple for array literals; always defers bracket access |
crates/mq-check/src/builtin.rs |
Adds String, Number, Array, Dict overloads for !/not; renames "unary-" to "-" for unary minus |
Comments suppressed due to low confidence (1)
crates/mq-check/src/builtin.rs:1482
- The
test_logical_type_errorstest was updated to expect!42,!"hello", and![1, 2, 3]to succeed, reflecting the new!overloads acceptingNumber,String, andArrayoperands. However, there are no test cases covering!none(None-propagation) or!{}(Dict operand) to verify the newly registeredDict(k,v) -> Booloverloads. Since the builtin registration adds Dict-accepting overloads for both!andnot, test coverage for those cases would strengthen confidence in the change.
#[rstest]
#[case::not_number("!42", true)] // number is truthy/falsy → bool
#[case::not_string("!\"hello\"", true)] // string is truthy/falsy → bool
#[case::not_array("![1, 2, 3]", true)] // array is truthy/falsy → bool
fn test_logical_type_errors(#[case] code: &str, #[case] should_succeed: bool) {
let result = check_types(code);
assert_eq!(
result.is_empty(),
should_succeed,
"Code: {}\nResult: {:?}",
code,
result
);
}
| // Skip None-propagation overloads: a None input producing a None output | ||
| // is the standard mq "propagate None" pattern and does not affect the | ||
| // consistency of the return type for non-None inputs. | ||
| if matches!(member, types::Type::None) && matches!(resolved_ret, types::Type::None) { | ||
| continue; | ||
| } | ||
|
|
||
| match &unique_ret { | ||
| None => unique_ret = Some(resolved_ret), | ||
| Some(prev) if prev == &resolved_ret => {} | ||
| _ => return false, | ||
| _ => return None, | ||
| } | ||
| } | ||
| } | ||
| unique_ret.is_some() | ||
| unique_ret | ||
| } |
There was a problem hiding this comment.
The new None-propagation skip in union_members_consistent_return (where None → None overloads are ignored) enables patterns like len(Union(Array, None)) to type-check successfully with return type Number. There are no integration tests for this specific new behavior path (function call with Union(T, None) argument where T overload returns a consistent type but None overload returns None). The existing len_on_string_or_array test exercises Union(String, Array) but not Union(Array, None). Adding a test like let x = if (true): [1,2] else: none; | len(x) would directly cover this new code path.
| types::Type::Union(members) => { | ||
| // Union type (e.g., `Union(Array(String), None)`) — extract element types | ||
| // from Array/Tuple members and use them as the index access result. | ||
| // If any union member is still an unresolved Var, re-queue this access | ||
| // for the next pass (after `resolve_deferred_overloads` has run). | ||
| let has_var_member = members.iter().any(|m| m.is_var()); | ||
| if has_var_member { | ||
| ctx.add_deferred_tuple_access(access.clone()); | ||
| continue; | ||
| } | ||
|
|
||
| let mut elem_types = Vec::new(); | ||
| for member in members { | ||
| match member { | ||
| types::Type::Array(elem) => elem_types.push(*elem.clone()), | ||
| types::Type::Tuple(elems) => { | ||
| if let Some(idx) = access.index { | ||
| if idx < elems.len() { | ||
| elem_types.push(elems[idx].clone()); | ||
| } | ||
| } else { | ||
| elem_types.extend(elems.iter().cloned()); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| if !elem_types.is_empty() { | ||
| let result_ty = if elem_types.len() == 1 { | ||
| elem_types.remove(0) | ||
| } else { | ||
| types::Type::union(elem_types) | ||
| }; | ||
| let call_ty = ctx.get_or_create_symbol_type(access.call_symbol_id); | ||
| ctx.add_constraint(constraint::Constraint::Equal(call_ty, result_ty, None)); | ||
| resolved_any = true; | ||
| } |
There was a problem hiding this comment.
The new Union branch in resolve_deferred_tuple_accesses handles Union(Array(String), None) by extracting String from the Array member and ignoring None, then setting the result type to String. This new behavior has no integration test coverage. A test exercising index access on a union-typed variable (e.g., let x = if (true): [1, 2] else: none; | x[0]) should be added to verify this path works correctly.
| // path returns None (no else in mq returns None). Add None to the break type | ||
| // list so the loop type becomes Union(break_value_type, None). | ||
| if !has_explicit_else && found_break { | ||
| result.push(Type::Var(ctx.fresh_var())); | ||
| result.push(Type::None); | ||
| } |
There was a problem hiding this comment.
The change from using a fresh Type::Var to Type::None for the implicit else-branch of an if without else inside a loop is semantically correct — in mq, if (cond): value without else evaluates to None when the condition is false. However, the existing test at line 965-968 (loop_if_only_break_with_add) only checks that an error IS produced, not what the produced union type is. It would be worth adding a test that checks loop: if (true): break: 42; produces Union(Number, None) rather than Union(Number, Var), to verify the exact type inference change and catch regressions.
crates/mq-check/src/constraint.rs
Outdated
| if (!all_same && concrete.len() >= 2) || (has_none_branch && resolved.len() >= 2) { | ||
| // Different concrete types in branches — use Union type | ||
| let unique_types: Vec<Type> = concrete.into_iter().cloned().collect(); | ||
| if unique_types.is_empty() { | ||
| ctx.set_symbol_type(symbol_id, then_ty.clone()); | ||
| } else { | ||
| let union_ty = Type::union(unique_types); | ||
| ctx.set_symbol_type(symbol_id, union_ty); | ||
| } | ||
| // Different concrete types across branches — use Union type. | ||
| // Include ALL resolved branch types (vars and concrete) so that | ||
| // branches whose types are not yet fully resolved can still be | ||
| // tracked and resolved later (e.g., `if (...): items else: None` | ||
| // where `items` is still a Var at constraint-generation time). | ||
| let union_ty = Type::union(resolved.clone()); | ||
| ctx.set_symbol_type(symbol_id, union_ty); |
There was a problem hiding this comment.
The new behavior at line 1622 includes ALL resolved branch types (including Vars) in the union: Type::union(resolved.clone()). Previously, only concrete types (non-Var) were included in the union. Including Vars in a union means the union might contain Var(N) entries that will be resolved later by unification, but if they remain unresolved, downstream code will see a union with a Var member — which is intentionally detected by the union_has_var guard in resolve_deferred_overloads (line 570-580). This is a deliberate design, but the interaction between this code and the union_has_var check could cause those deferred overloads to keep getting deferred if the Var inside the union never resolves. The comment explains the intention but it would be useful to have a test specifically for the case where items is a Var at constraint-generation time.
| if matches!(member, types::Type::None) && matches!(resolved_ret, types::Type::None) { | ||
| continue; | ||
| } | ||
|
|
||
| match &unique_ret { | ||
| None => unique_ret = Some(resolved_ret), | ||
| Some(prev) if prev == &resolved_ret => {} | ||
| _ => return false, | ||
| _ => return None, | ||
| } | ||
| } | ||
| } | ||
| unique_ret.is_some() | ||
| unique_ret |
There was a problem hiding this comment.
When a union contains only None members (e.g., Union(None, None)), all iterations of the inner for member in members loop will hit the continue branch (line 524-526), leaving unique_ret as None. The function then returns None, causing the caller in resolve_deferred_overloads to fall through to the error-reporting path ("union types cannot be used with binary operators"). This edge case is unlikely in practice but could produce a confusing error if a union somehow resolves to all-None members. Consider handling this by returning a sentinel Some(Type::None) when only None-propagating members are found.
| _ => {} | ||
| } | ||
| } | ||
| if !elem_types.is_empty() { | ||
| let result_ty = if elem_types.len() == 1 { | ||
| elem_types.remove(0) | ||
| } else { | ||
| types::Type::union(elem_types) | ||
| }; | ||
| let call_ty = ctx.get_or_create_symbol_type(access.call_symbol_id); | ||
| ctx.add_constraint(constraint::Constraint::Equal(call_ty, result_ty, None)); | ||
| resolved_any = true; | ||
| } |
There was a problem hiding this comment.
The new Union arm in resolve_deferred_tuple_accesses handles Union types by extracting element types from Array and Tuple members, but silently skips other member types (e.g., None, String, Number). If the union is Union(Array(String), None), the None member is ignored and the result type is String (the element type of the array). This is probably intentional behaviour for None-propagation, but it is not documented. Consider adding a comment to explain why non-array/non-tuple members (especially None) are skipped.
Co-authored-by: Copilot <[email protected]>
|
|
||
| // Unary minus: number -> number | ||
| register_unary(ctx, "unary-", Type::Number, Type::Number); | ||
| register_unary(ctx, "-", Type::Number, Type::Number); |
There was a problem hiding this comment.
The function header comment at line 196 still lists unary- in the operator list ("Logical operators: &&, ||, !, and, or, not, unary-, negate"), but the unary minus operator is now registered as "-" instead of "unary-". The comment should be updated to reflect this change.
crates/mq-check/src/infer.rs
Outdated
| Type::Union(members) => Type::union(members.iter().map(|m| self.resolve_type(m)).collect()), | ||
| Type::Tuple(elems) => Type::Tuple(elems.iter().map(|e| self.resolve_type(e)).collect()), |
There was a problem hiding this comment.
The new resolve_type handling for Union and Tuple types (lines 511-512) has no unit tests covering the new behavior. The existing test_resolve_type test only verifies simple variable chains (e.g., Var1 → Var2 → Number), not Union(Var, None) or Tuple(Var, String) resolution. A unit test verifying that type variables embedded within union members are correctly resolved after being bound in the substitution map would prevent regressions in this new code path.
| types::Type::Union(members) => { | ||
| // Union type (e.g., `Union(Array(String), None)`) — extract element types | ||
| // from Array/Tuple members and use them as the index access result. | ||
| // If any union member is still an unresolved Var, re-queue this access | ||
| // for the next pass (after `resolve_deferred_overloads` has run). | ||
| let has_var_member = members.iter().any(|m| m.is_var()); | ||
| if has_var_member { | ||
| ctx.add_deferred_tuple_access(access.clone()); | ||
| continue; | ||
| } | ||
|
|
||
| let mut elem_types = Vec::new(); | ||
| for member in members { | ||
| match member { | ||
| types::Type::Array(elem) => elem_types.push(*elem.clone()), | ||
| types::Type::Tuple(elems) => { | ||
| if let Some(idx) = access.index { | ||
| if idx < elems.len() { | ||
| elem_types.push(elems[idx].clone()); | ||
| } | ||
| } else { | ||
| elem_types.extend(elems.iter().cloned()); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| if !elem_types.is_empty() { | ||
| let result_ty = if elem_types.len() == 1 { | ||
| elem_types.remove(0) | ||
| } else { | ||
| types::Type::union(elem_types) | ||
| }; | ||
| let call_ty = ctx.get_or_create_symbol_type(access.call_symbol_id); | ||
| ctx.add_constraint(constraint::Constraint::Equal(call_ty, result_ty, None)); | ||
| resolved_any = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new Union type handling branch in resolve_deferred_tuple_accesses (lines 423-460) has no integration test covering index access on a Union(Array(T), None) variable. Per the codebase's testing conventions, all new behavior paths must have corresponding test cases.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: harehare <[email protected]>
… format Co-authored-by: harehare <[email protected]>
Add tests for Union(Array(T), None) index access in type checker
| let type_check_config = mq_check::TypeCheckerOptions { | ||
| strict_array: cli.type_check.strict_array, | ||
| tuple: cli.type_check.tuple, | ||
| }; |
There was a problem hiding this comment.
The TypeCheckArgs struct still has a tuple: bool field with its --tuple CLI flag declaration, but after this PR removed tuple from TypeCheckerOptions, the parsed value at cli.type_check.tuple is never used. This means mq-lsp continues to accept a --tuple argument on the command line, but it silently has no effect — a misleading API that could confuse users or integrators who set the flag expecting it to do something. The tuple field and its #[arg(...)] attribute and doc comment should also be removed from TypeCheckArgs.
| /// Helper function to run type checker with tuple mode enabled | ||
| fn check_types_tuple(code: &str) -> Vec<TypeError> { | ||
| let hir = create_hir(code); | ||
| let mut checker = TypeChecker::with_options(TypeCheckerOptions { | ||
| strict_array: false, | ||
| tuple: true, | ||
| }); | ||
| let mut checker = TypeChecker::with_options(TypeCheckerOptions { strict_array: false }); | ||
| checker.check(&hir) | ||
| } |
There was a problem hiding this comment.
The check_types_tuple helper function is now identical to the default check_types function — both create a TypeChecker::with_options(TypeCheckerOptions { strict_array: false }) which is equivalent to TypeChecker::new(). Since tuple typing is now always-on (the tuple flag has been removed), the "Tuple Mode Tests" section (using check_types_tuple) no longer tests any distinct behavior. The comment in the helper function should reflect that it is now the same as the default checker, or all tests using check_types_tuple should be migrated to use check_types directly to eliminate the confusion.
…ved type inference
…cess and improve deferred selector handling
| .iter() | ||
| .filter(|e| matches!(e, LspError::SyntaxError(_))) | ||
| .collect::<Vec<_>>() | ||
| .is_empty() |
There was a problem hiding this comment.
The check at lines 207-210 uses .filter(...).collect::<Vec<_>>().is_empty() to test whether any syntax errors exist. This unnecessarily allocates a full intermediate vector. A simpler and more idiomatic version is to use .any(|e| matches!(e, LspError::SyntaxError(_))) directly on the iterator, which avoids the allocation and the need to negate the condition.
No description provided.