feat(nix): lint let binding order

Cargo.lock

@@ -159,6 +159,7 @@ name = "nix-style-linter"
 version = "0.1.0"
 dependencies = [
  "rnix",
+ "rowan",
 ]
 
 [[package]]

src/nix-style-linter/BUILD

@@ -4,7 +4,10 @@ rust_library(
     name = "nix_style_linter_lib",
     crate_name = "nix_style_linter",
     srcs = ["lib.rs"],
-    deps = ["//third_party/crates:rnix"],
+    deps = [
+        "//third_party/crates:rnix",
+        "//third_party/crates:rowan",
+    ],
 )
 
 rust_binary(

src/nix-style-linter/Cargo.toml

@@ -16,3 +16,4 @@ path = "lib_test.rs"
 
 [dependencies]
 rnix = "0.14.0"
+rowan = "0.16.1"

src/nix-style-linter/lib.rs

@@ -1,9 +1,17 @@
+use std::collections::HashSet;
 use std::ffi::OsStr;
 use std::fmt;
 use std::fs;
 use std::io::{self, Write};
 use std::path::{Path, PathBuf};
 
+use rnix::SyntaxKind;
+use rnix::ast::{self, HasEntry};
+use rowan::ast::AstNode;
+
+const PARSE_ERROR_RULE: &str = "parse-error";
+const LET_ORDER_RULE: &str = "let-order";
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum RunStatus {
     Clean,
@@ -97,13 +105,22 @@ pub fn run(
 
 pub fn lint_file(path: &Path) -> io::Result<Vec<Diagnostic>> {
     let source = fs::read_to_string(path)?;
-    Ok(parse_diagnostics(path, &source))
+    let parse = rnix::Root::parse(&source);
+    let parse_diagnostics = parse_diagnostics(path, &source, parse.errors());
+
+    if !parse_diagnostics.is_empty() {
+        return Ok(parse_diagnostics);
     }
 
-fn parse_diagnostics(path: &Path, source: &str) -> Vec<Diagnostic> {
-    let parse = rnix::Root::parse(source);
-    parse
-        .errors()
+    Ok(let_order_diagnostics(path, &source, &parse.tree()))
+}
+
+fn parse_diagnostics(
+    path: &Path,
+    source: &str,
+    errors: &[rnix::ParseError],
+) -> Vec<Diagnostic> {
+    errors
         .iter()
         .map(|error| {
             let (line, column) = line_column(source, parse_error_offset(error, source.len()));
@@ -111,13 +128,274 @@ fn parse_diagnostics(path: &Path, source: &str) -> Vec<Diagnostic> {
                 path: path.to_path_buf(),
                 line,
                 column,
-                rule: "parse-error",
+                rule: PARSE_ERROR_RULE,
                 message: error.to_string(),
             }
         })
         .collect()
 }
 
+fn let_order_diagnostics(path: &Path, source: &str, root: &ast::Root) -> Vec<Diagnostic> {
+    root.syntax()
+        .descendants()
+        .filter_map(ast::LetIn::cast)
+        .flat_map(|let_in| let_order_diagnostics_for_let(path, source, &let_in))
+        .collect()
+}
+
+fn let_order_diagnostics_for_let(
+    path: &Path,
+    source: &str,
+    let_in: &ast::LetIn,
+) -> Vec<Diagnostic> {
+    let bindings: Vec<_> = let_in
+        .attrpath_values()
+        .filter_map(|binding| plain_binding(source, &binding))
+        .collect();
+    let names: Vec<_> = bindings
+        .iter()
+        .map(|binding| binding.name.clone())
+        .collect();
+    let mut diagnostics = Vec::new();
+
+    for (index, binding) in bindings.iter().enumerate() {
+        let later_names: HashSet<_> = names.iter().skip(index + 1).cloned().collect();
+        if let Some(referenced_name) =
+            first_forward_reference(binding.value.syntax(), &later_names, &mut HashSet::new())
+        {
+            if is_suppressed(source, binding.line, LET_ORDER_RULE) {
+                continue;
+            }
+
+            diagnostics.push(Diagnostic {
+                path: path.to_path_buf(),
+                line: binding.line,
+                column: binding.column,
+                rule: LET_ORDER_RULE,
+                message: format!(
+                    "`{binding}` references later let binding `{referenced}`",
+                    binding = binding.name,
+                    referenced = referenced_name
+                ),
+            });
+        }
+    }
+
+    diagnostics
+}
+
+#[derive(Debug, Clone)]
+struct Binding {
+    name: String,
+    line: usize,
+    column: usize,
+    value: ast::Expr,
+}
+
+fn plain_binding(source: &str, binding: &ast::AttrpathValue) -> Option<Binding> {
+    let attr = plain_attr(binding.attrpath()?)?;
+    let value = binding.value()?;
+    let offset = usize::from(attr.text_range().start());
+    let (line, column) = line_column(source, offset);
+
+    Some(Binding {
+        name: attr.text().to_string(),
+        line,
+        column,
+        value,
+    })
+}
+
+fn plain_attrpath_name(binding: &ast::AttrpathValue) -> Option<String> {
+    let attr = plain_attr(binding.attrpath()?)?;
+    Some(attr.text().to_string())
+}
+
+fn plain_attr(attrpath: ast::Attrpath) -> Option<rnix::SyntaxToken> {
+    let mut attrs = attrpath.attrs();
+    let attr = attrs.next()?;
+
+    if attrs.next().is_some() {
+        return None;
+    }
+
+    match attr {
+        ast::Attr::Ident(ident) => ident.ident_token(),
+        ast::Attr::Dynamic(_) | ast::Attr::Str(_) => None,
+    }
+}
+
+fn first_forward_reference(
+    node: &rnix::SyntaxNode,
+    later_names: &HashSet<String>,
+    shadowed: &mut HashSet<String>,
+) -> Option<String> {
+    match node.kind() {
+        SyntaxKind::NODE_LAMBDA => {
+            return with_shadowed(shadowed, lambda_bound_names(node), |shadowed| {
+                first_forward_reference_in_children(node, later_names, shadowed)
+            });
+        }
+        SyntaxKind::NODE_LET_IN => {
+            return with_shadowed(shadowed, plain_let_binding_names(node), |shadowed| {
+                first_forward_reference_in_children(node, later_names, shadowed)
+            });
+        }
+        SyntaxKind::NODE_ATTR_SET if rec_attrset_binds_names(node) => {
+            return with_shadowed(shadowed, plain_attrset_binding_names(node), |shadowed| {
+                first_forward_reference_in_children(node, later_names, shadowed)
+            });
+        }
+        _ => {}
+    }
+
+    if is_reference_ident(node) {
+        if let Some(name) = ident_name(node) {
+            if later_names.contains(&name) && !shadowed.contains(&name) {
+                return Some(name);
+            }
+        }
+    }
+
+    first_forward_reference_in_children(node, later_names, shadowed)
+}
+
+fn first_forward_reference_in_children(
+    node: &rnix::SyntaxNode,
+    later_names: &HashSet<String>,
+    shadowed: &mut HashSet<String>,
+) -> Option<String> {
+    node.children()
+        .find_map(|child| first_forward_reference(&child, later_names, shadowed))
+}
+
+fn with_shadowed<R>(
+    shadowed: &mut HashSet<String>,
+    names: Vec<String>,
+    action: impl FnOnce(&mut HashSet<String>) -> R,
+) -> R {
+    let inserted: Vec<_> = names
+        .into_iter()
+        .filter(|name| shadowed.insert(name.clone()))
+        .collect();
+    let result = action(shadowed);
+
+    for name in inserted {
+        shadowed.remove(&name);
+    }
+
+    result
+}
+
+fn is_reference_ident(node: &rnix::SyntaxNode) -> bool {
+    if node.kind() != SyntaxKind::NODE_IDENT {
+        return false;
+    }
+
+    !matches!(
+        node.parent().map(|parent| parent.kind()),
+        Some(
+            SyntaxKind::NODE_ATTRPATH
+                | SyntaxKind::NODE_IDENT_PARAM
+                | SyntaxKind::NODE_PAT_BIND
+                | SyntaxKind::NODE_PAT_ENTRY
+        )
+    )
+}
+
+fn ident_name(node: &rnix::SyntaxNode) -> Option<String> {
+    ast::Ident::cast(node.clone())
+        .and_then(|ident| ident.ident_token())
+        .map(|token| token.text().to_string())
+}
+
+fn lambda_bound_names(node: &rnix::SyntaxNode) -> Vec<String> {
+    let Some(lambda) = ast::Lambda::cast(node.clone()) else {
+        return Vec::new();
+    };
+    let Some(param) = lambda.param() else {
+        return Vec::new();
+    };
+
+    match param {
+        ast::Param::IdentParam(param) => param
+            .ident()
+            .and_then(|ident| ident.ident_token())
+            .map(|token| vec![token.text().to_string()])
+            .unwrap_or_default(),
+        ast::Param::Pattern(pattern) => {
+            let mut names: Vec<_> = pattern
+                .pat_entries()
+                .filter_map(|entry| entry.ident())
+                .filter_map(|ident| ident.ident_token())
+                .map(|token| token.text().to_string())
+                .collect();
+
+            if let Some(name) = pattern
+                .pat_bind()
+                .and_then(|bind| bind.ident())
+                .and_then(|ident| ident.ident_token())
+                .map(|token| token.text().to_string())
+            {
+                names.push(name);
+            }
+
+            names
+        }
+    }
+}
+
+fn plain_let_binding_names(node: &rnix::SyntaxNode) -> Vec<String> {
+    ast::LetIn::cast(node.clone())
+        .map(|let_in| {
+            let_in
+                .attrpath_values()
+                .filter_map(|binding| plain_attrpath_name(&binding))
+                .collect()
+        })
+        .unwrap_or_default()
+}
+
+fn rec_attrset_binds_names(node: &rnix::SyntaxNode) -> bool {
+    ast::AttrSet::cast(node.clone())
+        .and_then(|attrset| attrset.rec_token())
+        .is_some()
+}
+
+fn plain_attrset_binding_names(node: &rnix::SyntaxNode) -> Vec<String> {
+    ast::AttrSet::cast(node.clone())
+        .map(|attrset| {
+            attrset
+                .attrpath_values()
+                .filter_map(|binding| plain_attrpath_name(&binding))
+                .collect()
+        })
+        .unwrap_or_default()
+}
+
+fn is_suppressed(source: &str, line: usize, rule: &str) -> bool {
+    let Some(previous_line_index) = line.checked_sub(2) else {
+        return false;
+    };
+    let Some(previous_line) = source.lines().nth(previous_line_index) else {
+        return false;
+    };
+    let Some(comment) = previous_line.trim_start().strip_prefix('#') else {
+        return false;
+    };
+    let Some(rest) = comment.trim().strip_prefix("nix-style-linter:") else {
+        return false;
+    };
+    let Some((directive, reason)) = rest.trim().split_once("--") else {
+        return false;
+    };
+    let mut words = directive.split_whitespace();
+
+    words.next() == Some("disable-next-line")
+        && words.any(|word| word == rule)
+        && !reason.trim().is_empty()
+}
+
 fn collect_nix_files(path: &Path) -> Result<Vec<PathBuf>, InputError> {
     let metadata = fs::metadata(path).map_err(|source| InputError::Metadata {
         path: path.to_path_buf(),

src/nix-style-linter/lib_test.rs

@@ -19,6 +19,154 @@ fn malformed_file_reports_parse_error() {
         "{}:1:9: parse-error: unexpected TOKEN_SEMICOLON at 8..9",
         workspace.path("broken.nix").display()
     )));
+    assert!(!output.stdout.contains("let-order"));
+}
+
+#[test]
+fn forward_reference_reports_let_order() {
+    let workspace = TestWorkspace::new("forward_reference_reports_let_order");
+    let path = workspace.write(
+        "forward.nix",
+        "let\n  app = package;\n  package = \"hello\";\nin app\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Diagnostics);
+    assert_eq!(output.stderr, "");
+    assert_eq!(
+        output.stdout,
+        format!(
+            "{}:2:3: let-order: `app` references later let binding `package`\n",
+            workspace.path("forward.nix").display()
+        )
+    );
+}
+
+#[test]
+fn dependency_ordered_bindings_exit_clean() {
+    let workspace = TestWorkspace::new("dependency_ordered_bindings_exit_clean");
+    let path = workspace.write(
+        "ordered.nix",
+        "let\n  package = \"hello\";\n  app = package;\nin app\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn outer_scope_reference_exits_clean() {
+    let workspace = TestWorkspace::new("outer_scope_reference_exits_clean");
+    let path = workspace.write(
+        "outer.nix",
+        "let\n  package = \"hello\";\nin\nlet\n  app = package;\nin app\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn nested_lambda_parameter_shadows_later_binding() {
+    let workspace = TestWorkspace::new("nested_lambda_parameter_shadows_later_binding");
+    let path = workspace.write(
+        "lambda.nix",
+        "let\n  call = package: package;\n  package = \"hello\";\nin call\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn nested_let_binding_shadows_later_binding() {
+    let workspace = TestWorkspace::new("nested_let_binding_shadows_later_binding");
+    let path = workspace.write(
+        "nested-let.nix",
+        "let\n  app = let package = \"inner\"; in package;\n  package = \"outer\";\nin app\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn earlier_sibling_constant_shared_by_multiple_bindings_exits_clean() {
+    let workspace = TestWorkspace::new("earlier_sibling_constant_shared_by_multiple_bindings");
+    let path = workspace.write(
+        "shared.nix",
+        "let\n  package = \"hello\";\n  app = package;\n  shell = package;\nin [ app shell ]\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn previous_line_suppression_disables_let_order() {
+    let workspace = TestWorkspace::new("previous_line_suppression_disables_let_order");
+    let path = workspace.write(
+        "suppressed.nix",
+        "let\n  # nix-style-linter: disable-next-line let-order -- intentional recursion\n  app = package;\n  package = app;\nin app\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn malformed_suppression_does_not_disable_let_order() {
+    let workspace = TestWorkspace::new("malformed_suppression_does_not_disable_let_order");
+    let path = workspace.write(
+        "unsuppressed.nix",
+        "let\n  # nix-style-linter: disable-next-line let-order\n  app = package;\n  package = \"hello\";\nin app\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Diagnostics);
+    assert_eq!(
+        output.stdout,
+        format!(
+            "{}:3:3: let-order: `app` references later let binding `package`\n",
+            workspace.path("unsuppressed.nix").display()
+        )
+    );
+    assert_eq!(output.stderr, "");
+}
+
+#[test]
+fn unsupported_binding_syntax_is_skipped() {
+    let workspace = TestWorkspace::new("unsupported_binding_syntax_is_skipped");
+    let path = workspace.write(
+        "unsupported.nix",
+        "let\n  attrs.value = package;\n  inherit package;\n  package = \"hello\";\nin attrs\n",
+    );
+
+    let output = run_linter(&[path]);
+
+    assert_eq!(output.status, RunStatus::Clean);
+    assert_eq!(output.stdout, "");
+    assert_eq!(output.stderr, "");
 }
 
 #[test]

src/skills/nix-style/rules/let.md

@@ -15,17 +15,3 @@ Do not apply this rule when a binding names a local constant used to keep siblin
 
 Shared convenience bindings hide dependency direction.
 Explicit references show which value owns a concept and which value consumes it.
-
-## Order `let` bindings by dependency
-
-### Rule
-
-Define a `let` binding before other bindings that reference it.
-Use forward references only for intentional recursion or fixed-point wiring.
-
-### Rationale
-
-Nix permits recursive `let` bindings.
-Top-down dependency order makes ordinary wiring easier to read.
-This rule can become a linter rule because binding references are mechanically detectable.
-

third_party/crates/BUILD.bazel

@@ -55,6 +55,18 @@ alias(
     tags = ["manual"],
 )
 
+alias(
+    name = "rowan-0.16.1",
+    actual = "//third_party/crates/rowan-0.16.1:rowan",
+    tags = ["manual"],
+)
+
+alias(
+    name = "rowan",
+    actual = "//third_party/crates/rowan-0.16.1:rowan",
+    tags = ["manual"],
+)
+
 alias(
     name = "serde-1.0.229",
     actual = "//third_party/crates/serde-1.0.229:serde",

third_party/crates/crates.bzl

@@ -334,6 +334,7 @@ _NORMAL_DEPENDENCIES = {
     "src/nix-style-linter": {
         _COMMON_CONDITION: {
             "rnix": Label("//third_party/crates/rnix-0.14.0:rnix"),
+            "rowan": Label("//third_party/crates/rowan-0.16.1:rowan"),
         },
     },
 }