feat(natu): lint redundant parameter forwarding

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:
+    registryName: lib.mapAttrs (_hostName: host: host.config.registry.${registryName} or { }) hosts;
-    lib.mapAttrs (_hostName: host: host.config.registry.${registryName} or { }) hosts;
 in
-withOperations (collectData {
+withOperations collectData
-  inherit hosts;
-})

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",
     ],

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 {

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,

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<Finding> {
+    let mut findings = Vec::new();
+    check_expr(expr, &[], &mut findings);
+    findings
+}
+
+fn check_expr(expr: &ast::Expr, outer_parameters: &[ast::Ident], findings: &mut Vec<Finding>) {
+    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<Finding>,
+) {
+    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,
+                &parameter.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,
+    }
+}