diff --git a/src/xatu/BUILD b/src/xatu/BUILD index f60733b..e340ba5 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/ast.rs b/src/xatu/ast.rs index 1deb464..2627844 100644 --- a/src/xatu/ast.rs +++ b/src/xatu/ast.rs @@ -29,12 +29,20 @@ impl std::fmt::Display for SourceRange { #[derive(Debug, Clone, PartialEq, Eq)] pub enum NodeData { Binding(Binding), + LetBlock(LetBlock), } #[derive(Debug, Clone, PartialEq, Eq)] pub struct Binding { pub key: Ident, pub value: Expr, + pub references: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct LetBlock { + pub bindings: Vec, + pub body_references: Vec, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -46,6 +54,7 @@ pub struct Ident { pub enum Expr { Ident(Ident), Select(Select), + Other, } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/xatu/parser.rs b/src/xatu/parser.rs index 61545c3..f8f7624 100644 --- a/src/xatu/parser.rs +++ b/src/xatu/parser.rs @@ -1,4 +1,4 @@ -use rnix::ast::Attr; +use rnix::ast::{Attr, HasEntry}; use rowan::ast::AstNode; use crate::{FileId, ast}; @@ -38,6 +38,7 @@ fn convert_syntax(syntax: rnix::SyntaxNode) -> (Vec, Vec) fn convert_node(node: rnix::SyntaxNode) -> Option { match node.kind() { rnix::SyntaxKind::NODE_ATTRPATH_VALUE => convert_binding(node), + rnix::SyntaxKind::NODE_LET_IN => convert_let_block(node), _ => None, } } @@ -53,6 +54,34 @@ fn convert_binding(node: rnix::SyntaxNode) -> Option { }) } +fn convert_let_block(node: rnix::SyntaxNode) -> Option { + let let_block = rnix::ast::LetIn::cast(node)?; + let range = let_block.syntax().text_range().into(); + let data = let_block.try_into().ok()?; + + Some(ast::Node { + range, + data: ast::NodeData::LetBlock(data), + }) +} + +impl TryFrom for ast::LetBlock { + type Error = (); + + fn try_from(let_block: rnix::ast::LetIn) -> Result { + let bindings = let_block + .attrpath_values() + .map(ast::Binding::try_from) + .collect::, _>>()?; + let body_references = let_block.body().ok_or(())?.references(); + + Ok(Self { + bindings, + body_references, + }) + } +} + impl TryFrom for ast::Binding { type Error = (); @@ -67,9 +96,15 @@ impl TryFrom for ast::Binding { return Err(()); } - let value = binding.value().ok_or(())?.try_into()?; + let value_expr = binding.value().ok_or(())?; + let references = value_expr.references(); + let value = value_expr.try_into()?; - Ok(Self { key, value }) + Ok(Self { + key, + value, + references, + }) } } @@ -80,7 +115,7 @@ impl TryFrom for ast::Expr { match expr { rnix::ast::Expr::Ident(ident) => Ok(Self::Ident(ident.into())), rnix::ast::Expr::Select(select) => Ok(Self::Select(select.try_into()?)), - _ => Err(()), + _ => Ok(Self::Other), } } } @@ -93,6 +128,37 @@ impl From for ast::Ident { } } +trait ExprReferences { + fn references(&self) -> Vec; +} + +impl ExprReferences for rnix::ast::Expr { + fn references(&self) -> Vec { + self.syntax() + .descendants() + .filter_map(rnix::ast::Ident::cast) + .filter(is_reference) + .map(ast::Ident::from) + .collect() + } +} + +fn is_reference(ident: &rnix::ast::Ident) -> bool { + let parent = ident.syntax().parent(); + let grandparent = parent.as_ref().and_then(rowan::SyntaxNode::parent); + + !matches!( + ( + parent.map(|node| node.kind()), + grandparent.map(|node| node.kind()) + ), + ( + Some(rnix::SyntaxKind::NODE_ATTRPATH), + Some(rnix::SyntaxKind::NODE_ATTRPATH_VALUE | rnix::SyntaxKind::NODE_SELECT), + ) + ) +} + impl TryFrom for ast::Select { type Error = (); diff --git a/src/xatu/rules.rs b/src/xatu/rules.rs index b27570d..a75907a 100644 --- a/src/xatu/rules.rs +++ b/src/xatu/rules.rs @@ -1,9 +1,11 @@ use crate::FileId; use crate::ast; +use crate::rules::let_order::LetOrder; use crate::rules::manual_inherit::ManualInherit; use crate::rules::manual_inherit_from::ManualInheritFrom; use crate::rules::unquoted_uri::UnquotedUri; +pub mod let_order; pub mod manual_inherit; pub mod manual_inherit_from; pub mod unquoted_uri; @@ -18,7 +20,7 @@ pub struct Diagnostic { pub struct Finding { pub rule_id: &'static str, pub range: ast::SourceRange, - pub message: &'static str, + pub message: String, } impl std::fmt::Display for Diagnostic { @@ -37,7 +39,7 @@ pub trait NodeRule { const ID: &'static str; fn select(data: &ast::NodeData) -> Option<&Self::Node>; - fn check(node: &Self::Node) -> Option<&'static str>; + fn check(node: &Self::Node) -> Option; } pub trait TokenRule { @@ -46,7 +48,7 @@ pub trait TokenRule { const ID: &'static str; fn select(data: &ast::TokenData) -> Option<&Self::Token>; - fn check(token: &Self::Token) -> Option<&'static str>; + fn check(token: &Self::Token) -> Option; } pub fn run_rules(file: &ast::ParsedFile) -> Vec { @@ -55,6 +57,7 @@ pub fn run_rules(file: &ast::ParsedFile) -> Vec { .iter() .flat_map(|node| { [ + run_node_rule::(node), run_node_rule::(node), run_node_rule::(node), ] diff --git a/src/xatu/rules/let_order.rs b/src/xatu/rules/let_order.rs new file mode 100644 index 0000000..127106c --- /dev/null +++ b/src/xatu/rules/let_order.rs @@ -0,0 +1,104 @@ +use std::collections::{HashMap, HashSet}; + +use crate::{ast, rules::NodeRule}; + +pub struct LetOrder; + +impl NodeRule for LetOrder { + type Node = ast::LetBlock; + + const ID: &'static str = "let_order"; + + fn select(data: &ast::NodeData) -> Option<&Self::Node> { + match data { + ast::NodeData::LetBlock(let_block) => Some(let_block), + _ => None, + } + } + + fn check(node: &Self::Node) -> Option { + let expected_order = match expected_order(node) { + Some(expected_order) => expected_order, + None => return Some("Cannot order cyclic `let` bindings".to_string()), + }; + let actual_order = reachable_actual_order(node, &expected_order); + + if actual_order == expected_order { + return None; + } + + Some(format!( + "Order reachable `let` bindings by body-first dependency flow: {}", + expected_order.join(", ") + )) + } +} + +fn expected_order(let_block: &ast::LetBlock) -> Option> { + let bindings = let_block + .bindings + .iter() + .map(|binding| (binding.key.name.as_str(), binding)) + .collect::>(); + let mut order = Vec::new(); + let mut permanent_marks = HashSet::new(); + let mut temporary_marks = HashSet::new(); + + for reference in &let_block.body_references { + visit_binding( + &reference.name, + &bindings, + &mut order, + &mut permanent_marks, + &mut temporary_marks, + )?; + } + + Some(order) +} + +fn visit_binding( + name: &str, + bindings: &HashMap<&str, &ast::Binding>, + order: &mut Vec, + permanent_marks: &mut HashSet, + temporary_marks: &mut HashSet, +) -> Option<()> { + let Some(binding) = bindings.get(name) else { + return Some(()); + }; + + if permanent_marks.contains(name) { + return Some(()); + } + + if !temporary_marks.insert(name.to_string()) { + return None; + } + + order.push(name.to_string()); + for reference in &binding.references { + visit_binding( + &reference.name, + bindings, + order, + permanent_marks, + temporary_marks, + )?; + } + + temporary_marks.remove(name); + permanent_marks.insert(name.to_string()); + Some(()) +} + +fn reachable_actual_order(let_block: &ast::LetBlock, expected_order: &[String]) -> Vec { + let reachable_names = expected_order.iter().collect::>(); + + let_block + .bindings + .iter() + .filter(|binding| reachable_names.contains(&binding.key.name)) + .map(|binding| binding.key.name.clone()) + .collect() +} diff --git a/src/xatu/rules/manual_inherit.rs b/src/xatu/rules/manual_inherit.rs index 76c71c7..ec5a9ad 100644 --- a/src/xatu/rules/manual_inherit.rs +++ b/src/xatu/rules/manual_inherit.rs @@ -8,15 +8,16 @@ impl NodeRule for ManualInherit { const ID: &'static str = "manual_inherit"; fn select(data: &ast::NodeData) -> Option<&Self::Node> { - let ast::NodeData::Binding(binding) = data; - - Some(binding) + match data { + ast::NodeData::Binding(binding) => Some(binding), + _ => None, + } } - fn check(node: &Self::Node) -> Option<&'static str> { + fn check(node: &Self::Node) -> Option { match &node.value { ast::Expr::Ident(value) if node.key == *value => { - Some("Use `inherit` for this assignment") + Some("Use `inherit` for this assignment".to_string()) } _ => None, } diff --git a/src/xatu/rules/manual_inherit_from.rs b/src/xatu/rules/manual_inherit_from.rs index 5d8717a..1cf78d9 100644 --- a/src/xatu/rules/manual_inherit_from.rs +++ b/src/xatu/rules/manual_inherit_from.rs @@ -8,15 +8,16 @@ impl NodeRule for ManualInheritFrom { const ID: &'static str = "manual_inherit_from"; fn select(data: &ast::NodeData) -> Option<&Self::Node> { - let ast::NodeData::Binding(binding) = data; - - Some(binding) + match data { + ast::NodeData::Binding(binding) => Some(binding), + _ => None, + } } - fn check(node: &Self::Node) -> Option<&'static str> { + fn check(node: &Self::Node) -> Option { match &node.value { ast::Expr::Select(select) if node.key == select.attr => { - Some("Use `inherit` for this selected attribute") + Some("Use `inherit` for this selected attribute".to_string()) } _ => None, } diff --git a/src/xatu/rules/unquoted_uri.rs b/src/xatu/rules/unquoted_uri.rs index cb7410e..0f2bd53 100644 --- a/src/xatu/rules/unquoted_uri.rs +++ b/src/xatu/rules/unquoted_uri.rs @@ -13,7 +13,7 @@ impl TokenRule for UnquotedUri { } } - fn check(_token: &Self::Token) -> Option<&'static str> { - Some("URI literals should be quoted") + fn check(_token: &Self::Token) -> Option { + Some("URI literals should be quoted".to_string()) } } diff --git a/src/xatu/testdata/lint_findings.nix b/src/xatu/testdata/lint_findings.nix index 17b4837..74e396a 100644 --- a/src/xatu/testdata/lint_findings.nix +++ b/src/xatu/testdata/lint_findings.nix @@ -1,5 +1,9 @@ -{ +let + support = "support"; + primary = support.value; +in { package = package; from = pkgs.from; url = https://example.com; + ordered = primary; } diff --git a/src/xatu/xatu_test.rs b/src/xatu/xatu_test.rs index 2e82db6..464f206 100644 --- a/src/xatu/xatu_test.rs +++ b/src/xatu/xatu_test.rs @@ -65,6 +65,23 @@ fn xatu_reports_unquoted_uri() -> googletest::Result<()> { Ok(()) } +#[gtest] +fn xatu_reports_let_order() -> googletest::Result<()> { + let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?; + + let let_order_diagnostics = report + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.finding.rule_id == "let_order") + .collect::>(); + expect_that!(let_order_diagnostics.len(), eq(1)); + expect_that!( + let_order_diagnostics[0].finding.message.as_str(), + eq("Order reachable `let` bindings by body-first dependency flow: primary, support") + ); + Ok(()) +} + #[gtest] fn xatu_reports_manual_inherit() -> googletest::Result<()> { let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?;