♻️ refactor(module): introduce ModuleIO trait for testable module loading #829

harehare · 2025-11-09T06:57:25Z

Abstracts file system operations from the module loader by introducing a ModuleIO trait, making the module system more testable and flexible.

Changes:

Add ModuleIO trait to abstract module I/O operations
Implement FsModuleIO for file system-based module loading
Make Engine and ModuleLoader generic over ModuleIO
Add DefaultEngine and DefaultModuleLoader type aliases
Rename set_paths to set_search_paths for clarity
Update all examples, benchmarks, and tests to use DefaultEngine
Remove unused mq-macros crate and rule110.rs example
Update documentation comments to reflect new API

This refactoring enables easier testing with mock implementations and provides a cleaner separation of concerns between module loading logic and I/O operations.

…ding Abstracts file system operations from the module loader by introducing a ModuleIO trait, making the module system more testable and flexible. Changes: - Add ModuleIO trait to abstract module I/O operations - Implement FsModuleIO for file system-based module loading - Make Engine and ModuleLoader generic over ModuleIO - Add DefaultEngine and DefaultModuleLoader type aliases - Rename set_paths to set_search_paths for clarity - Update all examples, benchmarks, and tests to use DefaultEngine - Remove unused mq-macros crate and rule110.rs example - Update documentation comments to reflect new API This refactoring enables easier testing with mock implementations and provides a cleaner separation of concerns between module loading logic and I/O operations.

Copilot

Pull Request Overview

This PR refactors the module system to improve testability by introducing a ModuleIO trait that abstracts file system operations. The changes make the Engine and ModuleLoader generic over ModuleIO, while providing DefaultEngine and DefaultModuleLoader type aliases that use the default file system implementation.

Key changes include:

Introduction of ModuleIO trait with FsModuleIO as the default implementation
Making Engine and related types generic over ModuleIO
Renaming set_paths to set_search_paths for clarity
Updating all usage sites to use DefaultEngine instead of Engine
Removing unused mq-macros crate and rule110.rs example

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`crates/mq-lang/src/eval/module.rs`	Introduces `ModuleIO` trait and `FsModuleIO` implementation, refactors `ModuleLoader` to be generic
`crates/mq-lang/src/eval.rs`	Makes `Evaluator` generic over `ModuleIO`
`crates/mq-lang/src/engine.rs`	Makes `Engine` generic over `ModuleIO`, adds `new()` constructor, renames `set_paths` to `set_search_paths`
`crates/mq-lang/src/lib.rs`	Adds `DefaultEngine` and `DefaultModuleLoader` type aliases, updates exports
`crates/mq-lang/src/error.rs`	Updates error handling to work with generic `ModuleLoader`
`crates/mq-lang/src/eval/debugger.rs`	Updates `Evaluator` implementation to be generic
`crates/mq-lang/src/optimizer.rs`	Updates tests to use explicit `Engine` type annotations
`crates/mq-lang/tests/integration_tests.rs`	Updates fixture return types to use `DefaultEngine`
`crates/mq-repl/src/repl.rs`	Updates to use `DefaultEngine`
`crates/mq-repl/src/command_context.rs`	Updates to use `DefaultEngine` throughout
`crates/mq-cli/src/cli.rs`	Updates to use `DefaultEngine` and `set_search_paths`
`crates/mq-cli/src/debugger.rs`	Updates to use `DefaultEngine`
`crates/mq-dap/src/adapter.rs`	Updates to use `DefaultEngine`
`crates/mq-dap/src/executor.rs`	Updates to use `DefaultEngine`
`crates/mq-hir/src/hir.rs`	Updates to use `FsModuleIO::new()` and `BUILTIN_MODULE_FILE`
`crates/mq-python/src/lib.rs`	Updates to use `DefaultEngine`
`crates/mq-mcp/src/server.rs`	Updates to use `DefaultEngine`
`crates/mq-wasm/src/script.rs`	Updates to use `DefaultEngine`
`crates/mq-web-api/src/api.rs`	Updates to use `DefaultEngine`
`crates/mq-lsp/src/execute_command.rs`	Updates to use `DefaultEngine`
`crates/mq-crawler/src/crawler.rs`	Updates to use `DefaultEngine`
`crates/mq-c-api/src/lib.rs`	Updates to use `DefaultEngine`
`crates/mq-lang/examples/*.rs`	Updates all examples to use `DefaultEngine`
`crates/mq-lang/benches/benchmark.rs`	Updates benchmarks to use `DefaultEngine`
`crates/mq-macros/src/lib.rs`	Removes entire unused crate
`crates/mq-macros/tests/integration_tests.rs`	Removes tests for unused crate
`crates/mq-lang/examples/rule110.rs`	Removes unused example file
`crates/mq-lang/README.md`	Updates documentation to use `DefaultEngine`
`fuzz/fuzz_targets/interpreter.rs`	Updates to use `DefaultEngine`

