From 90b83d6f5d426c3dc556af9ee59fdb413c647688 Mon Sep 17 00:00:00 2001 From: alexion Date: Sat, 26 Sep 2026 23:14:08 -0400 Subject: [PATCH] feat(natu): report expected let order --- src/natu/natu.rs | 29 +++++++++++++++++++++++++++ src/natu/natu_test.rs | 13 +++++++++++- src/natu/rules.rs | 6 +++++- src/natu/rules/let_order.rs | 40 ++++++++++++++++++------------------- 4 files changed, 66 insertions(+), 22 deletions(-) diff --git a/src/natu/natu.rs b/src/natu/natu.rs index d942ea5..bc84de2 100644 --- a/src/natu/natu.rs +++ b/src/natu/natu.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + mod ast; mod file_store; mod parser; @@ -23,7 +25,34 @@ impl Report { impl std::fmt::Display for Report { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut reported_let_order_files = HashSet::new(); + for diagnostic in &self.diagnostics { + if diagnostic.finding.rule_id == "let_order" { + if !reported_let_order_files.insert(diagnostic.file_id) { + continue; + } + + let file = self.files.get(diagnostic.file_id); + writeln!( + formatter, + "{} [let_order] Let bindings must follow dependency order", + file.path.display() + )?; + + for finding in self.diagnostics.iter().filter(|candidate| { + candidate.file_id == diagnostic.file_id + && candidate.finding.rule_id == "let_order" + }) { + let (start, _) = line_range(&file.source, finding.finding.range); + let expected_order = + finding.finding.expected_order.as_ref().expect("let order"); + writeln!(formatter, " line {start}: {}", expected_order.join(", "))?; + } + + continue; + } + let file = self.files.get(diagnostic.file_id); let (start, end) = line_range(&file.source, diagnostic.finding.range); let location = if start == end { diff --git a/src/natu/natu_test.rs b/src/natu/natu_test.rs index 45087bd..87b3c3d 100644 --- a/src/natu/natu_test.rs +++ b/src/natu/natu_test.rs @@ -50,7 +50,18 @@ fn natu_reports_out_of_order_let_bindings() -> googletest::Result<()> { .iter() .filter(|diagnostic| diagnostic.finding.rule_id == "let_order") .count(); - expect_that!(let_order_count, eq(4)); + expect_that!(let_order_count, eq(2)); + expect_that!( + report.to_string(), + eq(concat!( + "testdata/lint_findings.nix:11 [manual_inherit] Use `inherit` for this assignment\n", + "testdata/lint_findings.nix:12 [manual_inherit_from] Use `inherit` for this selected attribute\n", + "testdata/lint_findings.nix:13 [unquoted_uri] URI literals should be quoted\n", + "testdata/lint_findings.nix [let_order] Let bindings must follow dependency order\n", + " line 5: nestedSecond, nestedFirst\n", + " line 2: first, second, nested\n", + )) + ); Ok(()) } diff --git a/src/natu/rules.rs b/src/natu/rules.rs index 558e302..7d0e6b8 100644 --- a/src/natu/rules.rs +++ b/src/natu/rules.rs @@ -22,6 +22,7 @@ pub struct Finding { pub range: ast::SourceRange, pub message: &'static str, pub binding: Option, + pub expected_order: Option>, } impl std::fmt::Display for Diagnostic { @@ -79,7 +80,8 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph) -> Vec rule_id: "let_order", range: finding.range, message: "Let bindings must follow dependency order", - binding: Some(finding.binding), + binding: None, + expected_order: Some(finding.expected_order), }); node_diagnostics @@ -104,6 +106,7 @@ where range: node.range, message, binding: None, + expected_order: None, }) } @@ -119,5 +122,6 @@ where range: token.range, message, binding: None, + expected_order: None, }) } diff --git a/src/natu/rules/let_order.rs b/src/natu/rules/let_order.rs index 7d01ebf..658291a 100644 --- a/src/natu/rules/let_order.rs +++ b/src/natu/rules/let_order.rs @@ -5,7 +5,7 @@ use crate::{ pub struct Finding { pub range: ast::SourceRange, - pub binding: String, + pub expected_order: Vec, } pub fn check(graph: &ScopeGraph) -> Vec { @@ -14,26 +14,26 @@ pub fn check(graph: &ScopeGraph) -> Vec { .iter() .flat_map(|let_scope| { let expected = expected_order(graph, let_scope); - let positions = expected - .iter() - .enumerate() - .map(|(position, declaration)| (*declaration, position)) - .collect::>(); + let is_ordered = expected.iter().enumerate().all(|(position, declaration)| { + let_scope + .bindings + .get(position) + .is_some_and(|binding| binding.declaration == *declaration) + }); - let_scope - .bindings - .iter() - .enumerate() - .filter_map(move |(position, binding)| { - positions - .iter() - .find(|(declaration, _)| *declaration == binding.declaration) - .filter(|(_, expected_position)| position != *expected_position) - .map(|_| Finding { - range: binding.range, - binding: binding.name.clone(), - }) - }) + (!is_ordered).then(|| Finding { + range: let_scope.bindings[0].range, + expected_order: expected + .iter() + .filter_map(|declaration| { + let_scope + .bindings + .iter() + .find(|binding| binding.declaration == *declaration) + .map(|binding| binding.name.clone()) + }) + .collect(), + }) }) .collect() }