♻️ refactor(macro): add AST runtime value and improve macro evaluation #1027

harehare · 2025-12-27T14:02:23Z

This change refactors the macro system to properly support quote/unquote and introduces a new MacroEvaluator trait for evaluating macro bodies during expansion.

Key changes:

Add RuntimeValue::Ast variant to represent quoted expressions
Introduce MacroEvaluator trait to decouple macro expansion from evaluation
Change quote expressions to evaluate to AST values instead of erroring
Add InvalidMacroResultAst and InvalidMacroResult error types
Refactor macro collection to evaluate macro bodies at definition time
Update macro expansion to properly handle AST values
Fix tests to use proper quote blocks in macro definitions

This change refactors the macro system to properly support quote/unquote and introduces a new MacroEvaluator trait for evaluating macro bodies during expansion. Key changes: - Add RuntimeValue::Ast variant to represent quoted expressions - Introduce MacroEvaluator trait to decouple macro expansion from evaluation - Change quote expressions to evaluate to AST values instead of erroring - Add InvalidMacroResultAst and InvalidMacroResult error types - Refactor macro collection to evaluate macro bodies at definition time - Update macro expansion to properly handle AST values - Fix tests to use proper quote blocks in macro definitions

Copilot

Pull request overview

This PR refactors the macro system to properly support quote/unquote operations by introducing a new RuntimeValue::Ast variant and a MacroEvaluator trait. The changes enable macros to be evaluated at definition time and allow quoted expressions to return AST values instead of erroring.

Key changes:

Added RuntimeValue::Ast variant to represent quoted expressions as runtime values
Introduced MacroEvaluator trait to decouple macro expansion from evaluation logic
Modified quote expressions to return AST values instead of producing errors
Updated macro collection to evaluate macro bodies at definition time using the evaluator

Reviewed changes

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

Show a summary per file

File	Description
crates/mq-python/src/value.rs	Added handling for `RuntimeValue::Ast` in Python value conversion
crates/mq-lang/tests/integration_tests.rs	Commented out tests for quote expressions at runtime
crates/mq-lang/src/macro_expand.rs	Introduced `MacroEvaluator` trait and refactored macro expansion to use it; updated all expansion methods to accept evaluator parameter
crates/mq-lang/src/eval/runtime_value.rs	Added `Ast` variant to `RuntimeValue` enum with implementations for equality, ordering, display, and utility methods
crates/mq-lang/src/eval/env.rs	Added variable representation for AST runtime values
crates/mq-lang/src/eval.rs	Implemented `MacroEvaluator` trait for `Evaluator`; changed quote/macro evaluation behavior; added temporary macro_expander swapping to avoid borrow conflicts
crates/mq-lang/src/error/runtime.rs	Added `InvalidMacroResultAst` and `InvalidMacroResult` error variants
crates/mq-lang/src/error.rs	Added diagnostic messages for new macro-related error types

Copilot · 2025-12-27T14:03:46Z

crates/mq-lang/src/macro_expand.rs