crates/mq-lang/src/eval/module.rs

Copilot · 2025-11-09T06:58:08Z

crates/mq-lang/src/optimizer.rs

            prop_assume!(program.is_ok());

-            let mut engine = crate::Engine::default();
+            let mut engine: Engine = Engine::default();


The type annotation : Engine is redundant since Engine::default() already specifies the concrete type. Remove the explicit type annotation.

Suggested change

let mut engine: Engine = Engine::default();

let mut engine = Engine::default();

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

codspeed-hq · 2025-11-09T07:07:13Z

CodSpeed Performance Report

Merging #829 will not alter performance

_{Comparing refactor/module-io-abstraction (3c612ff) with main (56d2242)}

Summary

✅ 16 untouched

Copilot

Pull Request Overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

crates/mq-lang/src/eval/module.rs

Copilot · 2025-11-09T07:07:33Z

crates/mq-lang/src/optimizer.rs

            expr_str in arb_nested_arithmetic_expr()
        ) {
-            let mut engine = crate::Engine::default();
+            let mut engine: Engine = Engine::default();


[nitpick] Inconsistent type annotation style. Some tests use DefaultEngine::default() while others use Engine::<FsModuleIO>::default() explicitly. Consider using DefaultEngine consistently for clarity.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Improved string handling and reduced function call overhead: CSV module (csv.mq): - Remove redundant to_array() conversions for direct string indexing - Replace join() with slice notation for string extraction - Cache length calculations to avoid repeated computations YAML module (yaml.mq): - Remove unnecessary to_array() calls for first() function - Leverage native string indexing These changes improve parsing performance while maintaining full compatibility with existing functionality.

Copilot

Pull Request Overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-09T09:22:38Z

crates/mq-lang/src/optimizer.rs

            expr_str in arb_div_mod_expr()
        ) {
-            let mut engine = crate::Engine::default();
+            let mut engine= DefaultEngine::default();


Missing space after engine before the = operator.

Copilot · 2025-11-09T09:22:38Z

crates/mq-lang/modules/yaml.mq

      _parse_yaml_boolean(trimmed)
    # number
-    elif (is_digit(trimmed) || first(to_array(trimmed)) == "-" || first(to_array(trimmed)) == "+"):
+    elif (is_digit(trimmed) || first(trimmed) == "-" || first(trimmed) == "+"):


The change from first(to_array(trimmed)) to first(trimmed) suggests that first() now works directly on strings. If this is a new feature, ensure it's consistently applied throughout the codebase and properly documented. If first() was always compatible with strings, this is a good simplification.

Copilot · 2025-11-09T09:22:38Z

crates/mq-lang/modules/csv.mq

                        end
            | let result = if (is_none(result)): end_pos else: result
-            | let field_content = join(input[pos:result], "")
+            | let field_content = line[pos:result]


The change from join(input[pos:result], \"\") to line[pos:result] suggests that slicing now returns a string directly instead of an array. Verify that this behavior is correct and that slice operations on strings are properly documented and tested throughout the codebase.

Suggested change

| let field_content = line[pos:result]

| let field_content = join(line[pos:result], "")

Copilot

Pull Request Overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 1 comment.

Copilot · 2025-11-09T13:37:49Z

crates/mq-lang/src/lib.rs

        let code = "add(\"world!\")";
        let input = mq_markdown::Markdown::from_markdown_str("Hello,").unwrap();
-        let mut engine = Engine::default();
+        let mut engine: Engine = Engine::default();


[nitpick] Redundant type annotation. The type Engine can be inferred from Engine::default(). Consider removing : Engine or using DefaultEngine for clarity.

Suggested change

let mut engine: Engine = Engine::default();

let mut engine = Engine::default();

Copilot

Pull Request Overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 4 comments.

Copilot · 2025-11-09T13:54:25Z

crates/mq-lang/src/module/resolver.rs

+            let path = origin
+                .clone()
+                .map(|p| {
+                    path.replace("$ORIGIN", p.to_str().unwrap_or(""))
+                        .replace("$HOME", home.as_str())
+                })
+                .unwrap_or_else(|| home.clone());
+
+            PathBuf::from(path).join(module_name(name).as_ref())


The path variable is a String, but PathBuf::from(path) will interpret it incorrectly. The replacements for $ORIGIN and $HOME produce a String but should be converted to PathBuf earlier to ensure proper path handling. Consider converting the path string to PathBuf immediately after the replacement.

Suggested change

let path = origin

.clone()

.map(|p| {

path.replace("$ORIGIN", p.to_str().unwrap_or(""))

.replace("$HOME", home.as_str())

})

.unwrap_or_else(|| home.clone());

PathBuf::from(path).join(module_name(name).as_ref())

// Perform variable replacement, then convert to PathBuf immediately

let replaced = origin

.clone()

.map(|p| {

path.replace("$ORIGIN", p.to_str().unwrap_or(""))

.replace("$HOME", home.as_str())

})

.unwrap_or_else(|| path.replace("$HOME", home.as_str()));

let mut base = PathBuf::from(replaced);

base.push(module_name(name).as_ref());

base

crates/mq-lang/src/optimizer.rs

Copilot · 2025-11-09T13:54:26Z

crates/mq-lang/src/module/resolver.rs

+        .map(|p| p.to_str().unwrap_or("").to_string())
+        .unwrap_or("".to_string());


The unwrap_or(\"\") silently converts path conversion failures to empty strings, which could lead to incorrect path resolution. Consider propagating this error or using a more explicit default value.

Suggested change

.map(|p| p.to_str().unwrap_or("").to_string())

.unwrap_or("".to_string());

.ok_or_else(|| ModuleError::IOError(Cow::Borrowed("Could not determine home directory")))?

.to_str()

.ok_or_else(|| ModuleError::IOError(Cow::Borrowed("Home directory path is not valid UTF-8")))?

.to_string();

Copilot · 2025-11-09T13:54:26Z

crates/mq-lang/src/module/resolver.rs

+    let home = dirs::home_dir()
+        .map(|p| p.to_str().unwrap_or("").to_string())
+        .unwrap_or("".to_string());
+    let origin = std::env::current_dir().ok();
+
+    search_paths
+        .as_ref()
+        .map(|p| {
+            p.iter()
+                .map(|p| p.to_str().map(|p| p.to_string()).unwrap_or_default())
+                .collect::<Vec<_>>()
+        })
+        .unwrap_or_else(|| DEFAULT_PATHS.iter().map(|p| p.to_string()).collect())
+        .iter()
+        .map(|path| {
+            let path = origin
+                .clone()
+                .map(|p| {
+                    path.replace("$ORIGIN", p.to_str().unwrap_or(""))
+                        .replace("$HOME", home.as_str())
+                })
+                .unwrap_or_else(|| home.clone());
+
+            PathBuf::from(path).join(module_name(name).as_ref())
+        })
+        .find(|p| p.is_file())
+        .ok_or_else(|| ModuleError::NotFound(Cow::Owned(module_name(name).to_string())))


Path conversion failure with unwrap_or(\"\") could cause silent failures in path resolution. Consider returning an error if path conversion fails instead of using an empty string as fallback.

Suggested change

let home = dirs::home_dir()

.map(|p| p.to_str().unwrap_or("").to_string())

.unwrap_or("".to_string());

let origin = std::env::current_dir().ok();

search_paths

.as_ref()

.map(|p| {

p.iter()

.map(|p| p.to_str().map(|p| p.to_string()).unwrap_or_default())

.collect::<Vec<_>>()

})

.unwrap_or_else(|| DEFAULT_PATHS.iter().map(|p| p.to_string()).collect())

.iter()

.map(|path| {

let path = origin

.clone()

.map(|p| {

path.replace("$ORIGIN", p.to_str().unwrap_or(""))

.replace("$HOME", home.as_str())

})

.unwrap_or_else(|| home.clone());

PathBuf::from(path).join(module_name(name).as_ref())

})

.find(|p| p.is_file())

.ok_or_else(|| ModuleError::NotFound(Cow::Owned(module_name(name).to_string())))

let home_path = dirs::home_dir()

.ok_or_else(|| ModuleError::IOError(Cow::Borrowed("Could not determine home directory")))?;

let home = home_path

.to_str()

.ok_or_else(|| ModuleError::IOError(Cow::Borrowed("Home directory path is not valid UTF-8")))?;

let origin_path = std::env::current_dir()

.map_err(|_| ModuleError::IOError(Cow::Borrowed("Could not determine current directory")))?;

let origin = origin_path

.to_str()

.ok_or_else(|| ModuleError::IOError(Cow::Borrowed("Current directory path is not valid UTF-8")))?;

let paths: Vec<String> = if let Some(p) = search_paths.as_ref() {

let mut out = Vec::with_capacity(p.len());

for pb in p {

let s = pb.to_str().ok_or_else(|| ModuleError::IOError(Cow::Borrowed("Search path is not valid UTF-8")))?;

out.push(s.to_string());

}

out

} else {

DEFAULT_PATHS.iter().map(|p| p.to_string()).collect()

};

for path in &paths {

let replaced = path

.replace("$ORIGIN", origin)

.replace("$HOME", home);

let candidate = PathBuf::from(replaced).join(module_name(name).as_ref());

if candidate.is_file() {

return Ok(candidate);

}

}

Err(ModuleError::NotFound(Cow::Owned(module_name(name).to_string())))

Copilot

Pull Request Overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-09T13:58:43Z

crates/mq-lang/src/module.rs

        ($name:ident) => {
            fn $name() -> &'static str {
-                include_str!(concat!("../../modules/", stringify!($name), ".mq"))
+                include_str!(concat!("../modules/", stringify!($name), ".mq"))


The path change from ../../modules/ to ../modules/ appears incorrect. Since module.rs is in crates/mq-lang/src/, the relative path ../modules/ would resolve to crates/mq-lang/modules/, but the original ../../modules/ would resolve to crates/modules/. Verify the actual location of the module files and ensure this path is correct.

Suggested change

include_str!(concat!("../modules/", stringify!($name), ".mq"))

include_str!(concat!("../../modules/", stringify!($name), ".mq"))

Copilot · 2025-11-09T13:58:43Z

crates/mq-lang/src/module.rs

-        Self::new(None)
-    }
-}
+pub const BUILTIN_FILE: &str = include_str!("../builtin.mq");


The path change from ../../builtin.mq to ../builtin.mq appears incorrect. Since module.rs is in crates/mq-lang/src/, the relative path ../builtin.mq would resolve to crates/mq-lang/builtin.mq, but the original ../../builtin.mq would resolve to crates/builtin.mq. Verify the actual location of builtin.mq and ensure this path is correct.

Suggested change

pub const BUILTIN_FILE: &str = include_str!("../builtin.mq");

pub const BUILTIN_FILE: &str = include_str!("../../builtin.mq");

Copilot · 2025-11-09T13:58:43Z

crates/mq-lang/src/eval.rs


    #[cfg(feature = "debugger")]
-    debugger: Shared<SharedCell<Debugger>>,
+    pub(crate) debugger: Shared<SharedCell<Debugger>>,


The visibility of the debugger field changed from private to pub(crate). This increases the exposure of this field within the crate. Ensure this visibility change is intentional and necessary for the refactoring, as it may allow unintended internal access.

Suggested change

pub(crate) debugger: Shared<SharedCell<Debugger>>,

debugger: Shared<SharedCell<Debugger>>,

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-09T14:06:50Z

crates/mq-lang/src/module/resolver.rs

+        .map(|p| p.to_str().unwrap_or("").to_string())
+        .ok_or(ModuleError::IOError(Cow::Borrowed(
+            "Could not determine home directory",
+        )))?


The unwrap_or_default() call on line 69 could return an empty string for invalid UTF-8 paths, which would then be used in path operations. Consider propagating the error or returning a more descriptive error message when path conversion fails.

Suggested change

.map(|p| p.to_str().unwrap_or("").to_string())

.ok_or(ModuleError::IOError(Cow::Borrowed(

"Could not determine home directory",

)))?

.ok_or(ModuleError::IOError(Cow::Borrowed(

"Could not determine home directory",

)))?

.to_str()

.ok_or(ModuleError::IOError(Cow::Borrowed(

"Home directory path is not valid UTF-8",

)))?

Copilot · 2025-11-09T14:06:51Z

crates/mq-lang/src/module/resolver.rs

+            let path = origin
+                .clone()
+                .map(|p| {
+                    path.replace("$ORIGIN", p.to_str().unwrap_or(""))
+                        .replace("$HOME", home.as_str())
+                })
+                .unwrap_or_else(|| home.clone());
+
+            PathBuf::from(path).join(module_name(name).as_ref())
+        })
+        .find(|p| p.is_file())
+        .ok_or_else(|| ModuleError::NotFound(Cow::Owned(module_name(name).to_string())))


When path conversion fails (unwrap_or("")), an empty string is silently substituted which could lead to incorrect path resolution. Consider returning an error or logging a warning when path conversion fails.

Suggested change

let path = origin

.clone()

.map(|p| {

path.replace("$ORIGIN", p.to_str().unwrap_or(""))

.replace("$HOME", home.as_str())

})

.unwrap_or_else(|| home.clone());

PathBuf::from(path).join(module_name(name).as_ref())

})

