Add provenance for relation uses and unused-relation diagnostics#245
Merged
Conversation
…entation Add a detailed execution plan for implementing the `unused-relation` lint rule. This document outlines the purpose, scope, design decisions, risks, and milestones for adding the first production correctness lint rule that warns about relations which are declared but never read from, distinguishing read vs write uses semantically. It also defines the semantic provenance metadata needed, testing strategies, and documentation and roadmap update plans. This ExecPlan serves as the blueprint before implementing code, reflecting all the project and quality constraints for item 4.1.1 in the roadmap. No implementation code changes were made; this is a comprehensive specification and planning document to guide future development. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideAdds explicit provenance (UseOrigin) to semantic use sites, wires it through traversal, and builds an Sequence diagram for unused-relation lint rule using semantic provenancesequenceDiagram
actor Developer
participant Parser
participant SemanticModelBuilder
participant SemanticModel
participant CstRuleStore
participant Runner
participant UnusedRelationRule
Developer->>Parser: parse(source)
Parser-->>Developer: ParsedProgram
Developer->>SemanticModelBuilder: build(ParsedProgram)
loop traverse_rules
SemanticModelBuilder->>SemanticModelBuilder: collect_head_expr(expr, ctx)
SemanticModelBuilder->>SemanticModelBuilder: record_top_level_relation_use(ctx, UseOrigin::RelationHead, expr)
SemanticModelBuilder->>SemanticModel: push UseSite(kind=UseKind::Relation, origin=UseOrigin::RelationHead)
SemanticModelBuilder->>SemanticModelBuilder: collect_expression_term(expr, context)
SemanticModelBuilder->>SemanticModelBuilder: record_top_level_relation_use(context, UseOrigin::RelationBody, expr)
SemanticModelBuilder->>SemanticModel: push UseSite(kind=UseKind::Relation, origin=UseOrigin::RelationBody)
SemanticModelBuilder->>SemanticModelBuilder: collect_for_loop(for_loop, context)
SemanticModelBuilder->>SemanticModelBuilder: record_top_level_relation_use(context, UseOrigin::ForIterable, for_loop.iterable)
SemanticModelBuilder->>SemanticModel: push UseSite(kind=UseKind::Relation, origin=UseOrigin::ForIterable)
opt guard_present
SemanticModelBuilder->>SemanticModelBuilder: record_top_level_relation_use(context, UseOrigin::ForGuard, guard)
SemanticModelBuilder->>SemanticModel: push UseSite(kind=UseKind::Relation, origin=UseOrigin::ForGuard)
end
end
SemanticModelBuilder-->>Developer: SemanticModel
Developer->>CstRuleStore: register(UnusedRelationRule)
Developer->>Runner: new(&CstRuleStore, source, &ParsedProgram, RuleConfig)
Developer->>Runner: run()
loop each N_RELATION_DECL node
Runner->>UnusedRelationRule: check_node(node, ctx, diagnostics)
UnusedRelationRule->>SemanticModel: relation_symbol_at_span(span)
SemanticModel-->>UnusedRelationRule: Option<SymbolId>
alt symbol_found
UnusedRelationRule->>SemanticModel: has_resolved_relation_read(symbol_id)
SemanticModel-->>UnusedRelationRule: bool
alt no_read_like_use
UnusedRelationRule->>UnusedRelationRule: format message
UnusedRelationRule->>Runner: push LintDiagnostic
else has_read_like_use
UnusedRelationRule->>Runner: no diagnostic
end
else no_symbol
UnusedRelationRule->>Runner: no diagnostic
end
end
Runner-->>Developer: Vec<LintDiagnostic>
Class diagram for semantic provenance and unused-relation lint ruleclassDiagram
class UseKind {
<<enum>>
Relation
Variable
}
class UseOrigin {
<<enum>>
RelationHead
RelationBody
ForIterable
ForGuard
Variable
+is_relation_read() bool
}
class Resolution {
<<enum>>
Unresolved
Builtin
Resolved
Shadowed
}
class SymbolId {
+0 usize
}
class Symbol {
+name() &str
+kind() DeclarationKind
+span() &Span
}
class DeclarationKind {
<<enum>>
Relation
Function
Type
}
class Span {
}
class ScopeId {
}
class UseSite {
-name String
-kind UseKind
-origin UseOrigin
-scope ScopeId
-span Span
-source_order usize
-resolution Resolution
+kind() UseKind
+origin() UseOrigin
+scope() ScopeId
+span() &Span
+resolution() Resolution
}
class SemanticModel {
-symbols Vec~Symbol~
-uses Vec~UseSite~
+symbols() &Vec~Symbol~
+relation_symbols() Iterator~(SymbolId, &Symbol)~
+uses() &Vec~UseSite~
+symbol(id SymbolId) Option~&Symbol~
+relation_symbol_at_span(span &Span) Option~SymbolId~
+has_resolved_relation_read(symbol_id SymbolId) bool
}
class SemanticModelBuilder {
+collect_head_expr(expr Expr, ctx RuleHeadContext)
+collect_expression_term(expr Expr, context VariableUseContext)
+collect_for_loop(for_loop RuleForLoop, context VariableUseContext)
-record_top_level_relation_use(context VariableUseContext, origin UseOrigin, expr Expr)
}
class Expr {
}
class RuleHeadContext {
+scope() ScopeId
+span() &Span
}
class VariableUseContext {
+current_scope() ScopeId
+span() &Span
+literal_index() usize
}
class Rule {
<<interface>>
}
class LintDiagnostic {
+new(name &str, message String, range TextRange) LintDiagnostic
+rule_name() &str
+message() &str
}
class TextRange {
}
class UnusedRelationRule {
+name() &str
+group() &str
+level() Level
+check_node(node SyntaxNode, ctx RuleCtx, diagnostics &mut Vec~LintDiagnostic~)
}
class RuleCtx {
+semantic_model() &SemanticModel
}
class SyntaxNode {
+text_range() TextRange
}
class SyntaxKind {
<<enum>>
N_RELATION_DECL
}
class Level {
<<enum>>
warn
}
UseSite --> UseKind
UseSite --> UseOrigin
UseSite --> ScopeId
UseSite --> Span
UseSite --> Resolution
SemanticModel --> SymbolId
SemanticModel --> Symbol
SemanticModel --> UseSite
SemanticModel --> Span
SemanticModel --> UseKind
SemanticModel --> UseOrigin
SemanticModel --> Resolution
SemanticModelBuilder --> SemanticModel : builds
SemanticModelBuilder --> Expr
SemanticModelBuilder --> RuleHeadContext
SemanticModelBuilder --> VariableUseContext
SemanticModelBuilder --> UseSite
SemanticModelBuilder --> UseOrigin
Symbol --> DeclarationKind
Symbol --> Span
UnusedRelationRule ..|> Rule
UnusedRelationRule --> LintDiagnostic
UnusedRelationRule --> SyntaxNode
UnusedRelationRule --> RuleCtx
UnusedRelationRule --> SemanticModel
UnusedRelationRule --> SymbolId
UnusedRelationRule --> Symbol
UnusedRelationRule --> SyntaxKind
UnusedRelationRule --> TextRange
RuleCtx --> SemanticModel
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughRecord per-use provenance distinguishing relation-head writes from rule-body/for-loop reads; expose semantic queries and indices to detect resolved relation reads; implement and export an Changes
Sequence Diagram(s)sequenceDiagram
participant Source as DDL Source
participant Parser as Parser
participant Builder as Semantic Builder
participant Model as Semantic Model
participant Rule as Unused-Relation Rule
participant Diag as Lint Diagnostic
Source->>Parser: Parse DDL
Parser->>Builder: Provide CST/AST
Builder->>Builder: Traverse nodes and record uses (with UseOrigin)
Builder->>Model: Build SemanticModel (with span_to_relation_symbol, symbols_with_reads)
Rule->>Model: relation_symbols()
loop For each declared relation
Rule->>Model: relation_symbol_at_span(span)
Rule->>Model: has_resolved_relation_read(symbol_id)
alt No read-like uses
Rule->>Diag: Emit "relation `<name>` is declared but never read from"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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.
… relations - Implement `unused-relation` lint rule to warn when relations are declared but never read from rule bodies, `for` iterables, or guards. - Introduce UseOrigin in semantic model to distinguish relation use provenance (head writes vs body/iterable/guard reads). - Extend semantic model with helpers for querying relation usage and read-status. - Add comprehensive unit and behavioral tests validating rule correctness and semantic use provenance. - Document the new rule contract, semantic provenance, and update design docs and roadmap to mark item 4.1.1 done. - Passed all formatting, linting, and test CI gates. 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. Comment on file &mut self,
context: VariableUseContext<'_>,
use_kind: UseKind,
origin: UseOrigin,❌ New issue: Excess Number of Function Arguments |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
…_use Removed the UseKind::Relation parameter from the record_top_level_relation_use method and hardcoded UseKind::Relation within the method body. This simplifies the method calls by removing unnecessary argument passing where UseKind::Relation was always used. 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.
leynos
changed the title
Refactored calls to record_top_level_relation_use by removing unnecessary multi-line formatting, consolidating them into single lines for improved readability and conciseness. 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.
![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.
![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: e441b98531
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sema/model.rs (1)
297-339: 🧹 Nitpick | 🔵 TrivialIndex relation lookups instead of rescanning the model.
relation_symbol_at_span()andhas_resolved_relation_read()both walk the stored vectors linearly, andUnusedRelationRule::check_node()invokes both for every relation declaration. Precompute a span index and a read-set, or expose unread relations directly, so this rule stays near-linear on large schemas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sema/model.rs` around lines 297 - 339, relation_symbol_at_span() and has_resolved_relation_read() scan vectors on every call, causing UnusedRelationRule::check_node() to be O(n^2); fix by precomputing indexes: add a lookup from Span to SymbolId (e.g., HashMap<Span, SymbolId>) and a read-set of relation SymbolIds (e.g., HashSet<SymbolId>) on the Model, populate them once when the model is built or when uses are finalized (walk self.symbols once to fill span->id and walk self.uses once to insert into read-set when use_site.kind()==UseKind::Relation && use_site.origin().is_relation_read() && use_site.resolution()==Resolution::Resolved(id)), and replace relation_symbol_at_span() and has_resolved_relation_read() to consult these precomputed structures (or add accessors like relation_by_span() and is_relation_read_resolved()) so UnusedRelationRule::check_node() can perform near-linear checks.
🤖 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/ddlint-design.md`:
- Around line 757-766: Add a short caption immediately above the rule-catalogue
markdown table (the block that begins with the header row starting "| Rule Name
| Group | Default Level | Autofixable | Description |" and contains entries like
"unused-relation" and "unused-variable"); insert a one-sentence caption (e.g.,
"Table: DDLint rule catalogue and metadata.") on its own line directly before
the table so the table can be referenced unambiguously in nearby prose.
In `@docs/execplans/4-1-1-implement-unused-relation-diagnostics.md`:
- Around line 12-15: Update the "purpose" wording for roadmap item 4.1.1 to
explicitly state that the change exports an unused-relation lint rule which
callers must register (rather than implying it will emit lint warnings by
default); mention the exported rule name `unused-relation` and that consumers
need to opt-in/register the rule, and apply the same rewording to the other
occurrence of this wording (the second instance referencing the same milestone).
In `@docs/roadmap.md`:
- Around line 359-360: Update roadmap item "4.1.1. Implement `unused-relation`
diagnostics" to use provenance-based read semantics by replacing the phrase "no
usage sites" with "no resolved read-like uses" (or equivalent wording "never
read-like / no resolved read-like uses") so that head-only writes are not
counted as satisfying usage; edit the text in the 4.1.1 bullet to reflect "no
resolved read-like uses" and ensure it references the same 3.3.2 and 3.3.4
requirements and ddlint-design.md §3.3.
In `@tests/semantic_scope_resolution.rs`:
- Around line 86-91: The current assertion uses source_uses.iter().all(...),
which vacuously passes for an empty collection; before the existing check in
tests/semantic_scope_resolution.rs add an explicit non-empty assertion (e.g.,
assert!(!source_uses.is_empty(), "Expected at least one Source use recorded"))
and then keep the existing assert that every use's origin().is_relation_read()
so you fail fast if no uses were collected while preserving the relation-read
validation.
In `@tests/unused_relation_rule.rs`:
- Around line 21-88: Add two additional rstest cases inside the
unused_relation_rule_matches_expected_messages test: one where a relation is
only read via a for-iterable (e.g. a rule body using "for x in SomeRel { ... }"
or the DSL equivalent that iterates over a relation) and one where a relation is
only read via a for-guard (e.g. "for x in OtherRel if cond { ... }"), both
expecting that relation not to be reported as unused; update the #[case(...)]
entries passed to run_rule (using the same concat! pattern and expected_messages
vector) so the test verifies the rule treats for-iterable and for-guard origins
as read-like, referencing the existing test function name
unused_relation_rule_matches_expected_messages and the helper run_rule to run
and assert diagnostics.
---
Outside diff comments:
In `@src/sema/model.rs`:
- Around line 297-339: relation_symbol_at_span() and
has_resolved_relation_read() scan vectors on every call, causing
UnusedRelationRule::check_node() to be O(n^2); fix by precomputing indexes: add
a lookup from Span to SymbolId (e.g., HashMap<Span, SymbolId>) and a read-set of
relation SymbolIds (e.g., HashSet<SymbolId>) on the Model, populate them once
when the model is built or when uses are finalized (walk self.symbols once to
fill span->id and walk self.uses once to insert into read-set when
use_site.kind()==UseKind::Relation && use_site.origin().is_relation_read() &&
use_site.resolution()==Resolution::Resolved(id)), and replace
relation_symbol_at_span() and has_resolved_relation_read() to consult these
precomputed structures (or add accessors like relation_by_span() and
is_relation_read_resolved()) so UnusedRelationRule::check_node() can perform
near-linear checks.
🪄 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: ed4b5701-3494-4d23-962f-049496766e0e
📒 Files selected for processing (15)
docs/ddlint-design.mddocs/execplans/4-1-1-implement-unused-relation-diagnostics.mddocs/parser-implementation-notes.mddocs/roadmap.mdsrc/linter/mod.rssrc/linter/rules/correctness/mod.rssrc/linter/rules/correctness/unused_relation.rssrc/linter/rules/mod.rssrc/sema/mod.rssrc/sema/model.rssrc/sema/tests.rssrc/sema/traverse.rssrc/sema/variables.rstests/semantic_scope_resolution.rstests/unused_relation_rule.rs
… relations The `unused-relation` lint rule (`UnusedRelationRule`) is implemented and exported for registration in the lint runner. It warns about declared relations that have no read-like uses, considering rule bodies, `for` iterables, and guards as reads, but not rule heads. Key changes include: - Semantic model enhancements to efficiently track relation reads and quickly identify unused relations. - A shared behavioral test helper introduced for running the unused-relation rule. - Comprehensive unit and behavioral tests covering the new lint rule semantics. - Documentation and roadmap updates describing the lint rule and its intended usage. This is the first of the correctness lints in the DDLint catalog, enabling better program correctness diagnostics around unused declarations. 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/linter/rules/correctness/unused_relation.rs (1)
1-1:⚠️ Potential issue | 🟡 MinorAlign the rule docs with the resolved read-like contract.
never read fromnow underspecifies the shipped behaviour: head-only writes and unresolved body reads still warn. Mirror thehas_resolved_relation_read()semantics in the module doc and Rustdoc.✏️ Suggested wording
-//! `unused-relation` warns about declared relations that are never read from. +//! `unused-relation` warns about declared relations that have no resolved +//! read-like uses. Rule-head occurrences count as writes, not reads. @@ - /// Detects relations that are declared but never read from. + /// Detects relations that are declared but have no resolved read-like uses. + /// Rule-head occurrences count as writes, not reads.Also applies to: 14-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/linter/rules/correctness/unused_relation.rs` at line 1, Update the module doc and any Rustdoc in the unused-relation rule to reflect the resolved read-like contract used by has_resolved_relation_read(): instead of saying relations that are "never read from", document that the rule only treats reads that are resolved as excluding a relation from warning (so head-only writes and unresolved body reads still trigger unused-relation warnings); explicitly reference has_resolved_relation_read() in the doc to mirror its semantics and ensure the wording matches the implementation in the unused_relation module.src/sema/tests.rs (1)
161-183:⚠️ Potential issue | 🟡 MinorFail fast when these origin vectors are empty.
all(...)passes on an empty collection, so theSink,Source, andItemsassertions can hide a regression where relation uses stop being recorded at all.🔧 Proposed hardening
// `Sink` only appears in rule heads. + assert!(!sink_origins.is_empty(), "expected at least one `Sink` use"); assert!( sink_origins .iter() .all(|origin| matches!(origin, UseOrigin::RelationHead)), "expected all `Sink` uses to be rule heads, got: {sink_origins:?}", ); // `Source` only appears as a body read. + assert!(!source_origins.is_empty(), "expected at least one `Source` use"); assert!( source_origins .iter() .all(|origin| matches!(origin, UseOrigin::RelationBody)), "expected all `Source` uses to be body reads, got: {source_origins:?}", ); // `Items` only appears as the iterable of a `for`. + assert!(!items_origins.is_empty(), "expected at least one `Items` use"); assert!( items_origins .iter() .all(|origin| matches!(origin, UseOrigin::ForIterable)), "expected all `Items` uses to be `for` iterables, got: {items_origins:?}", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sema/tests.rs` around lines 161 - 183, The assertions using all(...) on sink_origins, source_origins, and items_origins can silently succeed for empty vectors; update the checks to first assert that each vector is non-empty and then assert that every element matches the expected UseOrigin variant (UseOrigin::RelationHead for sink_origins, UseOrigin::RelationBody for source_origins, and UseOrigin::ForIterable for items_origins) so a regression that stops recording uses fails fast; locate and modify the assertions referencing sink_origins, source_origins, and items_origins in tests.rs to add the non-empty precondition before the existing all(...) match checks.
🤖 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/execplans/4-1-1-implement-unused-relation-diagnostics.md`:
- Around line 171-180: The doc's proposed provenance list must be updated to
match the shipped UseOrigin contract: replace the current bullet list (and drop
the separate "semantic-rule head/body" distinctions) with the actual exported
variants used on UseSite — RelationHead, RelationBody, ForIterable, ForGuard,
and Variable — and fold any semantic-rule head/body discussion into those
variants if needed; ensure you reference the UseOrigin enum and UseSite
placement so the documentation exactly mirrors the runtime API.
- Line 139: Rename the remaining title-cased headings to sentence case: change
"Decision Log" to "Decision log", each "Milestone N: Add/Implement/Update ..."
to "Milestone N: Add/implement/update ...", "Surprises & Discoveries" to
"Surprises & discoveries", and "Outcomes & Retrospective" to "Outcomes &
retrospective"; apply the same sentence-case changes at the other occurrences
noted (lines referenced in the comment) so all headings in this document follow
the repository's sentence case documentation style.
- Around line 12-13: Replace all occurrences of the American spelling "catalog"
with British "catalogue" across the exec plan prose — specifically update the
sentence mentioning Roadmap item `4.1.1` and the exported lint
`unused-relation`, and also fix the other instances noted (the blocks
corresponding to the comments referencing ranges 121-122 and 290-291); ensure
every prose occurrence (not code identifiers) uses "catalogue" to conform to the
en-GB-oxendict style guide.
In `@src/sema/builder.rs`:
- Around line 73-79: Extract the combined predicate into a helper that returns
Option<SymbolId>: create a private method (e.g., resolve_relation_read(&self,
use_site: &UseSite) -> Option<SymbolId>) that checks use_site.kind() ==
UseKind::Relation, use_site.origin().is_relation_read(), and matches
use_site.resolution() to Resolution::Resolved(symbol_id) and returns
Some(symbol_id) or None; then replace the inline if in the loop over self.uses
(the block that currently inserts into symbols_with_reads) with a call to that
helper and insert the returned symbol_id when Some, so finish() becomes a simple
map/insert using that helper.
In `@src/sema/tests.rs`:
- Around line 134-139: The string passed to the #[with(...)] attribute uses
backslash newline continuations which are forbidden; replace that escaped
multiline literal with a concat!() call that concatenates separate short string
literals (e.g. "Sink(x) :- ", "Source(x), ", "for (y in Items(x)) Check(y), ",
"for (z in Items(x) if Check(z)) Check(z).") so the #[with(...)] invocation
contains concat!(...) instead of a single backslash-continued string; update the
attribute surrounding the rule text accordingly (look for the #[with(...)]
attribute and the rule text containing Sink, Source, Items, Check).
In `@tests/support.rs`:
- Line 1: Update the module doc comment at the top of tests/support.rs to use
en-GB spelling: change the word "behavioral" to "behavioural" in the
module-level doc string so the module doc follows the repo's en-GB-oxendict
convention.
---
Duplicate comments:
In `@src/linter/rules/correctness/unused_relation.rs`:
- Line 1: Update the module doc and any Rustdoc in the unused-relation rule to
reflect the resolved read-like contract used by has_resolved_relation_read():
instead of saying relations that are "never read from", document that the rule
only treats reads that are resolved as excluding a relation from warning (so
head-only writes and unresolved body reads still trigger unused-relation
warnings); explicitly reference has_resolved_relation_read() in the doc to
mirror its semantics and ensure the wording matches the implementation in the
unused_relation module.
In `@src/sema/tests.rs`:
- Around line 161-183: The assertions using all(...) on sink_origins,
source_origins, and items_origins can silently succeed for empty vectors; update
the checks to first assert that each vector is non-empty and then assert that
every element matches the expected UseOrigin variant (UseOrigin::RelationHead
for sink_origins, UseOrigin::RelationBody for source_origins, and
UseOrigin::ForIterable for items_origins) so a regression that stops recording
uses fails fast; locate and modify the assertions referencing sink_origins,
source_origins, and items_origins in tests.rs to add the non-empty precondition
before the existing all(...) match checks.
🪄 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: 5ce412d3-814e-4cee-a072-19b24f88c087
📒 Files selected for processing (10)
docs/ddlint-design.mddocs/execplans/4-1-1-implement-unused-relation-diagnostics.mddocs/roadmap.mdsrc/linter/rules/correctness/unused_relation.rssrc/sema/builder.rssrc/sema/model.rssrc/sema/tests.rstests/semantic_scope_resolution.rstests/support.rstests/unused_relation_rule.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the current state of the relevant files. 🏁 Script executed: Length of output: 6993
Overall performance comments — ✅ Both addressed via precomputation in
Comment 1 — Comment 2 — deduplicate Comment 3 — article "a" → "an" — ✅ Resolved. The sentence was reworded and the article-before-backtick issue no longer appears. Comment 4 — expand "CLI" acronym — ✅ Resolved. The text now reads "command-line interface (CLI) rule listing". en-GB-oxendict / cyclomatic-complexity filter — no requirements to annul. None of the suggestions introduced non-en-GB-oxendict spellings or unnecessary branching. |
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 if use_site.kind() == UseKind::Relation
&& use_site.origin().is_relation_read()
&& let Resolution::Resolved(symbol_id) = use_site.resolution()❌ New issue: Complex Conditional |
This comment was marked as resolved.
This comment was marked as resolved.
…l updates - Add `unused-relation` lint rule detecting declared relations with no resolved read-like uses. - Extend semantic model with `UseOrigin` enum variants to distinguish relation use origins (head writes vs body reads, for iterables, guards, variables). - Introduce semantic helpers in `SemanticModelBuilder` for tracking resolved relation reads. - Implement `UnusedRelationRule` using the new semantic queries. - Add focused unit and behavioral tests to verify read vs write use distinctions. - Update documentation and roadmap to clarify read-versus-write distinction semantics. This change improves correctness linting by accurately differentiating read and write usages of relations, enabling meaningful unused relation warnings. 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.
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/execplans/4-1-1-implement-unused-relation-diagnostics.md`:
- Line 61: Replace the American spelling "rule-catalog" with the British English
"rule-catalogue" wherever the string occurs in this document (specifically
replace the token "rule-catalog" used in the sentence "There is no shipped
rule-catalog module yet.") so it adheres to the en-GB-oxendict -ogue convention.
- Around line 407-411: The list of gate command lines has inconsistent
indentation: remove the extra leading whitespace before the backticks on the
`set -o pipefail; make nixie 2>&1 | tee /tmp/4-1-1-make-nixie.log` line so it
matches the surrounding entries (ensure all five backticked command lines have
the same left alignment).
In `@src/linter/rules/correctness/unused_relation.rs`:
- Around line 17-20: Remove the local private function text_range_to_span and
instead import the existing pub(crate) implementation by adding use
crate::parser::ast::rule::text_range_to_span; at the top of the file; ensure any
local references (text_range_to_span) now resolve to that imported symbol so you
don't duplicate the function already defined in rule.rs.
🪄 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: b43e63a4-c325-47c0-9a61-8f0088cbb317
📒 Files selected for processing (5)
docs/execplans/4-1-1-implement-unused-relation-diagnostics.mdsrc/linter/rules/correctness/unused_relation.rssrc/sema/builder.rssrc/sema/tests.rstests/support.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 if use_site.kind() == UseKind::Relation
&& use_site.origin().is_relation_read()
&& let Resolution::Resolved(symbol_id) = use_site.resolution()❌ New issue: Complex Conditional |
This comment was marked as resolved.
This comment was marked as resolved.
…ad filtering Removed the unused `resolve_relation_read` helper function from `SemanticModelBuilder` and refactored relation read filtering logic inline for clarity and conciseness. Also removed redundant code in `unused_relation` lint rule (removing a local `text_range_to_span` that duplicates an imported function). Minor docs fix to wording in milestone documentation. 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/execplans/4-1-1-implement-unused-relation-diagnostics.md (2)
152-156:⚠️ Potential issue | 🟡 MinorReplace
rule-catalogwithrule-catalogue.Fix Line 155 to keep en-GB-oxendict spelling consistent in prose.
✏️ Proposed fix
- rule-catalog registration surface, and adding one would broaden scope beyond + rule-catalogue registration surface, and adding one would broaden scope beyondAs per coding guidelines, "Use British English based on the Oxford English Dictionary (en-GB-oxendict): use ... -ogue endings (analogue, catalogue)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/4-1-1-implement-unused-relation-diagnostics.md` around lines 152 - 156, Replace the American spelling "rule-catalog" with the British Oxford spelling "rule-catalogue" in the decision prose that begins "Decision: expose the first production rule..." (the sentence that also mentions CstRuleStore); update that token so the document uses en-GB-oxendict "-ogue" spelling consistently.
406-410:⚠️ Potential issue | 🟡 MinorFix misaligned command indentation in the gate list.
Remove the extra leading space on Line 409 to keep list continuation formatting consistent.
✏️ Proposed fix
- Passed gate commands: `set -o pipefail; make fmt 2>&1 | tee /tmp/4-1-1-final-make-fmt.log` `set -o pipefail; make markdownlint 2>&1 | tee /tmp/4-1-1-make-markdownlint.log` - `set -o pipefail; make nixie 2>&1 | tee /tmp/4-1-1-make-nixie.log` + `set -o pipefail; make nixie 2>&1 | tee /tmp/4-1-1-make-nixie.log` `set -o pipefail; make check-fmt 2>&1 | tee /tmp/4-1-1-final-check-fmt.log`As per coding guidelines, "Follow markdownlint-cli2 recommendations in documentation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/4-1-1-implement-unused-relation-diagnostics.md` around lines 406 - 410, Fix the misaligned markdown list item in the gate commands by removing the extra leading space before the backticked command on the line containing "`set -o pipefail; make nixie 2>&1 | tee /tmp/4-1-1-make-nixie.log`" so it is aligned with the other list entries in the gate list within the docs/execplans/4-1-1-implement-unused-relation-diagnostics.md file; ensure the three backticked command lines all start at the same indentation level to satisfy markdownlint-cli2 recommendations.
🤖 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/execplans/4-1-1-implement-unused-relation-diagnostics.md`:
- Around line 61-64: Replace the first occurrence of the acronym "CLI" with its
expanded form "Command-Line Interface (CLI)" in the document (so readers see the
full phrase at first use), and leave subsequent occurrences as "CLI"; search for
the string "CLI" in the section around the noted paragraph to locate the
instance to update.
- Around line 380-384: Update the paragraph discussing the top-level for
desugaring so it records a historical observation rather than asserting current
behavior: change the wording around the `ForIterable` origin to state that early
experiments recorded iterable relation reads as semantic-rule body reads, but
the shipped contract treats those uses as read-like and tests should assert
read-versus-write semantics (not the desugaring detail). Reference `ForIterable`
and the durable rule contract in the rewrite to make clear this is a historical
note, not the documented current behavior.
---
Duplicate comments:
In `@docs/execplans/4-1-1-implement-unused-relation-diagnostics.md`:
- Around line 152-156: Replace the American spelling "rule-catalog" with the
British Oxford spelling "rule-catalogue" in the decision prose that begins
"Decision: expose the first production rule..." (the sentence that also mentions
CstRuleStore); update that token so the document uses en-GB-oxendict "-ogue"
spelling consistently.
- Around line 406-410: Fix the misaligned markdown list item in the gate
commands by removing the extra leading space before the backticked command on
the line containing "`set -o pipefail; make nixie 2>&1 | tee
/tmp/4-1-1-make-nixie.log`" so it is aligned with the other list entries in the
gate list within the
docs/execplans/4-1-1-implement-unused-relation-diagnostics.md file; ensure the
three backticked command lines all start at the same indentation level to
satisfy markdownlint-cli2 recommendations.
🪄 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: 462dfa0e-2cc8-4467-8e0b-d4c25f18d78d
📒 Files selected for processing (3)
docs/execplans/4-1-1-implement-unused-relation-diagnostics.mdsrc/linter/rules/correctness/unused_relation.rssrc/sema/builder.rs
Owner
Author
|
@CodeRabbit Re: can you suggest an alternative solution using an extracted predicate function? |
This comment was marked as resolved.
This comment was marked as resolved.
…ostics - Expanded CLI to Command-Line Interface for clarity. - Corrected spelling of "rule-catalogue". - Reworded observations on for-loop desugaring for better understanding. - Converted multiline commands under Outcomes & retrospective to a fenced code block for proper markdown formatting. 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.
… logic Introduced a new helper method `is_relation_read_use` to encapsulate the check for relation read uses. This refactor improves code readability by avoiding inline complex filter expressions in the `finish` method. 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.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/execplans/4-1-1-implement-unused-relation-diagnostics.md`:
- Around line 190-197: Update the implemented-state docs to remove the
non-shipped helper relation_reads() and replace it with the shipped helpers:
keep model.relation_symbols() and model.has_resolved_relation_read(symbol_id)
and add model.relation_symbol_at_span(span) instead of relation_reads(); ensure
the snippet exactly matches the shipped API so readers won't see
relation_reads() referenced where it does not exist.
🪄 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: caea4e61-d212-483c-a18f-2e2caf7dbd55
📒 Files selected for processing (2)
docs/execplans/4-1-1-implement-unused-relation-diagnostics.mdsrc/sema/builder.rs
…diagnostics Update documentation to specify the shipped API for semantic helpers in the unused relation diagnostics rule. The commit revises method descriptions and guidance to encourage using `relation_symbol_at_span(span)` for symbol lookups instead of ad hoc filtering, making the rule implementation clearer and more maintainable. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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
Rationale
Implementation plan (summary)
Validation plan
Progress
📎 Task: https://www.devboxer.com/task/dc1f967e-59b4-4962-badb-63f1a0e6adcd
Summary by Sourcery
Add semantic provenance for relation use sites and implement an
unused-relationlint rule that flags declared relations which are never read, updating semantic queries, tests, and documentation accordingly.New Features:
unused-relationcorrectness lint rule that reports declared relations with no resolved read-like uses.Enhancements:
UseOriginenum onUseSiteto distinguish relation writes in heads from reads in bodies andforiterables/guards, and to tag variable uses.forconstructs, and propagate it through the public API.Documentation:
unused-relation.unused-relationand mark the corresponding roadmap item as completed.Tests:
unused-relationdiagnostics and their ordering.