feat(xatu): check let binding order

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"],

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<Node>,
     pub tokens: Vec<Token>,
     pub errors: Vec<ParseError>,
@@ -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<Binding>,
+        body: Box<Expr>,
+    },
+    AttrSet(Vec<Binding>),
+    Lambda {
+        parameters: Vec<Ident>,
+        body: Box<Expr>,
+    },
+    Other(Vec<Expr>),
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]

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<ast::Node>, Vec<ast::Token>)
     (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<ast::Binding> {
+    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<ast::Ident> {
+    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<ast::Node> {
     match node.kind() {
         rnix::SyntaxKind::NODE_ATTRPATH_VALUE => convert_binding(node),
@@ -69,7 +137,11 @@ impl TryFrom<rnix::ast::AttrpathValue> 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,
+        })
     }
 }
 

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<Diagnostic> {
+pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph) -> Vec<Diagnostic> {
     let node_diagnostics = file
         .nodes
         .iter()
@@ -66,8 +68,15 @@ pub fn run_rules(file: &ast::ParsedFile) -> Vec<Diagnostic> {
         .flat_map(|token| [run_token_rule::<UnquotedUri>(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,

src/xatu/rules/let_order.rs

@@ -0,0 +1,5 @@
+use crate::{ast, scope_graph::ScopeGraph};
+
+pub fn check(graph: &ScopeGraph) -> Vec<ast::SourceRange> {
+    graph.out_of_order_bindings()
+}

src/xatu/scope_graph.rs

@@ -1,184 +1,246 @@
-#[derive(Debug)]
+use crate::ast::{self, ParsedFile};
-pub struct ScopeGraph {
-    scopes: Vec<Scope>,
-    occurrences: Vec<Occurrence>,
-}
 
-impl Default for ScopeGraph {
+#[derive(Debug)]
-    fn default() -> Self {
+pub(crate) struct ScopeGraph {
-        Self::new()
+    nodes: Vec<Node>,
-    }
+    edges: Vec<Edge>,
+    lets: Vec<LetScope>,
 }
 
 impl ScopeGraph {
-    pub fn new() -> Self {
+    pub(crate) fn from_file(file: &ParsedFile) -> Self {
-        Self {
+        let mut graph = Self {
-            scopes: vec![Scope::new(None)],
+            nodes: Vec::new(),
-            occurrences: 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 {
+    pub(crate) fn out_of_order_bindings(&self) -> Vec<ast::SourceRange> {
-        ScopeId(0)
+        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::<Vec<_>>();
+
+                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 {
+    fn build_expr(&mut self, expr: &ast::Expr, scope: NodeId) -> Vec<NodeId> {
-        self.scope(parent);
+        match expr {
-        let scope_id = ScopeId(self.scopes.len());
+            ast::Expr::Ident(ident) => vec![self.add_reference(&ident.name, scope)],
-
+            ast::Expr::Let { bindings, body } => self.build_let(bindings, body, scope),
-        self.scopes.push(Scope::new(Some(parent)));
+            ast::Expr::AttrSet(bindings) => bindings
-
+                .iter()
-        scope_id
+                .flat_map(|binding| self.build_expr(&binding.value, scope))
-    }
+                .collect(),
-
+            ast::Expr::Lambda { parameters, body } => {
-    pub fn add_declaration(&mut self, scope: ScopeId, name: impl Into<String>) -> DeclarationId {
+                let scope = self.add_scope(Some(scope));
-        let declaration_id = DeclarationId(self.occurrences.len());
+                for parameter in parameters {
-        self.occurrences.push(Occurrence::Declaration(Declaration {
+                    self.add_declaration(&parameter.name, scope);
-            scope,
+                }
-            name: name.into(),
+                self.build_expr(body, scope)
-        }));
-        self.scope_mut(scope).declarations.push(declaration_id);
-
-        declaration_id
-    }
-
-    pub fn add_reference(&mut self, scope: ScopeId, name: impl Into<String>) -> 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<Resolution> {
-        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::<Vec<_>>();
-
-            if !resolutions.is_empty() {
-                return resolutions;
             }
-
+            ast::Expr::Other(expressions) => expressions
-            let Some(parent) = scope.parent else {
+                .iter()
-                return Vec::new();
+                .flat_map(|expression| self.build_expr(expression, scope))
-            };
+                .collect(),
-
+            ast::Expr::Select(_) => Vec::new(),
-            steps.push(PathStep::Parent);
-            scope_id = parent;
         }
     }
 
-    fn scope(&self, scope: ScopeId) -> &Scope {
+    fn build_let(
-        &self.scopes[scope.0]
+        &mut self,
+        bindings: &[ast::Binding],
+        body: &ast::Expr,
+        parent: NodeId,
+    ) -> Vec<NodeId> {
+        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::<Vec<_>>();
+
+        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 {
+    fn expected_order(&self, let_scope: &LetScope) -> Vec<NodeId> {
-        &mut self.scopes[scope.0]
+        let mut order = Vec::new();
+        self.visit_references(&let_scope.entry_references, let_scope, &mut order);
+        order
     }
 
-    fn declaration(&self, declaration: DeclarationId) -> &Declaration {
+    fn visit_references(
-        let Occurrence::Declaration(declaration) = &self.occurrences[declaration.0] else {
+        &self,
-            unreachable!("declaration IDs point to declarations")
+        references: &[NodeId],
+        let_scope: &LetScope,
+        order: &mut Vec<NodeId>,
+    ) {
+        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<NodeId> {
+        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::<Vec<_>>();
+            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>) -> 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 {
+    fn add_reference(&mut self, name: &str, scope: NodeId) -> NodeId {
-        let Occurrence::Reference(reference) = &self.occurrences[reference.0] else {
+        let reference = self.add_node(Node::Reference(name.to_owned()));
-            unreachable!("reference IDs point to references")
+        self.add_edge(reference, scope);
-        };
-
         reference
     }
-}
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+    fn add_node(&mut self, node: Node) -> NodeId {
-pub struct ScopeId(usize);
+        let id = NodeId(self.nodes.len());
+        self.nodes.push(node);
+        id
+    }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+    fn add_edge(&mut self, from: NodeId, to: NodeId) {
-pub struct DeclarationId(usize);
+        self.edges.push(Edge { from, to });
+    }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+    fn edge_target(&self, from: NodeId, predicate: fn(&Node) -> bool) -> Option<NodeId> {
-pub struct ReferenceId(usize);
+        self.edge_targets(from, predicate).next()
+    }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+    fn edge_targets<'graph>(
-pub struct Resolution {
+        &'graph self,
-    pub declaration: DeclarationId,
+        from: NodeId,
-    pub path: ResolutionPath,
+        predicate: impl Fn(&Node) -> bool + 'graph,
-}
+    ) -> impl Iterator<Item = NodeId> + 'graph {
-
+        self.edges
-#[derive(Debug, Clone, PartialEq, Eq)]
+            .iter()
-pub struct ResolutionPath {
+            .filter(move |edge| edge.from == from && predicate(&self.nodes[edge.to.0]))
-    pub steps: Vec<PathStep>,
+            .map(|edge| edge.to)
-}
-
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub enum PathStep {
-    Parent,
-    Declaration(DeclarationId),
-}
-
-#[derive(Debug)]
-struct Scope {
-    parent: Option<ScopeId>,
-    declarations: Vec<DeclarationId>,
-    references: Vec<ReferenceId>,
-}
-
-impl Scope {
-    fn new(parent: Option<ScopeId>) -> Self {
-        Self {
-            parent,
-            declarations: Vec::new(),
-            references: Vec::new(),
-        }
     }
 }
 
 #[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 {
+struct Edge {
-    scope: ScopeId,
+    from: NodeId,
-    name: String,
+    to: NodeId,
+}
+
+#[derive(Debug)]
+struct LetScope {
+    bindings: Vec<Binding>,
+    entry_references: Vec<NodeId>,
+}
+
+#[derive(Debug)]
+struct Binding {
+    declaration: NodeId,
+    references: Vec<NodeId>,
+    range: ast::SourceRange,
 }

src/xatu/scope_graph_test.rs

@@ -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::<Resolution>::new();
-    expect_that!(graph.resolve(reference), eq(&expected));
-}
-
-fn resolution(
-    declaration: xatu::DeclarationId,
-    steps: Vec<PathStep>,
-) -> Resolution {
-    Resolution {
-        declaration,
-        path: ResolutionPath { steps },
-    }
-}

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<SourceFile>) -> status::Result<Report> {
             )));
         }
 
-        diagnostics.extend(rules::run_rules(&parsed));
+        let graph = ScopeGraph::from_file(&parsed);
+        diagnostics.extend(rules::run_rules(&parsed, &graph));
     }
 
     Ok(Report {

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::<Vec<_>>(),
+        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()?;