From ad8c8a6d076003595d7ca1e2982e713d5d0d2a3c Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 21 Mar 2026 11:34:46 +0000 Subject: [PATCH 01/10] docs(execplans): add exec plan for unused-relation diagnostics implementation 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] --- ...1-implement-unused-relation-diagnostics.md | 375 ++++++++++++++++++ 1 file changed, 375 insertions(+) create mode 100644 docs/execplans/4-1-1-implement-unused-relation-diagnostics.md diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md new file mode 100644 index 00000000..a63a8fce --- /dev/null +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -0,0 +1,375 @@ +# Implement `unused-relation` diagnostics + +This ExecPlan (execution plan) is a living document. The sections +`Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, +`Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work +proceeds. + +Status: DRAFT + +## Purpose / big picture + +Roadmap item `4.1.1` is the first production lint rule in the initial +correctness catalog. After this change, a contributor should be able to lint a +DDlog file and receive a `unused-relation` warning for each declared relation +that is never read from anywhere in the analysed program. + +For this milestone, "read from" means a resolved relation-position use in a +rule body, a `for` iterable, or a `for` guard. A relation named in a rule head +is a write site, not a read site, so head-only relations must still be warned +about. Observable success is: + +- `ddlint` exports a concrete `unused-relation` rule that can be registered in + `CstRuleStore`. +- Running `Runner` with that rule emits one warning per unread relation + declaration and no warnings for relations with at least one resolved read. +- The semantic model exposes enough provenance to distinguish relation reads + from writes without re-walking the concrete syntax tree inside the rule. +- Unit tests cover semantic read-versus-write classification and the rule's + positive and negative cases. +- Behavioural tests prove end-to-end diagnostics through `Runner`. +- `docs/ddlint-design.md` records the final rule contract and the semantic-use + provenance added for it. +- `docs/roadmap.md` marks `4.1.1` done only after all quality gates pass. + +## Approval gate + +This document is a draft only. Do not begin implementation until the user +explicitly approves this ExecPlan or requests a revised version and then +approves the revision. + +## Context and orientation + +The current repository already contains the semantic substrate this rule will +build on: + +- `src/sema/build.rs` builds one `SemanticModel` from a parsed program. +- `src/sema/builder.rs` records top-level relation, function, and type + declarations. +- `src/sema/traverse.rs` records relation and variable uses while walking rule + heads and rule bodies. +- `src/sema/model.rs` stores `Symbol` and `UseSite` records, but today a + `UseSite` does not record whether it came from a rule head or a read-like + body position. +- `src/linter/rule.rs`, `src/linter/store.rs`, and `src/linter/runner.rs` + provide the rule trait surface, dispatch store, and parallel runner. +- `src/linter/macros.rs` provides `declare_lint!`, which should be used for + the new rule unless a documented blocker appears. + +There is no shipped rule-catalog module yet. Current behavioural tests register +ad hoc rules directly in `CstRuleStore`. This milestone therefore needs to add +the first exported production rule module and corresponding tests, but it does +not need to invent a full CLI-configured default ruleset. + +The key design gap is semantic provenance. `docs/ddlint-design.md` says +`unused-relation` detects relations "defined but never read from", yet the +current semantic layer records relation uses from both rule heads and rule +bodies with the same `UseKind::Relation`. Counting all relation-position uses +would therefore under-report unused relations by treating writes as reads. + +## Constraints + +- Treat `docs/ddlint-design.md` section `3.2.1` and section `3.3` plus + `docs/roadmap.md` item `4.1.1` as the normative design basis for this + milestone. +- Keep scope limited to implementing `unused-relation`, the additive semantic + provenance needed for it, its tests, and the required documentation and + roadmap updates. +- Do not implement `unused-variable`, `shadowed-variable`, CLI rule listing, + configuration-file loading, or rich `miette` conversion in this milestone. +- Keep parser grammar and parse-stage diagnostics unchanged unless a bug blocks + the rule and there is no narrower fix. +- Extend the semantic model additively. Existing `RuleCtx`, `Runner`, and + parser APIs must remain source-compatible. +- Preserve the current owned-data and `Send + Sync` guarantees of + `SemanticModel`; do not store `rowan` nodes inside semantic records. +- Use `declare_lint!` for the production rule unless a documented limitation + of the macro makes that impossible. +- Every new Rust module must begin with a `//!` module comment. +- Keep files below 400 lines by splitting rule modules, helpers, and tests as + needed. +- Update `docs/ddlint-design.md` with any rule-semantics decision taken during + implementation. +- Mark `docs/roadmap.md` item `4.1.1` done only after all gates pass. +- Run quality gates through Make targets, using `set -o pipefail` and `tee`. + +## Tolerances (exception triggers) + +- Scope: if implementation requires more than 10 changed files or more than + 650 net new lines, stop and re-evaluate the module split before continuing. +- Interface: if the rule cannot be implemented without a breaking change to + `RuleCtx`, `Runner`, `SemanticModel`, or `declare_lint!`, stop and escalate. +- Semantics: if the team decides that `output relation` declarations should be + exempt because of external consumers, stop and get explicit confirmation + before encoding that policy. +- Provenance: if read-versus-write classification cannot be represented + additively in `UseSite`, stop and redesign the semantic query surface before + proceeding. +- Iterations: if targeted tests or Clippy fixes still fail after three focused + rounds, stop and escalate with the exact failing test names or lint IDs. + +## Risks + +- Risk: head relation atoms are currently recorded as generic relation uses, so + a naïve implementation would treat writes as reads and miss real unused + relations. Severity: high. Likelihood: high. Mitigation: add explicit use + provenance and write failing tests before adding the rule. + +- Risk: the roadmap wording says "declared relations with no usage sites", + while the design catalog says "never read from". Severity: medium. + Likelihood: medium. Mitigation: document and implement the narrower + read-based rule semantics, because that is the more precise statement and + avoids counting rule-head writes as reads. + +- Risk: `output relation` declarations may represent externally consumed data, + but the current design documents do not define an exemption. Severity: + medium. Likelihood: medium. Mitigation: keep the milestone scoped to internal + program analysis and document that no external-consumer exemption is applied + unless the user approves a policy change. + +- Risk: the current runner API has only per-node and per-token hooks, so a + rule that repeatedly scans the full semantic model could become noisy or + awkward to maintain. Severity: low. Likelihood: medium. Mitigation: add small + semantic query helpers that make the rule implementation direct and testable, + but avoid introducing a broader precomputed lint-cache layer. + +## Decision Log + +- Decision: treat rule-head relation atoms as write sites, not read sites, for + `unused-relation`. Rationale: `docs/ddlint-design.md` defines the rule as + "defined but never read from", and counting heads as reads would make sink + relations appear used when they are only written. Date/Author: 2026-03-21 / + Codex. + +- Decision: do not add a special exemption for `output relation` declarations + in this milestone. Rationale: no current design document defines such an + exemption, and roadmap item `4.1.1` is phrased in terms of declarations plus + internal usage sites. Date/Author: 2026-03-21 / Codex. + +- Decision: expose the first production rule as a normal exported rule type + that tests register explicitly in `CstRuleStore`, rather than inventing a + global default ruleset now. Rationale: the current repository has no shipped + rule-catalog registration surface, and adding one would broaden scope beyond + `4.1.1`. Date/Author: 2026-03-21 / Codex. + +## Proposed design + +Add a small production-rules namespace under `src/linter/` so the rule has a +stable home and future correctness rules can follow the same pattern. A minimal +layout is: + +- `src/linter/rules/mod.rs` +- `src/linter/rules/correctness/mod.rs` +- `src/linter/rules/correctness/unused_relation.rs` + +Export the new module from `src/linter/mod.rs` so integration tests and later +CLI wiring can import the rule cleanly. + +Extend the semantic model with additive relation-use provenance. The smallest +useful shape is a new enum such as `UseOrigin` or `RelationUseOrigin` stored on +every `UseSite`. The enum should at least distinguish: + +- rule head writes from AST-backed rules; +- rule body reads from AST-backed rules; +- `for` iterable reads; +- `for` guard reads; +- semantic-rule head writes from top-level `for` desugaring; and +- semantic-rule body reads from top-level `for` desugaring. + +The exact enum names can change during implementation, but the model must allow +the rule to answer one clear question without inspecting syntax nodes: "does +this relation declaration have any resolved read-like uses?" + +Add semantic helper methods that keep rule code simple. The final names may +change, but the rule should be able to call helpers equivalent to: + +```rust +model.relation_symbols() +model.relation_reads() +model.has_resolved_relation_read(symbol_id) +``` + +If lookup by span is needed to associate an `N_RELATION_DECL` node with its +relation symbol, prefer a small helper such as +`SemanticModel::relation_symbol_at_span(span)` over ad hoc filtering inside the +rule. + +Implement the rule itself with `declare_lint!`. It should target +`SyntaxKind::N_RELATION_DECL`, use metadata `name: "unused-relation"`, +`group: "correctness"`, and `level: warn`, then emit one diagnostic per unread +relation declaration with a message equivalent to: + +```plaintext +relation `Foo` is declared but never read from +``` + +The diagnostic span should be the declaration node range, which already matches +the recorded relation declaration span in the semantic model. + +The rule must ignore malformed declarations that do not map cleanly to a named +relation symbol. Silent non-emission is preferable to a panic when semantic +facts are incomplete because of parse recovery. + +## Implementation plan + +### Milestone 1: Add semantic use provenance + +Update `src/sema/model.rs`, `src/sema/traverse.rs`, and any supporting helpers +so relation uses carry enough provenance to distinguish reads from writes. Keep +the existing `UseKind::Relation` and `UseKind::Variable` split; this milestone +only needs extra origin metadata, not a new top-level use-kind taxonomy. + +Adjust semantic-model unit tests in `src/sema/tests.rs` and behavioural tests +in `tests/semantic_scope_resolution.rs` so they assert the new provenance +contract. At minimum, add coverage proving: + +- a relation in a rule head is recorded as a write; +- the same relation name in a rule body is recorded as a read; and +- top-level `for` semantic rules preserve the same distinction. + +### Milestone 2: Add semantic query helpers for the rule + +Add focused helper methods on `SemanticModel` that answer the questions +`unused-relation` actually needs. Keep them additive and deterministic. The +goal is that the rule module reads as straightforward policy code rather than a +manually repeated scan over `symbols()` and `uses()`. + +Write unit tests for those helpers near the semantic-model tests. Include at +least these cases: + +- a relation with one resolved body read returns true for "has resolved read"; +- a relation mentioned only in rule heads returns false; and +- an unresolved relation-position use does not count as a read. + +### Milestone 3: Implement the production rule module + +Create the production rule module under `src/linter/rules/correctness/` and +export it through `src/linter/mod.rs`. Use `declare_lint!` for metadata and +dispatch boilerplate. + +In the rule body: + +1. Read the declaration node's span. +2. Resolve that span to the corresponding relation symbol via a semantic-model + helper. +3. Ask whether the symbol has any resolved read-like uses. +4. Emit a warning diagnostic only when the answer is no. + +Keep helper functions in the same rule module if they are specific to this +rule. If they become reusable across multiple correctness rules, move them to a +small sibling helper module only after the first rule works and the need is +real. + +### Milestone 4: Add rule-focused tests + +Add unit tests close to the rule module and behavioural tests under `tests/` +that run the real parser and runner. Use `rstest` fixtures and parameterized +cases where they reduce repetition. + +The minimum coverage set is: + +- positive: a declared relation with no reads emits exactly one + `unused-relation` diagnostic; +- negative: a relation read in a rule body emits no diagnostic; +- head-only write: a relation that appears only in rule heads still emits a + diagnostic; +- unresolved name safety: an unresolved relation-position use does not mark a + declaration as used; +- multi-relation ordering: diagnostics are deterministic when several unused + relations exist. + +Use a dedicated behavioural test file, for example +`tests/unused_relation_rule.rs`, rather than burying these cases inside the +generic runner tests. + +### Milestone 5: Update docs and roadmap + +Update `docs/ddlint-design.md` in the semantic-model section and the initial +lint catalog section so the implemented read-versus-write distinction is +explicit. If the semantic-model contract section already describes relation +uses too loosely, tighten that wording there rather than spreading the rule +semantics across multiple unrelated docs. + +If semantic provenance becomes a durable invariant rather than a rule-specific +detail, add a short note to `docs/parser-implementation-notes.md` explaining +that relation use sites now record whether they are reads or writes. This is +the right place for implementation-level semantic invariants that future rules +will rely on. + +After all tests and gates pass, change `docs/roadmap.md` item `4.1.1` from +unchecked to done. + +## Validation plan + +Start with targeted tests that go red before the implementation is complete, +then finish with the full repository gates. + +Suggested targeted commands during development: + +```bash +set -o pipefail; cargo test sema::tests 2>&1 | tee /tmp/4-1-1-sema-unit.log +set -o pipefail; cargo test --test semantic_scope_resolution 2>&1 | tee /tmp/4-1-1-sema-behaviour.log +set -o pipefail; cargo test unused_relation 2>&1 | tee /tmp/4-1-1-unused-relation-targeted.log +``` + +Required final commands: + +```bash +set -o pipefail; make fmt 2>&1 | tee /tmp/4-1-1-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 check-fmt 2>&1 | tee /tmp/4-1-1-make-check-fmt.log +set -o pipefail; make lint 2>&1 | tee /tmp/4-1-1-make-lint.log +set -o pipefail; make test 2>&1 | tee /tmp/4-1-1-make-test.log +``` + +Acceptance evidence for the rule should include at least one behavioural test +with source equivalent to: + +```plaintext +input relation Source(x: u32) +relation Sink(x: u32) +Sink(x) :- Source(x). +``` + +Expected outcome: `Source` is not warned because it is read in the body; `Sink` +is warned because it is only written in the head. + +## Progress + +- [x] (2026-03-21 00:00Z) Reviewed roadmap item `4.1.1`, the `execplans` + skill, current semantic-analysis code, and adjacent ExecPlans. +- [x] (2026-03-21 00:10Z) Identified the key semantic gap: relation uses do + not yet distinguish reads from writes. +- [x] (2026-03-21 00:20Z) Wrote this draft ExecPlan to + `docs/execplans/4-1-1-implement-unused-relation-diagnostics.md`. +- [ ] Await user approval before implementation. +- [ ] Implement semantic provenance, rule module, tests, and documentation. +- [ ] Run full quality gates and mark roadmap item `4.1.1` done. + +## Surprises & Discoveries + +- Observation: roadmap prerequisites `3.3.2` and `3.3.4` are already complete, + and the semantic model does record relation declarations and relation uses. + Impact: the missing work is rule-policy plumbing and provenance, not parser + or symbol-table construction. + +- Observation: `src/sema/traverse.rs` currently records relation uses from rule + heads via `collect_head_expr`, so the semantic model does not yet encode the + "read from" language used by the rule catalog. Impact: `unused-relation` + cannot be implemented correctly without extending `UseSite`. + +- Observation: the current linter module exports engine primitives only; there + is no production rule namespace yet. Impact: this milestone should introduce + a small `src/linter/rules/` module tree, but should not broaden into a full + default ruleset or CLI registry. + +## Outcomes & Retrospective + +Not started. Update this section after implementation with: + +- the final rule module paths; +- the shipped semantic provenance shape; +- test files and what they verify; +- documentation and roadmap updates; and +- the exact gate commands that passed. From 23649bee3159ceb81738b1f3a3aaa7c7647625d1 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 22 Mar 2026 03:09:29 +0000 Subject: [PATCH 02/10] feat(linter): add unused-relation lint rule detecting unused declared 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] --- docs/ddlint-design.md | 33 +++-- ...1-implement-unused-relation-diagnostics.md | 58 +++++++-- docs/parser-implementation-notes.md | 2 + docs/roadmap.md | 2 +- src/linter/mod.rs | 1 + src/linter/rules/correctness/mod.rs | 5 + .../rules/correctness/unused_relation.rs | 120 ++++++++++++++++++ src/linter/rules/mod.rs | 3 + src/sema/mod.rs | 2 +- src/sema/model.rs | 60 +++++++++ src/sema/tests.rs | 89 ++++++++++++- src/sema/traverse.rs | 26 +++- src/sema/variables.rs | 3 +- tests/semantic_scope_resolution.rs | 31 ++++- tests/unused_relation_rule.rs | 88 +++++++++++++ 15 files changed, 488 insertions(+), 35 deletions(-) create mode 100644 src/linter/rules/correctness/mod.rs create mode 100644 src/linter/rules/correctness/unused_relation.rs create mode 100644 src/linter/rules/mod.rs create mode 100644 tests/unused_relation_rule.rs diff --git a/docs/ddlint-design.md b/docs/ddlint-design.md index 8d84754a..6f709698 100644 --- a/docs/ddlint-design.md +++ b/docs/ddlint-design.md @@ -711,7 +711,18 @@ The semantic model records: - rule-local bindings introduced by rule heads, assignment patterns, and `for`-loop patterns; - explicit scope records with parent links; and -- relation and variable use sites together with a final resolution result. +- relation and variable use sites together with a final resolution result and + use-site provenance. + +Relation use-site provenance is part of the contract for correctness rules: + +- rule-head relation atoms are recorded as write sites; +- rule-body atoms are recorded as read sites; and +- `for` iterable and guard relation atoms are recorded as read sites. + +This distinction is what allows `unused-relation` to warn on head-only sink +relations while still treating resolved body, iterable, and guard references as +genuine reads. Resolution is deliberately tri-state: @@ -743,16 +754,16 @@ immediate value to users, the following catalog of rules is proposed. This list prioritizes correctness checks, followed by performance hints, and stylistic suggestions, establishing a solid foundation of essential lints. -| Rule Name | Group | Default Level | Autofixable | Description | -| ---------------------- | ----------- | ------------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------- | -| unused-relation | correctness | warn | No | Detects relations that are defined but never read from. | -| unused-variable | correctness | warn | No | Detects variables bound in a rule head that are not used in the body. | -| shadowed-variable | correctness | warn | No | Detects when a variable binding in a literal shadows one from a preceding literal in the same rule body. | -| recursive-negation | correctness | error | No | Detects rules with recursion through negation, which leads to unsafe, non-monotonic programs. | -| inefficient-join-order | performance | hint | No | Evaluates rule bodies and suggests reordering atoms for a more efficient join plan, e.g., placing more restrictive literals first. | -| superfluous-group-by | performance | warn | Yes | Detects group_by clauses where the aggregation is trivial (e.g., grouping by all variables) and can be removed. | -| consistent-casing | style | allow | Yes | Enforces a consistent casing style for relation and type identifiers (e.g., PascalCase) and variables (e.g., snake_case). | -| no-magic-numbers | style | allow | No | Flags the use of unnamed numeric literals in rule bodies where a named constant might be clearer. | +| Rule Name | Group | Default Level | Autofixable | Description | +| ---------------------- | ----------- | ------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------- | +| unused-relation | correctness | warn | No | Detects declared relations with no resolved read-like uses in rule bodies, `for` iterables, or `for` guards; rule heads count only as writes. | +| unused-variable | correctness | warn | No | Detects variables bound in a rule head that are not used in the body. | +| shadowed-variable | correctness | warn | No | Detects when a variable binding in a literal shadows one from a preceding literal in the same rule body. | +| recursive-negation | correctness | error | No | Detects rules with recursion through negation, which leads to unsafe, non-monotonic programs. | +| inefficient-join-order | performance | hint | No | Evaluates rule bodies and suggests reordering atoms for a more efficient join plan, e.g., placing more restrictive literals first. | +| superfluous-group-by | performance | warn | Yes | Detects group_by clauses where the aggregation is trivial (e.g., grouping by all variables) and can be removed. | +| consistent-casing | style | allow | Yes | Enforces a consistent casing style for relation and type identifiers (e.g., PascalCase) and variables (e.g., snake_case). | +| no-magic-numbers | style | allow | No | Flags the use of unnamed numeric literals in rule bodies where a named constant might be clearer. | This table serves as a concrete work breakdown for the engineering team and clearly communicates the linter's initial capabilities and priorities to early diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md index a63a8fce..4fd34d11 100644 --- a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -5,7 +5,7 @@ This ExecPlan (execution plan) is a living document. The sections `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. -Status: DRAFT +Status: Implemented ## Purpose / big picture @@ -34,9 +34,8 @@ about. Observable success is: ## Approval gate -This document is a draft only. Do not begin implementation until the user -explicitly approves this ExecPlan or requests a revised version and then -approves the revision. +Approved by the user on 2026-03-22 when they requested implementation of this +ExecPlan. ## Context and orientation @@ -343,9 +342,16 @@ is warned because it is only written in the head. not yet distinguish reads from writes. - [x] (2026-03-21 00:20Z) Wrote this draft ExecPlan to `docs/execplans/4-1-1-implement-unused-relation-diagnostics.md`. -- [ ] Await user approval before implementation. -- [ ] Implement semantic provenance, rule module, tests, and documentation. -- [ ] Run full quality gates and mark roadmap item `4.1.1` done. +- [x] (2026-03-22 00:05Z) User approved implementation by requesting execution + of this plan. +- [x] (2026-03-22 00:40Z) Implemented semantic use-site provenance, + relation-read query helpers, the exported `unused-relation` rule module, and + unit plus behavioural coverage. +- [x] (2026-03-22 00:50Z) Updated `docs/ddlint-design.md`, + `docs/parser-implementation-notes.md`, and `docs/roadmap.md` to reflect the + shipped contract. +- [x] (2026-03-22 01:25Z) Ran `make fmt`, `make markdownlint`, `make nixie`, + `make check-fmt`, `make lint`, and `CI=1 make test`; all passed. ## Surprises & Discoveries @@ -364,12 +370,36 @@ is warned because it is only written in the head. a small `src/linter/rules/` module tree, but should not broaden into a full default ruleset or CLI registry. -## Outcomes & Retrospective +- Observation: top-level `for` desugaring currently records iterable relation + reads as semantic-rule body reads rather than a dedicated `ForIterable` + origin. Impact: the durable rule contract is still satisfied because those + uses remain read-like, but tests should assert read-versus-write semantics + rather than overfitting to the current lowering detail. -Not started. Update this section after implementation with: +## Outcomes & Retrospective -- the final rule module paths; -- the shipped semantic provenance shape; -- test files and what they verify; -- documentation and roadmap updates; and -- the exact gate commands that passed. +- Final rule modules: + `src/linter/rules/mod.rs`, `src/linter/rules/correctness/mod.rs`, and + `src/linter/rules/correctness/unused_relation.rs`. +- Shipped semantic provenance shape: `UseSite` now carries `UseOrigin` with + `RelationHead`, `RelationBody`, `ForIterable`, `ForGuard`, and `Variable`. + `SemanticModel` now exposes `relation_symbols()`, + `relation_symbol_at_span()`, and `has_resolved_relation_read()`. +- Test coverage: + `src/sema/tests.rs` covers relation use origins and helper queries; + `tests/semantic_scope_resolution.rs` covers end-to-end semantic provenance; + `src/linter/rules/correctness/unused_relation.rs` contains focused rule unit + tests; and `tests/unused_relation_rule.rs` covers end-to-end diagnostics and + deterministic ordering through `Runner`. +- Documentation updates: + `docs/ddlint-design.md` now documents relation read-versus-write provenance + and the exact `unused-relation` contract; + `docs/parser-implementation-notes.md` records relation-use provenance as a + current semantic invariant; and `docs/roadmap.md` marks item `4.1.1` done. +- 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 check-fmt 2>&1 | tee /tmp/4-1-1-final-check-fmt.log` + `set -o pipefail; make lint 2>&1 | tee /tmp/4-1-1-final-lint.log` + `set -o pipefail; CI=1 make test 2>&1 | tee /tmp/4-1-1-final-test.log` diff --git a/docs/parser-implementation-notes.md b/docs/parser-implementation-notes.md index 8c1980d0..348636df 100644 --- a/docs/parser-implementation-notes.md +++ b/docs/parser-implementation-notes.md @@ -202,6 +202,8 @@ Current guarantees are intentionally narrow: scopes. - Top-level relation, function, and type declarations are recorded in source order. +- Relation use sites record provenance that distinguishes rule-head writes from + rule-body reads and `for` iterable/guard reads. - Rule-head bindings are visible from the start of their rule scope. - Assignment-pattern bindings become visible only after the literal that introduces them. diff --git a/docs/roadmap.md b/docs/roadmap.md index e44136e9..707a3eeb 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -356,7 +356,7 @@ catalog. ### 4.1. Correctness rules -- [ ] 4.1.1. Implement `unused-relation` diagnostics for declared relations with +- [x] 4.1.1. Implement `unused-relation` diagnostics for declared relations with no usage sites. Requires 3.3.2 and 3.3.4. See docs/ddlint-design.md §3.3. - [ ] 4.1.2. Implement `unused-variable` diagnostics for variables defined but not used within a rule, treating `_` as explicit ignore. Requires 3.3.3 and diff --git a/src/linter/mod.rs b/src/linter/mod.rs index 3493191c..7c1c92ee 100644 --- a/src/linter/mod.rs +++ b/src/linter/mod.rs @@ -6,6 +6,7 @@ mod macros; mod rule; +pub mod rules; mod runner; mod store; diff --git a/src/linter/rules/correctness/mod.rs b/src/linter/rules/correctness/mod.rs new file mode 100644 index 00000000..45e80926 --- /dev/null +++ b/src/linter/rules/correctness/mod.rs @@ -0,0 +1,5 @@ +//! Correctness lint rules that flag likely semantic mistakes. + +mod unused_relation; + +pub use unused_relation::UnusedRelationRule; diff --git a/src/linter/rules/correctness/unused_relation.rs b/src/linter/rules/correctness/unused_relation.rs new file mode 100644 index 00000000..16494c6a --- /dev/null +++ b/src/linter/rules/correctness/unused_relation.rs @@ -0,0 +1,120 @@ +//! `unused-relation` warns about declared relations that are never read from. + +use rowan::TextRange; + +use crate::linter::{LintDiagnostic, Rule}; +use crate::{SyntaxKind, declare_lint}; + +/// Convert a `rowan` range into the crate's byte-span type. +fn text_range_to_span(range: TextRange) -> crate::Span { + usize::from(range.start())..usize::from(range.end()) +} + +declare_lint! { + /// Detects relations that are declared but never read from. + pub UnusedRelationRule { + name: "unused-relation", + group: "correctness", + level: warn, + target_kinds: [SyntaxKind::N_RELATION_DECL], + fn check_node(&self, node, ctx, diagnostics) { + let declaration_range = node.text_range(); + let declaration_span = text_range_to_span(declaration_range); + let Some(symbol_id) = ctx + .semantic_model() + .relation_symbol_at_span(&declaration_span) + else { + return; + }; + let Some(symbol) = ctx.semantic_model().symbol(symbol_id) else { + return; + }; + + if ctx.semantic_model().has_resolved_relation_read(symbol_id) { + return; + } + + diagnostics.push(LintDiagnostic::new( + self.name(), + format!("relation `{}` is declared but never read from", symbol.name()), + declaration_range, + )); + } + } +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + + use crate::linter::rules::correctness::UnusedRelationRule; + use crate::linter::{CstRuleStore, RuleConfig, Runner}; + use crate::parse; + + fn run_rule(source: &str) -> Vec { + let parsed = parse(source); + assert!( + parsed.errors().is_empty(), + "unused-relation test source should parse cleanly: {:?}", + parsed.errors() + ); + let mut store = CstRuleStore::new(); + store.register(Box::new(UnusedRelationRule)); + Runner::new(&store, source, &parsed, RuleConfig::new()).run() + } + + #[rstest] + fn warns_for_declared_relation_with_no_reads() { + let diagnostics = run_rule(concat!( + "input relation Source(x: u32)\n", + "relation Sink(x: u32)\n", + "Sink(x) :- Source(x).\n", + )); + + assert_eq!(diagnostics.len(), 1); + assert_eq!( + diagnostics + .first() + .map(crate::linter::LintDiagnostic::rule_name), + Some("unused-relation") + ); + assert_eq!( + diagnostics + .first() + .map(crate::linter::LintDiagnostic::message), + Some("relation `Sink` is declared but never read from") + ); + } + + #[rstest] + fn does_not_warn_for_relation_with_resolved_read() { + let diagnostics = run_rule(concat!( + "input relation Source(x: u32)\n", + "relation Used(x: u32)\n", + "relation Sink(x: u32)\n", + "Used(x) :- Source(x).\n", + "Sink(x) :- Used(x).\n", + )); + + assert!( + diagnostics.iter().all(|diagnostic| diagnostic.message() + != "relation `Used` is declared but never read from"), + "Used should not be reported once it is read from a rule body", + ); + } + + #[rstest] + fn ignores_unresolved_relation_uses_when_checking_reads() { + let diagnostics = run_rule(concat!( + "relation Declared(x: u32)\n", + "relation Sink(x: u32)\n", + "Sink(x) :- Missing(x).\n", + )); + + let messages: Vec<_> = diagnostics + .iter() + .map(crate::linter::LintDiagnostic::message) + .collect(); + assert!(messages.contains(&"relation `Declared` is declared but never read from")); + } +} diff --git a/src/linter/rules/mod.rs b/src/linter/rules/mod.rs new file mode 100644 index 00000000..ce86c638 --- /dev/null +++ b/src/linter/rules/mod.rs @@ -0,0 +1,3 @@ +//! Production lint rules shipped by the linter. + +pub mod correctness; diff --git a/src/sema/mod.rs b/src/sema/mod.rs index fcae8467..42dcd104 100644 --- a/src/sema/mod.rs +++ b/src/sema/mod.rs @@ -14,7 +14,7 @@ mod variables; pub use build::{build, build_from_parts, build_from_root}; pub use model::{ DeclarationKind, Resolution, Scope, ScopeId, ScopeKind, ScopeOrigin, SemanticModel, Symbol, - SymbolId, SymbolOrigin, UseKind, UseSite, + SymbolId, SymbolOrigin, UseKind, UseOrigin, UseSite, }; #[cfg(test)] diff --git a/src/sema/model.rs b/src/sema/model.rs index 2bab6291..b8ee03a4 100644 --- a/src/sema/model.rs +++ b/src/sema/model.rs @@ -82,6 +82,32 @@ pub enum UseKind { Variable, } +/// Provenance for one recorded use site. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum UseOrigin { + /// Relation use from a rule head, which writes to the relation. + RelationHead, + /// Relation use from a normal rule-body atom or semantic-rule body atom. + RelationBody, + /// Relation use from a `for` iterable expression. + ForIterable, + /// Relation use from a `for` guard expression. + ForGuard, + /// Variable use recorded while traversing expressions. + Variable, +} + +impl UseOrigin { + /// Return `true` when this origin is a read-like relation position. + #[must_use] + pub fn is_relation_read(self) -> bool { + matches!( + self, + Self::RelationBody | Self::ForIterable | Self::ForGuard + ) + } +} + /// Final name-resolution result for one use site. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Resolution { @@ -189,6 +215,7 @@ impl Symbol { pub struct UseSite { pub(crate) name: String, pub(crate) kind: UseKind, + pub(crate) origin: UseOrigin, pub(crate) scope: ScopeId, pub(crate) span: Span, pub(crate) source_order: usize, @@ -208,6 +235,12 @@ impl UseSite { self.kind } + /// Provenance for this use site. + #[must_use] + pub fn origin(&self) -> UseOrigin { + self.origin + } + /// Scope in which the use occurred. #[must_use] pub fn scope(&self) -> ScopeId { @@ -261,6 +294,15 @@ impl SemanticModel { &self.symbols } + /// Return every recorded relation declaration together with its symbol id. + pub fn relation_symbols(&self) -> impl Iterator + '_ { + self.symbols + .iter() + .enumerate() + .filter(|(_, symbol)| symbol.kind() == DeclarationKind::Relation) + .map(|(index, symbol)| (SymbolId(index), symbol)) + } + /// Return every recorded use site in stable build order. #[must_use] pub fn uses(&self) -> &[UseSite] { @@ -279,6 +321,24 @@ impl SemanticModel { self.symbols.get(id.0) } + /// Return the relation symbol declared at the given span, if any. + #[must_use] + pub fn relation_symbol_at_span(&self, span: &Span) -> Option { + self.relation_symbols() + .find(|(_, symbol)| symbol.span() == span) + .map(|(symbol_id, _)| symbol_id) + } + + /// Return `true` when the relation has at least one resolved read-like use. + #[must_use] + pub fn has_resolved_relation_read(&self, symbol_id: SymbolId) -> bool { + self.uses.iter().any(|use_site| { + use_site.kind() == UseKind::Relation + && use_site.origin().is_relation_read() + && use_site.resolution() == Resolution::Resolved(symbol_id) + }) + } + /// Return the resolved symbol for a use site when resolution succeeded. #[must_use] pub fn resolved_symbol(&self, use_site: &UseSite) -> Option<&Symbol> { diff --git a/src/sema/tests.rs b/src/sema/tests.rs index 3a672dcf..fe5bcaad 100644 --- a/src/sema/tests.rs +++ b/src/sema/tests.rs @@ -4,7 +4,8 @@ use crate::parse; use rstest::{fixture, rstest}; use super::{ - DeclarationKind, Resolution, ScopeKind, SymbolOrigin, UseKind, build, build_from_root, + DeclarationKind, Resolution, ScopeKind, SymbolOrigin, UseKind, UseOrigin, build, + build_from_root, }; fn parse_ok(source: &str) -> crate::Parsed { @@ -58,6 +59,12 @@ fn symbols_named<'a>( ) } +fn relation_symbol_id(model: &super::SemanticModel, name: &str) -> Option { + model + .relation_symbols() + .find_map(|(symbol_id, symbol)| (symbol.name() == name).then_some(symbol_id)) +} + #[fixture] fn parsed_case(#[default("")] source: &str) -> crate::Parsed { parse_ok(source) @@ -122,6 +129,34 @@ fn head_bindings_are_visible_from_rule_start( ); } +#[rstest] +fn relation_use_origins_distinguish_heads_from_reads( + #[with("Sink(x) :- Source(x), for (y in Items(x)) Check(y).")] + semantic_model: super::SemanticModel, +) { + let sink_uses = uses_named(&semantic_model, "Sink", UseKind::Relation); + let source_uses = uses_named(&semantic_model, "Source", UseKind::Relation); + let items_uses = uses_named(&semantic_model, "Items", UseKind::Relation); + let check_uses = uses_named(&semantic_model, "Check", UseKind::Relation); + + assert_eq!( + sink_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationHead) + ); + assert_eq!( + source_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationBody) + ); + assert_eq!( + items_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::ForIterable) + ); + assert_eq!( + check_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationBody) + ); +} + #[rstest] #[expect( clippy::expect_used, @@ -197,6 +232,8 @@ fn for_loop_bindings_do_not_escape_loop_scope( fn top_level_for_semantic_rules_participate_in_analysis( #[with("for (x in Source(x)) Output(0).")] semantic_model: super::SemanticModel, ) { + let output_uses = uses_named(&semantic_model, "Output", UseKind::Relation); + let source_uses = uses_named(&semantic_model, "Source", UseKind::Relation); let x_uses = uses_named(&semantic_model, "x", UseKind::Variable); assert!( @@ -207,9 +244,19 @@ fn top_level_for_semantic_rules_participate_in_analysis( "semantic rule should produce a rule scope", ); assert!( - !uses_named(&semantic_model, "Output", UseKind::Relation).is_empty(), + !output_uses.is_empty(), "semantic rule head should record relation use", ); + assert_eq!( + output_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationHead) + ); + assert_eq!( + source_uses + .first() + .map(|use_site| use_site.origin().is_relation_read()), + Some(true) + ); assert!( x_uses .iter() @@ -253,3 +300,41 @@ fn relation_use_prefers_relation_over_function_with_same_name( assert_eq!(resolved_symbol, foo_relation); } } + +#[rstest] +#[expect( + clippy::expect_used, + reason = "tests should fail with concise lookup messages" +)] +fn relation_read_helpers_ignore_head_only_and_unresolved_uses( + #[with(concat!( + "input relation Source(x: u32)\n", + "relation Sink(x: u32)\n", + "relation HeadOnly(x: u32)\n", + "relation NeverRead(x: u32)\n", + "Sink(x) :- Source(x), Missing(x).\n", + "HeadOnly(x) :- Source(x).\n", + ))] + semantic_model: super::SemanticModel, +) { + let source_id = relation_symbol_id(&semantic_model, "Source").expect("missing Source"); + let sink_id = relation_symbol_id(&semantic_model, "Sink").expect("missing Sink"); + let head_only_id = relation_symbol_id(&semantic_model, "HeadOnly").expect("missing HeadOnly"); + let never_read_id = + relation_symbol_id(&semantic_model, "NeverRead").expect("missing NeverRead"); + + assert!(semantic_model.has_resolved_relation_read(source_id)); + assert!(!semantic_model.has_resolved_relation_read(sink_id)); + assert!(!semantic_model.has_resolved_relation_read(head_only_id)); + assert!(!semantic_model.has_resolved_relation_read(never_read_id)); + + let source_span = semantic_model + .symbol(source_id) + .expect("missing Source symbol") + .span() + .clone(); + assert_eq!( + semantic_model.relation_symbol_at_span(&source_span), + Some(source_id) + ); +} diff --git a/src/sema/traverse.rs b/src/sema/traverse.rs index 3f961529..0fe6475f 100644 --- a/src/sema/traverse.rs +++ b/src/sema/traverse.rs @@ -4,7 +4,7 @@ use crate::Span; use crate::parser::ast; use crate::parser::ast::{Expr, RuleBodyTerm}; use crate::sema::model::{ - DeclarationKind, ScopeId, ScopeKind, ScopeOrigin, SymbolOrigin, UseKind, UseSite, + DeclarationKind, ScopeId, ScopeKind, ScopeOrigin, SymbolOrigin, UseKind, UseOrigin, UseSite, }; use super::builder::{ScopeSpec, SemanticModelBuilder, SymbolSpec}; @@ -37,6 +37,7 @@ impl SemanticModelBuilder { self.record_top_level_relation_use( VariableUseContext::new(ctx.scope, 0, ctx.span, 0), UseKind::Relation, + UseOrigin::RelationHead, expr, ); for binding_name in collect_head_binding_names(expr) { @@ -68,7 +69,12 @@ impl SemanticModelBuilder { } pub(crate) fn collect_expression_term(&mut self, expr: &Expr, context: VariableUseContext<'_>) { - self.record_top_level_relation_use(context, UseKind::Relation, expr); + self.record_top_level_relation_use( + context, + UseKind::Relation, + UseOrigin::RelationBody, + expr, + ); self.walk_variable_uses(expr, context); } @@ -105,10 +111,20 @@ impl SemanticModelBuilder { for_loop: &ast::RuleForLoop, context: VariableUseContext<'_>, ) { - self.record_top_level_relation_use(context, UseKind::Relation, &for_loop.iterable); + self.record_top_level_relation_use( + context, + UseKind::Relation, + UseOrigin::ForIterable, + &for_loop.iterable, + ); self.walk_variable_uses(&for_loop.iterable, context); if let Some(guard) = for_loop.guard.as_ref() { - self.record_top_level_relation_use(context, UseKind::Relation, guard); + self.record_top_level_relation_use( + context, + UseKind::Relation, + UseOrigin::ForGuard, + guard, + ); self.walk_variable_uses(guard, context); } @@ -149,6 +165,7 @@ impl SemanticModelBuilder { &mut self, context: VariableUseContext<'_>, use_kind: UseKind, + origin: UseOrigin, expr: &Expr, ) { let Some(name) = relation_name(expr) else { @@ -158,6 +175,7 @@ impl SemanticModelBuilder { self.uses.push(UseSite { name: name.to_string(), kind: use_kind, + origin, scope: context.current_scope(), span: context.span().clone(), source_order: context.literal_index(), diff --git a/src/sema/variables.rs b/src/sema/variables.rs index 6ea2ed6a..47843080 100644 --- a/src/sema/variables.rs +++ b/src/sema/variables.rs @@ -2,7 +2,7 @@ use crate::Span; use crate::parser::ast::Expr; -use crate::sema::model::{ScopeId, UseKind, UseSite}; +use crate::sema::model::{ScopeId, UseKind, UseOrigin, UseSite}; use super::builder::SemanticModelBuilder; @@ -202,6 +202,7 @@ impl SemanticModelBuilder { self.uses.push(UseSite { name: name.to_string(), kind: UseKind::Variable, + origin: UseOrigin::Variable, scope: context.current_scope(), span: context.span().clone(), source_order: context.literal_index(), diff --git a/tests/semantic_scope_resolution.rs b/tests/semantic_scope_resolution.rs index a9f0597a..587964e3 100644 --- a/tests/semantic_scope_resolution.rs +++ b/tests/semantic_scope_resolution.rs @@ -1,7 +1,7 @@ //! Behavioural tests for semantic symbol tables and scope resolution. use ddlint::linter::{CstRule, CstRuleStore, LintDiagnostic, Rule, RuleConfig, Runner}; -use ddlint::sema::{self, DeclarationKind, Resolution, UseKind}; +use ddlint::sema::{self, DeclarationKind, Resolution, UseKind, UseOrigin}; use ddlint::{SyntaxKind, parse}; use rstest::{fixture, rstest}; @@ -50,6 +50,10 @@ fn semantic_model_records_declarations_and_resolved_uses_end_to_end( assert_eq!(source_declarations.len(), 1); assert_eq!(source_uses.len(), 1); + assert_eq!( + source_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationBody) + ); assert!(matches!( source_uses.first().map(|use_site| use_site.resolution()), Some(Resolution::Resolved(_)) @@ -62,6 +66,31 @@ fn semantic_model_records_declarations_and_resolved_uses_end_to_end( ); } +#[rstest] +fn semantic_model_keeps_relation_reads_distinct_from_head_writes( + #[with("for (x in Source(x)) Output(x).\nHeadOnly(x) :- Source(x).")] + semantic_model: ddlint::sema::SemanticModel, +) { + let output_uses = uses_named(&semantic_model, "Output", UseKind::Relation); + let head_only_uses = uses_named(&semantic_model, "HeadOnly", UseKind::Relation); + let source_uses = uses_named(&semantic_model, "Source", UseKind::Relation); + + assert_eq!( + output_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationHead) + ); + assert_eq!( + head_only_uses.first().map(|use_site| use_site.origin()), + Some(UseOrigin::RelationHead) + ); + assert!( + source_uses + .iter() + .all(|use_site| use_site.origin().is_relation_read()), + "Source should only appear in read-like relation positions", + ); +} + #[rstest] fn semantic_model_keeps_unresolved_names_without_crashing( #[with("Output(x) :- Source(x), Missing(y).")] semantic_model: ddlint::sema::SemanticModel, diff --git a/tests/unused_relation_rule.rs b/tests/unused_relation_rule.rs new file mode 100644 index 00000000..9b569166 --- /dev/null +++ b/tests/unused_relation_rule.rs @@ -0,0 +1,88 @@ +//! Behavioural tests for the shipped `unused-relation` lint rule. + +use ddlint::linter::rules::correctness::UnusedRelationRule; +use ddlint::linter::{CstRuleStore, RuleConfig, Runner}; +use ddlint::parse; +use rstest::rstest; + +fn run_rule(source: &str) -> Vec { + let parsed = parse(source); + assert!( + parsed.errors().is_empty(), + "unused-relation behavioural source should parse cleanly: {:?}", + parsed.errors() + ); + + let mut store = CstRuleStore::new(); + store.register(Box::new(UnusedRelationRule)); + Runner::new(&store, source, &parsed, RuleConfig::new()).run() +} + +#[rstest] +#[case( + concat!( + "input relation Source(x: u32)\n", + "relation Sink(x: u32)\n", + "Sink(x) :- Source(x).\n", + ), + vec!["relation `Sink` is declared but never read from"], +)] +#[case( + concat!( + "input relation Source(x: u32)\n", + "relation Used(x: u32)\n", + "relation Sink(x: u32)\n", + "Used(x) :- Source(x).\n", + "Sink(x) :- Used(x).\n", + ), + vec!["relation `Sink` is declared but never read from"], +)] +#[case( + concat!( + "input relation Source(x: u32)\n", + "relation HeadOnly(x: u32)\n", + "HeadOnly(x) :- Source(x).\n", + ), + vec!["relation `HeadOnly` is declared but never read from"], +)] +#[case( + concat!( + "relation Declared(x: u32)\n", + "relation Sink(x: u32)\n", + "Sink(x) :- Missing(x).\n", + ), + vec![ + "relation `Declared` is declared but never read from", + "relation `Sink` is declared but never read from", + ], +)] +#[case( + concat!( + "relation Zebra(x: u32)\n", + "relation Alpha(x: u32)\n", + "relation Middle(x: u32)\n", + ), + vec![ + "relation `Zebra` is declared but never read from", + "relation `Alpha` is declared but never read from", + "relation `Middle` is declared but never read from", + ], +)] +fn unused_relation_rule_matches_expected_messages( + #[case] source: &str, + #[case] expected_messages: Vec<&str>, +) { + let diagnostics = run_rule(source); + let actual_messages: Vec<_> = diagnostics + .iter() + .map(ddlint::linter::LintDiagnostic::message) + .collect(); + + assert_eq!(actual_messages, expected_messages); + assert!( + diagnostics + .iter() + .all(|diagnostic| diagnostic.rule_name() == "unused-relation"), + "all diagnostics should come from the shipped rule", + ); +} From bf03ed1aca3d248959ba5b543a4060bcefb81864 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 23 Mar 2026 02:52:55 +0000 Subject: [PATCH 03/10] refactor(sema): inline UseKind::Relation in record_top_level_relation_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] --- src/sema/traverse.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/sema/traverse.rs b/src/sema/traverse.rs index 0fe6475f..00eb9813 100644 --- a/src/sema/traverse.rs +++ b/src/sema/traverse.rs @@ -36,7 +36,6 @@ impl SemanticModelBuilder { pub(crate) fn collect_head_expr(&mut self, expr: &Expr, ctx: RuleHeadContext<'_>) { self.record_top_level_relation_use( VariableUseContext::new(ctx.scope, 0, ctx.span, 0), - UseKind::Relation, UseOrigin::RelationHead, expr, ); @@ -71,7 +70,6 @@ impl SemanticModelBuilder { pub(crate) fn collect_expression_term(&mut self, expr: &Expr, context: VariableUseContext<'_>) { self.record_top_level_relation_use( context, - UseKind::Relation, UseOrigin::RelationBody, expr, ); @@ -113,7 +111,6 @@ impl SemanticModelBuilder { ) { self.record_top_level_relation_use( context, - UseKind::Relation, UseOrigin::ForIterable, &for_loop.iterable, ); @@ -121,7 +118,6 @@ impl SemanticModelBuilder { if let Some(guard) = for_loop.guard.as_ref() { self.record_top_level_relation_use( context, - UseKind::Relation, UseOrigin::ForGuard, guard, ); @@ -164,10 +160,10 @@ impl SemanticModelBuilder { fn record_top_level_relation_use( &mut self, context: VariableUseContext<'_>, - use_kind: UseKind, origin: UseOrigin, expr: &Expr, ) { + let use_kind = UseKind::Relation; let Some(name) = relation_name(expr) else { return; }; From e441b985313c9dd88bb0c17f2247268e4a502e3b Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 23 Mar 2026 03:03:45 +0000 Subject: [PATCH 04/10] style(sema): simplify multi-line calls to record_top_level_relation_use 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] --- src/sema/traverse.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/sema/traverse.rs b/src/sema/traverse.rs index 00eb9813..dc93dbce 100644 --- a/src/sema/traverse.rs +++ b/src/sema/traverse.rs @@ -68,11 +68,7 @@ impl SemanticModelBuilder { } pub(crate) fn collect_expression_term(&mut self, expr: &Expr, context: VariableUseContext<'_>) { - self.record_top_level_relation_use( - context, - UseOrigin::RelationBody, - expr, - ); + self.record_top_level_relation_use(context, UseOrigin::RelationBody, expr); self.walk_variable_uses(expr, context); } @@ -109,18 +105,10 @@ impl SemanticModelBuilder { for_loop: &ast::RuleForLoop, context: VariableUseContext<'_>, ) { - self.record_top_level_relation_use( - context, - UseOrigin::ForIterable, - &for_loop.iterable, - ); + self.record_top_level_relation_use(context, UseOrigin::ForIterable, &for_loop.iterable); self.walk_variable_uses(&for_loop.iterable, context); if let Some(guard) = for_loop.guard.as_ref() { - self.record_top_level_relation_use( - context, - UseOrigin::ForGuard, - guard, - ); + self.record_top_level_relation_use(context, UseOrigin::ForGuard, guard); self.walk_variable_uses(guard, context); } From 7bd94fe2cf90e4838b846b83aadb477da30a141e Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Mar 2026 11:06:46 +0000 Subject: [PATCH 05/10] feat(linter): add unused-relation lint rule detecting unread declared 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] --- docs/ddlint-design.md | 2 + ...1-implement-unused-relation-diagnostics.md | 22 ++++--- docs/roadmap.md | 3 +- .../rules/correctness/unused_relation.rs | 16 ++--- src/sema/builder.rs | 28 +++++++- src/sema/model.rs | 16 ++--- src/sema/tests.rs | 66 +++++++++++++++---- tests/semantic_scope_resolution.rs | 4 ++ tests/support.rs | 24 +++++++ tests/unused_relation_rule.rs | 37 +++++++---- 10 files changed, 163 insertions(+), 55 deletions(-) create mode 100644 tests/support.rs diff --git a/docs/ddlint-design.md b/docs/ddlint-design.md index 6f709698..1a893015 100644 --- a/docs/ddlint-design.md +++ b/docs/ddlint-design.md @@ -754,6 +754,8 @@ immediate value to users, the following catalog of rules is proposed. This list prioritizes correctness checks, followed by performance hints, and stylistic suggestions, establishing a solid foundation of essential lints. +Table: DDLint rule catalogue and metadata. + | Rule Name | Group | Default Level | Autofixable | Description | | ---------------------- | ----------- | ------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------- | | unused-relation | correctness | warn | No | Detects declared relations with no resolved read-like uses in rule bodies, `for` iterables, or `for` guards; rule heads count only as writes. | diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md index 4fd34d11..ce665747 100644 --- a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -10,19 +10,22 @@ Status: Implemented ## Purpose / big picture Roadmap item `4.1.1` is the first production lint rule in the initial -correctness catalog. After this change, a contributor should be able to lint a -DDlog file and receive a `unused-relation` warning for each declared relation -that is never read from anywhere in the analysed program. +correctness catalog. After this change, `ddlint` exports an `unused-relation` +lint rule (`UnusedRelationRule`) that callers must explicitly register in a +`CstRuleStore` before running the `Runner`. Once registered, the rule emits an +`unused-relation` warning for each declared relation that has no resolved +read-like uses anywhere in the analysed program. For this milestone, "read from" means a resolved relation-position use in a rule body, a `for` iterable, or a `for` guard. A relation named in a rule head is a write site, not a read site, so head-only relations must still be warned about. Observable success is: -- `ddlint` exports a concrete `unused-relation` rule that can be registered in - `CstRuleStore`. -- Running `Runner` with that rule emits one warning per unread relation - declaration and no warnings for relations with at least one resolved read. +- `ddlint` exports a concrete `unused-relation` rule (`UnusedRelationRule`) + that callers register in `CstRuleStore`; the rule is not enabled by default. +- Running `Runner` with that rule registered emits one warning per unread + relation declaration and no warnings for relations with at least one resolved + read. - The semantic model exposes enough provenance to distinguish relation reads from writes without re-walking the concrete syntax tree inside the rule. - Unit tests cover semantic read-versus-write classification and the rule's @@ -74,8 +77,9 @@ would therefore under-report unused relations by treating writes as reads. - Keep scope limited to implementing `unused-relation`, the additive semantic provenance needed for it, its tests, and the required documentation and roadmap updates. -- Do not implement `unused-variable`, `shadowed-variable`, CLI rule listing, - configuration-file loading, or rich `miette` conversion in this milestone. +- Do not implement `unused-variable`, `shadowed-variable`, command-line + interface (CLI) rule listing, configuration-file loading, or rich `miette` + conversion in this milestone. - Keep parser grammar and parse-stage diagnostics unchanged unless a bug blocks the rule and there is no narrower fix. - Extend the semantic model additively. Existing `RuleCtx`, `Runner`, and diff --git a/docs/roadmap.md b/docs/roadmap.md index 707a3eeb..41288300 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -357,7 +357,8 @@ catalog. ### 4.1. Correctness rules - [x] 4.1.1. Implement `unused-relation` diagnostics for declared relations with - no usage sites. Requires 3.3.2 and 3.3.4. See docs/ddlint-design.md §3.3. + no resolved read-like uses (rule-head writes do not count as reads). Requires + 3.3.2 and 3.3.4. See docs/ddlint-design.md §3.3. - [ ] 4.1.2. Implement `unused-variable` diagnostics for variables defined but not used within a rule, treating `_` as explicit ignore. Requires 3.3.3 and 3.3.4. See docs/ddlint-design.md §3.3. diff --git a/src/linter/rules/correctness/unused_relation.rs b/src/linter/rules/correctness/unused_relation.rs index 16494c6a..852b1a13 100644 --- a/src/linter/rules/correctness/unused_relation.rs +++ b/src/linter/rules/correctness/unused_relation.rs @@ -47,20 +47,20 @@ declare_lint! { mod tests { use rstest::rstest; - use crate::linter::rules::correctness::UnusedRelationRule; - use crate::linter::{CstRuleStore, RuleConfig, Runner}; - use crate::parse; - + // Import the shared test helper from the tests/support module. + // This ensures unit and behavioral tests use identical rule-running logic. fn run_rule(source: &str) -> Vec { - let parsed = parse(source); + // We can't directly use the tests/support module from src/ unit tests, + // so we duplicate the minimal logic here but keep it aligned. + let parsed = crate::parse(source); assert!( parsed.errors().is_empty(), "unused-relation test source should parse cleanly: {:?}", parsed.errors() ); - let mut store = CstRuleStore::new(); - store.register(Box::new(UnusedRelationRule)); - Runner::new(&store, source, &parsed, RuleConfig::new()).run() + let mut store = crate::linter::CstRuleStore::new(); + store.register(Box::new(super::UnusedRelationRule)); + crate::linter::Runner::new(&store, source, &parsed, crate::linter::RuleConfig::new()).run() } #[rstest] diff --git a/src/sema/builder.rs b/src/sema/builder.rs index 2e680d55..e396ab80 100644 --- a/src/sema/builder.rs +++ b/src/sema/builder.rs @@ -1,6 +1,6 @@ //! Internal semantic-model builder state and high-level collection passes. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use rowan::SyntaxNode; @@ -10,8 +10,8 @@ use crate::parser::ast; use crate::parser::ast::SemanticRule; use crate::parser::ast::rule::text_range_to_span; use crate::sema::model::{ - DeclarationKind, Scope, ScopeId, ScopeKind, ScopeOrigin, SemanticModel, Symbol, SymbolId, - SymbolOrigin, + DeclarationKind, Resolution, Scope, ScopeId, ScopeKind, ScopeOrigin, SemanticModel, Symbol, + SymbolId, SymbolOrigin, UseKind, }; use super::resolve::collect_pattern_binding_names; @@ -59,11 +59,33 @@ impl SemanticModelBuilder { } pub(crate) fn finish(self) -> SemanticModel { + // Precompute span-to-relation-symbol index + let span_to_relation_symbol: HashMap = self + .symbols + .iter() + .enumerate() + .filter(|(_, symbol)| symbol.kind() == DeclarationKind::Relation) + .map(|(index, symbol)| (symbol.span().clone(), SymbolId(index))) + .collect(); + + // Precompute symbols-with-reads set + let mut symbols_with_reads: HashSet = HashSet::new(); + for use_site in &self.uses { + if use_site.kind() == UseKind::Relation + && use_site.origin().is_relation_read() + && let Resolution::Resolved(symbol_id) = use_site.resolution() + { + symbols_with_reads.insert(symbol_id); + } + } + SemanticModel { program_scope: self.program_scope, scopes: self.scopes, symbols: self.symbols, uses: self.uses, + span_to_relation_symbol, + symbols_with_reads, } } diff --git a/src/sema/model.rs b/src/sema/model.rs index b8ee03a4..e37f1596 100644 --- a/src/sema/model.rs +++ b/src/sema/model.rs @@ -3,6 +3,8 @@ //! The semantic model stores only owned data and opaque identifiers so it can //! be shared safely across linter worker threads. +use std::collections::{HashMap, HashSet}; + use crate::Span; use crate::parser::ast::SemanticRuleOrigin; @@ -273,6 +275,10 @@ pub struct SemanticModel { pub(crate) scopes: Vec, pub(crate) symbols: Vec, pub(crate) uses: Vec, + /// Precomputed index mapping relation declaration spans to symbol IDs. + pub(crate) span_to_relation_symbol: HashMap, + /// Precomputed set of symbol IDs that have at least one resolved read-like use. + pub(crate) symbols_with_reads: HashSet, } impl SemanticModel { @@ -324,19 +330,13 @@ impl SemanticModel { /// Return the relation symbol declared at the given span, if any. #[must_use] pub fn relation_symbol_at_span(&self, span: &Span) -> Option { - self.relation_symbols() - .find(|(_, symbol)| symbol.span() == span) - .map(|(symbol_id, _)| symbol_id) + self.span_to_relation_symbol.get(span).copied() } /// Return `true` when the relation has at least one resolved read-like use. #[must_use] pub fn has_resolved_relation_read(&self, symbol_id: SymbolId) -> bool { - self.uses.iter().any(|use_site| { - use_site.kind() == UseKind::Relation - && use_site.origin().is_relation_read() - && use_site.resolution() == Resolution::Resolved(symbol_id) - }) + self.symbols_with_reads.contains(&symbol_id) } /// Return the resolved symbol for a use site when resolution succeeded. diff --git a/src/sema/tests.rs b/src/sema/tests.rs index fe5bcaad..417594ed 100644 --- a/src/sema/tests.rs +++ b/src/sema/tests.rs @@ -131,7 +131,12 @@ fn head_bindings_are_visible_from_rule_start( #[rstest] fn relation_use_origins_distinguish_heads_from_reads( - #[with("Sink(x) :- Source(x), for (y in Items(x)) Check(y).")] + #[with( + "Sink(x) :- \ + Source(x), \ + for (y in Items(x)) Check(y), \ + for (z in Items(x) if Check(z)) Check(z)." + )] semantic_model: super::SemanticModel, ) { let sink_uses = uses_named(&semantic_model, "Sink", UseKind::Relation); @@ -139,21 +144,56 @@ fn relation_use_origins_distinguish_heads_from_reads( let items_uses = uses_named(&semantic_model, "Items", UseKind::Relation); let check_uses = uses_named(&semantic_model, "Check", UseKind::Relation); - assert_eq!( - sink_uses.first().map(|use_site| use_site.origin()), - Some(UseOrigin::RelationHead) + let sink_origins: Vec<_> = sink_uses.iter().map(|use_site| use_site.origin()).collect(); + let source_origins: Vec<_> = source_uses + .iter() + .map(|use_site| use_site.origin()) + .collect(); + let items_origins: Vec<_> = items_uses + .iter() + .map(|use_site| use_site.origin()) + .collect(); + let check_origins: Vec<_> = check_uses + .iter() + .map(|use_site| use_site.origin()) + .collect(); + + // `Sink` only appears in rule heads. + assert!( + sink_origins + .iter() + .all(|origin| matches!(origin, UseOrigin::RelationHead)), + "expected all `Sink` uses to be rule heads, got: {sink_origins:?}", ); - assert_eq!( - source_uses.first().map(|use_site| use_site.origin()), - Some(UseOrigin::RelationBody) + + // `Source` only appears as a body read. + assert!( + source_origins + .iter() + .all(|origin| matches!(origin, UseOrigin::RelationBody)), + "expected all `Source` uses to be body reads, got: {source_origins:?}", ); - assert_eq!( - items_uses.first().map(|use_site| use_site.origin()), - Some(UseOrigin::ForIterable) + + // `Items` only appears as the iterable of a `for`. + assert!( + items_origins + .iter() + .all(|origin| matches!(origin, UseOrigin::ForIterable)), + "expected all `Items` uses to be `for` iterables, got: {items_origins:?}", ); - assert_eq!( - check_uses.first().map(|use_site| use_site.origin()), - Some(UseOrigin::RelationBody) + + // `Check` appears both in the `for` body and in the `for` guard. + assert!( + check_origins + .iter() + .any(|origin| matches!(origin, UseOrigin::RelationBody)), + "expected at least one body use for `Check`, got: {check_origins:?}", + ); + assert!( + check_origins + .iter() + .any(|origin| matches!(origin, UseOrigin::ForGuard)), + "expected at least one guard use for `Check`, got: {check_origins:?}", ); } diff --git a/tests/semantic_scope_resolution.rs b/tests/semantic_scope_resolution.rs index 587964e3..9cb57988 100644 --- a/tests/semantic_scope_resolution.rs +++ b/tests/semantic_scope_resolution.rs @@ -83,6 +83,10 @@ fn semantic_model_keeps_relation_reads_distinct_from_head_writes( head_only_uses.first().map(|use_site| use_site.origin()), Some(UseOrigin::RelationHead) ); + assert!( + !source_uses.is_empty(), + "expected at least one Source use recorded", + ); assert!( source_uses .iter() diff --git a/tests/support.rs b/tests/support.rs new file mode 100644 index 00000000..ba92508b --- /dev/null +++ b/tests/support.rs @@ -0,0 +1,24 @@ +//! Shared test utilities for behavioral tests. + +use ddlint::linter::rules::correctness::UnusedRelationRule; +use ddlint::linter::{CstRuleStore, RuleConfig, Runner}; +use ddlint::parse; + +/// Run the `unused-relation` lint rule on the given source code. +/// +/// # Panics +/// +/// Panics if the source code fails to parse cleanly. +#[must_use] +pub fn run_unused_relation_rule(source: &str) -> Vec { + let parsed = parse(source); + assert!( + parsed.errors().is_empty(), + "unused-relation test source should parse cleanly: {:?}", + parsed.errors() + ); + + let mut store = CstRuleStore::new(); + store.register(Box::new(UnusedRelationRule)); + Runner::new(&store, source, &parsed, RuleConfig::new()).run() +} diff --git a/tests/unused_relation_rule.rs b/tests/unused_relation_rule.rs index 9b569166..52eb4a2b 100644 --- a/tests/unused_relation_rule.rs +++ b/tests/unused_relation_rule.rs @@ -1,21 +1,12 @@ //! Behavioural tests for the shipped `unused-relation` lint rule. -use ddlint::linter::rules::correctness::UnusedRelationRule; -use ddlint::linter::{CstRuleStore, RuleConfig, Runner}; -use ddlint::parse; use rstest::rstest; -fn run_rule(source: &str) -> Vec { - let parsed = parse(source); - assert!( - parsed.errors().is_empty(), - "unused-relation behavioural source should parse cleanly: {:?}", - parsed.errors() - ); +mod support; +use support::run_unused_relation_rule; - let mut store = CstRuleStore::new(); - store.register(Box::new(UnusedRelationRule)); - Runner::new(&store, source, &parsed, RuleConfig::new()).run() +fn run_rule(source: &str) -> Vec { + run_unused_relation_rule(source) } #[rstest] @@ -68,6 +59,26 @@ fn run_rule(source: &str) -> Vec { "relation `Middle` is declared but never read from", ], )] +#[case( + concat!( + "input relation Source(x: u32)\n", + "relation ForRead(x: u32)\n", + "relation Sink(x: u32)\n", + "ForRead(x) :- Source(x).\n", + "Sink(x) :- for (y in ForRead(x)) Inner(y).\n", + ), + vec!["relation `Sink` is declared but never read from"], +)] +#[case( + concat!( + "input relation Source(x: u32)\n", + "relation GuardRead(x: u32)\n", + "relation Sink(x: u32)\n", + "GuardRead(x) :- Source(x).\n", + "Sink(x) :- for (y in Source(x) if GuardRead(y)) Inner(y).\n", + ), + vec!["relation `Sink` is declared but never read from"], +)] fn unused_relation_rule_matches_expected_messages( #[case] source: &str, #[case] expected_messages: Vec<&str>, From 045e9647765bb94117cee521e787607e2fd65dee Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 26 Mar 2026 09:43:28 +0000 Subject: [PATCH 06/10] feat(linter): implement unused relation diagnostics and semantic model 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] --- ...1-implement-unused-relation-diagnostics.md | 51 ++++++++++--------- .../rules/correctness/unused_relation.rs | 17 ++++++- src/sema/builder.rs | 25 +++++---- src/sema/tests.rs | 21 +++++--- tests/support.rs | 2 +- 5 files changed, 74 insertions(+), 42 deletions(-) diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md index ce665747..269afb3f 100644 --- a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -10,7 +10,7 @@ Status: Implemented ## Purpose / big picture Roadmap item `4.1.1` is the first production lint rule in the initial -correctness catalog. After this change, `ddlint` exports an `unused-relation` +correctness catalogue. After this change, `ddlint` exports an `unused-relation` lint rule (`UnusedRelationRule`) that callers must explicitly register in a `CstRuleStore` before running the `Runner`. Once registered, the rule emits an `unused-relation` warning for each declared relation that has no resolved @@ -119,7 +119,7 @@ would therefore under-report unused relations by treating writes as reads. provenance and write failing tests before adding the rule. - Risk: the roadmap wording says "declared relations with no usage sites", - while the design catalog says "never read from". Severity: medium. + while the design catalogue says "never read from". Severity: medium. Likelihood: medium. Mitigation: document and implement the narrower read-based rule semantics, because that is the more precise statement and avoids counting rule-head writes as reads. @@ -136,7 +136,7 @@ would therefore under-report unused relations by treating writes as reads. semantic query helpers that make the rule implementation direct and testable, but avoid introducing a broader precomputed lint-cache layer. -## Decision Log +## Decision log - Decision: treat rule-head relation atoms as write sites, not read sites, for `unused-relation`. Rationale: `docs/ddlint-design.md` defines the rule as @@ -168,20 +168,23 @@ layout is: Export the new module from `src/linter/mod.rs` so integration tests and later CLI wiring can import the rule cleanly. -Extend the semantic model with additive relation-use provenance. The smallest -useful shape is a new enum such as `UseOrigin` or `RelationUseOrigin` stored on -every `UseSite`. The enum should at least distinguish: +Extend the semantic model with additive relation-use provenance. The shipped +implementation uses a `UseOrigin` enum stored on every `UseSite` with the +following variants: -- rule head writes from AST-backed rules; -- rule body reads from AST-backed rules; -- `for` iterable reads; -- `for` guard reads; -- semantic-rule head writes from top-level `for` desugaring; and -- semantic-rule body reads from top-level `for` desugaring. +- `UseOrigin::RelationHead` — relation use from a rule head (write site); +- `UseOrigin::RelationBody` — relation use from a rule-body atom or + semantic-rule body atom (read site); +- `UseOrigin::ForIterable` — relation use from a `for` iterable expression + (read site); +- `UseOrigin::ForGuard` — relation use from a `for` guard expression (read + site); and +- `UseOrigin::Variable` — variable use recorded while traversing expressions. -The exact enum names can change during implementation, but the model must allow -the rule to answer one clear question without inspecting syntax nodes: "does -this relation declaration have any resolved read-like uses?" +The `UseOrigin::is_relation_read()` method returns `true` for `RelationBody`, +`ForIterable`, and `ForGuard`, allowing the rule to answer one clear question +without inspecting syntax nodes: "does this relation declaration have any +resolved read-like uses?" Add semantic helper methods that keep rule code simple. The final names may change, but the rule should be able to call helpers equivalent to: @@ -215,7 +218,7 @@ facts are incomplete because of parse recovery. ## Implementation plan -### Milestone 1: Add semantic use provenance +### Milestone 1: add semantic use provenance Update `src/sema/model.rs`, `src/sema/traverse.rs`, and any supporting helpers so relation uses carry enough provenance to distinguish reads from writes. Keep @@ -230,7 +233,7 @@ contract. At minimum, add coverage proving: - the same relation name in a rule body is recorded as a read; and - top-level `for` semantic rules preserve the same distinction. -### Milestone 2: Add semantic query helpers for the rule +### Milestone 2: add semantic query helpers for the rule Add focused helper methods on `SemanticModel` that answer the questions `unused-relation` actually needs. Keep them additive and deterministic. The @@ -244,7 +247,7 @@ least these cases: - a relation mentioned only in rule heads returns false; and - an unresolved relation-position use does not count as a read. -### Milestone 3: Implement the production rule module +### Milestone 3: implement the production rule module Create the production rule module under `src/linter/rules/correctness/` and export it through `src/linter/mod.rs`. Use `declare_lint!` for metadata and @@ -263,7 +266,7 @@ rule. If they become reusable across multiple correctness rules, move them to a small sibling helper module only after the first rule works and the need is real. -### Milestone 4: Add rule-focused tests +### Milestone 4: add rule-focused tests Add unit tests close to the rule module and behavioural tests under `tests/` that run the real parser and runner. Use `rstest` fixtures and parameterized @@ -285,10 +288,10 @@ Use a dedicated behavioural test file, for example `tests/unused_relation_rule.rs`, rather than burying these cases inside the generic runner tests. -### Milestone 5: Update docs and roadmap +### Milestone 5: update docs and roadmap Update `docs/ddlint-design.md` in the semantic-model section and the initial -lint catalog section so the implemented read-versus-write distinction is +lint catalogue section so the implemented read-versus-write distinction is explicit. If the semantic-model contract section already describes relation uses too loosely, tighten that wording there rather than spreading the rule semantics across multiple unrelated docs. @@ -357,7 +360,7 @@ is warned because it is only written in the head. - [x] (2026-03-22 01:25Z) Ran `make fmt`, `make markdownlint`, `make nixie`, `make check-fmt`, `make lint`, and `CI=1 make test`; all passed. -## Surprises & Discoveries +## Surprises & discoveries - Observation: roadmap prerequisites `3.3.2` and `3.3.4` are already complete, and the semantic model does record relation declarations and relation uses. @@ -366,7 +369,7 @@ is warned because it is only written in the head. - Observation: `src/sema/traverse.rs` currently records relation uses from rule heads via `collect_head_expr`, so the semantic model does not yet encode the - "read from" language used by the rule catalog. Impact: `unused-relation` + "read from" language used by the rule catalogue. Impact: `unused-relation` cannot be implemented correctly without extending `UseSite`. - Observation: the current linter module exports engine primitives only; there @@ -380,7 +383,7 @@ is warned because it is only written in the head. uses remain read-like, but tests should assert read-versus-write semantics rather than overfitting to the current lowering detail. -## Outcomes & Retrospective +## Outcomes & retrospective - Final rule modules: `src/linter/rules/mod.rs`, `src/linter/rules/correctness/mod.rs`, and diff --git a/src/linter/rules/correctness/unused_relation.rs b/src/linter/rules/correctness/unused_relation.rs index 852b1a13..8064f14a 100644 --- a/src/linter/rules/correctness/unused_relation.rs +++ b/src/linter/rules/correctness/unused_relation.rs @@ -1,4 +1,13 @@ -//! `unused-relation` warns about declared relations that are never read from. +//! `unused-relation` warns about declared relations with no resolved read-like uses. +//! +//! A relation counts as read when it appears in a rule body, `for` iterable, or +//! `for` guard position and that use resolves to the declaration. Rule-head +//! writes and unresolved relation uses do not count as reads, so head-only +//! relations and relations referenced only in broken rules still trigger +//! warnings. +//! +//! This rule uses `SemanticModel::has_resolved_relation_read()` to check +//! whether a relation has at least one resolved read-like use. use rowan::TextRange; @@ -11,7 +20,11 @@ fn text_range_to_span(range: TextRange) -> crate::Span { } declare_lint! { - /// Detects relations that are declared but never read from. + /// Detects relations declared but with no resolved read-like uses. + /// + /// A relation is considered read when it appears in a rule body, `for` + /// iterable, or `for` guard and that use resolves to the declaration. + /// Rule-head writes and unresolved uses do not count. pub UnusedRelationRule { name: "unused-relation", group: "correctness", diff --git a/src/sema/builder.rs b/src/sema/builder.rs index e396ab80..8387ac69 100644 --- a/src/sema/builder.rs +++ b/src/sema/builder.rs @@ -58,6 +58,17 @@ impl SemanticModelBuilder { } } + /// Extract a resolved relation read from a use site, if it is one. + fn resolve_relation_read(use_site: &crate::sema::UseSite) -> Option { + if use_site.kind() == UseKind::Relation + && use_site.origin().is_relation_read() + && let Resolution::Resolved(symbol_id) = use_site.resolution() + { + return Some(symbol_id); + } + None + } + pub(crate) fn finish(self) -> SemanticModel { // Precompute span-to-relation-symbol index let span_to_relation_symbol: HashMap = self @@ -69,15 +80,11 @@ impl SemanticModelBuilder { .collect(); // Precompute symbols-with-reads set - let mut symbols_with_reads: HashSet = HashSet::new(); - for use_site in &self.uses { - if use_site.kind() == UseKind::Relation - && use_site.origin().is_relation_read() - && let Resolution::Resolved(symbol_id) = use_site.resolution() - { - symbols_with_reads.insert(symbol_id); - } - } + let symbols_with_reads: HashSet = self + .uses + .iter() + .filter_map(Self::resolve_relation_read) + .collect(); SemanticModel { program_scope: self.program_scope, diff --git a/src/sema/tests.rs b/src/sema/tests.rs index 417594ed..e8f10e49 100644 --- a/src/sema/tests.rs +++ b/src/sema/tests.rs @@ -131,12 +131,12 @@ fn head_bindings_are_visible_from_rule_start( #[rstest] fn relation_use_origins_distinguish_heads_from_reads( - #[with( - "Sink(x) :- \ - Source(x), \ - for (y in Items(x)) Check(y), \ - for (z in Items(x) if Check(z)) Check(z)." - )] + #[with(concat!( + "Sink(x) :- ", + "Source(x), ", + "for (y in Items(x)) Check(y), ", + "for (z in Items(x) if Check(z)) Check(z)." + ))] semantic_model: super::SemanticModel, ) { let sink_uses = uses_named(&semantic_model, "Sink", UseKind::Relation); @@ -159,6 +159,7 @@ fn relation_use_origins_distinguish_heads_from_reads( .collect(); // `Sink` only appears in rule heads. + assert!(!sink_origins.is_empty(), "expected at least one `Sink` use"); assert!( sink_origins .iter() @@ -167,6 +168,10 @@ fn relation_use_origins_distinguish_heads_from_reads( ); // `Source` only appears as a body read. + assert!( + !source_origins.is_empty(), + "expected at least one `Source` use" + ); assert!( source_origins .iter() @@ -175,6 +180,10 @@ fn relation_use_origins_distinguish_heads_from_reads( ); // `Items` only appears as the iterable of a `for`. + assert!( + !items_origins.is_empty(), + "expected at least one `Items` use" + ); assert!( items_origins .iter() diff --git a/tests/support.rs b/tests/support.rs index ba92508b..d0f38a5d 100644 --- a/tests/support.rs +++ b/tests/support.rs @@ -1,4 +1,4 @@ -//! Shared test utilities for behavioral tests. +//! Shared test utilities for behavioural tests. use ddlint::linter::rules::correctness::UnusedRelationRule; use ddlint::linter::{CstRuleStore, RuleConfig, Runner}; From b060b04ffc95cb99eb429e1fd0f0b728db768c36 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 26 Mar 2026 10:23:57 +0000 Subject: [PATCH 07/10] refactor(linter, sema): remove unused helper and simplify relation read 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] --- ...1-1-implement-unused-relation-diagnostics.md | 8 ++++---- src/linter/rules/correctness/unused_relation.rs | 8 +------- src/sema/builder.rs | 17 +++++------------ 3 files changed, 10 insertions(+), 23 deletions(-) diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md index 269afb3f..3717a890 100644 --- a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -58,10 +58,10 @@ build on: - `src/linter/macros.rs` provides `declare_lint!`, which should be used for the new rule unless a documented blocker appears. -There is no shipped rule-catalog module yet. Current behavioural tests register -ad hoc rules directly in `CstRuleStore`. This milestone therefore needs to add -the first exported production rule module and corresponding tests, but it does -not need to invent a full CLI-configured default ruleset. +There is no shipped rule-catalogue module yet. Current behavioural tests +register ad hoc rules directly in `CstRuleStore`. This milestone therefore +needs to add the first exported production rule module and corresponding tests, +but it does not need to invent a full CLI-configured default ruleset. The key design gap is semantic provenance. `docs/ddlint-design.md` says `unused-relation` detects relations "defined but never read from", yet the diff --git a/src/linter/rules/correctness/unused_relation.rs b/src/linter/rules/correctness/unused_relation.rs index 8064f14a..652660cb 100644 --- a/src/linter/rules/correctness/unused_relation.rs +++ b/src/linter/rules/correctness/unused_relation.rs @@ -9,16 +9,10 @@ //! This rule uses `SemanticModel::has_resolved_relation_read()` to check //! whether a relation has at least one resolved read-like use. -use rowan::TextRange; - use crate::linter::{LintDiagnostic, Rule}; +use crate::parser::ast::rule::text_range_to_span; use crate::{SyntaxKind, declare_lint}; -/// Convert a `rowan` range into the crate's byte-span type. -fn text_range_to_span(range: TextRange) -> crate::Span { - usize::from(range.start())..usize::from(range.end()) -} - declare_lint! { /// Detects relations declared but with no resolved read-like uses. /// diff --git a/src/sema/builder.rs b/src/sema/builder.rs index 8387ac69..34cf5cdb 100644 --- a/src/sema/builder.rs +++ b/src/sema/builder.rs @@ -58,17 +58,6 @@ impl SemanticModelBuilder { } } - /// Extract a resolved relation read from a use site, if it is one. - fn resolve_relation_read(use_site: &crate::sema::UseSite) -> Option { - if use_site.kind() == UseKind::Relation - && use_site.origin().is_relation_read() - && let Resolution::Resolved(symbol_id) = use_site.resolution() - { - return Some(symbol_id); - } - None - } - pub(crate) fn finish(self) -> SemanticModel { // Precompute span-to-relation-symbol index let span_to_relation_symbol: HashMap = self @@ -83,7 +72,11 @@ impl SemanticModelBuilder { let symbols_with_reads: HashSet = self .uses .iter() - .filter_map(Self::resolve_relation_read) + .filter(|u| u.kind() == UseKind::Relation && u.origin().is_relation_read()) + .filter_map(|u| match u.resolution() { + Resolution::Resolved(symbol_id) => Some(symbol_id), + _ => None, + }) .collect(); SemanticModel { From 45ca8c1a64a1bae484298fa8bed36f1716f811f9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 26 Mar 2026 12:28:10 +0000 Subject: [PATCH 08/10] docs(execplans): improve formatting and clarify unused relation diagnostics - 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] --- ...1-implement-unused-relation-diagnostics.md | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md index 3717a890..c6fe5400 100644 --- a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -61,7 +61,8 @@ build on: There is no shipped rule-catalogue module yet. Current behavioural tests register ad hoc rules directly in `CstRuleStore`. This milestone therefore needs to add the first exported production rule module and corresponding tests, -but it does not need to invent a full CLI-configured default ruleset. +but it does not need to invent a full Command-Line Interface (CLI)-configured +default ruleset. The key design gap is semantic provenance. `docs/ddlint-design.md` says `unused-relation` detects relations "defined but never read from", yet the @@ -152,8 +153,8 @@ would therefore under-report unused relations by treating writes as reads. - Decision: expose the first production rule as a normal exported rule type that tests register explicitly in `CstRuleStore`, rather than inventing a global default ruleset now. Rationale: the current repository has no shipped - rule-catalog registration surface, and adding one would broaden scope beyond - `4.1.1`. Date/Author: 2026-03-21 / Codex. + rule-catalogue registration surface, and adding one would broaden scope + beyond `4.1.1`. Date/Author: 2026-03-21 / Codex. ## Proposed design @@ -377,11 +378,13 @@ is warned because it is only written in the head. a small `src/linter/rules/` module tree, but should not broaden into a full default ruleset or CLI registry. -- Observation: top-level `for` desugaring currently records iterable relation - reads as semantic-rule body reads rather than a dedicated `ForIterable` - origin. Impact: the durable rule contract is still satisfied because those - uses remain read-like, but tests should assert read-versus-write semantics - rather than overfitting to the current lowering detail. +- Observation: early experiments with top-level `for` desugaring recorded + iterable relation reads as semantic-rule body reads rather than a dedicated + `ForIterable` origin. The shipped contract treats those uses as read-like, + and tests should assert read-versus-write semantics per the durable rule + contract rather than overfitting to any particular desugaring detail. + Historical note: the `ForIterable` origin variant was introduced to + distinguish iterable positions from rule-body positions. ## Outcomes & retrospective @@ -404,9 +407,12 @@ is warned because it is only written in the head. `docs/parser-implementation-notes.md` records relation-use provenance as a current semantic invariant; and `docs/roadmap.md` marks item `4.1.1` done. - 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 check-fmt 2>&1 | tee /tmp/4-1-1-final-check-fmt.log` - `set -o pipefail; make lint 2>&1 | tee /tmp/4-1-1-final-lint.log` - `set -o pipefail; CI=1 make test 2>&1 | tee /tmp/4-1-1-final-test.log` + + ```bash + 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 check-fmt 2>&1 | tee /tmp/4-1-1-final-check-fmt.log + set -o pipefail; make lint 2>&1 | tee /tmp/4-1-1-final-lint.log + set -o pipefail; CI=1 make test 2>&1 | tee /tmp/4-1-1-final-test.log + ``` From 14beb6356886c44647379cc469db058642d0e9cc Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 26 Mar 2026 12:43:59 +0000 Subject: [PATCH 09/10] refactor(sema): extract is_relation_read_use helper to clarify filter 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] --- src/sema/builder.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sema/builder.rs b/src/sema/builder.rs index 34cf5cdb..b2f6dd7c 100644 --- a/src/sema/builder.rs +++ b/src/sema/builder.rs @@ -58,6 +58,10 @@ impl SemanticModelBuilder { } } + fn is_relation_read_use(use_site: &crate::sema::UseSite) -> bool { + use_site.kind() == UseKind::Relation && use_site.origin().is_relation_read() + } + pub(crate) fn finish(self) -> SemanticModel { // Precompute span-to-relation-symbol index let span_to_relation_symbol: HashMap = self @@ -72,7 +76,7 @@ impl SemanticModelBuilder { let symbols_with_reads: HashSet = self .uses .iter() - .filter(|u| u.kind() == UseKind::Relation && u.origin().is_relation_read()) + .filter(|u| Self::is_relation_read_use(u)) .filter_map(|u| match u.resolution() { Resolution::Resolved(symbol_id) => Some(symbol_id), _ => None, From 1fb75cbc2b3187ecd4b03dfa94a6ee02b4b41753 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 26 Mar 2026 13:35:33 +0000 Subject: [PATCH 10/10] docs(execplans): clarify semantic helper methods for unused relation 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] --- ...1-1-implement-unused-relation-diagnostics.md | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md index c6fe5400..ed095846 100644 --- a/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md +++ b/docs/execplans/4-1-1-implement-unused-relation-diagnostics.md @@ -187,19 +187,18 @@ The `UseOrigin::is_relation_read()` method returns `true` for `RelationBody`, without inspecting syntax nodes: "does this relation declaration have any resolved read-like uses?" -Add semantic helper methods that keep rule code simple. The final names may -change, but the rule should be able to call helpers equivalent to: +Add semantic helper methods that keep rule code simple. The shipped API +provides: ```rust -model.relation_symbols() -model.relation_reads() -model.has_resolved_relation_read(symbol_id) +model.relation_symbols() // Iterator over (SymbolId, &Symbol) for relations +model.relation_symbol_at_span(span) // Lookup symbol by span +model.has_resolved_relation_read(symbol_id) // Check if symbol has reads ``` -If lookup by span is needed to associate an `N_RELATION_DECL` node with its -relation symbol, prefer a small helper such as -`SemanticModel::relation_symbol_at_span(span)` over ad hoc filtering inside the -rule. +These helpers allow the rule to iterate over relation declarations and check +whether each has resolved read-like uses, without inspecting syntax nodes or +filtering the full symbol table. Implement the rule itself with `declare_lint!`. It should target `SyntaxKind::N_RELATION_DECL`, use metadata `name: "unused-relation"`,