📝 CodeRabbit Chat: Implement requested code changes#258
📝 CodeRabbit Chat: Implement requested code changes#258coderabbitai[bot] wants to merge 5 commits into
Conversation
This introduces an optional `name_span` field on symbols, capturing the exact identifier token span when available. The semantic model builder and AST nodes now extract and store precise name spans for top-level declarations (relations, functions, types) and for rule-local bindings (head bindings, assignments, for-loop patterns). The unused-variable lint and diagnostics have been updated to use these precise spans where possible for improved diagnostic highlighting. Also includes new semantic-model tests verifying that the precise identifier spans are captured and differ from the coarser span previously used. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
… function Refactor collect_program_declarations by removing inlined closure and pattern matching, replacing it with a new helper method program_declaration_spec. This improves code readability and modularity without changing behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…tic model This change augments the RuleHead AST with span information for the head atom and its bindings. It introduces functionality in the parser to collect and store identifier-token spans for bindings introduced by the head atom. The semantic model builder now uses these spans to accurately track declaration name spans for rule head bindings. Enhancements include: - New functions to find identifier spans within given spans for precise range matching. - Collection of binding spans in parsed rule heads excluding non-bindings like those following dots or certain punctuations. - Usage of binding spans in semantic symbol declarations for improved source mapping. - Additional tests to verify correct capturing and handling of name spans for rule head bindings. This feature improves diagnostic accuracy and tooling support by providing detailed source location information for rule head bindings. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Nested bodies recurse through `collect_rule_term`, but the `VariableUseContext::new(...)` path no longer retains CST handles for those inner terms. Passing `None` deliberately prevents nested bindings from claiming precise `name_span`s we cannot source faithfully. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the AST identifier span search helper to use iterator combinators ( File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the
NodeOrToken::Nodearm, consider replacingranges_overlap(...).then(|| find_identifier_span_in_range(...)).flatten()withif ranges_overlap(...) { find_identifier_span_in_range(...) } else { None }(orand_then) to avoid the intermediateOption<Option<_>>and improve readability. - The
NodeOrToken::Token(_) => Nonearm could be simplified by using a wildcard match arm (e.g.,_ => None) if there are no other variants to handle, making the match slightly more concise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `NodeOrToken::Node` arm, consider replacing `ranges_overlap(...).then(|| find_identifier_span_in_range(...)).flatten()` with `if ranges_overlap(...) { find_identifier_span_in_range(...) } else { None }` (or `and_then`) to avoid the intermediate `Option<Option<_>>` and improve readability.
- The `NodeOrToken::Token(_) => None` arm could be simplified by using a wildcard match arm (e.g., `_ => None`) if there are no other variants to handle, making the match slightly more concise.
## Individual Comments
### Comment 1
<location path="src/parser/ast/mod.rs" line_range="37" />
<code_context>
- }
- }
- NodeOrToken::Token(_) => {}
+ syntax.children_with_tokens().find_map(|child| match child {
+ NodeOrToken::Token(token)
+ if token.kind() == SyntaxKind::T_IDENT && token.text() == name =>
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This new `match` expression has three branches and should be extracted into a separate predicate/helper function to comply with the conditional branching guideline.
The closure passed to `find_map` contains a `match` with three arms. Per the instruction to move conditionals with more than two branches into a predicate function, consider extracting this logic into a small helper (e.g. `fn identifier_span_for_child(...) -> Option<Span>`), and then call that from `find_map` instead of inlining the full `match` here. That keeps the branching logic isolated and the main function simpler.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Move conditionals with >2 branches into a predicate function.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
| NodeOrToken::Token(_) => {} | ||
| syntax.children_with_tokens().find_map(|child| match child { |
There was a problem hiding this comment.
suggestion (review_instructions): This new match expression has three branches and should be extracted into a separate predicate/helper function to comply with the conditional branching guideline.
The closure passed to find_map contains a match with three arms. Per the instruction to move conditionals with more than two branches into a predicate function, consider extracting this logic into a small helper (e.g. fn identifier_span_for_child(...) -> Option<Span>), and then call that from find_map instead of inlining the full match here. That keeps the branching logic isolated and the main function simpler.
Review instructions:
Path patterns: **/*.rs
Instructions:
Move conditionals with >2 branches into a predicate function.
590b15d to
03a0ad5
Compare
Code changes was requested by @leynos.
The following files were modified:
src/parser/ast/mod.rsSummary by Sourcery
Enhancements: