From 9d3eedc8e72503a7dffda964cd31eeff8178d533 Mon Sep 17 00:00:00 2001 From: alexion Date: Sun, 4 Oct 2026 21:17:01 -0400 Subject: [PATCH] feat(natu): detect redundant let expressions --- src/natu/BUILD | 1 + src/natu/natu_test.rs | 46 +++++++++++++++++ src/natu/rules.rs | 12 +++++ src/natu/rules/redundant_let.rs | 91 +++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+) create mode 100644 src/natu/rules/redundant_let.rs diff --git a/src/natu/BUILD b/src/natu/BUILD index 4e5fd17..8c74955 100644 --- a/src/natu/BUILD +++ b/src/natu/BUILD @@ -13,6 +13,7 @@ rust_library( "rules/list_to_attrs_map.rs", "rules/manual_inherit.rs", "rules/manual_inherit_from.rs", + "rules/redundant_let.rs", "rules/redundant_parameter_forwarding.rs", "rules/unquoted_uri.rs", "scope_graph.rs", diff --git a/src/natu/natu_test.rs b/src/natu/natu_test.rs index 973a12c..15411ca 100644 --- a/src/natu/natu_test.rs +++ b/src/natu/natu_test.rs @@ -101,6 +101,52 @@ entry Ok(()) } +#[gtest] +fn natu_reports_redundant_let() -> googletest::Result<()> { + let report = natu::future_sight(vec![SourceFile { + path: PathBuf::from("testdata/redundant_let.nix"), + source: r#" +let + value = 1; +in +value +"# + .into(), + }]) + .or_fail()?; + + let count = report + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.finding.rule_id == "redundant_let") + .count(); + expect_that!(count, eq(1)); + Ok(()) +} + +#[gtest] +fn natu_accepts_nonredundant_let() -> googletest::Result<()> { + let report = natu::future_sight(vec![SourceFile { + path: PathBuf::from("testdata/nonredundant_let.nix"), + source: r#" +let + recursive = recursive; +in +recursive +"# + .into(), + }]) + .or_fail()?; + + let count = report + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.finding.rule_id == "redundant_let") + .count(); + expect_that!(count, eq(0)); + Ok(()) +} + #[gtest] fn natu_reports_redundant_parameter_forwarding() -> googletest::Result<()> { let report = natu::future_sight(vec![ diff --git a/src/natu/rules.rs b/src/natu/rules.rs index 6031246..c036b8b 100644 --- a/src/natu/rules.rs +++ b/src/natu/rules.rs @@ -10,6 +10,7 @@ pub mod let_order; pub mod list_to_attrs_map; pub mod manual_inherit; pub mod manual_inherit_from; +pub mod redundant_let; pub mod redundant_parameter_forwarding; pub mod unquoted_uri; @@ -90,6 +91,16 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph, source: &str) -> Ve binding: None, expected_order: Some(finding.expected_order), }); + let redundant_let_diagnostics = + redundant_let::check(&file.root) + .into_iter() + .map(|finding| Finding { + rule_id: "redundant_let", + range: finding.range, + message: "Inline this single-binding let expression", + binding: None, + expected_order: None, + }); let redundant_parameter_forwarding_diagnostics = redundant_parameter_forwarding::check(&file.root) .into_iter() @@ -103,6 +114,7 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph, source: &str) -> Ve node_diagnostics .chain(token_diagnostics) .chain(let_order_diagnostics) + .chain(redundant_let_diagnostics) .chain(redundant_parameter_forwarding_diagnostics) .filter(|finding| !is_suppressed(source, finding.range, finding.rule_id)) .map(|finding| Diagnostic { diff --git a/src/natu/rules/redundant_let.rs b/src/natu/rules/redundant_let.rs new file mode 100644 index 0000000..9b027e3 --- /dev/null +++ b/src/natu/rules/redundant_let.rs @@ -0,0 +1,91 @@ +use crate::ast; + +pub struct Finding { + pub range: ast::SourceRange, +} + +pub fn check(expr: &ast::Expr) -> Vec { + let mut findings = Vec::new(); + check_expr(expr, &mut findings); + findings +} + +fn check_expr(expr: &ast::Expr, findings: &mut Vec) { + match expr { + ast::Expr::Let { + range, + bindings, + body, + } => { + if is_redundant(bindings, body) { + findings.push(Finding { range: *range }); + } + for binding in bindings { + check_expr(&binding.value, findings); + } + check_expr(body, findings); + } + ast::Expr::AttrSet(bindings) => { + for binding in bindings { + check_expr(&binding.value, findings); + } + } + ast::Expr::Lambda { body, .. } => check_expr(body, findings), + ast::Expr::Apply(application) => { + check_expr(&application.lambda, findings); + check_expr(&application.argument, findings); + } + ast::Expr::Other(expressions) => { + for expression in expressions { + check_expr(expression, findings); + } + } + ast::Expr::Ident(_) | ast::Expr::Select(_) => { } + } +} + +fn is_redundant(bindings: &[ast::Binding], body: &ast::Expr) -> bool { + let [binding] = bindings else { + return false; + }; + + is_ident(body, &binding.key.name) && !references_name(&binding.value, &binding.key.name) +} + +fn is_ident(expr: &ast::Expr, name: &str) -> bool { + matches!(unwrap_expr(expr), ast::Expr::Ident(ident) if ident.name == name) +} + +fn references_name(expr: &ast::Expr, name: &str) -> bool { + match expr { + ast::Expr::Ident(ident) => ident.name == name, + ast::Expr::Select(select) => select.receiver == name, + ast::Expr::Apply(application) => { + references_name(&application.lambda, name) || references_name(&application.argument, name) + } + ast::Expr::Let { bindings, body, .. } => { + !bindings.iter().any(|binding| binding.key.name == name) + && (bindings + .iter() + .any(|binding| references_name(&binding.value, name)) + || references_name(body, name)) + } + ast::Expr::AttrSet(bindings) => bindings + .iter() + .any(|binding| references_name(&binding.value, name)), + ast::Expr::Lambda { parameters, body } => { + !parameters.iter().any(|parameter| parameter.name == name) + && references_name(body, name) + } + ast::Expr::Other(expressions) => expressions + .iter() + .any(|expression| references_name(expression, name)), + } +} + +fn unwrap_expr(expr: &ast::Expr) -> &ast::Expr { + match expr { + ast::Expr::Other(expressions) if expressions.len() == 1 => unwrap_expr(&expressions[0]), + _ => expr, + } +}