+                            },
+                        );
+                    } else {
+                        unreachable!("Macro body did not evaluate to AST");


This unreachable! will panic if the macro body doesn't evaluate to an AST. Consider returning a proper error instead of using unreachable!, as the MacroEvaluator implementation might have bugs or return unexpected values. Use RuntimeError::InvalidMacroResult or similar.

Suggested change

unreachable!("Macro body did not evaluate to AST");

return Err(RuntimeError::InvalidMacroResult(

"Macro body did not evaluate to AST".to_string(),

));

Copilot · 2025-12-27T14:03:46Z

crates/mq-lang/src/eval.rs

+        // Temporarily take the macro_expander to avoid borrow conflicts
+        let mut macro_expander = std::mem::take(&mut self.macro_expander);
+        let expanded_result = macro_expander.expand(&program, self);
+        self.macro_expander = macro_expander;


This pattern of temporarily moving macro_expander to avoid borrow conflicts appears three times in this file (lines 175-177, 266-270, 438-440). Consider refactoring this into a helper method to reduce duplication and improve maintainability.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-27T14:03:47Z

crates/mq-lang/src/eval.rs

+            Ok(_) | Err(_) => {
+                // Return the body as AST, not the evaluated result
+                // This allows macros without quote to work
+                Ok(RuntimeValue::Ast(Shared::clone(body)))
+            }


Silently converting errors to AST values may hide issues during macro body evaluation. The error case should at least be logged or handled differently from the Ok(_) case to aid debugging.

crates/mq-lang/src/eval.rs

codspeed-hq · 2025-12-27T14:10:00Z

CodSpeed Performance Report

Merging #1027 will degrade performance by 12.86%

_{Comparing refactor/macro-evaluation-and-ast-support (75bf430) with main (6a5c5a0)}

Summary

❌ 1 regression
✅ 28 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Efficiency
❌	`eval_variable_assignment_chain`	830.9 µs	953.5 µs	-12.86%

Copilot · 2025-12-27T14:24:11Z

@harehare I've opened a new pull request, #1028, to work on those changes. Once the pull request is ready, I'll request review from you.

…tution scenarios

…method Co-authored-by: harehare <533078+harehare@users.noreply.github.com>

Refactor duplicated macro_expander take/restore pattern into helper method

Copilot

Pull request overview

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

crates/mq-lang/src/eval.rs

crates/mq-lang/src/error/runtime.rs

…luation

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Copilot · 2025-12-28T02:18:03Z

crates/mq-lang/src/macro_expand.rs

+                            },
+                        );
+                    } else {
+                        unreachable!("Macro body did not evaluate to AST");


This unreachable message is misleading. Based on the code logic at lines 101-116, if eval_macro_body returns a non-Ast value, this line is reached. Consider changing the message to explain that this represents an internal error where the evaluator failed to return an AST value, or convert this to a proper error return instead of unreachable.

Suggested change

unreachable!("Macro body did not evaluate to AST");

unreachable!(

"internal error: macro body evaluation did not return an AST RuntimeValue"

);

Copilot · 2025-12-28T02:18:03Z

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

+                    Self::NativeFunction(_) => Cow::Borrowed("native_function"),
+                    Self::Dict(_) => self.string(),
+                    Self::Module(module_name) => Cow::Owned(format!(r#"module "{}""#, module_name.name)),
+                    Self::Ast(_) => unreachable!(),


This unreachable is incorrect. The Ast case is already handled in the outer match at lines 247-252, but if that pattern matching logic is ever refactored, this could be reached. Additionally, this makes the code harder to understand. Consider removing the outer match arm for Ast and letting it fall through to the inner match, or add a clear comment explaining why this is truly unreachable.

Copilot · 2025-12-28T02:18:04Z

crates/mq-lang/src/eval.rs

+impl<T: ModuleResolver> MacroEvaluator for Evaluator<T> {
+    fn eval_macro_body(&mut self, body: &Shared<ast::Node>, _token_id: TokenId) -> Result<RuntimeValue, RuntimeError> {
+        // Try to evaluate the macro body
+        let value = self.eval_macro(body);


The variable value is defined but the error case in the match at line 6139 doesn't use it. The error path returns Ok(RuntimeValue::Ast(Shared::clone(body))) regardless of whether eval_macro returned an error or a non-Ast value. This could hide evaluation errors. Consider handling errors explicitly or documenting why they should be ignored.

Copilot · 2025-12-28T02:18:04Z

crates/mq-lang/src/eval.rs

+    /// Helper method to temporarily take the macro_expander to avoid borrow conflicts.
+    /// This pattern is needed when the macro_expander needs to call methods that require
+    /// mutable access to the evaluator (self).
+    ///
+    /// # Arguments
+    /// * `f` - A closure that operates on the taken macro_expander and returns a result
+    ///
+    /// # Example
+    /// ```ignore
+    /// self.with_macro_expander(|expander, evaluator| {
+    ///     expander.expand(&program, evaluator)
+    /// })?;


The documentation example shows a usage pattern that includes ?; at the end, but the actual function signature returns Result<R, RuntimeError>, so the caller should handle the Result. The example should either show proper error handling or clarify that this is示意代码. Additionally, document why this pattern is necessary (to work around Rust's borrow checker when MacroEvaluator needs &mut self).

Suggested change

/// Helper method to temporarily take the macro_expander to avoid borrow conflicts.

/// This pattern is needed when the macro_expander needs to call methods that require

/// mutable access to the evaluator (self).

///

/// # Arguments

/// * `f` - A closure that operates on the taken macro_expander and returns a result

///

/// # Example

/// ```ignore

/// self.with_macro_expander(|expander, evaluator| {

/// expander.expand(&program, evaluator)

/// })?;

/// Helper method to temporarily take the `macro_expander` to avoid borrow conflicts.

///

/// Rust's borrow checker does not allow holding `&mut self` and, at the same time,

/// a `&mut` borrow of one of its fields (here, `self.macro_expander`) across a call.

/// By `take`‑ing the `macro_expander` out of `self`, this helper lets the closure

/// borrow both `&mut Macro` and `&mut Self` safely for the duration of the call.

///

/// # Arguments

/// * `f` - A closure that operates on the taken `macro_expander` and returns a result.

///

/// # Example

/// In a method that itself returns `Result<R, RuntimeError>` you might write:

///

/// ```ignore

/// fn eval_with_macros(&mut self, program: &Program) -> Result<R, RuntimeError> {

/// self.with_macro_expander(|expander, evaluator| {

/// expander.expand(program, evaluator)

/// })

/// }

Copilot · 2025-12-28T02:18:04Z

crates/mq-lang/src/macro_expand.rs

+    /// Mock MacroEvaluator for testing.
+    /// Returns the program as-is wrapped in RuntimeValue::Ast.
+    struct MockMacroEvaluator;
+
+    impl MacroEvaluator for MockMacroEvaluator {
+        fn eval_macro_body(&mut self, body: &Shared<Node>, _token_id: TokenId) -> Result<RuntimeValue, RuntimeError> {
+            // Return the body as-is wrapped in AST


The comment says 'Returns the program as-is' but the implementation at line 1042-1045 wraps the body's expr in a new Node with a hardcoded token_id. This is not 'as-is'. Update the documentation to accurately reflect what the mock evaluator does.

Suggested change

/// Mock MacroEvaluator for testing.

/// Returns the program as-is wrapped in RuntimeValue::Ast.

struct MockMacroEvaluator;

impl MacroEvaluator for MockMacroEvaluator {

fn eval_macro_body(&mut self, body: &Shared<Node>, _token_id: TokenId) -> Result<RuntimeValue, RuntimeError> {

// Return the body as-is wrapped in AST

/// Mock `MacroEvaluator` for testing.

/// Wraps the macro body's expression in a new `Node` with a fixed `TokenId`

/// and returns it as `RuntimeValue::Ast`.

struct MockMacroEvaluator;

impl MacroEvaluator for MockMacroEvaluator {

fn eval_macro_body(&mut self, body: &Shared<Node>, _token_id: TokenId) -> Result<RuntimeValue, RuntimeError> {

// Wrap the body's expression in a new Node with a fixed TokenId and return it as AST

… values and improve macro body processing

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Copilot · 2025-12-28T13:44:13Z

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

+                    Self::NativeFunction(_) => Cow::Borrowed("native_function"),
+                    Self::Dict(_) => self.string(),
+                    Self::Module(module_name) => Cow::Owned(format!(r#"module "{}""#, module_name.name)),
+                    Self::Ast(_) => unreachable!(),


The unreachable!() here is inconsistent with the actual handling above at line 248-252 where Ast is explicitly handled. Since Ast formatting is already implemented, this unreachable should never be hit. Consider adding a comment explaining why this branch cannot be reached, or documenting that this is a defensive check.

Suggested change

Self::Ast(_) => unreachable!(),

// SAFETY: This branch is unreachable because `RuntimeValue::Ast`

// is already handled by the outer `match self { Self::Ast(node) => ... }` arm above.

// If this ever triggers, it indicates a bug in the `Display` implementation.

Self::Ast(_) => unreachable!("`RuntimeValue::Ast` should be handled in the outer Display match arm"),

Copilot · 2025-12-28T13:44:13Z

crates/mq-lang/src/macro_expand.rs

+            .expand(&program, &mut EmptyBlockMacroEvaluator)
+            .expect("Failed to expand program");
+
+        assert_eq!(macro_expander.macros.len(), 1, "Expected no macros to be registered");


The assertion message says 'Expected no macros to be registered' but the assertion checks that exactly 1 macro is registered (len() == 1). The message should be 'Expected 1 macro to be registered' to match the assertion.

Copilot · 2025-12-28T13:44:13Z

crates/mq-lang/src/macro_expand.rs

+            .expand(&program, &mut EmptyBlockMacroEvaluator)
+            .expect("Failed to expand program");
+
+        assert_eq!(macro_expander.macros.len(), 2, "Expected no macros to be registered");


The assertion message says 'Expected no macros to be registered' but the assertion checks that exactly 2 macros are registered (len() == 2). The message should be 'Expected 2 macros to be registered' to match the assertion.

Copilot · 2025-12-28T13:44:13Z

crates/mq-lang/src/macro_expand.rs

+        assert_eq!(macro_expander.macros.len(), 2, "Expected no macros to be registered");
+        assert_eq!(expanded.len(), 0, "Expected two macro definition nodes to remain");


The first assertion message says 'Expected no macros to be registered' but checks for 2 macros. The second assertion message says 'Expected two macro definition nodes to remain' but checks for 0 expanded nodes. Both messages are inconsistent with their assertions. They should be 'Expected 2 macros to be registered' and 'Expected no nodes in expanded program' respectively.

Copilot · 2025-12-28T13:44:14Z

crates/mq-lang/src/eval.rs

+        // If the result is already an AST (from quote), return it as-is
+        // If the result is None (e.g., from if(false) without else), return None to indicate removal
+        // Otherwise, wrap the body itself as an AST (for macros without quote)
+        match value {
+            Ok(RuntimeValue::Ast(ast)) => Ok(RuntimeValue::Ast(ast)),
+            Ok(RuntimeValue::None) => {
+                // Return None instead of empty block - this will cause the macro to be skipped
+                Ok(RuntimeValue::None)
+            }
+            Ok(_) | Err(_) => {
+                // Return the body as AST, not the evaluated result
+                // This allows macros without quote to work


The comment states that returning None will cause the macro to be skipped, but looking at the macro collection code in macro_expand.rs at lines 101-116, it uses an if-let pattern that would panic with 'unreachable' if None is returned. This suggests the comment may be inaccurate or the implementation is incomplete. Consider clarifying the intended behavior or handling None explicitly in the collection logic.

Suggested change

// If the result is already an AST (from quote), return it as-is

// If the result is None (e.g., from if(false) without else), return None to indicate removal

// Otherwise, wrap the body itself as an AST (for macros without quote)

match value {

Ok(RuntimeValue::Ast(ast)) => Ok(RuntimeValue::Ast(ast)),

Ok(RuntimeValue::None) => {

// Return None instead of empty block - this will cause the macro to be skipped

Ok(RuntimeValue::None)

}

Ok(_) | Err(_) => {

// Return the body as AST, not the evaluated result

// This allows macros without quote to work

// If the result is already an AST (from quote), return it as-is.

// Otherwise, ignore the evaluated value and wrap the body itself as an AST

// so that macros without quote still operate on their original syntax.

match value {

Ok(RuntimeValue::Ast(ast)) => Ok(RuntimeValue::Ast(ast)),

Ok(_) | Err(_) => {

// Return the body as AST, not the evaluated result.

// This allows macros without quote to work and avoids returning None,

// which the macro collector does not expect.

…ke syntax

…y in integration tests

Copilot

Pull request overview

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

Copilot · 2025-12-28T14:10:19Z

crates/mq-lang/src/macro_expand.rs

+                            },
+                        );
+                    } else {
+                        unreachable!("Macro body did not evaluate to AST");


The unreachable! macro should include more context about what was received instead of just stating it 'did not evaluate to AST'. Consider including the actual type or value received to aid debugging.

Suggested change

unreachable!("Macro body did not evaluate to AST");

unreachable!(

"Macro body did not evaluate to AST. Got value: {ast:?}"

);

Copilot · 2025-12-28T14:10:19Z

crates/mq-lang/src/eval.rs

+    /// * `f` - A closure that operates on the taken macro_expander and returns a result
+    ///
+    /// # Example


The documentation example shows a trailing ? operator but the actual return type from with_macro_expander already handles the Result. The example should either remove the ? or clarify that the closure returns a Result that gets propagated by the method.

Suggested change

/// * `f` - A closure that operates on the taken macro_expander and returns a result

///

/// # Example

/// * `f` - A closure that operates on the taken macro_expander and returns a

/// [`Result`] that is propagated by this method.

///

/// # Example

/// The closure's [`Result`] is propagated, so callers can use `?` at the call site:

Copilot · 2025-12-28T14:10:20Z

crates/mq-lang/src/eval.rs

+        match value {
+            Ok(RuntimeValue::Ast(ast)) => Ok(RuntimeValue::Ast(ast)),
+            Ok(RuntimeValue::None) => {
+                // Return None instead of empty block - this will cause the macro to be skipped


This comment states that returning None 'will cause the macro to be skipped', but it's unclear what 'skipped' means in this context - does it mean the macro definition is not registered, or that macro calls produce no output? The comment should clarify the specific behavior.

Suggested change

// Return None instead of empty block - this will cause the macro to be skipped

// Return None instead of an empty block: the macro expander will

// treat this as "remove this macro invocation", so the call

// contributes no AST nodes to the expanded output (the macro

// definition itself remains registered as usual).

Copilot · 2025-12-28T14:10:20Z

crates/mq-lang/src/macro_expand.rs

+        assert_eq!(macro_expander.macros.len(), 2, "Expected no macros to be registered");
+        assert_eq!(expanded.len(), 0, "Expected two macro definition nodes to remain");


Line 2543's assertion message says 'Expected no macros to be registered' but checks for 2 macros. Line 2544's message says 'Expected two macro definition nodes to remain' but checks for 0 nodes. Both messages contradict their assertions and should be corrected.

Copilot AI review requested due to automatic review settings December 27, 2025 14:02

Copilot AI reviewed Dec 27, 2025

View reviewed changes

Initial plan

c785c90

Copilot AI mentioned this pull request Dec 27, 2025

Refactor duplicated macro_expander take/restore pattern into helper method #1028

Merged

harehare and others added 3 commits December 27, 2025 23:30

✅ test(macro): add comprehensive tests for macro expansion and substi…

52b0cbf

…tution scenarios

♻️ refactor: extract macro_expander take/restore pattern into helper …

bf85b02

…method Co-authored-by: harehare <533078+harehare@users.noreply.github.com>

Merge pull request #1028 from harehare/copilot/sub-pr-1027

ccf2395

Refactor duplicated macro_expander take/restore pattern into helper method

Copilot AI review requested due to automatic review settings December 27, 2025 14:42

Copilot AI reviewed Dec 27, 2025

View reviewed changes

crates/mq-lang/src/eval.rs Show resolved Hide resolved

crates/mq-lang/src/error/runtime.rs Show resolved Hide resolved

harehare added 2 commits December 27, 2025 23:52

♻️ Refactor(runtime): enhance RuntimeValue display and AST formatting

9041a9d

♻️ refactor(eval): update eval_macro to use macro environment for eva…

c88d7af

…luation

Copilot AI review requested due to automatic review settings December 28, 2025 02:16

Copilot AI reviewed Dec 28, 2025

View reviewed changes

harehare added 2 commits December 28, 2025 22:11

♻️ refactor(eval): enhance quote/unquote evaluation to handle non-AST…

ec87b65

… values and improve macro body processing

♻️ test(macro): add tests for macro expansion with empty blocks

8b494b7

Copilot AI review requested due to automatic review settings December 28, 2025 13:43

harehare force-pushed the refactor/macro-evaluation-and-ast-support branch from 6994516 to 8b494b7 Compare December 28, 2025 13:43

Copilot AI reviewed Dec 28, 2025

View reviewed changes

harehare added 2 commits December 28, 2025 22:53

♻️ refactor(runtime): update AST node formatting to use Rust Debug-li…

912472f

…ke syntax

♻️ refactor(tests): remove commented-out cases for quote functionalit…

75bf430

…y in integration tests

Copilot AI review requested due to automatic review settings December 28, 2025 14:09

Copilot AI reviewed Dec 28, 2025

View reviewed changes

harehare merged commit 4f5010b into main Dec 28, 2025
6 of 7 checks passed

harehare deleted the refactor/macro-evaluation-and-ast-support branch December 28, 2025 14:31

BrewTestBot mentioned this pull request Dec 30, 2025

mq 0.5.9 Homebrew/homebrew-core#260671

Merged

-                        unreachable!("Macro body did not evaluate to AST");
+                        return Err(RuntimeError::InvalidMacroResult(
+                            "Macro body did not evaluate to AST".to_string(),
+                        ));

-                    Self::Ast(_) => unreachable!(),
+                    // SAFETY: This branch is unreachable because `RuntimeValue::Ast`
+                    // is already handled by the outer `match self { Self::Ast(node) => ... }` arm above.
+                    // If this ever triggers, it indicates a bug in the `Display` implementation.
+                    Self::Ast(_) => unreachable!("`RuntimeValue::Ast` should be handled in the outer Display match arm"),

		assert_eq!(macro_expander.macros.len(), 2, "Expected no macros to be registered");
		assert_eq!(expanded.len(), 0, "Expected two macro definition nodes to remain");

-                // Return None instead of empty block - this will cause the macro to be skipped
+                // Return None instead of an empty block: the macro expander will
+                // treat this as "remove this macro invocation", so the call
+                // contributes no AST nodes to the expanded output (the macro
+                // definition itself remains registered as usual).

♻️ refactor(macro): add AST runtime value and improve macro evaluation #1027

♻️ refactor(macro): add AST runtime value and improve macro evaluation #1027

Uh oh!

Conversation

harehare commented Dec 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

harehare Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codspeed-hq bot commented Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #1027 will degrade performance by 12.86%

Summary

Benchmarks breakdown

Uh oh!

Copilot AI commented Dec 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

codspeed-hq bot commented Dec 27, 2025 •

edited

Loading