Extend Symbol with precise identifier span and two-provenance model#256
Merged
Extend Symbol with precise identifier span and two-provenance model#256
Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Reviewer's GuideExtends the semantic Symbol model with an optional precise identifier span (name_span), wires it through AST, semantic builder/traversal, and lints so diagnostics like unused-variable can highlight the exact identifier token while preserving existing coarse spans for provenance. Sequence diagram for diagnostics using Symbol.name_span in unused-variablesequenceDiagram
actor Developer
participant Editor
participant Linter
participant Parser
participant Ast as AstNodes
participant Builder as SemanticModelBuilder
participant SymbolTable
participant Rule as unused_variable_rule
Developer->>Editor: Save ddlog source file
Editor->>Linter: Run ddlog lints
Linter->>Parser: parse(source)
Parser-->>Linter: AstRoot
Linter->>Builder: build_semantic_model(AstRoot)
Builder->>Ast: iterate_declarations()
Ast-->>Builder: Function/Relation/TypeDef nodes
Builder->>Ast: function.name()
Ast-->>Builder: name
Builder->>Ast: function.name_span()
Ast->>Ast: find_identifier_span(syntax, name)
Ast-->>Builder: Option name_span
Builder->>SymbolTable: declare_symbol(SymbolSpec span, name_span)
Builder->>Ast: rule.body_expression_nodes()
Ast-->>Builder: nodes
Builder->>Ast: rule.body_terms()
Ast-->>Builder: terms
loop for each term
Builder->>Builder: collect_rule_term(context, Some(node.syntax), term)
Builder->>SymbolTable: declare_symbol(SymbolSpec span, name_span from find_identifier_span)
end
Linter->>Rule: run(unused_variable, SymbolTable)
loop for each rule_local_symbol
Rule->>SymbolTable: get_symbol()
SymbolTable-->>Rule: Symbol
Rule->>Rule: span = symbol.name_span().unwrap_or(symbol.span())
Rule-->>Linter: LintDiagnostic(span)
end
Linter-->>Editor: diagnostics with precise identifier ranges
Editor-->>Developer: highlight unused variable identifiers
Class diagram for Symbol, SymbolSpec, and AST name_span propagationclassDiagram
class Span
class Symbol {
+String name
+DeclarationKind kind
+SymbolOrigin origin
+ScopeId scope
+Span span
+Option~Span~ name_span
+usize source_order
+usize visible_from_rule_order
+fn span() Span
+fn name_span() Option~Span~
+fn source_order() usize
+fn visible_from_rule_order() usize
}
class SymbolSpec {
+String name
+DeclarationKind kind
+SymbolOrigin origin
+ScopeId scope
+Span span
+Option~Span~ name_span
+usize source_order
+usize visible_from_rule_order
}
class SemanticModelBuilder {
-Vec~SymbolSpec~ symbols
+fn declare_symbol(spec SymbolSpec) void
+fn collect_rule_term(context VariableUseContext, syntax Option~SyntaxNode~, term RuleBodyTerm) void
+fn collect_assignment_term(assign RuleAssignment, syntax Option~SyntaxNode~, context VariableUseContext) void
+fn collect_for_loop_term(for_loop RuleForLoop, syntax Option~SyntaxNode~, context VariableUseContext) void
+fn collect_head_expr(expr RelationHeadExpr, ctx RuleContext) void
}
class Function {
+SyntaxNode syntax
+fn name() Option~String~
+fn name_span() Option~Span~
+fn is_extern() bool
}
class Relation {
+SyntaxNode syntax
+fn name() Option~String~
+fn name_span() Option~Span~
+fn is_input() bool
+fn is_output() bool
}
class TypeDef {
+SyntaxNode syntax
+fn name() Option~String~
+fn name_span() Option~Span~
+fn is_extern() bool
}
class RuleBodyTerm {
}
class RuleAssignment {
+Pattern pattern
+Expression value
}
class RuleForLoop {
+Pattern pattern
+Expression iterable
+RuleBodyTerm body
}
class DdlogRule {
+SyntaxNode syntax
+fn heads() Result~Vec~RuleHead~~
+fn body_terms() Result~Vec~RuleBodyTerm~~
+fn body_expression_nodes() Vec~AstNode~
}
class RuleHead {
+RelationHeadExpr atom
+Option~LocationExpr~ location
}
class VariableUseContext {
+fn new(scope ScopeId, span Span, rule_order_limit usize, literal_span Span, literal_index usize) VariableUseContext
+fn current_scope() ScopeId
+fn span() Span
+fn rule_order_limit() usize
+fn literal_index() usize
}
class SyntaxNode {
}
class find_identifier_span {
<<function>>
+fn find_identifier_span(syntax SyntaxNode, name &str) Option~Span~
}
class unused_variable_rule {
<<lint_rule>>
+fn check(symbol Symbol) Option~LintDiagnostic~
}
SymbolSpec ..> Span
Symbol ..> Span
Function ..> Span
Relation ..> Span
TypeDef ..> Span
SemanticModelBuilder o--> SymbolSpec : builds
SemanticModelBuilder o--> Symbol : stores
DdlogRule --> RuleHead : has
DdlogRule --> RuleBodyTerm : has
RuleBodyTerm <|-- RuleAssignment
RuleBodyTerm <|-- RuleForLoop
Function ..> SyntaxNode
Relation ..> SyntaxNode
TypeDef ..> SyntaxNode
DdlogRule ..> SyntaxNode
find_identifier_span ..> SyntaxNode : traverses
find_identifier_span ..> Span : returns
Function ..> find_identifier_span : uses
Relation ..> find_identifier_span : uses
TypeDef ..> find_identifier_span : uses
SemanticModelBuilder ..> DdlogRule : reads
SemanticModelBuilder ..> VariableUseContext : uses
unused_variable_rule ..> Symbol : reads
unused_variable_rule ..> Span : uses span_or_name_span
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
WalkthroughAdd an additive precise identifier-token provenance field Changes
Sequence DiagramsequenceDiagram
participant Parser as Parser (CST / AST)
participant Builder as SemanticBuilder
participant Model as SemanticModel
participant Linter as UnusedVariableRule
Parser->>Builder: Provide AST nodes and optional SyntaxNode handle
Builder->>Parser: Call find_identifier_span(syntax, name)
Builder->>Builder: Create SymbolSpec { span, name_span? }
Builder->>Model: Declare Symbol including optional name_span
Linter->>Model: Query Symbol (span, name_span)
Linter->>Linter: Choose name_span if present else span
Linter->>Linter: Emit diagnostic at chosen span
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7783aabdc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sema/builder.rs`:
- Around line 156-160: Add a defensive invariant check before zipping body nodes
and terms: verify rule.body_expression_nodes().len() == rule.body_terms().len()
(e.g., with debug_assert_eq!) so node/term provenance cannot silently
desynchronize; locate the block around the if let Ok(terms) = rule.body_terms()
{ ... } that obtains body_nodes and insert the assertion before the for
(literal_index, (node, term)) in body_nodes.into_iter().zip(terms).enumerate()
loop (affecting rule.body_expression_nodes(), rule.body_terms(), span(), and
collect_rule_term()).
In `@src/sema/tests/name_span.rs`:
- Around line 70-88: The test currently validates that a head binding's
name_span points to the identifier token, but doesn't cover the collision case
where the relation name and the head binding share the same text; update the
test function rule_head_bindings_capture_identifier_name_spans to use a source
where the relation and head binding are the same token (e.g. "Output(Output) :-
Output(_)."), then ensure you still call find_symbol with
DeclarationKind::RuleBinding and SymbolOrigin::RuleHead to locate the binding
symbol and assert span_text(source, name_span) equals the binding token text and
span_text(source, symbol.span()) equals the full source so the name_span
resolves to the binding occurrence (not the relation occurrence).
In `@src/sema/traverse.rs`:
- Around line 29-37: The code currently derives name_span by searching the whole
rule text via find_identifier_span(rule.syntax(), &binding_name) which can pick
the wrong token; instead determine the precise identifier token from the binding
site syntax and pass that to find_identifier_span. Concretely, iterate or access
the binding syntax node for each binding produced by
collect_head_binding_names(&head.atom) (i.e., the head/atom binding node), then
call find_identifier_span(binding_syntax_node, &binding_name) and use that
result as SymbolSpec.name_span when calling self.declare_symbol(SymbolSpec { ...
}), keeping SymbolSpec.span as the broader span and leaving name_span optional
if the precise token cannot be found; also ensure diagnostics prefer the precise
name_span when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d74ef601-245f-400d-8ac4-de62eb8263aa
📒 Files selected for processing (11)
docs/ddlint-design.mdsrc/linter/rules/correctness/unused_variable.rssrc/parser/ast/function.rssrc/parser/ast/mod.rssrc/parser/ast/relation.rssrc/parser/ast/type_def.rssrc/sema/builder.rssrc/sema/model.rssrc/sema/tests.rssrc/sema/tests/name_span.rssrc/sema/traverse.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sema/traverse.rs (1)
147-158: 🧹 Nitpick | 🔵 TrivialClarify the intentional loss of precision for nested for-loop bindings.
Passing
Noneforsyntaxin the recursivecollect_rule_termcall means nested for-loop body bindings will lack precisename_span. Add a brief comment explaining this is intentional (CST handles are not preserved through nested traversal) to prevent future maintainers from "fixing" this.Suggested clarification
for (term_offset, nested_term) in for_loop.body_terms.iter().enumerate() { self.collect_rule_term( VariableUseContext::new( child_scope, context.literal_index() + term_offset, context.span(), context.rule_order_limit(), ), + // Nested body terms do not retain CST handles; name_span unavailable. None, nested_term, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sema/traverse.rs` around lines 147 - 158, The recursive call to collect_rule_term from the for-loop (using VariableUseContext::new with context.literal_index() + term_offset) intentionally passes None for the syntax parameter, which causes nested for-loop bindings to lack precise name_span; add a concise comment above this call explaining that this is deliberate because CST handles/name-span information are not preserved across nested traversal and so we intentionally drop syntax here to avoid misleading precision, referencing collect_rule_term and VariableUseContext::new so future maintainers know this was a conscious choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/sema/traverse.rs`:
- Around line 147-158: The recursive call to collect_rule_term from the for-loop
(using VariableUseContext::new with context.literal_index() + term_offset)
intentionally passes None for the syntax parameter, which causes nested for-loop
bindings to lack precise name_span; add a concise comment above this call
explaining that this is deliberate because CST handles/name-span information are
not preserved across nested traversal and so we intentionally drop syntax here
to avoid misleading precision, referencing collect_rule_term and
VariableUseContext::new so future maintainers know this was a conscious choice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 281716ce-f2d4-4217-9e68-488aeec27394
📒 Files selected for processing (5)
src/parser/ast/mod.rssrc/parser/ast/rule_head.rssrc/sema/builder.rssrc/sema/tests/name_span.rssrc/sema/traverse.rs
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file pub(crate) fn find_identifier_span_in_range(
syntax: &SyntaxNode<DdlogLanguage>,
search_span: &Span,
name: &str,
) -> Option<Span> {
for child in syntax.children_with_tokens() {
match child {
NodeOrToken::Token(token)
if token.kind() == SyntaxKind::T_IDENT && token.text() == name =>
{
let token_span = text_range_to_span(token.text_range());
if span_contains(search_span, &token_span) {
return Some(token_span);
}
}
NodeOrToken::Node(node) => {
if !ranges_overlap(search_span, &text_range_to_span(node.text_range())) {
continue;
}
if let Some(span) = find_identifier_span_in_range(&node, search_span, name) {
return Some(span);
}
}
NodeOrToken::Token(_) => {}
}
}
None
}❌ New issue: Deep, Nested Complexity |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sema/traverse.rs (1)
90-101: 🧹 Nitpick | 🔵 TrivialExtract one helper for pattern-binding symbol declaration.
Remove duplicated
declare_symbol(SymbolSpec { ... })construction across assignment and for-loop paths to prevent drift in provenance fields.Refactor sketch
+ fn declare_pattern_bindings( + &mut self, + pattern: &ast::Pattern, + syntax: Option<&SyntaxNode<crate::DdlogLanguage>>, + origin: SymbolOrigin, + scope: ScopeId, + span: &Span, + visible_from_rule_order: usize, + ) { + for binding_name in collect_pattern_binding_names(pattern) { + let name_span = syntax.and_then(|node| find_identifier_span(node, &binding_name)); + self.declare_symbol(SymbolSpec { + name: binding_name, + kind: DeclarationKind::RuleBinding, + origin, + scope, + span: span.clone(), + name_span, + source_order: self.symbols.len(), + visible_from_rule_order, + }); + } + } - - for binding_name in collect_pattern_binding_names(&assign.pattern) { - self.declare_symbol(SymbolSpec { ... }); - } + self.declare_pattern_bindings( + &assign.pattern, + syntax, + SymbolOrigin::AssignmentPattern, + context.current_scope(), + context.span(), + context.literal_index() + 1, + ); - - for binding_name in collect_pattern_binding_names(&for_loop.pattern) { - self.declare_symbol(SymbolSpec { ... }); - } + self.declare_pattern_bindings( + &for_loop.pattern, + syntax, + SymbolOrigin::ForPattern, + child_scope, + context.span(), + 0, + );As per coding guidelines, "Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and the Command Query Responsibility Segregation pattern."
Also applies to: 134-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sema/traverse.rs` around lines 90 - 101, Duplicate construction and call to declare_symbol(SymbolSpec { ... }) for pattern bindings should be extracted into a single helper; create a new function (e.g., declare_pattern_binding or declare_pattern_binding_symbols) that takes the binding name, context and relevant metadata and builds the SymbolSpec (using DeclarationKind::RuleBinding and SymbolOrigin::AssignmentPattern where appropriate) then calls self.declare_symbol. Replace both the loop using collect_pattern_binding_names(&assign.pattern) and the other similar site (lines ~134-145) to call this helper so provenance fields (scope, span, name_span, source_order, visible_from_rule_order) are set in one place and cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sema/traverse.rs`:
- Line 7: Refactor the traversal inside the function
find_identifier_span_in_range to replace its nested recursive branching with an
explicit DFS using a Vec<SyntaxElement> stack: keep the public
find_identifier_span unchanged, add small helpers token_overlaps_range(token,
range)->bool, node_overlaps_range(node, range)->bool and
is_matching_ident(token, name)->bool, push the root SyntaxElement(s) then loop
popping elements, skip tokens/nodes that don't overlap via the helpers
(preserving pruning), for nodes push their children in deterministic order and
for tokens check is_matching_ident and return the first matching identifier span
exactly as current behaviour; update parser AST unit tests to cover repeated
identifier names in separate subtrees, non-overlapping ranges, and exact nested
matches, then rustfmt and fix clippy warnings.
---
Outside diff comments:
In `@src/sema/traverse.rs`:
- Around line 90-101: Duplicate construction and call to
declare_symbol(SymbolSpec { ... }) for pattern bindings should be extracted into
a single helper; create a new function (e.g., declare_pattern_binding or
declare_pattern_binding_symbols) that takes the binding name, context and
relevant metadata and builds the SymbolSpec (using DeclarationKind::RuleBinding
and SymbolOrigin::AssignmentPattern where appropriate) then calls
self.declare_symbol. Replace both the loop using
collect_pattern_binding_names(&assign.pattern) and the other similar site (lines
~134-145) to call this helper so provenance fields (scope, span, name_span,
source_order, visible_from_rule_order) are set in one place and cannot drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca06b703-0a25-496c-8158-c24a613409cf
📒 Files selected for processing (1)
src/sema/traverse.rs
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>
devboxerhub
Bot
force-pushed
the
add-symbol-name-span-5p0bo1
branch
from
590b15d to
03a0ad5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
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 `@src/sema/builder.rs`:
- Around line 203-214: Add a brief comment above the loop that declares
rule-head bindings explaining the intentional asymmetry: in the AST traversal
(traverse.rs) rule-head bindings get name_span: Some(...) from binding_spans,
but here in the semantic builder we set SymbolSpec.name_span = None for
DeclarationKind::RuleBinding / SymbolOrigin::SemanticRuleHead to mirror the same
asymmetry noted elsewhere (see the adjacent comment pattern around the other
rule-binding handling). Mention traverse.rs and SymbolSpec.name_span in the
comment so readers know where the name_span is captured for AST vs semantic
phases.
In `@src/sema/tests/name_span.rs`:
- Around line 137-158: Add a new test named
nested_for_loop_bindings_lack_name_span that parses the rule with a nested for
(e.g. "for (outer in Items(x)) for (inner in Nested(outer)) ..."), builds the
semantic model via build(parse_ok(...)), locates the inner binding using
find_symbol with DeclarationKind::RuleBinding and SymbolOrigin::ForPattern, and
assert that symbol.name_span().is_none() to lock in the deliberate behaviour
from traverse::collect_rule_term where nested for-loop term syntax handles are
passed as None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eacbfeb5-125a-448a-92ee-94af67ed5511
📒 Files selected for processing (12)
docs/ddlint-design.mdsrc/linter/rules/correctness/unused_variable.rssrc/parser/ast/function.rssrc/parser/ast/mod.rssrc/parser/ast/relation.rssrc/parser/ast/rule_head.rssrc/parser/ast/type_def.rssrc/sema/builder.rssrc/sema/model.rssrc/sema/tests.rssrc/sema/tests/name_span.rssrc/sema/traverse.rs
Refactored `SemanticModelBuilder` to use a new `declare_pattern_binding` helper method when declaring pattern bindings for assignment and for-patterns. This removes duplication and centralizes symbol declaration logic. Additionally, improved identifier span search in AST by rewriting `find_identifier_span_in_range` to use iterative traversal with helper functions. Added related tests for identifier search correctness. Minor doc formatting fixes also included. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file fn declare_pattern_binding(
&mut self,
binding_name: &str,
origin: SymbolOrigin,
scope: ScopeId,
span: &Span,
syntax: Option<&SyntaxNode<crate::DdlogLanguage>>,
visible_from_rule_order: usize,
) {
self.declare_symbol(SymbolSpec {
name: binding_name.to_string(),
kind: DeclarationKind::RuleBinding,
origin,
scope,
span: span.clone(),
name_span: syntax.and_then(|syntax| find_identifier_span(syntax, binding_name)),
source_order: self.symbols.len(),
visible_from_rule_order,
});
}❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
Add a new test verifying that nested for-loop bindings lack precise name_span information due to missing CST handles, ensuring semantic model correctly represents symbol spans in this case. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on file fn rule_head_bindings_capture_identifier_name_spans() {
let source = "Output(Output) :- Output(_).";
let parsed = parse_ok(source);
let semantic_model = super::super::build(&parsed);
let symbol = find_symbol(
&semantic_model,
"Output",
DeclarationKind::RuleBinding,
SymbolOrigin::RuleHead,
);
let name_span = symbol
.name_span()
.unwrap_or_else(|| panic!("missing head binding name_span"));
assert_eq!(span_text(source, name_span), "Output");
assert_eq!(name_span.start, 7);
assert_eq!(span_text(source, symbol.span()), source);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…g parameters Refactored declare_pattern_binding method to accept a PatternBindingSpec struct instead of multiple separate parameters. This consolidates related data into a single struct for improved readability and maintainability in traverse.rs. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/sema/tests/name_span.rs: What lead to degradation?The module contains 3 functions with similar structure: assignment_bindings_capture_identifier_name_spans,for_loop_bindings_capture_identifier_name_spans,rule_head_bindings_capture_identifier_name_spans Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Refactor binding declaration tests into a single parameterized test function with multiple cases to improve readability and maintainability. The new test covers rule head bindings, assignment pattern bindings, and for loop bindings uniformly. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ions and bindings This change enhances diagnostic precision by capturing exact spans of declaration and rule-local binding identifiers during parsing and semantic collection rather than re-walking the CST during linting. - Added `find_identifier_span` helpers in `src/parser/ast/mod.rs` for locating identifier tokens within syntax nodes or ranges. - Exposed `name_span()` accessors in AST nodes for functions, relations, and typedefs. - Implemented `RuleHead::binding_spans` in `src/parser/ast/rule_head.rs` to record declaration-like binding identifiers from rule heads. - Modified semantic model builder and traverser to thread and record precise `name_span` information for top-level declarations and rule-local bindings. - Added extensive tests to validate correct span extraction and binding recognition. - Documented implementation details in `docs/parser-implementation-notes.md`. This improves diagnostics by providing accurate source spans that tools and linters can utilize consistently. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai Have the following now been resolved? ❌ Failed checks (3 warnings)
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19872 🏁 Script executed: Length of output: 3057 🏁 Script executed: Length of output: 5142
|
| Symbol | Doc comment |
|---|---|
find_identifier_span |
✓ (src/parser/ast/mod.rs line 24) |
find_identifier_span_in_range |
✓ (line 30) |
Function::name_span |
✓ (function.rs line 32) |
Relation::name_span |
✓ (relation.rs line 61) |
TypeDef::name_span |
✓ (type_def.rs line 28) |
Symbol::name_span |
✓ (model.rs line 214) |
PatternBindingSpec |
✓ (traverse.rs line 23) |
declare_pattern_binding |
✓ (traverse.rs line 195) |
Whether the 80% threshold is crossed now depends on the denominator (total functions across the entire codebase). Run cargo doc --no-deps 2>&1 | grep -c 'missing documentation' or the project's coverage tool to confirm.
✅ Testing — RESOLVED
All three sub-items are now covered:
| Sub-item | Resolution |
|---|---|
collect_head_binding_spans |
1 test in src/parser/ast/rule_head/tests.rs: collect_head_binding_spans_skips_relation_callee_collision ✓ |
should_skip_binding_ident |
2 tests in the same file: should_skip_binding_ident_rejects_callee_but_keeps_argument_binding, should_skip_binding_ident_rejects_dotted_and_constructor_positions ✓ |
name_span() on Function / Relation / TypeDef |
Per-file inline tests present (relation_name_span_points_to_declaration_identifier confirmed; analogous tests confirmed in function.rs and type_def.rs) ✓ |
find_identifier_span edge cases |
5 tests in src/parser/ast/identifier_span.rs (repeated-name subtree, non-overlapping range, exact nested token, absent name, first-match ordering) ✓ |
✅ Developer Documentation — RESOLVED
docs/parser-implementation-notes.md now contains a "Precise identifier span sourcing" section that covers all three required items:
- The
find_identifier_span/find_identifier_span_in_rangehelpers insrc/parser/ast/mod.rs. RuleHead::binding_spanscomputation and thecollect_head_binding_spansfilter logic.- Threading of the
syntaxparameter throughcollect_rule_term,collect_assignment_term, andcollect_for_loop_term, including the deliberatesyntax: Nonesentinel for nested traversals.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/parser-implementation-notes.md`:
- Around line 218-220: Reword the sentence that currently reads "Precise
diagnostic spans for declarations and rule-local bindings are captured once
during syntax or semantic collection rather than by re-walking the CST in lint
rules" to make the parallel explicit; replace "syntax or semantic collection"
with a clearer construction such as "either during syntax collection or during
semantic collection" (or "during syntax collection or during semantic analysis")
and ensure the phrase parallels "captured once" (e.g., "are captured once,
either during syntax collection or during semantic collection, rather than by
re-walking the CST in lint rules") so readers cannot misread the intended
meaning.
In `@src/parser/ast/function.rs`:
- Around line 87-94: The span_text helper is duplicated across function.rs,
relation.rs, type_def.rs, and tests/identifier_span.rs; extract it into a shared
test utility (e.g., add pub fn span_text(...) into src/test_util/mod.rs or
src/test_util/span.rs) and replace the local copies by importing that function
(use crate::test_util::span_text or appropriate module path) in each file
(function.rs, relation.rs, type_def.rs, tests/identifier_span.rs), removing the
duplicate definitions so all tests call the single shared span_text helper.
In `@src/sema/builder.rs`:
- Around line 155-176: The loop currently indexes body_nodes by literal_index
even when body_nodes.len() != terms.len(), which can mis-pair CST nodes to AST
terms; modify the iteration so once the counts diverge you stop using positional
pairing: compute body_nodes_len = rule.body_expression_nodes().len() and inside
the for loop set maybe_node = if literal_index < body_nodes_len {
body_nodes.get(literal_index) } else { None }, and then derive span via
maybe_node.map_or_else(|| rule_span.clone(), ast::RuleBodyExpression::span)
before calling collect_rule_term (keep usage of rule.body_terms(),
rule.body_expression_nodes(), collect_rule_term, VariableUseContext::new,
ast::RuleBodyExpression::span, rule_span, maybe_node as the targeted
identifiers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84d19628-12b3-43dd-8f55-8b70780b10c3
📒 Files selected for processing (11)
docs/parser-implementation-notes.mdsrc/parser/ast/function.rssrc/parser/ast/mod.rssrc/parser/ast/relation.rssrc/parser/ast/rule_head.rssrc/parser/ast/rule_head/tests.rssrc/parser/ast/tests/identifier_span.rssrc/parser/ast/type_def.rssrc/sema/builder.rssrc/sema/tests/name_span.rssrc/sema/traverse.rs
Replaced duplicated local span_text functions in multiple AST test modules with a single shared implementation from src/test_util/mod.rs. This reduces code duplication and improves test utility usage. Also adjusted semantic builder logic to safely handle rule body nodes, ensuring debug assertions remain valid during semantic collection. Minor docs update for identifier span sourcing explanatory text. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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.
Summary
Changes
Core
AST
Parser
Semantic model builder/traversal
Lint rules
Tests
Documentation
Docs: Additional parsing notes updated in docs/parser-implementation-notes.md
Tests plan
Notes
Closes #253
📎 Task: https://www.devboxer.com/task/e372029c-5fe5-45f1-840c-7a52066a4fcf