From 5e5c3e2534df29f5aa3279af542effe744f493d6 Mon Sep 17 00:00:00 2001 From: alexion Date: Sat, 26 Sep 2026 23:20:00 -0400 Subject: [PATCH] feat(natu): report expected let order --- src/natu/ast.rs | 1 + src/natu/natu.rs | 40 +++++++++++++++++++++++++++++++++++++ src/natu/natu_test.rs | 13 +++++++++++- src/natu/parser.rs | 1 + src/natu/rules.rs | 6 +++++- src/natu/rules/let_order.rs | 40 ++++++++++++++++++------------------- src/natu/scope_graph.rs | 11 +++++++--- 7 files changed, 87 insertions(+), 25 deletions(-) diff --git a/src/natu/ast.rs b/src/natu/ast.rs index c24b52c..61038d9 100644 --- a/src/natu/ast.rs +++ b/src/natu/ast.rs @@ -49,6 +49,7 @@ pub enum Expr { Ident(Ident), Select(Select), Let { + range: SourceRange, bindings: Vec, body: Box, }, diff --git a/src/natu/natu.rs b/src/natu/natu.rs index d942ea5..94ea9b2 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,45 @@ 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() + )?; + + let mut findings = self + .diagnostics + .iter() + .filter(|candidate| { + candidate.file_id == diagnostic.file_id + && candidate.finding.rule_id == "let_order" + }) + .collect::>(); + findings.sort_by_key(|finding| finding.finding.range.start); + + for finding in findings { + let (start, _) = line_range(&file.source, finding.finding.range); + let expected_order = + finding.finding.expected_order.as_ref().expect("let order"); + writeln!( + formatter, + " let at 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..bc7c81e 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", + " let at line 1: first, second, nested\n", + " let at line 4: nestedSecond, nestedFirst\n", + )) + ); Ok(()) } diff --git a/src/natu/parser.rs b/src/natu/parser.rs index 419188c..9ece9db 100644 --- a/src/natu/parser.rs +++ b/src/natu/parser.rs @@ -43,6 +43,7 @@ fn convert_expr(expr: rnix::ast::Expr) -> ast::Expr { match expr { rnix::ast::Expr::Ident(ident) => ast::Expr::Ident(ident.into()), rnix::ast::Expr::LetIn(let_in) => ast::Expr::Let { + range: let_in.syntax().text_range().into(), bindings: let_in .attrpath_values() .filter_map(convert_binding_ast) 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..92dc6d6 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.range, + expected_order: expected + .iter() + .filter_map(|declaration| { + let_scope + .bindings + .iter() + .find(|binding| binding.declaration == *declaration) + .map(|binding| binding.name.clone()) + }) + .collect(), + }) }) .collect() } diff --git a/src/natu/scope_graph.rs b/src/natu/scope_graph.rs index 6795458..46dff26 100644 --- a/src/natu/scope_graph.rs +++ b/src/natu/scope_graph.rs @@ -26,7 +26,11 @@ impl ScopeGraph { fn build_expr(&mut self, expr: &ast::Expr, scope: NodeId) -> Vec { match expr { ast::Expr::Ident(ident) => vec![self.add_reference(&ident.name, scope)], - ast::Expr::Let { bindings, body } => self.build_let(bindings, body, scope), + ast::Expr::Let { + range, + bindings, + body, + } => self.build_let(*range, bindings, body, scope), ast::Expr::AttrSet(bindings) => bindings .iter() .flat_map(|binding| self.build_expr(&binding.value, scope)) @@ -48,6 +52,7 @@ impl ScopeGraph { fn build_let( &mut self, + range: ast::SourceRange, bindings: &[ast::Binding], body: &ast::Expr, parent: NodeId, @@ -59,7 +64,6 @@ impl ScopeGraph { declaration: self.add_declaration(&binding.key.name, scope), name: binding.key.name.clone(), references: Vec::new(), - range: binding.range, }) .collect::>(); @@ -74,6 +78,7 @@ impl ScopeGraph { .chain(entry_references.iter().copied()) .collect(); self.lets.push(LetScope { + range, bindings: graph_bindings, entry_references, }); @@ -181,6 +186,7 @@ struct Edge { #[derive(Debug)] pub(crate) struct LetScope { + pub(crate) range: ast::SourceRange, pub(crate) bindings: Vec, pub(crate) entry_references: Vec, } @@ -190,5 +196,4 @@ pub(crate) struct Binding { pub(crate) declaration: NodeId, pub(crate) name: String, pub(crate) references: Vec, - pub(crate) range: ast::SourceRange, } -- 2.47.3