Conversation
📝 WalkthroughWalkthroughAdds ArrayIndexSet statement support: parser, semantic tokens, and analysis/type-checking for assignments to array elements (including generic inference and index validation). Updates Statement enum and type-matching logic, and adds grammar and analysis tests covering successful and error scenarios. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/server/tests/analysis/alpha050.rs (1)
442-693: Extract shared backend setup into a helper to eliminate boilerplate duplication.Each of the five new tests (and all existing tests in this file) repeats ~45 lines of identical setup:
LspService::new,backend.files.fs, platform-conditionalPath::new,Uri::from_file_path,vfs.write, andopen_document. Consider a helper like:async fn setup_backend(code: &str) -> (Backend, FileId) { let (service, _) = tower_lsp_server::LspService::new(|client| { Backend::new(client, AmberVersion::Alpha050, Some(Arc::new(MemoryFS::new()))) }); let backend = service.inner().clone(); // or keep a ref // ... platform path, uri, vfs.write, open_document (backend, file_id) }This is pre-existing in all tests in the file, so the refactor scope is broader than this PR, but the five new tests are the right moment to introduce it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/tests/analysis/alpha050.rs` around lines 442 - 693, Extract the repeated test setup into an async helper (e.g., async fn setup_backend(code: &str) -> (Backend, FileId)) that performs LspService::new with Backend::new(…, AmberVersion::Alpha050, Some(Arc::new(MemoryFS::new()))), gets service.inner() (clone if needed), builds the platform Path and Uri via Uri::from_file_path, writes the provided code with vfs.write, and calls open_document to return the backend and file_id; then replace each test's duplicated block (the LspService::new/Backend::new creation, vfs.write, Uri::from_file_path, and open_document calls) with a call to setup_backend(code) and use the returned Backend and FileId.crates/grammar/src/alpha050/statements/array_index_set.rs (1)
46-47: Use.labelled()before.boxed()for consistency with the rest of the codebase.While
.boxed()is only for type-erasure and doesn't affect label visibility, the conventional order in this project is.labelled(...).boxed(), as seen in other parser definitions throughout the codebase (e.g., in the expressions modules).♻️ Proposed fix
- .boxed() - .labelled("array index set") + .labelled("array index set") + .boxed()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grammar/src/alpha050/statements/array_index_set.rs` around lines 46 - 47, Change the parser builder so the label is applied before boxing for consistency: in the array index set parser (the chain that currently ends with .boxed().labelled("array index set")), swap the calls to use .labelled("array index set").boxed() instead so the label appears before type-erasure like other parsers.
🤖 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/stmnts.rs`:
- Around line 961-1001: The analyze_exp call for index_exp is ignored so its
is_propagating_failure and return_ty are lost; capture its result (e.g., let
index_analysis = analyze_exp(..., index_exp, ...)), then when constructing the
StmntAnalysisResult combine both analyses: set is_propagating_failure =
index_analysis.is_propagating_failure || exp_analysis.is_propagating_failure and
set return_ty to a sensible merge (e.g.,
index_analysis.return_ty.or(exp_analysis.return_ty) or otherwise prefer the
first non-none), and keep the existing element_ty generic constraint and
insert_symbol_reference logic intact.
In `@crates/server/tests/analysis/alpha050.rs`:
- Around line 683-686: The comment before the assertion about generic inference
is incorrect: examine the call site set_first([1, 2, 3], 42) and update the
comment to reflect the actual inferred types captured in symbol_table.symbols
(e.g., that 'arr' and 'value' are constrained to concrete [Int] and Int if the
snapshot shows that) so the comment matches the snapshot; reference set_first,
'arr', 'value', and symbol_table.symbols when editing the comment to accurately
describe the observed inference outcome.
---
Nitpick comments:
In `@crates/grammar/src/alpha050/statements/array_index_set.rs`:
- Around line 46-47: Change the parser builder so the label is applied before
boxing for consistency: in the array index set parser (the chain that currently
ends with .boxed().labelled("array index set")), swap the calls to use
.labelled("array index set").boxed() instead so the label appears before
type-erasure like other parsers.
In `@crates/server/tests/analysis/alpha050.rs`:
- Around line 442-693: Extract the repeated test setup into an async helper
(e.g., async fn setup_backend(code: &str) -> (Backend, FileId)) that performs
LspService::new with Backend::new(…, AmberVersion::Alpha050,
Some(Arc::new(MemoryFS::new()))), gets service.inner() (clone if needed), builds
the platform Path and Uri via Uri::from_file_path, writes the provided code with
vfs.write, and calls open_document to return the backend and file_id; then
replace each test's duplicated block (the LspService::new/Backend::new creation,
vfs.write, Uri::from_file_path, and open_document calls) with a call to
setup_backend(code) and use the returned Backend and FileId.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
crates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set-2.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set-3.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_const_error-2.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_const_error-3.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_const_error.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_generic-2.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_generic-3.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_generic.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_non_array_error-2.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_non_array_error-3.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_non_array_error.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_type_mismatch_error-2.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_type_mismatch_error-3.snapis excluded by!**/*.snapcrates/server/tests/analysis/snapshots/r#mod__analysis__alpha050__array_index_set_type_mismatch_error.snapis excluded by!**/*.snapcrates/server/tests/grammar/snapshots/r#mod__grammar__alpha050__array_index_set.snapis excluded by!**/*.snapcrates/server/tests/grammar/snapshots/r#mod__grammar__alpha050__array_index_set_expression_index.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/analysis/src/alpha050/stmnts.rscrates/grammar/src/alpha050/mod.rscrates/grammar/src/alpha050/semantic_tokens.rscrates/grammar/src/alpha050/statements/array_index_set.rscrates/grammar/src/alpha050/statements/mod.rscrates/server/tests/analysis/alpha050.rscrates/server/tests/grammar/alpha050.rscrates/types/src/data_type.rs
💤 Files with no reviewable changes (1)
- crates/types/src/data_type.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/analysis/src/alpha050/stmnts.rs (1)
890-906: The variable-lookup + function/constant guard block is duplicated across ~7 match arms.This exact pattern (lines 891–906) —
get_symbol_definition_info→ checkFunction/is_const→ fall through toinfo.data_type/DataType::Any— appears inVariableSet, everyShorthand*arm, and nowArrayIndexSet. Consider extracting a small helper (e.g.,resolve_assignable_var_ty) to reduce the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/analysis/src/alpha050/stmnts.rs` around lines 890 - 906, Extract the duplicated variable-lookup and guard logic into a helper (e.g., resolve_assignable_var_ty) that wraps get_symbol_definition_info and performs the SymbolType::Function and SymbolType::Variable(var) if var.is_const checks, returning the resolved DataType (or DataType::Any) and reporting errors via files.report_error as currently done; then replace the duplicated blocks in Statement::ArrayIndexSet, VariableSet, all Shorthand* arms, etc., by calling resolve_assignable_var_ty(file/files, var, var_span) to get var_ty. Ensure the helper preserves the existing error messages and uses the same parameters/behavior so callers only need to accept the returned DataType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/analysis/src/alpha050/stmnts.rs`:
- Around line 890-906: Extract the duplicated variable-lookup and guard logic
into a helper (e.g., resolve_assignable_var_ty) that wraps
get_symbol_definition_info and performs the SymbolType::Function and
SymbolType::Variable(var) if var.is_const checks, returning the resolved
DataType (or DataType::Any) and reporting errors via files.report_error as
currently done; then replace the duplicated blocks in Statement::ArrayIndexSet,
VariableSet, all Shorthand* arms, etc., by calling
resolve_assignable_var_ty(file/files, var, var_span) to get var_ty. Ensure the
helper preserves the existing error messages and uses the same
parameters/behavior so callers only need to accept the returned DataType.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/analysis/src/alpha050/stmnts.rscrates/server/tests/analysis/alpha050.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/server/tests/analysis/alpha050.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #112
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests