From 3e9295134e1a5adafcdc120222ce31cabe0c6e6d Mon Sep 17 00:00:00 2001 From: alexion Date: Sun, 4 Oct 2026 21:23:35 -0400 Subject: [PATCH] feat(natu): detect redundant let expressions --- AGENTS.md | 2 + src/dotfiles/registry/validation.nix | 21 +++--- src/natu/BUILD | 1 + src/natu/natu_test.rs | 62 +++++++++++++++++- src/natu/rules.rs | 12 ++++ src/natu/rules/redundant_let.rs | 98 ++++++++++++++++++++++++++++ 6 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 src/natu/rules/redundant_let.rs diff --git a/AGENTS.md b/AGENTS.md index 05a4030..00f373c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,6 +108,8 @@ - During the Nix refactor, address each issue that comes up by adding either a Natu rule or a new Nix style rule. - Git pathspec searches started in a subdirectory default to that subtree. Use `git -C ` or top-anchored pathspecs for repository-wide history searches. +- A `**/*.nix` Git pathspec does not include Nix files directly in its starting directory. + Include both `*.nix` and `**/*.nix`, or use a recursive `*.nix` pathspec, when linting every Nix file. - Ariadne extraction must preserve the game-root season prefix for archived source. Different game installs can contain compiled scripts with the same internal path. - Clew owns Ren'Py extraction, preparation, and compiled-script AST export. Ariadne owns analysis over Clew-produced data. diff --git a/src/dotfiles/registry/validation.nix b/src/dotfiles/registry/validation.nix index 1840ff5..41704a3 100644 --- a/src/dotfiles/registry/validation.nix +++ b/src/dotfiles/registry/validation.nix @@ -3,17 +3,6 @@ lib, }: let - assertValid = - data: - let - assertions = validationAssertions data; - failures = lib.filter (assertion: !assertion.assertion) assertions; - in - assert lib.assertMsg (lib.all (assertion: assertion.assertion) assertions) ( - lib.concatMapStringsSep "\n" (assertion: assertion.message) failures - ); - data; - validationAssertions = data: lib.concatLists ( @@ -41,4 +30,12 @@ let ) data ); in -assertValid +data: +let + assertions = validationAssertions data; + failures = lib.filter (assertion: !assertion.assertion) assertions; +in +assert lib.assertMsg (lib.all (assertion: assertion.assertion) assertions) ( + lib.concatMapStringsSep "\n" (assertion: assertion.message) failures +); +data 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..ba97834 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 # NOLINT(redundant_let)\n cfg = config.example; # NOLINT(let_order)\n value = cfg;\nin value\n".into(), }]) .or_fail()?; @@ -101,6 +101,66 @@ 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 + helper = 1; + value = helper; +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/recursive_let.nix"), + source: r#" +let + recursive = recursive; +in +recursive +"# + .into(), + }, + SourceFile { + path: PathBuf::from("testdata/dependent_let.nix"), + source: r#" +let + value = 1; + dependent = value; +in +value +"# + .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..1ba952a 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 returned let binding", + 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..8a19057 --- /dev/null +++ b/src/natu/rules/redundant_let.rs @@ -0,0 +1,98 @@ +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 Some((binding_index, binding)) = bindings + .iter() + .enumerate() + .find(|(_, binding)| is_ident(body, &binding.key.name)) + else { + return false; + }; + + !references_name(&binding.value, &binding.key.name) + && !bindings.iter().enumerate().any(|(candidate_index, candidate)| { + candidate_index != binding_index && references_name(&candidate.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, + } +} -- 2.47.3