diff --git a/src/xatu/BUILD b/src/xatu/BUILD index 2a35aeb..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", @@ -27,15 +28,6 @@ rust_test( deps = [":xatu_lib"], ) -rust_test( - name = "scope_graph_test", - srcs = ["scope_graph_test.rs"], - deps = [ - ":xatu_lib", - "//third_party/crates:googletest", - ], -) - rust_test( name = "xatu_test", srcs = ["xatu_test.rs"], 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..f84b1e4 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; @@ -49,7 +51,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 +68,15 @@ 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(|range| Finding { + rule_id: "let_order", + range, + message: "Let bindings must follow dependency order", + }); + node_diagnostics .chain(token_diagnostics) + .chain(let_order_diagnostics) .map(|finding| Diagnostic { file_id: file.file_id, finding, diff --git a/src/xatu/rules/let_order.rs b/src/xatu/rules/let_order.rs new file mode 100644 index 0000000..56ac038 --- /dev/null +++ b/src/xatu/rules/let_order.rs @@ -0,0 +1,5 @@ +use crate::{ast, scope_graph::ScopeGraph}; + +pub fn check(graph: &ScopeGraph) -> Vec { + graph.out_of_order_bindings() +} diff --git a/src/xatu/scope_graph.rs b/src/xatu/scope_graph.rs index f5f5d28..ce1a5d0 100644 --- a/src/xatu/scope_graph.rs +++ b/src/xatu/scope_graph.rs @@ -1,184 +1,246 @@ -#[derive(Debug)] -pub struct ScopeGraph { - scopes: Vec, - occurrences: Vec, -} +use crate::ast::{self, ParsedFile}; -impl Default for ScopeGraph { - fn default() -> Self { - Self::new() - } +#[derive(Debug)] +pub(crate) struct ScopeGraph { + nodes: Vec, + edges: Vec, + lets: Vec, } impl ScopeGraph { - pub fn new() -> Self { - Self { - scopes: vec![Scope::new(None)], - occurrences: Vec::new(), - } + 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 fn root_scope(&self) -> ScopeId { - ScopeId(0) + pub(crate) fn out_of_order_bindings(&self) -> Vec { + self.lets + .iter() + .flat_map(|let_scope| { + let expected = self.expected_order(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(|_| binding.range) + }) + }) + .collect() } - pub fn add_scope(&mut self, parent: ScopeId) -> ScopeId { - self.scope(parent); - let scope_id = ScopeId(self.scopes.len()); - - self.scopes.push(Scope::new(Some(parent))); - - scope_id - } - - pub fn add_declaration(&mut self, scope: ScopeId, name: impl Into) -> DeclarationId { - let declaration_id = DeclarationId(self.occurrences.len()); - self.occurrences.push(Occurrence::Declaration(Declaration { - scope, - name: name.into(), - })); - self.scope_mut(scope).declarations.push(declaration_id); - - declaration_id - } - - pub fn add_reference(&mut self, scope: ScopeId, name: impl Into) -> ReferenceId { - let reference_id = ReferenceId(self.occurrences.len()); - self.occurrences.push(Occurrence::Reference(Reference { - scope, - name: name.into(), - })); - self.scope_mut(scope).references.push(reference_id); - - reference_id - } - - pub fn declaration_scope(&self, declaration: DeclarationId) -> ScopeId { - self.declaration(declaration).scope - } - - pub fn reference_scope(&self, reference: ReferenceId) -> ScopeId { - self.reference(reference).scope - } - - pub fn resolve(&self, reference: ReferenceId) -> Vec { - let reference = self.reference(reference); - let mut scope_id = reference.scope; - let mut steps = Vec::new(); - - loop { - let scope = self.scope(scope_id); - let declarations = - scope.declarations.iter().copied().filter(|declaration_id| { - self.declaration(*declaration_id).name == reference.name - }); - - let resolutions = declarations - .map(|declaration| { - let mut path = steps.clone(); - path.push(PathStep::Declaration(declaration)); - Resolution { - declaration, - path: ResolutionPath { steps: path }, - } - }) - .collect::>(); - - if !resolutions.is_empty() { - return resolutions; + 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) } - - let Some(parent) = scope.parent else { - return Vec::new(); - }; - - steps.push(PathStep::Parent); - scope_id = parent; + ast::Expr::Other(expressions) => expressions + .iter() + .flat_map(|expression| self.build_expr(expression, scope)) + .collect(), + ast::Expr::Select(_) => Vec::new(), } } - fn scope(&self, scope: ScopeId) -> &Scope { - &self.scopes[scope.0] + 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), + 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 } - fn scope_mut(&mut self, scope: ScopeId) -> &mut Scope { - &mut self.scopes[scope.0] + fn expected_order(&self, let_scope: &LetScope) -> Vec { + let mut order = Vec::new(); + self.visit_references(&let_scope.entry_references, let_scope, &mut order); + order } - fn declaration(&self, declaration: DeclarationId) -> &Declaration { - let Occurrence::Declaration(declaration) = &self.occurrences[declaration.0] else { - unreachable!("declaration IDs point to declarations") + fn visit_references( + &self, + references: &[NodeId], + let_scope: &LetScope, + order: &mut Vec, + ) { + for reference in references { + for declaration in self.resolve(*reference) { + let Some(binding) = let_scope + .bindings + .iter() + .find(|binding| binding.declaration == declaration) + else { + continue; + }; + + if order.contains(&declaration) { + continue; + } + + order.push(declaration); + self.visit_references(&binding.references, let_scope, order); + } + } + } + + 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 reference(&self, reference: ReferenceId) -> &Reference { - let Occurrence::Reference(reference) = &self.occurrences[reference.0] else { - unreachable!("reference IDs point to references") - }; - + 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 } -} -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct ScopeId(usize); + fn add_node(&mut self, node: Node) -> NodeId { + let id = NodeId(self.nodes.len()); + self.nodes.push(node); + id + } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct DeclarationId(usize); + fn add_edge(&mut self, from: NodeId, to: NodeId) { + self.edges.push(Edge { from, to }); + } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct ReferenceId(usize); + fn edge_target(&self, from: NodeId, predicate: fn(&Node) -> bool) -> Option { + self.edge_targets(from, predicate).next() + } -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct Resolution { - pub declaration: DeclarationId, - pub path: ResolutionPath, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct ResolutionPath { - pub steps: Vec, -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PathStep { - Parent, - Declaration(DeclarationId), -} - -#[derive(Debug)] -struct Scope { - parent: Option, - declarations: Vec, - references: Vec, -} - -impl Scope { - fn new(parent: Option) -> Self { - Self { - parent, - declarations: Vec::new(), - references: Vec::new(), - } + 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 Occurrence { +enum Node { + Scope, Declaration(Declaration), - Reference(Reference), + Reference(String), +} + +impl Node { + fn is_scope(&self) -> bool { + matches!(self, Self::Scope) + } } #[derive(Debug)] struct Declaration { - scope: ScopeId, name: String, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct NodeId(usize); + #[derive(Debug)] -struct Reference { - scope: ScopeId, - name: String, +struct Edge { + from: NodeId, + to: NodeId, +} + +#[derive(Debug)] +struct LetScope { + bindings: Vec, + entry_references: Vec, +} + +#[derive(Debug)] +struct Binding { + declaration: NodeId, + references: Vec, + range: ast::SourceRange, } diff --git a/src/xatu/scope_graph_test.rs b/src/xatu/scope_graph_test.rs deleted file mode 100644 index 69e27f6..0000000 --- a/src/xatu/scope_graph_test.rs +++ /dev/null @@ -1,76 +0,0 @@ -use googletest::matchers::eq; -use googletest::{expect_that, gtest}; -use xatu::{PathStep, Resolution, ResolutionPath, ScopeGraph}; - -#[gtest] -fn resolves_a_local_declaration() { - let mut graph = ScopeGraph::new(); - let scope = graph.root_scope(); - let declaration = graph.add_declaration(scope, "value"); - let reference = graph.add_reference(scope, "value"); - - let expected = vec![resolution(declaration, vec![PathStep::Declaration(declaration)])]; - expect_that!(graph.resolve(reference), eq(&expected)); -} - -#[gtest] -fn resolves_a_parent_declaration() { - let mut graph = ScopeGraph::new(); - let parent = graph.root_scope(); - let declaration = graph.add_declaration(parent, "value"); - let child = graph.add_scope(parent); - let reference = graph.add_reference(child, "value"); - - let expected = vec![resolution( - declaration, - vec![PathStep::Parent, PathStep::Declaration(declaration)], - )]; - expect_that!(graph.resolve(reference), eq(&expected)); -} - -#[gtest] -fn local_declarations_shadow_parent_declarations() { - let mut graph = ScopeGraph::new(); - let parent = graph.root_scope(); - graph.add_declaration(parent, "value"); - let child = graph.add_scope(parent); - let declaration = graph.add_declaration(child, "value"); - let reference = graph.add_reference(child, "value"); - - let expected = vec![resolution(declaration, vec![PathStep::Declaration(declaration)])]; - expect_that!(graph.resolve(reference), eq(&expected)); -} - -#[gtest] -fn preserves_ambiguous_local_declarations() { - let mut graph = ScopeGraph::new(); - let scope = graph.root_scope(); - let first = graph.add_declaration(scope, "value"); - let second = graph.add_declaration(scope, "value"); - let reference = graph.add_reference(scope, "value"); - - let expected = vec![ - resolution(first, vec![PathStep::Declaration(first)]), - resolution(second, vec![PathStep::Declaration(second)]), - ]; - expect_that!(graph.resolve(reference), eq(&expected)); -} - -#[gtest] -fn leaves_unresolved_references_without_resolutions() { - let mut graph = ScopeGraph::new(); - let reference = graph.add_reference(graph.root_scope(), "value"); - - let expected = Vec::::new(); - expect_that!(graph.resolve(reference), eq(&expected)); -} - -fn resolution( - declaration: xatu::DeclarationId, - steps: Vec, -) -> Resolution { - Resolution { - declaration, - path: ResolutionPath { steps }, - } -} diff --git a/src/xatu/xatu.rs b/src/xatu/xatu.rs index a906eae..c8cb67a 100644 --- a/src/xatu/xatu.rs +++ b/src/xatu/xatu.rs @@ -4,11 +4,10 @@ mod parser; mod rules; mod scope_graph; +use scope_graph::ScopeGraph; + pub use file_store::{FileId, FileStore, SourceFile}; pub use rules::{Diagnostic, Finding}; -pub use scope_graph::{ - DeclarationId, PathStep, ReferenceId, Resolution, ResolutionPath, ScopeGraph, ScopeId, -}; #[derive(Debug)] pub struct Report { @@ -50,7 +49,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..f41212f 100644 --- a/src/xatu/xatu_test.rs +++ b/src/xatu/xatu_test.rs @@ -41,6 +41,26 @@ fn xatu_rejects_invalid_source() -> googletest::Result<()> { Ok(()) } +#[gtest] +fn xatu_reports_out_of_order_let_binding() -> googletest::Result<()> { + let report = xatu::future_sight(vec![SourceFile { + path: PathBuf::from("test.nix"), + source: "let\n second = 2;\n first = 1;\nin {\n firstField = first;\n secondField = second;\n}\n" + .to_owned(), + }]) + .or_fail()?; + + expect_that!( + report + .diagnostics + .iter() + .map(|diagnostic| diagnostic.finding.rule_id) + .collect::>(), + contains(eq(&"let_order")) + ); + Ok(()) +} + #[gtest] fn xatu_reports_unquoted_uri() -> googletest::Result<()> { let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?;