From 2265dfc5583f62d5cc8416ec11caaf91aaa48a1d Mon Sep 17 00:00:00 2001 From: alexion Date: Wed, 30 Sep 2026 00:19:27 -0400 Subject: [PATCH] feat(natu): lint redundant parameter forwarding --- src/dotfiles/registry/collection.nix | 14 +- src/natu/BUILD | 1 + src/natu/natu_test.rs | 58 +++++ src/natu/rules.rs | 12 + .../rules/redundant_parameter_forwarding.rs | 244 ++++++++++++++++++ 5 files changed, 320 insertions(+), 9 deletions(-) create mode 100644 src/natu/rules/redundant_parameter_forwarding.rs diff --git a/src/dotfiles/registry/collection.nix b/src/dotfiles/registry/collection.nix index 57897c2..b5de52d 100644 --- a/src/dotfiles/registry/collection.nix +++ b/src/dotfiles/registry/collection.nix @@ -21,21 +21,20 @@ let ); collectData = - { hosts }: let data = lib.mapAttrs ( registryName: registry: if registry.replicate or false then - replicatedData registryName hosts raw.${registryName} + replicatedData registryName raw.${registryName} else lib.mapAttrs (hostName: host: host.config.registry.${registryName}.${hostName} or { }) hosts ) definitions; - raw = lib.mapAttrs (registryName: _registry: rawDeclarations registryName hosts) definitions; + raw = lib.mapAttrs (registryName: _registry: rawDeclarations registryName) definitions; in validate data; replicatedData = - registryName: hosts: raw: + registryName: raw: let entries = lib.mapAttrs (_entryName: entry: entry.value) (replicatedEntries registryName raw); in @@ -74,9 +73,6 @@ let ) { } entries; rawDeclarations = - registryName: hosts: - lib.mapAttrs (_hostName: host: host.config.registry.${registryName} or { }) hosts; + registryName: lib.mapAttrs (_hostName: host: host.config.registry.${registryName} or { }) hosts; in -withOperations (collectData { - inherit hosts; -}) +withOperations collectData diff --git a/src/natu/BUILD b/src/natu/BUILD index bf6efcf..4e5fd17 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_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 90078bf..973a12c 100644 --- a/src/natu/natu_test.rs +++ b/src/natu/natu_test.rs @@ -101,6 +101,64 @@ entry Ok(()) } +#[gtest] +fn natu_reports_redundant_parameter_forwarding() -> googletest::Result<()> { + let report = natu::future_sight(vec![ + SourceFile { + path: PathBuf::from("testdata/attribute_parameter.nix"), + source: r#" +{ hosts, ... }: +let + collect = { hosts }: hosts; +in +collect { inherit hosts; } +"# + .into(), + }, + SourceFile { + path: PathBuf::from("testdata/plain_parameter.nix"), + source: r#" +value: +let + transform = value: value; +in +transform value +"# + .into(), + }, + SourceFile { + path: PathBuf::from("testdata/curried_parameter.nix"), + source: r#" +{ hosts, ... }: +let + transform = prefix: hosts: suffix: hosts; +in +transform "prefix" hosts "suffix" +"# + .into(), + }, + SourceFile { + path: PathBuf::from("testdata/local_parameter.nix"), + source: r#" +let + transform = value: value; +in +transform value +"# + .into(), + }, + ]) + .or_fail()?; + + let count = report + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.finding.rule_id == "redundant_parameter_forwarding") + .count(); + expect_that!(count, eq(3)); + 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 4ab650f..fc5d4d8 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_parameter_forwarding; pub mod unquoted_uri; #[derive(Debug)] @@ -89,10 +90,21 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph, source: &str) -> Ve binding: None, expected_order: Some(finding.expected_order), }); + let redundant_parameter_forwarding_diagnostics = + redundant_parameter_forwarding::check(&file.root) + .into_iter() + .map(|finding| Finding { + rule_id: "redundant_parameter_forwarding", + range: finding.range, + message: "Use the outer parameter instead of forwarding it", + binding: None, + expected_order: None, + }); node_diagnostics .chain(token_diagnostics) .chain(let_order_diagnostics) + .chain(redundant_parameter_forwarding_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/redundant_parameter_forwarding.rs b/src/natu/rules/redundant_parameter_forwarding.rs new file mode 100644 index 0000000..256ded8 --- /dev/null +++ b/src/natu/rules/redundant_parameter_forwarding.rs @@ -0,0 +1,244 @@ +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, outer_parameters: &[ast::Ident], findings: &mut Vec) { + match expr { + ast::Expr::Let { bindings, body, .. } => { + check_let(bindings, body, outer_parameters, findings); + for binding in bindings { + check_expr(&binding.value, outer_parameters, findings); + } + check_expr(body, outer_parameters, findings); + } + ast::Expr::AttrSet(bindings) => { + for binding in bindings { + check_expr(&binding.value, outer_parameters, findings); + } + } + ast::Expr::Lambda { parameters, body } => check_expr(body, parameters, findings), + ast::Expr::Apply(application) => { + check_expr(&application.lambda, outer_parameters, findings); + check_expr(&application.argument, outer_parameters, findings); + } + ast::Expr::Other(expressions) => { + for expression in expressions { + check_expr(expression, outer_parameters, findings); + } + } + ast::Expr::Ident(_) | ast::Expr::Select(_) => { } + } +} + +fn check_let( + bindings: &[ast::Binding], + let_body: &ast::Expr, + outer_parameters: &[ast::Ident], + findings: &mut Vec, +) { + for binding in bindings { + let parameters = curried_parameters(&binding.value); + for (parameter_index, parameter) in parameters.iter().enumerate() { + if !outer_parameters + .iter() + .any(|outer_parameter| outer_parameter.name == parameter.name) + { + continue; + } + + let usage = helper_usage( + bindings, + let_body, + &binding.key.name, + ¶meter.name, + parameter_index, + ); + if usage.calls > 0 && !usage.invalid { + findings.push(Finding { + range: binding.range, + }); + break; + } + } + } +} + +#[derive(Default)] +struct Usage { + calls: usize, + invalid: bool, +} + +fn helper_usage( + bindings: &[ast::Binding], + body: &ast::Expr, + helper_name: &str, + parameter_name: &str, + parameter_index: usize, +) -> Usage { + let mut usage = Usage::default(); + for binding in bindings { + inspect_usage( + &binding.value, + helper_name, + parameter_name, + parameter_index, + &mut usage, + ); + } + inspect_usage(body, helper_name, parameter_name, parameter_index, &mut usage); + usage +} + +fn inspect_usage( + expr: &ast::Expr, + helper_name: &str, + parameter_name: &str, + parameter_index: usize, + usage: &mut Usage, +) { + match expr { + ast::Expr::Ident(ident) => { + if ident.name == helper_name { + usage.invalid = true; + } + } + ast::Expr::Select(_) => { } + ast::Expr::Apply(_) => { + let (lambda, arguments) = application_parts(expr); + if is_ident(lambda, helper_name) { + let Some(argument) = arguments.get(parameter_index) else { + usage.invalid = true; + return; + }; + if is_forwarded_parameter(argument, parameter_name) { + usage.calls += 1; + } else { + usage.invalid = true; + } + for argument in arguments { + inspect_usage( + argument, + helper_name, + parameter_name, + parameter_index, + usage, + ); + } + } else { + for argument in arguments { + inspect_usage( + argument, + helper_name, + parameter_name, + parameter_index, + usage, + ); + } + inspect_usage(lambda, helper_name, parameter_name, parameter_index, usage); + } + } + ast::Expr::Let { bindings, body, .. } => { + if bindings.iter().any(|binding| { + binding.key.name == helper_name || binding.key.name == parameter_name + }) { + return; + } + for binding in bindings { + inspect_usage( + &binding.value, + helper_name, + parameter_name, + parameter_index, + usage, + ); + } + inspect_usage(body, helper_name, parameter_name, parameter_index, usage); + } + ast::Expr::AttrSet(bindings) => { + for binding in bindings { + inspect_usage( + &binding.value, + helper_name, + parameter_name, + parameter_index, + usage, + ); + } + } + ast::Expr::Lambda { parameters, body } => { + if parameters.iter().any(|parameter| { + parameter.name == helper_name || parameter.name == parameter_name + }) { + return; + } + inspect_usage(body, helper_name, parameter_name, parameter_index, usage); + } + ast::Expr::Other(expressions) => { + for expression in expressions { + inspect_usage( + expression, + helper_name, + parameter_name, + parameter_index, + usage, + ); + } + } + } +} + +fn curried_parameters(expr: &ast::Expr) -> Vec<&ast::Ident> { + let mut parameters = Vec::new(); + let mut body = expr; + while let ast::Expr::Lambda { + parameters: lambda_parameters, + body: lambda_body, + } = body + { + parameters.extend(lambda_parameters); + body = lambda_body; + } + parameters +} + +fn application_parts(expr: &ast::Expr) -> (&ast::Expr, Vec<&ast::Expr>) { + let mut arguments = Vec::new(); + let mut lambda = unwrap_expr(expr); + while let ast::Expr::Apply(application) = lambda { + arguments.push(&*application.argument); + lambda = unwrap_expr(&application.lambda); + } + arguments.reverse(); + (lambda, arguments) +} + +fn is_ident(expr: &ast::Expr, name: &str) -> bool { + matches!(unwrap_expr(expr), ast::Expr::Ident(ident) if ident.name == name) +} + +fn is_forwarded_parameter(expr: &ast::Expr, parameter_name: &str) -> bool { + match unwrap_expr(expr) { + ast::Expr::Ident(ident) => ident.name == parameter_name, + ast::Expr::AttrSet(bindings) => matches!( + bindings.as_slice(), + [binding] if binding.key.name == parameter_name && is_ident(&binding.value, parameter_name) + ), + _ => false, + } +} + +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