diff --git a/src/xatu/BUILD b/src/xatu/BUILD index f60733b..0ca6ac7 100644 --- a/src/xatu/BUILD +++ b/src/xatu/BUILD @@ -8,9 +8,11 @@ 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", + "scope_graph.rs", "xatu.rs", ], deps = [ diff --git a/src/xatu/ast.rs b/src/xatu/ast.rs index 1deb464..c24b52c 100644 --- a/src/xatu/ast.rs +++ b/src/xatu/ast.rs @@ -3,6 +3,7 @@ use crate::FileId; #[derive(Debug)] pub struct ParsedFile { pub file_id: FileId, + pub root: Expr, pub nodes: Vec, pub tokens: Vec, pub errors: Vec, @@ -33,6 +34,7 @@ pub enum NodeData { #[derive(Debug, Clone, PartialEq, Eq)] pub struct Binding { + pub range: SourceRange, pub key: Ident, pub value: Expr, } @@ -46,6 +48,16 @@ pub struct Ident { pub enum Expr { Ident(Ident), Select(Select), + Let { + bindings: Vec, + body: Box, + }, + AttrSet(Vec), + Lambda { + parameters: Vec, + body: Box, + }, + Other(Vec), } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/xatu/parser.rs b/src/xatu/parser.rs index 61545c3..cf60b40 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}; @@ -7,10 +7,14 @@ pub fn parse_file(file_id: FileId, source: &str) -> ast::ParsedFile { let parse = rnix::Root::parse(source); let source_len = source.len(); - let (nodes, tokens) = convert_syntax(parse.syntax()); + let syntax = parse.syntax(); + let root = rnix::ast::Root::cast(syntax.clone()).expect("parser root is a root node"); + let root = convert_expr(root.expr().expect("parser root has an expression")); + let (nodes, tokens) = convert_syntax(syntax); ast::ParsedFile { file_id, + root, nodes, tokens, errors: parse @@ -35,6 +39,70 @@ fn convert_syntax(syntax: rnix::SyntaxNode) -> (Vec, Vec) (nodes, tokens) } +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 { + bindings: let_in + .attrpath_values() + .filter_map(convert_binding_ast) + .collect(), + body: Box::new(convert_expr( + let_in.body().expect("let expressions have bodies"), + )), + }, + rnix::ast::Expr::AttrSet(attr_set) => ast::Expr::AttrSet( + attr_set + .attrpath_values() + .filter_map(convert_binding_ast) + .collect(), + ), + rnix::ast::Expr::Lambda(lambda) => ast::Expr::Lambda { + parameters: lambda_parameters(&lambda), + body: Box::new(convert_expr(lambda.body().expect("lambdas have bodies"))), + }, + expr => ast::Expr::Other( + expr.syntax() + .children() + .filter_map(rnix::ast::Expr::cast) + .map(convert_expr) + .collect(), + ), + } +} + +fn convert_binding_ast(binding: rnix::ast::AttrpathValue) -> Option { + let mut attrs = binding.attrpath()?.attrs(); + let Attr::Ident(key) = attrs.next()? else { + return None; + }; + + if attrs.next().is_some() { + return None; + } + + Some(ast::Binding { + range: binding.syntax().text_range().into(), + key: key.into(), + value: convert_expr(binding.value()?), + }) +} + +fn lambda_parameters(lambda: &rnix::ast::Lambda) -> Vec { + match lambda.param() { + Some(rnix::ast::Param::IdentParam(parameter)) => { + parameter.ident().map(Into::into).into_iter().collect() + } + Some(rnix::ast::Param::Pattern(pattern)) => pattern + .pat_entries() + .filter_map(|entry| entry.ident()) + .chain(pattern.pat_bind().and_then(|binding| binding.ident())) + .map(Into::into) + .collect(), + None => Vec::new(), + } +} + fn convert_node(node: rnix::SyntaxNode) -> Option { match node.kind() { rnix::SyntaxKind::NODE_ATTRPATH_VALUE => convert_binding(node), @@ -69,7 +137,11 @@ impl TryFrom for ast::Binding { let value = binding.value().ok_or(())?.try_into()?; - Ok(Self { key, value }) + Ok(Self { + range: binding.syntax().text_range().into(), + key, + value, + }) } } 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/scope_graph.rs b/src/xatu/scope_graph.rs new file mode 100644 index 0000000..6795458 --- /dev/null +++ b/src/xatu/scope_graph.rs @@ -0,0 +1,194 @@ +use crate::ast::{self, ParsedFile}; + +#[derive(Debug)] +pub(crate) struct ScopeGraph { + nodes: Vec, + edges: Vec, + lets: Vec, +} + +impl ScopeGraph { + pub(crate) fn from_file(file: &ParsedFile) -> Self { + let mut graph = Self { + nodes: Vec::new(), + edges: Vec::new(), + lets: Vec::new(), + }; + let root = graph.add_scope(None); + graph.build_expr(&file.root, root); + graph + } + + pub(crate) fn let_scopes(&self) -> &[LetScope] { + &self.lets + } + + 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::AttrSet(bindings) => bindings + .iter() + .flat_map(|binding| self.build_expr(&binding.value, scope)) + .collect(), + ast::Expr::Lambda { parameters, body } => { + let scope = self.add_scope(Some(scope)); + for parameter in parameters { + self.add_declaration(¶meter.name, scope); + } + self.build_expr(body, scope) + } + ast::Expr::Other(expressions) => expressions + .iter() + .flat_map(|expression| self.build_expr(expression, scope)) + .collect(), + ast::Expr::Select(_) => Vec::new(), + } + } + + fn build_let( + &mut self, + bindings: &[ast::Binding], + body: &ast::Expr, + parent: NodeId, + ) -> Vec { + let scope = self.add_scope(Some(parent)); + let mut graph_bindings = bindings + .iter() + .map(|binding| Binding { + declaration: self.add_declaration(&binding.key.name, scope), + name: binding.key.name.clone(), + references: Vec::new(), + range: binding.range, + }) + .collect::>(); + + for (binding, graph_binding) in bindings.iter().zip(&mut graph_bindings) { + graph_binding.references = self.build_expr(&binding.value, scope); + } + + let entry_references = self.build_expr(body, scope); + let references = graph_bindings + .iter() + .flat_map(|binding| binding.references.iter().copied()) + .chain(entry_references.iter().copied()) + .collect(); + self.lets.push(LetScope { + bindings: graph_bindings, + entry_references, + }); + + references + } + + pub(crate) fn resolve(&self, reference: NodeId) -> Vec { + let Node::Reference(name) = &self.nodes[reference.0] else { + return Vec::new(); + }; + let Some(mut scope) = self.edge_target(reference, Node::is_scope) else { + return Vec::new(); + }; + + loop { + let declarations = self + .edge_targets(scope, |node| matches!(node, Node::Declaration(declaration) if declaration.name == *name)) + .collect::>(); + if !declarations.is_empty() { + return declarations; + } + + let Some(parent) = self.edge_target(scope, Node::is_scope) else { + return Vec::new(); + }; + scope = parent; + } + } + + fn add_scope(&mut self, parent: Option) -> NodeId { + let scope = self.add_node(Node::Scope); + if let Some(parent) = parent { + self.add_edge(scope, parent); + } + scope + } + + fn add_declaration(&mut self, name: &str, scope: NodeId) -> NodeId { + let declaration = self.add_node(Node::Declaration(Declaration { + name: name.to_owned(), + })); + self.add_edge(scope, declaration); + declaration + } + + fn add_reference(&mut self, name: &str, scope: NodeId) -> NodeId { + let reference = self.add_node(Node::Reference(name.to_owned())); + self.add_edge(reference, scope); + reference + } + + fn add_node(&mut self, node: Node) -> NodeId { + let id = NodeId(self.nodes.len()); + self.nodes.push(node); + id + } + + fn add_edge(&mut self, from: NodeId, to: NodeId) { + self.edges.push(Edge { from, to }); + } + + fn edge_target(&self, from: NodeId, predicate: fn(&Node) -> bool) -> Option { + self.edge_targets(from, predicate).next() + } + + fn edge_targets<'graph>( + &'graph self, + from: NodeId, + predicate: impl Fn(&Node) -> bool + 'graph, + ) -> impl Iterator + 'graph { + self.edges + .iter() + .filter(move |edge| edge.from == from && predicate(&self.nodes[edge.to.0])) + .map(|edge| edge.to) + } +} + +#[derive(Debug)] +enum Node { + Scope, + Declaration(Declaration), + Reference(String), +} + +impl Node { + fn is_scope(&self) -> bool { + matches!(self, Self::Scope) + } +} + +#[derive(Debug)] +struct Declaration { + name: String, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct NodeId(usize); + +#[derive(Debug)] +struct Edge { + from: NodeId, + to: NodeId, +} + +#[derive(Debug)] +pub(crate) struct LetScope { + pub(crate) bindings: Vec, + pub(crate) entry_references: Vec, +} + +#[derive(Debug)] +pub(crate) struct Binding { + pub(crate) declaration: NodeId, + pub(crate) name: String, + pub(crate) references: Vec, + pub(crate) range: ast::SourceRange, +} 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 bc8b40f..f718d89 100644 --- a/src/xatu/xatu.rs +++ b/src/xatu/xatu.rs @@ -2,6 +2,9 @@ mod ast; mod file_store; mod parser; mod rules; +mod scope_graph; + +use scope_graph::ScopeGraph; pub use file_store::{FileId, FileStore, SourceFile}; pub use rules::{Diagnostic, Finding}; @@ -22,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(); @@ -46,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()?;