diff --git a/src/xatu/BUILD b/src/xatu/BUILD index ae529fc..0ca6ac7 100644 --- a/src/xatu/BUILD +++ b/src/xatu/BUILD @@ -8,6 +8,7 @@ rust_library( "file_store.rs", "parser.rs", "rules.rs", + "rules/let_order.rs", "rules/manual_inherit.rs", "rules/manual_inherit_from.rs", "rules/unquoted_uri.rs", diff --git a/src/xatu/rules.rs b/src/xatu/rules.rs index b27570d..558e302 100644 --- a/src/xatu/rules.rs +++ b/src/xatu/rules.rs @@ -3,7 +3,9 @@ use crate::ast; use crate::rules::manual_inherit::ManualInherit; use crate::rules::manual_inherit_from::ManualInheritFrom; use crate::rules::unquoted_uri::UnquotedUri; +use crate::scope_graph::ScopeGraph; +pub mod let_order; pub mod manual_inherit; pub mod manual_inherit_from; pub mod unquoted_uri; @@ -19,15 +21,22 @@ pub struct Finding { pub rule_id: &'static str, pub range: ast::SourceRange, pub message: &'static str, + pub binding: Option, } impl std::fmt::Display for Diagnostic { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( formatter, - "{}: {}: {}", - self.finding.range, self.finding.rule_id, self.finding.message, - ) + "[{}] {}", + self.finding.rule_id, self.finding.message + )?; + + if let Some(binding) = &self.finding.binding { + write!(formatter, ": {binding}")?; + } + + Ok(()) } } @@ -49,7 +58,7 @@ pub trait TokenRule { fn check(token: &Self::Token) -> Option<&'static str>; } -pub fn run_rules(file: &ast::ParsedFile) -> Vec { +pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph) -> Vec { let node_diagnostics = file .nodes .iter() @@ -66,8 +75,16 @@ pub fn run_rules(file: &ast::ParsedFile) -> Vec { .flat_map(|token| [run_token_rule::(token)]) .flatten(); + let let_order_diagnostics = let_order::check(graph).into_iter().map(|finding| Finding { + rule_id: "let_order", + range: finding.range, + message: "Let bindings must follow dependency order", + binding: Some(finding.binding), + }); + node_diagnostics .chain(token_diagnostics) + .chain(let_order_diagnostics) .map(|finding| Diagnostic { file_id: file.file_id, finding, @@ -86,6 +103,7 @@ where rule_id: R::ID, range: node.range, message, + binding: None, }) } @@ -100,5 +118,6 @@ where rule_id: R::ID, range: token.range, message, + binding: None, }) } diff --git a/src/xatu/rules/let_order.rs b/src/xatu/rules/let_order.rs new file mode 100644 index 0000000..7d01ebf --- /dev/null +++ b/src/xatu/rules/let_order.rs @@ -0,0 +1,71 @@ +use crate::{ + ast, + scope_graph::{LetScope, NodeId, ScopeGraph}, +}; + +pub struct Finding { + pub range: ast::SourceRange, + pub binding: String, +} + +pub fn check(graph: &ScopeGraph) -> Vec { + graph + .let_scopes() + .iter() + .flat_map(|let_scope| { + let expected = expected_order(graph, let_scope); + let positions = expected + .iter() + .enumerate() + .map(|(position, declaration)| (*declaration, position)) + .collect::>(); + + 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(), + }) + }) + }) + .collect() +} + +fn expected_order(graph: &ScopeGraph, let_scope: &LetScope) -> Vec { + let mut order = Vec::new(); + visit_references(graph, &let_scope.entry_references, let_scope, &mut order); + order +} + +fn visit_references( + graph: &ScopeGraph, + references: &[NodeId], + let_scope: &LetScope, + order: &mut Vec, +) { + for reference in references { + for declaration in graph.resolve(*reference) { + let Some(binding) = let_scope + .bindings + .iter() + .find(|binding| binding.declaration == declaration) + else { + continue; + }; + + if order.contains(&declaration) { + continue; + } + + order.push(declaration); + visit_references(graph, &binding.references, let_scope, order); + } + } +} diff --git a/src/xatu/testdata/clean.nix b/src/xatu/testdata/clean.nix index fa09933..480b498 100644 --- a/src/xatu/testdata/clean.nix +++ b/src/xatu/testdata/clean.nix @@ -1,6 +1,22 @@ { pkgs }: -{ +let + first = 1; + second = first; +in { + firstField = first; + secondField = second; + nested = let + innerSecond = innerFirst; + innerFirst = first; + in { + result = innerSecond; + }; + parameter = first: let + usesParameter = first; + in { + result = usesParameter; + }; path = ./. + "/foo"; url = "https://example.com"; inherit pkgs; diff --git a/src/xatu/testdata/lint_findings.nix b/src/xatu/testdata/lint_findings.nix index 17b4837..a4eb0d8 100644 --- a/src/xatu/testdata/lint_findings.nix +++ b/src/xatu/testdata/lint_findings.nix @@ -1,5 +1,17 @@ -{ +let + second = first; + first = 1; + nested = let + nestedFirst = first; + nestedSecond = nestedFirst; + in { + result = nestedSecond; + }; +in { package = package; from = pkgs.from; url = https://example.com; + firstField = first; + secondField = second; + nestedField = nested; } diff --git a/src/xatu/xatu.rs b/src/xatu/xatu.rs index 4c450f7..f718d89 100644 --- a/src/xatu/xatu.rs +++ b/src/xatu/xatu.rs @@ -4,6 +4,8 @@ mod parser; mod rules; mod scope_graph; +use scope_graph::ScopeGraph; + pub use file_store::{FileId, FileStore, SourceFile}; pub use rules::{Diagnostic, Finding}; @@ -23,13 +25,34 @@ impl std::fmt::Display for Report { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { for diagnostic in &self.diagnostics { let file = self.files.get(diagnostic.file_id); - writeln!(formatter, "{}:{diagnostic}", file.path.display())?; + let (start, end) = line_range(&file.source, diagnostic.finding.range); + let location = if start == end { + start.to_string() + } else { + format!("{start}-{end}") + }; + writeln!(formatter, "{}:{location} {diagnostic}", file.path.display())?; } Ok(()) } } +fn line_range(source: &str, range: ast::SourceRange) -> (usize, usize) { + let line_at = |offset| { + source + .bytes() + .take(offset) + .filter(|byte| *byte == b'\n') + .count() + + 1 + }; + let start = line_at(range.start); + let end = line_at(range.end); + + (start, end) +} + /// Analyzes source files and returns the diagnostics Xatu finds. pub fn future_sight(files: Vec) -> status::Result { let mut store = FileStore::default(); @@ -47,7 +70,8 @@ pub fn future_sight(files: Vec) -> status::Result { ))); } - diagnostics.extend(rules::run_rules(&parsed)); + let graph = ScopeGraph::from_file(&parsed); + diagnostics.extend(rules::run_rules(&parsed, &graph)); } Ok(Report { diff --git a/src/xatu/xatu_test.rs b/src/xatu/xatu_test.rs index 2e82db6..66ebd71 100644 --- a/src/xatu/xatu_test.rs +++ b/src/xatu/xatu_test.rs @@ -41,6 +41,19 @@ fn xatu_rejects_invalid_source() -> googletest::Result<()> { Ok(()) } +#[gtest] +fn xatu_reports_out_of_order_let_bindings() -> googletest::Result<()> { + let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?; + + let let_order_count = report + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.finding.rule_id == "let_order") + .count(); + expect_that!(let_order_count, eq(4)); + Ok(()) +} + #[gtest] fn xatu_reports_unquoted_uri() -> googletest::Result<()> { let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?;