From c91094234f0431af8096f8e8e2c283fae2363653 Mon Sep 17 00:00:00 2001 From: alexion Date: Sun, 4 Oct 2026 14:08:13 -0400 Subject: [PATCH] refactor(natu): remove single-use binding rule --- src/dotfiles/default.nix | 1 - src/dotfiles/registry.nix | 1 - src/dotfiles/registry/schema.nix | 2 - src/natu/BUILD | 1 - src/natu/natu_test.rs | 84 +--------------------------- src/natu/rules.rs | 13 ----- src/natu/rules/single_use_binding.rs | 46 --------------- src/natu/scope_graph.rs | 74 +++--------------------- src/natu/testdata/clean.nix | 8 +-- src/natu/testdata/lint_findings.nix | 8 +-- 10 files changed, 16 insertions(+), 222 deletions(-) delete mode 100644 src/natu/rules/single_use_binding.rs diff --git a/src/dotfiles/default.nix b/src/dotfiles/default.nix index a38a11b..055abd4 100644 --- a/src/dotfiles/default.nix +++ b/src/dotfiles/default.nix @@ -23,7 +23,6 @@ let }; # TODO: Refactor guest-module composition. - # NOLINT(single_use_binding) guestModules = import ./guests.nix { inherit inputs diff --git a/src/dotfiles/registry.nix b/src/dotfiles/registry.nix index 2582ce4..1bee98b 100644 --- a/src/dotfiles/registry.nix +++ b/src/dotfiles/registry.nix @@ -9,7 +9,6 @@ let path: lib.nameValuePair (util.removeNixSuffix path) (mkRegistry path) ); - # NOLINT(single_use_binding) mkRegistry = path: (lib.evalModules { diff --git a/src/dotfiles/registry/schema.nix b/src/dotfiles/registry/schema.nix index 2fcfe37..0bdc521 100644 --- a/src/dotfiles/registry/schema.nix +++ b/src/dotfiles/registry/schema.nix @@ -1,7 +1,5 @@ { lib }: let - # TODO: Add full Nix evaluation to single-use binding analysis. - # NOLINT(single_use_binding) checkType = lib.types.submodule { options = { assertion = lib.mkOption { diff --git a/src/natu/BUILD b/src/natu/BUILD index 2603b5c..4e5fd17 100644 --- a/src/natu/BUILD +++ b/src/natu/BUILD @@ -14,7 +14,6 @@ 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 475afc6..973a12c 100644 --- a/src/natu/natu_test.rs +++ b/src/natu/natu_test.rs @@ -69,28 +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, single_use_binding)\n value = cfg; # NOLINT(single_use_binding)\nin value\n".into(), - }]) - .or_fail()?; - - expect_that!(report.is_clean(), is_true()); - Ok(()) -} - -#[gtest] -fn natu_preserves_preceding_suppressed_binding() -> googletest::Result<()> { - let report = natu::future_sight(vec![SourceFile { - path: PathBuf::from("testdata/preceding_nolint.nix"), - source: r#" -let - # NOLINT(single_use_binding) - value = { - nested = true; - }; -in -value -"# - .into(), + source: "{ config, ... }: let\n cfg = config.example; # NOLINT(let_order)\n value = cfg;\nin value\n".into(), }]) .or_fail()?; @@ -180,67 +159,6 @@ 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_accepts_single_use_import_binding() -> googletest::Result<()> { - let report = natu::future_sight(vec![SourceFile { - path: PathBuf::from("testdata/import_binding.nix"), - source: r#" -let - imported = import ./module.nix { }; -in -imported -"# - .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 9fda5f7..6031246 100644 --- a/src/natu/rules.rs +++ b/src/natu/rules.rs @@ -11,7 +11,6 @@ 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)] @@ -101,22 +100,10 @@ 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 deleted file mode 100644 index 54aaedd..0000000 --- a/src/natu/rules/single_use_binding.rs +++ /dev/null @@ -1,46 +0,0 @@ -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::>(); - - !binding.is_import - && 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 59c3925..2dd2139 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, ScopeKind::Root); + let root = graph.add_scope(None); 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), ScopeKind::Lambda); + let scope = self.add_scope(Some(scope)); for parameter in parameters { self.add_declaration(¶meter.name, scope); } @@ -62,15 +62,13 @@ impl ScopeGraph { body: &ast::Expr, parent: NodeId, ) -> Vec { - let scope = self.add_scope(Some(parent), ScopeKind::Let); + 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(), - is_import: is_import(&binding.value), - is_lambda: matches!(binding.value, ast::Expr::Lambda { .. }), range: binding.range, }) .collect::>(); @@ -94,33 +92,6 @@ 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(); @@ -144,8 +115,8 @@ impl ScopeGraph { } } - fn add_scope(&mut self, parent: Option, kind: ScopeKind) -> NodeId { - let scope = self.add_node(Node::Scope(kind)); + 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); } @@ -192,48 +163,19 @@ impl ScopeGraph { } } -fn is_import(expr: &ast::Expr) -> bool { - match unwrap_expr(expr) { - ast::Expr::Apply(application) => is_import_callee(&application.lambda), - _ => false, - } -} - -fn is_import_callee(expr: &ast::Expr) -> bool { - match unwrap_expr(expr) { - ast::Expr::Apply(application) => is_import_callee(&application.lambda), - ast::Expr::Ident(ident) => ident.name == "import", - _ => false, - } -} - -fn unwrap_expr(expr: &ast::Expr) -> &ast::Expr { - match expr { - ast::Expr::Other(expressions) if expressions.len() == 1 => unwrap_expr(&expressions[0]), - _ => expr, - } -} - #[derive(Debug)] enum Node { - Scope(ScopeKind), + Scope, 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, @@ -260,7 +202,5 @@ pub(crate) struct Binding { pub(crate) declaration: NodeId, pub(crate) name: String, pub(crate) references: Vec, - pub(crate) is_import: bool, - 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 4987c41..480b498 100644 --- a/src/natu/testdata/clean.nix +++ b/src/natu/testdata/clean.nix @@ -2,18 +2,18 @@ let first = 1; - second = first; # NOLINT(single_use_binding) + second = first; in { firstField = first; secondField = second; nested = let - innerSecond = innerFirst; # NOLINT(single_use_binding) - innerFirst = first; # NOLINT(single_use_binding) + innerSecond = innerFirst; + innerFirst = first; in { result = innerSecond; }; parameter = first: let - usesParameter = first; # NOLINT(single_use_binding) + usesParameter = first; in { result = usesParameter; }; diff --git a/src/natu/testdata/lint_findings.nix b/src/natu/testdata/lint_findings.nix index f89fa4d..a4eb0d8 100644 --- a/src/natu/testdata/lint_findings.nix +++ b/src/natu/testdata/lint_findings.nix @@ -1,9 +1,9 @@ let - second = first; # NOLINT(single_use_binding) + second = first; first = 1; - nested = let # NOLINT(single_use_binding) - nestedFirst = first; # NOLINT(single_use_binding) - nestedSecond = nestedFirst; # NOLINT(single_use_binding) + nested = let + nestedFirst = first; + nestedSecond = nestedFirst; in { result = nestedSecond; };