Refine tools, getting more docs #268
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds docs-aware execution and error reporting, preloads embedded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / cr
participant Lib as lib::run_program_with_docs
participant PP as Preprocessor
participant Eval as Evaluator
participant CS as call_stack::display_stack_with_docs
note over CLI,Lib: Docs-aware run entry (new)
CLI->>Lib: run_program_with_docs(ns, def, params)
Lib->>PP: preprocess()
alt preprocessing ok
Lib->>Eval: evaluate(ns, def, params)
alt success
Eval-->>Lib: result
Lib-->>CLI: result
else failure
Eval-->>Lib: error(stack, location)
Lib->>CS: display_stack_with_docs(failure, stack, location)
CS-->>Lib: printed snapshot + docs/examples
Lib-->>CLI: error
end
else preprocessing error
PP-->>Lib: error(stack, location)
Lib->>CS: display_stack_with_docs(...)
CS-->>Lib: printed snapshot + docs/examples
Lib-->>CLI: error
end
sequenceDiagram
autonumber
participant Client as MCP Client
participant Read as mcp::read_handlers
participant SM as StateManager
note over Client,Read: List namespace definitions (current → deps fallback)
Client->>Read: list_namespace_definitions(ns)
Read->>SM: with_current_module(ns)?
alt found in current module
SM-->>Read: file data
Read-->>Client: {source: "current", data}
else not found
Read->>SM: find_namespace_in_dependencies(ns)
alt found in deps
SM-->>Read: (pkg, file data)
Read-->>Client: {source: "dependency", package: pkg, data}
else not found
Read->>SM: get_all_dependency_namespaces()
SM-->>Read: namespaces
Read-->>Client: error with suggestions
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Calcit tooling by adding documentation support throughout the codebase. The main purpose is to improve the developer experience by providing better error messages with documentation context and extending MCP (Model Context Protocol) capabilities to handle dependency namespaces.
- Refactored program data structures to include documentation alongside code definitions
- Enhanced error handling and call stack display to show function documentation
- Added comprehensive documentation to built-in Calcit core functions and syntax elements
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/program.rs | Added ProgramDefEntry struct to store code with documentation and updated related functions |
| src/mcp/state_manager.rs | Enhanced MCP state management with calcit-core module loading and dependency namespace queries |
| src/mcp/read_handlers.rs | Improved namespace definition listing to search both current project and dependencies |
| src/mcp/mcp_handlers.rs | Updated version number to 0.9.19 |
| src/lib.rs | Added run_program_with_docs function and updated error handling to use documentation-aware stack display |
| src/cirru/calcit-core.cirru | Added extensive documentation for built-in functions and syntax elements |
| src/call_stack.rs | Enhanced stack display to include documentation when available |
| package.json | Version bump to 0.9.19 |
| Cargo.toml | Version bump to 0.9.19 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let doc = if entry.doc.is_empty() { | ||
| Arc::from("") | ||
| } else { | ||
| Arc::from(entry.doc.as_str()) | ||
| }; |
There was a problem hiding this comment.
The conditional check and Arc creation is redundant. You can simplify this to Arc::from(entry.doc.as_str()) since Arc::from("") and Arc::from(entry.doc.as_str()) when entry.doc is empty will produce equivalent results.
| let doc = if entry.doc.is_empty() { | |
| Arc::from("") | |
| } else { | |
| Arc::from(entry.doc.as_str()) | |
| }; | |
| let doc = Arc::from(entry.doc.as_str()); |
Copilot uses AI. Check for mistakes.
| let calcit_code = code_to_calcit(code, ns, def, coord0.to_owned())?; | ||
| let entry = ProgramDefEntry { | ||
| code: calcit_code, | ||
| doc: Arc::from(""), // No doc info in changes, use empty string |
There was a problem hiding this comment.
Consider using a constant for the empty documentation string to avoid code duplication. This magic string appears multiple times in the code and could be extracted to a constant like const EMPTY_DOC: &str = \"\";.
Copilot uses AI. Check for mistakes.
| let calcit_code = code_to_calcit(code, ns, def, coord0.to_owned())?; | ||
| let entry = ProgramDefEntry { | ||
| code: calcit_code, | ||
| doc: Arc::from(""), // No doc info in changes, use empty string |
There was a problem hiding this comment.
This is the same magic string as in the previous comment. Consider using a constant for the empty documentation string to improve maintainability.
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
src/mcp/mcp_handlers.rs (1)
218-221: Avoid hardcoding version; derive from Cargo metadata.Use env!("CARGO_PKG_VERSION") to prevent drift across files.
Apply this diff:
server_info: ServerInfo { name: "calcit-mcp-server".to_string(), - version: "0.9.19".to_string(), + version: env!("CARGO_PKG_VERSION").to_string(), },src/bin/cr.rs (5)
272-274: Comment-only addition.If this is a TODO, consider marking explicitly to aid tracking.
339-341: Don’t let stack display failure short-circuit error handling.Avoid
?so we still write COMPILE_ERRORS_FILE and return the original failure.Apply this diff:
- call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref())?; + if let Err(e) = call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref()) { + eprintln!("failed to display stack: {e}"); + }
357-359: Same: avoid propagating display failure in preprocessing reload path.- call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref())?; + if let Err(e) = call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref()) { + eprintln!("failed to display stack: {e}"); + }
375-378: Same: codegen(IR) error path shouldn’t be blocked by display failure.- call_stack::display_stack_with_docs(&failure, &gen_stack::get_gen_stack(), None)?; + if let Err(e) = call_stack::display_stack_with_docs(&failure, &gen_stack::get_gen_stack(), None) { + eprintln!("failed to display stack: {e}"); + }
385-388: Same: codegen(JS) error path shouldn’t be blocked by display failure.- call_stack::display_stack_with_docs(&failure, &gen_stack::get_gen_stack(), None)?; + if let Err(e) = call_stack::display_stack_with_docs(&failure, &gen_stack::get_gen_stack(), None) { + eprintln!("failed to display stack: {e}"); + }src/lib.rs (2)
55-58: Don’t propagate stack display failure; preserve original error.Using
?can return a different error type and lose the original failure context.Apply this diff:
- call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref())?; + if let Err(e) = call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref()) { + eprintln!("failed to display stack: {e}"); + }
79-81: Same concern in function-execution failure branch.- call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref())?; + if let Err(e) = call_stack::display_stack_with_docs(&failure.msg, &failure.stack, failure.location.as_ref()) { + eprintln!("failed to display stack: {e}"); + }src/call_stack.rs (1)
137-142: Avoid repeated global lookups inside hot loop.
lookup_def_docacquires a read lock onPROGRAM_CODE_DATAevery time we iterate a stack frame. For a deep stack this adds avoidable contention. Cache thePROGRAM_CODE_DATAsnapshot once before the loop and reuse it when you enrich each frame.src/mcp/state_manager.rs (2)
414-428: Surface core load failures to callers.Swallowing the
load_core_moduleerror with a println means tooling can’t detect when the embedded core failed to load (e.g. due to corrupted assets), leading to confusing downstream errors. Bubble the error up instead so the caller can decide whether to abort or report loudly.
635-669: Replace printlns in tests with assertions/logging macros.Tests should avoid
println!noise; use assertions to verify expectations oreprintln!guarded by a feature flag if you need diagnostics.src/program.rs (2)
127-136: Don’t promote borrowed doc strings to empty Arc constants unnecessarily.When
entry.docis empty we can skip allocating a brand-newArc<str>and reuseArc::<str>::default()(or leave it asArc<str>created once). Consider retaining the shared empty arc rather than creating a new allocation per definition.
167-173: Clarify doc lifetime when returningString.Returning
Some(entry.doc.to_string())clones the doc on every lookup. If we expect hot-path reads, consider returningArc<str>(orCow<'static, str>) instead to avoid repeated allocations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)package.json(1 hunks)src/bin/cr.rs(7 hunks)src/call_stack.rs(2 hunks)src/cirru/calcit-core.cirru(3 hunks)src/lib.rs(2 hunks)src/mcp/mcp_handlers.rs(1 hunks)src/mcp/read_handlers.rs(1 hunks)src/mcp/state_manager.rs(4 hunks)src/program.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/lib.rs (1)
src/call_stack.rs (1)
display_stack_with_docs(111-163)
src/call_stack.rs (3)
src/calcit.rs (1)
tag(696-698)src/data/cirru.rs (3)
s(90-90)s(92-92)calcit_to_cirru(240-274)src/program.rs (1)
lookup_def_doc(168-173)
src/mcp/read_handlers.rs (2)
src/mcp/definition_handlers.rs (1)
snapshot(775-775)src/mcp/state_manager.rs (1)
new(60-66)
src/mcp/state_manager.rs (2)
src/mcp/read_handlers.rs (2)
snapshot(61-61)list_namespace_definitions(6-89)src/snapshot.rs (1)
load_snapshot_data(110-123)
src/program.rs (2)
src/data/cirru.rs (4)
code_to_calcit(8-161)s(90-90)s(92-92)xs(254-254)src/snapshot.rs (6)
from(32-37)from(48-50)from(67-72)from(76-84)from(197-231)from(235-238)
src/bin/cr.rs (3)
src/lib.rs (1)
run_program_with_docs(48-87)src/call_stack.rs (1)
display_stack_with_docs(111-163)src/codegen/emit_js/gen_stack.rs (1)
get_gen_stack(37-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Publish to cargo
🔇 Additional comments (6)
Cargo.toml (1)
4-4: Cargo version updated — LGTM.src/bin/cr.rs (2)
168-171: Switch to docs-aware run path — LGTM.
296-300: Docs-aware reload run — LGTM.src/lib.rs (2)
45-46: Keeprun_programdelegating — LGTM.
48-58:?compatibility ondisplay_stack_with_docsis verified
impl From<String> for CalcitErrexists, so the?ondisplay_stack_with_docs(also at lines 79–81) compiles correctly.package.json (1)
3-3: Version consistency confirmed. package.json, Cargo.toml, and MCP server all at 0.9.19.
| :code $ quote | ||
| defn + (x & ys) (reduce ys x &+) |
There was a problem hiding this comment.
Ensure docstring wording matches actual behavior.
The new docstring for + claims the function “requires at least one argument,” but (+) still works (returns 0). Adjust the notes to avoid misleading users.
🤖 Prompt for AI Agents
In src/cirru/calcit-core.cirru around lines 475 to 476, the docstring for the +
function incorrectly states it “requires at least one argument” while the
implementation still allows zero-arity (+) returning 0; update the docstring to
accurately reflect behavior (either state that it accepts zero or more arguments
and returns 0 for no args, or change wording to “accepts zero or more
arguments”); ensure the notes align with the implementation so users are not
misled.
| let definitions: Vec<String> = file_data.defs.keys().cloned().collect(); | ||
| ResponseJson(serde_json::json!({ |
There was a problem hiding this comment.
Fix type mismatch when collecting definition names.
file_data.defs.keys().cloned().collect() now yields Arc<str> items, but the target type is Vec<String>. This no longer compiles since String doesn’t implement From<Arc<str>>. Convert each key explicitly (e.g. map(|k| k.to_string())) before collecting.
- let definitions: Vec<String> = file_data.defs.keys().cloned().collect();
+ let definitions: Vec<String> = file_data
+ .defs
+ .keys()
+ .map(|k| k.to_string())
+ .collect();🤖 Prompt for AI Agents
In src/mcp/read_handlers.rs around lines 45-46, collecting definition names uses
file_data.defs.keys().cloned().collect() which now yields Arc<str> items and
doesn't match the target Vec<String>; change the collection to convert each key
to a String (e.g. map over keys and call to_string() or to_owned() before
collect) so the resulting type is Vec<String>.
Apply the same fix when returning dependency namespaces.
This branch has the identical Vec<String> collection pattern and will hit the same compile-time error; apply the same to_string conversion there.
- let definitions: Vec<String> = file_data.defs.keys().cloned().collect();
+ let definitions: Vec<String> = file_data
+ .defs
+ .keys()
+ .map(|k| k.to_string())
+ .collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let definitions: Vec<String> = file_data.defs.keys().cloned().collect(); | |
| ResponseJson(serde_json::json!({ | |
| let definitions: Vec<String> = file_data | |
| .defs | |
| .keys() | |
| .map(|k| k.to_string()) | |
| .collect(); | |
| ResponseJson(serde_json::json!({ |
🤖 Prompt for AI Agents
In src/mcp/read_handlers.rs around lines 45-46, the code collects map keys into
a Vec<String> using keys().cloned().collect(), which fails when keys are &str or
non-String; apply the same fix used elsewhere by mapping each key to an owned
String before collecting (e.g., replace keys().cloned() with keys().map(|k|
k.to_string())) so the dependency namespaces are returned as Vec<String> without
compile errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/bin/cr.rs (1)
507-510: Consider restoring program data after running the examplesWe overwrite
PROGRAM_CODE_DATAwith the synthetic&calcit:check-examplesentry and leave it in place. Even thoughcheck-examplescurrently exits immediately, keeping the mutated program state around could surprise future flows if this utility ever runs before other tasks (e.g. watch mode). I'd snapshot and restore the original program data—even on early returns—to keep globals deterministic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/bin/cr.rs(9 hunks)src/call_stack.rs(2 hunks)src/cirru/calcit-core.cirru(10 hunks)src/cli_args.rs(2 hunks)src/detailed_snapshot.rs(1 hunks)src/mcp/definition_handlers.rs(4 hunks)src/snapshot.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli_args.rs
- src/call_stack.rs
- src/detailed_snapshot.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/bin/cr.rs (3)
src/lib.rs (1)
run_program_with_docs(48-87)src/call_stack.rs (2)
display_stack_with_docs(111-159)len(69-71)src/program.rs (1)
extract_program_data(140-149)
🔇 Additional comments (1)
src/cirru/calcit-core.cirru (1)
478-484: Docstring contradicts zero-arity behavior.Implementation still allows
(+)with no args returning0, so the note “requires at least one argument” is misleading. Please align the wording with the actual behavior.
| ResponseJson(serde_json::json!({ | ||
| "documentation": def_entry.doc, | ||
| "examples": def_entry.examples, | ||
| "source": "dependency", |
There was a problem hiding this comment.
Clone doc/examples before serializing dependency response.
def_entry is only borrowed here, so moving def_entry.doc / def_entry.examples into the JSON response fails to compile (“cannot move out of def_entry because it is borrowed”). Clone the fields before inserting them into json!.
- "documentation": def_entry.doc,
- "examples": def_entry.examples,
+ "documentation": def_entry.doc.clone(),
+ "examples": def_entry.examples.clone(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseJson(serde_json::json!({ | |
| "documentation": def_entry.doc, | |
| "examples": def_entry.examples, | |
| "source": "dependency", | |
| ResponseJson(serde_json::json!({ | |
| "documentation": def_entry.doc.clone(), | |
| "examples": def_entry.examples.clone(), | |
| "source": "dependency", |
🤖 Prompt for AI Agents
In src/mcp/definition_handlers.rs around lines 472 to 475, the code attempts to
move def_entry.doc and def_entry.examples into serde_json::json!, but def_entry
is only borrowed so this causes a move error; fix it by cloning those fields
before constructing the JSON (e.g., use def_entry.doc.clone() and
def_entry.examples.clone() or assign cloned values to locals and insert those
into the json! macro) so the borrow remains valid.
| let snapshot: Snapshot = load_snapshot_data(&edn_data, "calcit-core.cirru").expect("Failed to parse snapshot"); | ||
|
|
There was a problem hiding this comment.
Fix: use the actual file path when calling load_snapshot_data.
Line 346 currently passes only "calcit-core.cirru", so gen_meta_ns receives a path whose parent() is None and the call panics at runtime. This makes the new test fail every time. Pass the full "src/cirru/calcit-core.cirru" path (or otherwise ensure a parent directory exists) before landing this.
- let snapshot: Snapshot = load_snapshot_data(&edn_data, "calcit-core.cirru").expect("Failed to parse snapshot");
+ let snapshot: Snapshot = load_snapshot_data(&edn_data, "src/cirru/calcit-core.cirru").expect("Failed to parse snapshot");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let snapshot: Snapshot = load_snapshot_data(&edn_data, "calcit-core.cirru").expect("Failed to parse snapshot"); | |
| let snapshot: Snapshot = load_snapshot_data(&edn_data, "src/cirru/calcit-core.cirru").expect("Failed to parse snapshot"); |
🤖 Prompt for AI Agents
In src/snapshot.rs around lines 346-347, the call to load_snapshot_data uses
only "calcit-core.cirru", causing gen_meta_ns to receive a path without a parent
and panic at runtime; change the argument to the full relative path
"src/cirru/calcit-core.cirru" (or otherwise pass a Path with an existing parent)
so load_snapshot_data receives a path whose parent() is Some, preventing the
panic and fixing the failing test.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/call_stack.rs(2 hunks)src/program.rs(4 hunks)src/snapshot.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/call_stack.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/program.rs (2)
src/data/cirru.rs (4)
code_to_calcit(8-161)s(90-90)s(92-92)xs(254-254)src/snapshot.rs (6)
from(32-37)from(71-73)from(92-101)from(105-114)from(232-266)from(270-273)
🔇 Additional comments (1)
src/snapshot.rs (1)
341-348: Pass the real path intoload_snapshot_data.
gen_meta_nscallsPath::parent().expect(...). Providing only"calcit-core.cirru"yieldsNone, so this test will always panic instead of verifying examples. Use the same"src/cirru/calcit-core.cirru"path you read from.- let snapshot: Snapshot = load_snapshot_data(&edn_data, "calcit-core.cirru").expect("Failed to parse snapshot"); + let snapshot: Snapshot = + load_snapshot_data(&edn_data, "src/cirru/calcit-core.cirru").expect("Failed to parse snapshot");
| let calcit_code = code_to_calcit(code, ns, def, coord0.to_owned())?; | ||
| let entry = ProgramDefEntry { | ||
| code: calcit_code, | ||
| doc: Arc::from(""), // No doc info in changes, use empty string | ||
| examples: vec![], // No examples info in changes, use empty vector | ||
| }; | ||
| file.defs.insert(def.to_owned().into(), entry); | ||
| } |
There was a problem hiding this comment.
Preserve docs/examples when updating changed defs.
info.changed_defs currently overwrites every entry with blank doc and examples, so hot reloads silently discard previously stored documentation and usage snippets. That breaks the doc-aware tooling added in this PR. Reuse the existing metadata when reinserting the updated code.
for (def, code) in &info.changed_defs {
let calcit_code = code_to_calcit(code, ns, def, coord0.to_owned())?;
- let entry = ProgramDefEntry {
- code: calcit_code,
- doc: Arc::from(""), // No doc info in changes, use empty string
- examples: vec![], // No examples info in changes, use empty vector
- };
- file.defs.insert(def.to_owned().into(), entry);
+ let (doc, examples) = match file.defs.get(def.as_str()) {
+ Some(existing) => (existing.doc.clone(), existing.examples.clone()),
+ None => (Arc::from(""), Vec::new()),
+ };
+ file
+ .defs
+ .insert(def.to_owned().into(), ProgramDefEntry { code: calcit_code, doc, examples });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let calcit_code = code_to_calcit(code, ns, def, coord0.to_owned())?; | |
| let entry = ProgramDefEntry { | |
| code: calcit_code, | |
| doc: Arc::from(""), // No doc info in changes, use empty string | |
| examples: vec![], // No examples info in changes, use empty vector | |
| }; | |
| file.defs.insert(def.to_owned().into(), entry); | |
| } | |
| for (def, code) in &info.changed_defs { | |
| let calcit_code = code_to_calcit(code, ns, def, coord0.to_owned())?; | |
| let (doc, examples) = match file.defs.get(def.as_str()) { | |
| Some(existing) => (existing.doc.clone(), existing.examples.clone()), | |
| None => (Arc::from(""), Vec::new()), | |
| }; | |
| file.defs.insert( | |
| def.to_owned().into(), | |
| ProgramDefEntry { code: calcit_code, doc, examples }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/program.rs around lines 298-305, the code always reinserts updated defs
with blank doc and examples, discarding existing metadata; compute the map key
first (e.g. let key = def.to_owned().into()), lookup existing entry with
file.defs.get(&key) and reuse its doc and examples when constructing the new
ProgramDefEntry (clone or Arc::clone the existing doc and clone the examples),
falling back to Arc::from("") and vec![] only if no prior entry exists, then
insert using the same key.
No description provided.