.find(|p| p.is_file())

.ok_or_else(|| ModuleError::NotFound(Cow::Owned(module_name(name).to_string())))

let path = match origin.clone() {

Some(p) => {

let origin_str = p.to_str().ok_or_else(|| {

ModuleError::IOError(Cow::Borrowed("Current directory path is not valid UTF-8"))

})?;

path.replace("$ORIGIN", origin_str)

ModuleError::IOError(Cow::Borrowed("Current directory path is not valid UTF-8"))

})?;

path.replace("$ORIGIN", origin_str)

.replace("$HOME", home.as_str())

}

None => home.clone(),

};

};

Ok(PathBuf::from(path).join(module_name(name).as_ref()))

})

.find_map(|res| match res {

Ok(p) if p.is_file() => Some(Ok(p)),

Err(e) => Some(Err(e)),

_ => None,

})

.unwrap_or_else(|| Err(ModuleError::NotFound(Cow::Owned(module_name(name).to_string()))))

Copilot · 2025-11-09T14:06:52Z

crates/mq-lang/src/engine.rs

+use crate::{LocalFsModuleResolver, ModuleResolver, MqResult, RuntimeValue, Shared, SharedCell, token_alloc};

 use crate::{
    ModuleLoader, Token,


[nitpick] The import list should be formatted with one import per line or properly grouped for better readability. Consider using rustfmt to format this file.

…tegration

…gument handling

Copilot

Pull Request Overview

Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.

crates/mq-wasm/src/script.rs

…tency

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

Copilot reviewed 50 out of 51 changed files in this pull request and generated 4 comments.

Copilot · 2025-11-10T12:56:30Z

crates/mq-lang/src/optimizer.rs

+            // Only inline if the number of arguments matches the number of parameters
+            // If params.len() == args.len() + 1, it means implicit first argument is needed,
+            // which we cannot handle during static optimization
+            if params.len() != args.len() {
+                new_program.push(node);
+                return;
+            }


The comment on lines 397-398 describes a specific condition (params.len() == args.len() + 1) but the actual check on line 399 is more general (params.len() != args.len()). Consider clarifying the comment to explain all cases where inlining should be skipped, or adjust the check to match the comment's description.

crates/mq-lang/src/module/resolver.rs

Copilot · 2025-11-10T12:56:30Z

crates/mq-wasm/src/script.rs

+pub async fn run(code: &str, content: &str, options: JsValue) -> Result<String, JsValue> {
+    let resolver = WasmModuleResolver::new();
+    resolver.initialize().await?;
+    resolver.preload_modules().await?;


The WasmModuleResolver is created, initialized, and modules are preloaded on every call to run(). Consider caching the resolver instance across calls to avoid redundant OPFS initialization and module loading, which could significantly improve performance for repeated queries.

Copilot · 2025-11-10T12:56:30Z

crates/mq-wasm/src/script.rs

+                        .map_err(|e| JsValue::from_str(&format!("Failed to decode file '{}' as UTF-8: {}", name, e)))?;
+
+                    // Store with module name (without .mq extension)
+                    let module_name = name.strip_suffix(".mq").unwrap_or(&name);


Using unwrap_or(&name) means files without .mq extension would be stored with their full name including extension. Since line 239 filters for .mq files only, this fallback is unreachable. Consider using unwrap() with a comment explaining the invariant, or handle the case explicitly if other extensions might be supported in the future.

Suggested change

let module_name = name.strip_suffix(".mq").unwrap_or(&name);

// At this point, name is guaranteed to end with ".mq" due to the filter above.

let module_name = name.strip_suffix(".mq").unwrap();

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.

Copilot · 2025-11-10T13:11:53Z

crates/mq-wasm/src/script.rs

-pub fn to_ast(code: &str) -> Result<String, JsValue> {
-    let token_arena = mq_lang::Shared::new(mq_lang::SharedCell::new(mq_lang::Arena::new(10240)));
+pub async fn to_ast(code: &str) -> Result<String, JsValue> {
+    let token_arena = mq_lang::Shared::new(mq_lang::SharedCell::new(mq_lang::Arena::new(1024)));


The arena size changed from 10240 to 1024. This is a significant reduction (90%) that could impact performance for larger ASTs. If this is intentional, consider documenting the rationale for this change, or verify that 1024 is sufficient for typical use cases.

Suggested change

let token_arena = mq_lang::Shared::new(mq_lang::SharedCell::new(mq_lang::Arena::new(1024)));

let token_arena = mq_lang::Shared::new(mq_lang::SharedCell::new(mq_lang::Arena::new(10240)));

harehare requested a review from Copilot November 9, 2025 06:57