From 852b14cff06b2396a2c8a7ad1af830d279062d7d Mon Sep 17 00:00:00 2001 From: alexion Date: Wed, 30 Sep 2026 10:41:42 -0400 Subject: [PATCH] feat(natu): detect single-use let bindings --- AGENTS.md | 1 + src/natu/BUILD | 1 + src/natu/natu_test.rs | 45 ++++++++++++++++++++++++- src/natu/rules.rs | 12 +++++++ src/natu/rules/single_use_binding.rs | 45 +++++++++++++++++++++++++ src/natu/scope_graph.rs | 50 ++++++++++++++++++++++++---- src/natu/testdata/clean.nix | 8 ++--- src/natu/testdata/lint_findings.nix | 8 ++--- 8 files changed, 154 insertions(+), 16 deletions(-) create mode 100644 src/natu/rules/single_use_binding.rs diff --git a/AGENTS.md b/AGENTS.md index 05a4030..6859b23 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,3 +122,4 @@ Gitea rebases the synthetic upstream root commit without the `third_party/pi` prefix. Preserve `git-subtree-dir` and `git-subtree-split` trailers on Pi vendor commits. - The default Pi model for NixOS user configuration is set in `configs/nixos/modules/shell/pi/default.nix`, not in vendored Pi source. +- Nix evaluates a `let` binding at most once per enclosing expression evaluation. A closure shares bindings from its enclosing `let` scope across calls. diff --git a/src/natu/BUILD b/src/natu/BUILD index 4e5fd17..2603b5c 100644 --- a/src/natu/BUILD +++ b/src/natu/BUILD @@ -14,6 +14,7 @@ rust_library( "rules/manual_inherit.rs", "rules/manual_inherit_from.rs", "rules/redundant_parameter_forwarding.rs", + "rules/single_use_binding.rs", "rules/unquoted_uri.rs", "scope_graph.rs", ], diff --git a/src/natu/natu_test.rs b/src/natu/natu_test.rs index 973a12c..c746a75 100644 --- a/src/natu/natu_test.rs +++ b/src/natu/natu_test.rs @@ -69,7 +69,7 @@ fn natu_reports_out_of_order_let_bindings() -> googletest::Result<()> { fn natu_preserves_suppressed_bindings() -> googletest::Result<()> { let report = natu::future_sight(vec![SourceFile { path: PathBuf::from("testdata/nolint.nix"), - source: "{ config, ... }: let\n cfg = config.example; # NOLINT(let_order)\n value = cfg;\nin value\n".into(), + source: "{ config, ... }: let\n cfg = config.example; # NOLINT(let_order, single_use_binding)\n value = cfg; # NOLINT(single_use_binding)\nin value\n".into(), }]) .or_fail()?; @@ -159,6 +159,49 @@ transform value Ok(()) } +#[gtest] +fn natu_accepts_value_captured_by_reused_lambda() -> googletest::Result<()> { + let report = natu::future_sight(vec![SourceFile { + path: PathBuf::from("testdata/captured_value.nix"), + source: r#" +let + mapper = _: captured; + captured = 1; +in +[ (mapper 1) (mapper 2) ] +"# + .into(), + }]) + .or_fail()?; + + expect_that!(report.is_clean(), is_true()); + Ok(()) +} + +#[gtest] +fn natu_reports_single_use_binding() -> googletest::Result<()> { + let report = natu::future_sight(vec![SourceFile { + path: PathBuf::from("testdata/single_use_binding.nix"), + source: r#" +let + single = 1; + recursive = recursive; +in +{ inherit single recursive; } +"# + .into(), + }]) + .or_fail()?; + + let count = report + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.finding.rule_id == "single_use_binding") + .count(); + expect_that!(count, eq(1)); + Ok(()) +} + #[gtest] fn natu_reports_list_to_attrs_after_map() -> googletest::Result<()> { let report = natu::future_sight(vec![SourceFile { diff --git a/src/natu/rules.rs b/src/natu/rules.rs index fc5d4d8..a7e5a35 100644 --- a/src/natu/rules.rs +++ b/src/natu/rules.rs @@ -11,6 +11,7 @@ pub mod list_to_attrs_map; pub mod manual_inherit; pub mod manual_inherit_from; pub mod redundant_parameter_forwarding; +pub mod single_use_binding; pub mod unquoted_uri; #[derive(Debug)] @@ -100,11 +101,22 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph, source: &str) -> Ve binding: None, expected_order: None, }); + let single_use_binding_diagnostics = + single_use_binding::check(graph) + .into_iter() + .map(|finding| Finding { + rule_id: "single_use_binding", + range: finding.range, + message: "Inline this single-use let binding", + binding: Some(finding.name), + expected_order: None, + }); node_diagnostics .chain(token_diagnostics) .chain(let_order_diagnostics) .chain(redundant_parameter_forwarding_diagnostics) + .chain(single_use_binding_diagnostics) .filter(|finding| !is_suppressed(source, finding.range, finding.rule_id)) .map(|finding| Diagnostic { file_id: file.file_id, diff --git a/src/natu/rules/single_use_binding.rs b/src/natu/rules/single_use_binding.rs new file mode 100644 index 0000000..adf03e9 --- /dev/null +++ b/src/natu/rules/single_use_binding.rs @@ -0,0 +1,45 @@ +use crate::{ + ast, + scope_graph::{Binding, LetScope, ScopeGraph}, +}; + +pub struct Finding { + pub range: ast::SourceRange, + pub name: String, +} + +pub fn check(graph: &ScopeGraph) -> Vec { + graph + .let_scopes() + .iter() + .flat_map(|let_scope| { + let_scope.bindings.iter().filter_map(|binding| { + is_single_use(graph, let_scope, binding).then(|| Finding { + range: binding.range, + name: binding.name.clone(), + }) + }) + }) + .collect() +} + +fn is_single_use(graph: &ScopeGraph, let_scope: &LetScope, binding: &Binding) -> bool { + let self_references = binding + .references + .iter() + .filter(|reference| graph.resolve(**reference).contains(&binding.declaration)) + .count(); + let external_references = let_scope + .bindings + .iter() + .filter(|candidate| candidate.declaration != binding.declaration) + .flat_map(|candidate| candidate.references.iter()) + .chain(let_scope.entry_references.iter()) + .filter(|reference| graph.resolve(**reference).contains(&binding.declaration)) + .collect::>(); + + self_references == 0 + && external_references.len() == 1 + && (binding.is_lambda + || !graph.crosses_lambda_scope(*external_references[0], binding.declaration)) +} diff --git a/src/natu/scope_graph.rs b/src/natu/scope_graph.rs index 2dd2139..7ad0526 100644 --- a/src/natu/scope_graph.rs +++ b/src/natu/scope_graph.rs @@ -14,7 +14,7 @@ impl ScopeGraph { edges: Vec::new(), lets: Vec::new(), }; - let root = graph.add_scope(None); + let root = graph.add_scope(None, ScopeKind::Root); graph.build_expr(&file.root, root); graph } @@ -36,7 +36,7 @@ impl ScopeGraph { .flat_map(|binding| self.build_expr(&binding.value, scope)) .collect(), ast::Expr::Lambda { parameters, body } => { - let scope = self.add_scope(Some(scope)); + let scope = self.add_scope(Some(scope), ScopeKind::Lambda); for parameter in parameters { self.add_declaration(¶meter.name, scope); } @@ -62,13 +62,14 @@ impl ScopeGraph { body: &ast::Expr, parent: NodeId, ) -> Vec { - let scope = self.add_scope(Some(parent)); + let scope = self.add_scope(Some(parent), ScopeKind::Let); 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(), + is_lambda: matches!(binding.value, ast::Expr::Lambda { .. }), range: binding.range, }) .collect::>(); @@ -92,6 +93,33 @@ impl ScopeGraph { references } + pub(crate) fn crosses_lambda_scope(&self, reference: NodeId, declaration: NodeId) -> bool { + let Some(mut scope) = self.edge_target(reference, Node::is_scope) else { + return false; + }; + let Some(declaration_scope) = self + .edges + .iter() + .find(|edge| edge.to == declaration && self.nodes[edge.from.0].is_scope()) + .map(|edge| edge.from) + else { + return false; + }; + + loop { + if scope == declaration_scope { + return false; + } + if matches!(self.nodes[scope.0], Node::Scope(ScopeKind::Lambda)) { + return true; + } + let Some(parent) = self.edge_target(scope, Node::is_scope) else { + return false; + }; + scope = parent; + } + } + pub(crate) fn resolve(&self, reference: NodeId) -> Vec { let Node::Reference(name) = &self.nodes[reference.0] else { return Vec::new(); @@ -115,8 +143,8 @@ impl ScopeGraph { } } - fn add_scope(&mut self, parent: Option) -> NodeId { - let scope = self.add_node(Node::Scope); + fn add_scope(&mut self, parent: Option, kind: ScopeKind) -> NodeId { + let scope = self.add_node(Node::Scope(kind)); if let Some(parent) = parent { self.add_edge(scope, parent); } @@ -165,17 +193,24 @@ impl ScopeGraph { #[derive(Debug)] enum Node { - Scope, + Scope(ScopeKind), Declaration(Declaration), Reference(String), } impl Node { fn is_scope(&self) -> bool { - matches!(self, Self::Scope) + matches!(self, Self::Scope(_)) } } +#[derive(Debug)] +enum ScopeKind { + Root, + Let, + Lambda, +} + #[derive(Debug)] struct Declaration { name: String, @@ -202,5 +237,6 @@ pub(crate) struct Binding { pub(crate) declaration: NodeId, pub(crate) name: String, pub(crate) references: Vec, + pub(crate) is_lambda: bool, pub(crate) range: ast::SourceRange, } diff --git a/src/natu/testdata/clean.nix b/src/natu/testdata/clean.nix index 480b498..4987c41 100644 --- a/src/natu/testdata/clean.nix +++ b/src/natu/testdata/clean.nix @@ -2,18 +2,18 @@ let first = 1; - second = first; + second = first; # NOLINT(single_use_binding) in { firstField = first; secondField = second; nested = let - innerSecond = innerFirst; - innerFirst = first; + innerSecond = innerFirst; # NOLINT(single_use_binding) + innerFirst = first; # NOLINT(single_use_binding) in { result = innerSecond; }; parameter = first: let - usesParameter = first; + usesParameter = first; # NOLINT(single_use_binding) in { result = usesParameter; }; diff --git a/src/natu/testdata/lint_findings.nix b/src/natu/testdata/lint_findings.nix index a4eb0d8..f89fa4d 100644 --- a/src/natu/testdata/lint_findings.nix +++ b/src/natu/testdata/lint_findings.nix @@ -1,9 +1,9 @@ let - second = first; + second = first; # NOLINT(single_use_binding) first = 1; - nested = let - nestedFirst = first; - nestedSecond = nestedFirst; + nested = let # NOLINT(single_use_binding) + nestedFirst = first; # NOLINT(single_use_binding) + nestedSecond = nestedFirst; # NOLINT(single_use_binding) in { result = nestedSecond; };