Refactored expr analysis and fixed command multiline bug#108
Conversation
📝 WalkthroughWalkthroughRefactors expression analysis in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 refactors expression analysis to reduce duplicated logic and adjusts command parsing to handle multiline commands correctly.
Changes:
- Added a grammar regression test + snapshot to cover
$ ... \newline continuation in commands. - Refactored
analyze_expby introducing helper macros to centralize propagation/return tracking and binop handling. - Updated analysis snapshots to reflect the new inference / error reporting behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/server/tests/grammar/alpha050.rs | Adds a new snapshot-based test for multiline command parsing. |
| crates/server/tests/grammar/snapshots/r#mod__grammar__alpha050__command_new_line.snap | New snapshot capturing the expected AST for the multiline command case. |
| crates/grammar/src/alpha050/expressions/atom/command.rs | Adjusts token filtering so backslash/newline can be included in command text. |
| crates/analysis/src/alpha050/exp.rs | Refactors expression analysis with macros and consolidates multiple match arms. |
| crates/server/tests/analysis/snapshots/*.snap | Updates snapshots for the refactored/changed analysis behavior and paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rhs_result.exp_ty.to_string(&scoped_generic_types), | ||
| lhs_result.exp_ty.to_string(&scoped_generic_types), |
There was a problem hiding this comment.
The error message has expected and found reversed. Here, lhs_result.exp_ty is the expected type used to analyze the RHS, while rhs_result.exp_ty is the actual type inferred for the RHS; the message should print expected lhs_result.exp_ty and found rhs_result.exp_ty.
| rhs_result.exp_ty.to_string(&scoped_generic_types), | |
| lhs_result.exp_ty.to_string(&scoped_generic_types), | |
| lhs_result.exp_ty.to_string(&scoped_generic_types), | |
| rhs_result.exp_ty.to_string(&scoped_generic_types), |
| Expression::Multiply(exp1, exp2) | ||
| | Expression::Divide(exp1, exp2) | ||
| | Expression::Modulo(exp1, exp2) | ||
| | Expression::Subtract(exp1, exp2) => { | ||
| analyze_binop_codep!( | ||
| exp1, | ||
| DataType::Union([DataType::Number, DataType::Int].to_vec()), | ||
| files, | ||
| scoped_generic_types, | ||
| contexts, | ||
| ); | ||
| let ExpAnalysisResult { | ||
| return_ty: return2, | ||
| is_propagating_failure: prop2, | ||
| exp_ty: exp_ty2, | ||
| } = analyze_exp( | ||
| file_id, | ||
| file_version, | ||
| exp2, | ||
| DataType::Union([DataType::Number, DataType::Int].to_vec()), | ||
| files, | ||
| scoped_generic_types, | ||
| contexts, | ||
| ); | ||
|
|
||
| is_propagating_failure |= prop1 || prop2; | ||
| return_types.extend(return1); | ||
| return_types.extend(return2); | ||
|
|
||
| if exp_ty1 == DataType::Number || exp_ty2 == DataType::Number { | ||
| DataType::Number | ||
| } else { | ||
| DataType::Int | ||
| } | ||
| exp2 | ||
| ) | ||
| } |
There was a problem hiding this comment.
These numeric operators previously returned Number if either operand was Number, otherwise Int. After the refactor, the result is whatever type the LHS analyzed to (often Int | Number), which changes the inferred result type and can propagate a less-precise or incorrect type. Consider restoring the numeric promotion logic (compute from both operand types) instead of returning the LHS type.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/server/tests/grammar/alpha050.rs (1)
263-271: Considerparse_unwrap()if error-free parsing is the expected contract.Using
parse()folds errors silently into the snapshot, so a regression that introduces parse errors would show up only as a snapshot diff rather than a test failure with a clear message. If the intention is that this input should parse without errors after the fix,parse_unwrap()makes that invariant explicit and produces a more actionable failure.🔧 Suggested change
- assert_debug_snapshot!( - "command_new_line", - parse(&compiler.tokenize( - r#" - $ MY_VAR=1 \ - my_command $ - "# - )) - ); + assert_debug_snapshot!( + "command_new_line", + parse_unwrap(&compiler.tokenize( + r#" + $ MY_VAR=1 \ + my_command $ + "# + )) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/tests/grammar/alpha050.rs` around lines 263 - 271, The test currently calls parse(&compiler.tokenize(...)) which will embed parse errors into the snapshot; replace that call with parse_unwrap(&compiler.tokenize(...)) so the test asserts a no-error parse invariant and fails fast on parse errors—update the invocation in the assert_debug_snapshot! for the "command_new_line" case to use parse_unwrap instead of parse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/analysis/src/alpha050/exp.rs`:
- Around line 100-118: The match on lhs_result.exp_ty moves the DataType out
causing later uses to be a use-after-move; change the pattern to inspect a
cloned value instead (e.g., use lhs_result.exp_ty.clone() when matching Generic)
so that scoped_generic_types.constrain_generic_type still receives the id while
subsequent calls to matches_type(&rhs_result.exp_ty, &lhs_result.exp_ty,
&scoped_generic_types) and the return can use lhs_result.exp_ty; mirror the
approach used in the Ge/Gt arm (if let DataType::Generic(id) =
lhs.exp_ty.clone()) to avoid moving lhs_result.exp_ty.
---
Nitpick comments:
In `@crates/server/tests/grammar/alpha050.rs`:
- Around line 263-271: The test currently calls parse(&compiler.tokenize(...))
which will embed parse errors into the snapshot; replace that call with
parse_unwrap(&compiler.tokenize(...)) so the test asserts a no-error parse
invariant and fails fast on parse errors—update the invocation in the
assert_debug_snapshot! for the "command_new_line" case to use parse_unwrap
instead of parse.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/analysis/src/alpha050/exp.rs`:
- Around line 95-122: The macro analyze_binop_codep! currently checks types with
matches_type(&rhs, &lhs, ...) and reports "Expected {rhs}, found {lhs}" at
$exp1.1, which inverts expected/actual and uses the wrong span, and it
duplicates errors because analyze_expr!($exp2, lhs_result.exp_ty.clone())
already reports mismatches; fix by removing the explicit !matches_type
error-reporting entirely (keep only the Generic(id) constrain_generic_type call)
so analyze_expr handles non-generic mismatches, or if you must keep an explicit
check, call matches_type(&lhs_result.exp_ty, &rhs_result.exp_ty,
&scoped_generic_types), format the message "Expected type {lhs}, found type
{rhs}", and report it at the RHS span $exp2.1; locate these changes in the
analyze_binop_codep! macro and around the analyze_expr!($exp2, ...) call.
- Around line 136-153: Matching on array_result.exp_ty consumes it, causing a
use-after-move when you later try to return the original generic variant; change
the pattern to avoid moving the scrutinee (e.g., match by reference or bind the
id without consuming the whole DataType) and when the branch intends to return
the original generic type return DataType::Generic(id_clone_or_copy)
reconstructed from the bound id (or clone/copy the id beforehand) instead of
referencing array_result.exp_ty; specifically update the DataType::Generic(id)
arm and its call to scoped_generic_types.get_recursive(id) so id is not moved
(or use a taken copy) and return a reconstructed DataType::Generic(...) when the
slice-indexing case requires the original generic rather than the inner element
type.
Summary by CodeRabbit
Improvements
Tests