feat(natu): detect single-use let bindings #116
@@ -14,6 +14,7 @@ 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",
|
||||
],
|
||||
|
||||
@@ -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\n cfg = config.example; # NOLINT(let_order, single_use_binding)\n value = cfg; # NOLINT(single_use_binding)\nin value\n".into(),
|
||||
}])
|
||||
.or_fail()?;
|
||||
|
||||
@@ -159,6 +159,49 @@ 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_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 {
|
||||
|
||||
@@ -11,6 +11,7 @@ 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)]
|
||||
@@ -100,11 +101,22 @@ 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,
|
||||
|
||||
45
src/natu/rules/single_use_binding.rs
Normal file
45
src/natu/rules/single_use_binding.rs
Normal file
@@ -0,0 +1,45 @@
|
||||
use crate::{
|
||||
ast,
|
||||
scope_graph::{Binding, LetScope, ScopeGraph},
|
||||
};
|
||||
|
||||
pub struct Finding {
|
||||
pub range: ast::SourceRange,
|
||||
pub name: String,
|
||||
}
|
||||
|
||||
pub fn check(graph: &ScopeGraph) -> Vec<Finding> {
|
||||
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::<Vec<_>>();
|
||||
|
||||
self_references == 0
|
||||
&& external_references.len() == 1
|
||||
&& (binding.is_lambda
|
||||
|| !graph.crosses_lambda_scope(*external_references[0], binding.declaration))
|
||||
}
|
||||
@@ -14,7 +14,7 @@ impl ScopeGraph {
|
||||
edges: Vec::new(),
|
||||
lets: Vec::new(),
|
||||
};
|
||||
let root = graph.add_scope(None);
|
||||
let root = graph.add_scope(None, ScopeKind::Root);
|
||||
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));
|
||||
let scope = self.add_scope(Some(scope), ScopeKind::Lambda);
|
||||
for parameter in parameters {
|
||||
self.add_declaration(¶meter.name, scope);
|
||||
}
|
||||
@@ -62,13 +62,14 @@ impl ScopeGraph {
|
||||
body: &ast::Expr,
|
||||
parent: NodeId,
|
||||
) -> Vec<NodeId> {
|
||||
let scope = self.add_scope(Some(parent));
|
||||
let scope = self.add_scope(Some(parent), ScopeKind::Let);
|
||||
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_lambda: matches!(binding.value, ast::Expr::Lambda { .. }),
|
||||
range: binding.range,
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
@@ -92,6 +93,33 @@ 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<NodeId> {
|
||||
let Node::Reference(name) = &self.nodes[reference.0] else {
|
||||
return Vec::new();
|
||||
@@ -115,8 +143,8 @@ impl ScopeGraph {
|
||||
}
|
||||
}
|
||||
|
||||
fn add_scope(&mut self, parent: Option<NodeId>) -> NodeId {
|
||||
let scope = self.add_node(Node::Scope);
|
||||
fn add_scope(&mut self, parent: Option<NodeId>, kind: ScopeKind) -> NodeId {
|
||||
let scope = self.add_node(Node::Scope(kind));
|
||||
if let Some(parent) = parent {
|
||||
self.add_edge(scope, parent);
|
||||
}
|
||||
@@ -165,17 +193,24 @@ impl ScopeGraph {
|
||||
|
||||
#[derive(Debug)]
|
||||
enum Node {
|
||||
Scope,
|
||||
Scope(ScopeKind),
|
||||
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,
|
||||
@@ -202,5 +237,6 @@ pub(crate) struct Binding {
|
||||
pub(crate) declaration: NodeId,
|
||||
pub(crate) name: String,
|
||||
pub(crate) references: Vec<NodeId>,
|
||||
pub(crate) is_lambda: bool,
|
||||
pub(crate) range: ast::SourceRange,
|
||||
}
|
||||
|
||||
8
src/natu/testdata/clean.nix
vendored
8
src/natu/testdata/clean.nix
vendored
@@ -2,18 +2,18 @@
|
||||
|
||||
let
|
||||
first = 1;
|
||||
second = first;
|
||||
second = first; # NOLINT(single_use_binding)
|
||||
in {
|
||||
firstField = first;
|
||||
secondField = second;
|
||||
nested = let
|
||||
innerSecond = innerFirst;
|
||||
innerFirst = first;
|
||||
innerSecond = innerFirst; # NOLINT(single_use_binding)
|
||||
innerFirst = first; # NOLINT(single_use_binding)
|
||||
in {
|
||||
result = innerSecond;
|
||||
};
|
||||
parameter = first: let
|
||||
usesParameter = first;
|
||||
usesParameter = first; # NOLINT(single_use_binding)
|
||||
in {
|
||||
result = usesParameter;
|
||||
};
|
||||
|
||||
8
src/natu/testdata/lint_findings.nix
vendored
8
src/natu/testdata/lint_findings.nix
vendored
@@ -1,9 +1,9 @@
|
||||
let
|
||||
second = first;
|
||||
second = first; # NOLINT(single_use_binding)
|
||||
first = 1;
|
||||
nested = let
|
||||
nestedFirst = first;
|
||||
nestedSecond = nestedFirst;
|
||||
nested = let # NOLINT(single_use_binding)
|
||||
nestedFirst = first; # NOLINT(single_use_binding)
|
||||
nestedSecond = nestedFirst; # NOLINT(single_use_binding)
|
||||
in {
|
||||
result = nestedSecond;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user