Conversation
…ous arrays and add related tests
There was a problem hiding this comment.
Pull request overview
Adds a “strict array” typechecking mode to enforce homogeneous array literals, wires it through the mq-check CLI/library, and expands the test suite to cover the new behavior and array indexing propagation.
Changes:
- Introduces
TypeCheckerOptions { strict_array: bool }and plumbs it through inference/constraint generation, emitting a newTypeError::HeterogeneousArray. - Adds CLI support for
--strict-arrayand updates error rendering for the new type error. - Renames the crate/binary from
mq-typechecker/mq-typechecktomq-check, and updates tests/imports accordingly.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mq-typechecker/tests/type_errors_test.rs | Adds strict-array mode tests and array element access propagation tests; updates imports to mq_check. |
| crates/mq-typechecker/tests/integration_test.rs | Updates imports to mq_check. |
| crates/mq-typechecker/tests/error_location_test.rs | Updates imports to mq_check. |
| crates/mq-typechecker/src/main.rs | Adds --strict-array CLI flag and passes option into typechecker; prints new heterogeneous-array error. |
| crates/mq-typechecker/src/lib.rs | Adds TypeCheckerOptions, TypeChecker::with_options, and TypeError::HeterogeneousArray; passes strict flag into inference context. |
| crates/mq-typechecker/src/infer.rs | Adds strict-array option storage/accessor on InferenceContext. |
| crates/mq-typechecker/src/constraint.rs | Emits HeterogeneousArray in strict mode; changes bracket access constraints to propagate element types. |
| crates/mq-typechecker/README.md | Renames crate in docs/examples to mq-check. |
| crates/mq-typechecker/Cargo.toml | Renames package and binary to mq-check. |
| Cargo.lock | Updates lockfile for renamed package entry. |
| ```rust | ||
| use mq_hir::Hir; | ||
| use mq_typechecker::TypeChecker; | ||
| use mq_check::TypeChecker; | ||
|
|
||
| // Build HIR from mq code |
There was a problem hiding this comment.
The README updates the crate name but doesn’t document the new strict array feature (CLI flag --strict-array and the TypeCheckerOptions { strict_array: ... } library API). Since this is a user-facing behavior change, please add a short example showing how to enable strict array mode via both the CLI and library usage.
| @@ -16,7 +16,7 @@ version = "0.5.17" | |||
| cli = ["dep:clap", "dep:colored", "dep:url"] | |||
|
|
|||
| [[bin]] | |||
| name = "mq-typecheck" | |||
| name = "mq-check" | |||
| path = "src/main.rs" | |||
There was a problem hiding this comment.
The PR title focuses on strict array mode, but this change also renames the crate/package and binary from mq-typechecker/mq-typecheck to mq-check. If the rename is intentional, it should be called out explicitly in the PR title/description (it’s a breaking/user-facing change for anyone invoking the binary or depending on the crate name).
| // Add structural constraint: var_type = Array(elem_type) | ||
| // so that element type propagates through unification. | ||
| // This avoids deferred overload resolution issues where | ||
| // the `get` builtin return type Var doesn't get bound. | ||
| let elem_var = ctx.fresh_var(); | ||
| let elem_ty = Type::Var(elem_var); | ||
| ctx.add_constraint(Constraint::Equal( | ||
| original_func_ty.clone(), | ||
| Type::array(elem_ty.clone()), | ||
| range, | ||
| )); | ||
| if let Some(index_ty) = explicit_arg_tys.first() { | ||
| ctx.add_constraint(Constraint::Equal( | ||
| index_ty.clone(), | ||
| Type::Number, | ||
| range, | ||
| )); | ||
| } |
There was a problem hiding this comment.
The new “Non-function variable with bracket access” handling forces the target’s type to Array(elem_type). This will incorrectly type d["a"] (dict indexing with a string key, which is valid syntax in the language) as an array, causing spurious type errors. Consider deriving the container constraint from the index type (number => array, string/symbol => dict/record), or falling back to the existing get-style behavior rather than unconditionally assuming array indexing here.
| // Add structural constraint: var_type = Array(elem_type) | |
| // so that element type propagates through unification. | |
| // This avoids deferred overload resolution issues where | |
| // the `get` builtin return type Var doesn't get bound. | |
| let elem_var = ctx.fresh_var(); | |
| let elem_ty = Type::Var(elem_var); | |
| ctx.add_constraint(Constraint::Equal( | |
| original_func_ty.clone(), | |
| Type::array(elem_ty.clone()), | |
| range, | |
| )); | |
| if let Some(index_ty) = explicit_arg_tys.first() { | |
| ctx.add_constraint(Constraint::Equal( | |
| index_ty.clone(), | |
| Type::Number, | |
| range, | |
| )); | |
| } | |
| // When the index is clearly numeric, treat this as array | |
| // indexing and add a structural constraint: | |
| // var_type = Array(elem_type) | |
| // so that element type propagates through unification. | |
| // For non-numeric indices (e.g., string/symbol keys used | |
| // for dict/record-style access), avoid forcing the | |
| // container to be an array or the index to be a number. | |
| let elem_var = ctx.fresh_var(); | |
| let elem_ty = Type::Var(elem_var); | |
| if let Some(index_ty) = explicit_arg_tys.first() { | |
| // Only impose array + numeric index constraints when | |
| // the index type is known to be Number. This prevents | |
| // mis-typing expressions like d["a"] as array access. | |
| if matches!(index_ty, Type::Number) { | |
| ctx.add_constraint(Constraint::Equal( | |
| original_func_ty.clone(), | |
| Type::array(elem_ty.clone()), | |
| range, | |
| )); | |
| ctx.add_constraint(Constraint::Equal( | |
| index_ty.clone(), | |
| Type::Number, | |
| range, | |
| )); | |
| } | |
| } | |
| // Regardless of the container/index shape, the bracket | |
| // expression itself has some element type. |
| // Add structural constraint: var_type = Array(elem_type) | ||
| // so that element type propagates through unification. | ||
| // This avoids deferred overload resolution issues where | ||
| // the `get` builtin return type Var doesn't get bound. | ||
| let elem_var = ctx.fresh_var(); | ||
| let elem_ty = Type::Var(elem_var); | ||
| ctx.add_constraint(Constraint::Equal( | ||
| original_func_ty.clone(), | ||
| Type::array(elem_ty.clone()), | ||
| range, | ||
| )); | ||
| if let Some(index_ty) = explicit_arg_tys.first() { | ||
| ctx.add_constraint(Constraint::Equal( | ||
| index_ty.clone(), | ||
| Type::Number, | ||
| range, | ||
| )); |
There was a problem hiding this comment.
This branch constrains the bracket index argument to Number. That breaks valid dict/record access forms like d["a"] (string key) and potentially d[:a] when it doesn’t go through the deferred-record-access path. The index type constraint should be conditional (e.g., only for array indexing) or support both numeric and string/symbol keys depending on the inferred container type.
| // Add structural constraint: var_type = Array(elem_type) | |
| // so that element type propagates through unification. | |
| // This avoids deferred overload resolution issues where | |
| // the `get` builtin return type Var doesn't get bound. | |
| let elem_var = ctx.fresh_var(); | |
| let elem_ty = Type::Var(elem_var); | |
| ctx.add_constraint(Constraint::Equal( | |
| original_func_ty.clone(), | |
| Type::array(elem_ty.clone()), | |
| range, | |
| )); | |
| if let Some(index_ty) = explicit_arg_tys.first() { | |
| ctx.add_constraint(Constraint::Equal( | |
| index_ty.clone(), | |
| Type::Number, | |
| range, | |
| )); | |
| // | |
| // When the index is clearly numeric, treat this as an array | |
| // access and add a structural constraint: var_type = Array(elem_type) | |
| // so that the element type propagates through unification. | |
| // For non-numeric indices (e.g., dict/record access like d["a"]), | |
| // we intentionally avoid constraining the index type to Number | |
| // or the container type to Array here. | |
| let elem_var = ctx.fresh_var(); | |
| let elem_ty = Type::Var(elem_var); | |
| if let Some(index_ty) = explicit_arg_tys.first() { | |
| if matches!(index_ty, Type::Number) { | |
| ctx.add_constraint(Constraint::Equal( | |
| original_func_ty.clone(), | |
| Type::array(elem_ty.clone()), | |
| range, | |
| )); | |
| ctx.add_constraint(Constraint::Equal( | |
| index_ty.clone(), | |
| Type::Number, | |
| range, | |
| )); | |
| } |
| if is_heterogeneous || concrete_tys.len() != elem_tys.len() { | ||
| // In strict array mode, report heterogeneous arrays as errors | ||
| if is_heterogeneous && ctx.strict_array() { | ||
| let range = get_symbol_range(hir, symbol_id); | ||
| let types_str = concrete_tys |
There was a problem hiding this comment.
In strict array mode, this code still treats “partially unresolved” arrays (concrete_tys.len() != elem_tys.len()) as tuple-like and skips unifying element types. That means an array like [1, x] can later allow x to resolve to a non-number without ever producing a strict-array error. If strict mode is meant to enforce homogeneity, consider adding equality constraints among element types even when some are vars (and only error once they resolve incompatibly).
| /// Enforce homogeneous arrays (reject mixed-type arrays like [1, "hello"]) | ||
| #[arg(long)] | ||
| strict_array: bool, |
There was a problem hiding this comment.
Cargo.toml renames the binary to mq-check, but the clap metadata still declares #[command(name = "mq-typecheck", ...)], so --help/usage output will show the old name. Please update the clap name to match the new binary name (and ensure any docs/examples reflect the rename).
No description provided.