feat(xatu): check let binding order

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 = [

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;
@@ -19,15 +21,22 @@ pub struct Finding {
     pub rule_id: &'static str,
     pub range: ast::SourceRange,
     pub message: &'static str,
+    pub binding: Option<String>,
 }
 
 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<Diagnostic> {
+pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph) -> Vec<Diagnostic> {
     let node_diagnostics = file
         .nodes
         .iter()
@@ -66,8 +75,16 @@ 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(|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,
     })
 }

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<Finding> {
+    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::<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(|_| Finding {
+                            range: binding.range,
+                            binding: binding.name.clone(),
+                        })
+                })
+        })
+        .collect()
+}
+
+fn expected_order(graph: &ScopeGraph, let_scope: &LetScope) -> Vec<NodeId> {
+    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<NodeId>,
+) {
+    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);
+        }
+    }
+}

src/xatu/scope_graph.rs

@@ -0,0 +1,194 @@
+use crate::ast::{self, ParsedFile};
+
+#[derive(Debug)]
+pub(crate) struct ScopeGraph {
+    nodes: Vec<Node>,
+    edges: Vec<Edge>,
+    lets: Vec<LetScope>,
+}
+
+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<NodeId> {
+        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(&parameter.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<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),
+                name: binding.key.name.clone(),
+                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
+    }
+
+    pub(crate) 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 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<NodeId> {
+        self.edge_targets(from, predicate).next()
+    }
+
+    fn edge_targets<'graph>(
+        &'graph self,
+        from: NodeId,
+        predicate: impl Fn(&Node) -> bool + 'graph,
+    ) -> impl Iterator<Item = NodeId> + '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<Binding>,
+    pub(crate) entry_references: Vec<NodeId>,
+}
+
+#[derive(Debug)]
+pub(crate) struct Binding {
+    pub(crate) declaration: NodeId,
+    pub(crate) name: String,
+    pub(crate) references: Vec<NodeId>,
+    pub(crate) range: ast::SourceRange,
+}

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;

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;
 }

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<SourceFile>) -> status::Result<Report> {
     let mut store = FileStore::default();
@@ -46,7 +70,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,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()?;