refactor(natu): remove single-use binding rule #123
@@ -23,7 +23,6 @@ let
|
|||||||
};
|
};
|
||||||
|
|
||||||
# TODO: Refactor guest-module composition.
|
# TODO: Refactor guest-module composition.
|
||||||
# NOLINT(single_use_binding)
|
|
||||||
guestModules = import ./guests.nix {
|
guestModules = import ./guests.nix {
|
||||||
inherit
|
inherit
|
||||||
inputs
|
inputs
|
||||||
|
|||||||
@@ -9,7 +9,6 @@ let
|
|||||||
path: lib.nameValuePair (util.removeNixSuffix path) (mkRegistry path)
|
path: lib.nameValuePair (util.removeNixSuffix path) (mkRegistry path)
|
||||||
);
|
);
|
||||||
|
|
||||||
# NOLINT(single_use_binding)
|
|
||||||
mkRegistry =
|
mkRegistry =
|
||||||
path:
|
path:
|
||||||
(lib.evalModules {
|
(lib.evalModules {
|
||||||
|
|||||||
@@ -1,7 +1,5 @@
|
|||||||
{ lib }:
|
{ lib }:
|
||||||
let
|
let
|
||||||
# TODO: Add full Nix evaluation to single-use binding analysis.
|
|
||||||
# NOLINT(single_use_binding)
|
|
||||||
checkType = lib.types.submodule {
|
checkType = lib.types.submodule {
|
||||||
options = {
|
options = {
|
||||||
assertion = lib.mkOption {
|
assertion = lib.mkOption {
|
||||||
|
|||||||
@@ -14,7 +14,6 @@ rust_library(
|
|||||||
"rules/manual_inherit.rs",
|
"rules/manual_inherit.rs",
|
||||||
"rules/manual_inherit_from.rs",
|
"rules/manual_inherit_from.rs",
|
||||||
"rules/redundant_parameter_forwarding.rs",
|
"rules/redundant_parameter_forwarding.rs",
|
||||||
"rules/single_use_binding.rs",
|
|
||||||
"rules/unquoted_uri.rs",
|
"rules/unquoted_uri.rs",
|
||||||
"scope_graph.rs",
|
"scope_graph.rs",
|
||||||
],
|
],
|
||||||
|
|||||||
@@ -69,28 +69,7 @@ fn natu_reports_out_of_order_let_bindings() -> googletest::Result<()> {
|
|||||||
fn natu_preserves_suppressed_bindings() -> googletest::Result<()> {
|
fn natu_preserves_suppressed_bindings() -> googletest::Result<()> {
|
||||||
let report = natu::future_sight(vec![SourceFile {
|
let report = natu::future_sight(vec![SourceFile {
|
||||||
path: PathBuf::from("testdata/nolint.nix"),
|
path: PathBuf::from("testdata/nolint.nix"),
|
||||||
source: "{ config, ... }: let\n cfg = config.example; # NOLINT(let_order, single_use_binding)\n value = cfg; # NOLINT(single_use_binding)\nin value\n".into(),
|
source: "{ config, ... }: let\n cfg = config.example; # NOLINT(let_order)\n value = cfg;\nin value\n".into(),
|
||||||
}])
|
|
||||||
.or_fail()?;
|
|
||||||
|
|
||||||
expect_that!(report.is_clean(), is_true());
|
|
||||||
Ok(())
|
|
||||||
}
|
|
||||||
|
|
||||||
#[gtest]
|
|
||||||
fn natu_preserves_preceding_suppressed_binding() -> googletest::Result<()> {
|
|
||||||
let report = natu::future_sight(vec![SourceFile {
|
|
||||||
path: PathBuf::from("testdata/preceding_nolint.nix"),
|
|
||||||
source: r#"
|
|
||||||
let
|
|
||||||
# NOLINT(single_use_binding)
|
|
||||||
value = {
|
|
||||||
nested = true;
|
|
||||||
};
|
|
||||||
in
|
|
||||||
value
|
|
||||||
"#
|
|
||||||
.into(),
|
|
||||||
}])
|
}])
|
||||||
.or_fail()?;
|
.or_fail()?;
|
||||||
|
|
||||||
@@ -180,67 +159,6 @@ transform value
|
|||||||
Ok(())
|
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_accepts_single_use_import_binding() -> googletest::Result<()> {
|
|
||||||
let report = natu::future_sight(vec![SourceFile {
|
|
||||||
path: PathBuf::from("testdata/import_binding.nix"),
|
|
||||||
source: r#"
|
|
||||||
let
|
|
||||||
imported = import ./module.nix { };
|
|
||||||
in
|
|
||||||
imported
|
|
||||||
"#
|
|
||||||
.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]
|
#[gtest]
|
||||||
fn natu_reports_list_to_attrs_after_map() -> googletest::Result<()> {
|
fn natu_reports_list_to_attrs_after_map() -> googletest::Result<()> {
|
||||||
let report = natu::future_sight(vec![SourceFile {
|
let report = natu::future_sight(vec![SourceFile {
|
||||||
|
|||||||
@@ -11,7 +11,6 @@ pub mod list_to_attrs_map;
|
|||||||
pub mod manual_inherit;
|
pub mod manual_inherit;
|
||||||
pub mod manual_inherit_from;
|
pub mod manual_inherit_from;
|
||||||
pub mod redundant_parameter_forwarding;
|
pub mod redundant_parameter_forwarding;
|
||||||
pub mod single_use_binding;
|
|
||||||
pub mod unquoted_uri;
|
pub mod unquoted_uri;
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
@@ -101,22 +100,10 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph, source: &str) -> Ve
|
|||||||
binding: None,
|
binding: None,
|
||||||
expected_order: 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
|
node_diagnostics
|
||||||
.chain(token_diagnostics)
|
.chain(token_diagnostics)
|
||||||
.chain(let_order_diagnostics)
|
.chain(let_order_diagnostics)
|
||||||
.chain(redundant_parameter_forwarding_diagnostics)
|
.chain(redundant_parameter_forwarding_diagnostics)
|
||||||
.chain(single_use_binding_diagnostics)
|
|
||||||
.filter(|finding| !is_suppressed(source, finding.range, finding.rule_id))
|
.filter(|finding| !is_suppressed(source, finding.range, finding.rule_id))
|
||||||
.map(|finding| Diagnostic {
|
.map(|finding| Diagnostic {
|
||||||
file_id: file.file_id,
|
file_id: file.file_id,
|
||||||
|
|||||||
@@ -1,46 +0,0 @@
|
|||||||
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<_>>();
|
|
||||||
|
|
||||||
!binding.is_import
|
|
||||||
&& 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(),
|
edges: Vec::new(),
|
||||||
lets: Vec::new(),
|
lets: Vec::new(),
|
||||||
};
|
};
|
||||||
let root = graph.add_scope(None, ScopeKind::Root);
|
let root = graph.add_scope(None);
|
||||||
graph.build_expr(&file.root, root);
|
graph.build_expr(&file.root, root);
|
||||||
graph
|
graph
|
||||||
}
|
}
|
||||||
@@ -36,7 +36,7 @@ impl ScopeGraph {
|
|||||||
.flat_map(|binding| self.build_expr(&binding.value, scope))
|
.flat_map(|binding| self.build_expr(&binding.value, scope))
|
||||||
.collect(),
|
.collect(),
|
||||||
ast::Expr::Lambda { parameters, body } => {
|
ast::Expr::Lambda { parameters, body } => {
|
||||||
let scope = self.add_scope(Some(scope), ScopeKind::Lambda);
|
let scope = self.add_scope(Some(scope));
|
||||||
for parameter in parameters {
|
for parameter in parameters {
|
||||||
self.add_declaration(¶meter.name, scope);
|
self.add_declaration(¶meter.name, scope);
|
||||||
}
|
}
|
||||||
@@ -62,15 +62,13 @@ impl ScopeGraph {
|
|||||||
body: &ast::Expr,
|
body: &ast::Expr,
|
||||||
parent: NodeId,
|
parent: NodeId,
|
||||||
) -> Vec<NodeId> {
|
) -> Vec<NodeId> {
|
||||||
let scope = self.add_scope(Some(parent), ScopeKind::Let);
|
let scope = self.add_scope(Some(parent));
|
||||||
let mut graph_bindings = bindings
|
let mut graph_bindings = bindings
|
||||||
.iter()
|
.iter()
|
||||||
.map(|binding| Binding {
|
.map(|binding| Binding {
|
||||||
declaration: self.add_declaration(&binding.key.name, scope),
|
declaration: self.add_declaration(&binding.key.name, scope),
|
||||||
name: binding.key.name.clone(),
|
name: binding.key.name.clone(),
|
||||||
references: Vec::new(),
|
references: Vec::new(),
|
||||||
is_import: is_import(&binding.value),
|
|
||||||
is_lambda: matches!(binding.value, ast::Expr::Lambda { .. }),
|
|
||||||
range: binding.range,
|
range: binding.range,
|
||||||
})
|
})
|
||||||
.collect::<Vec<_>>();
|
.collect::<Vec<_>>();
|
||||||
@@ -94,33 +92,6 @@ impl ScopeGraph {
|
|||||||
references
|
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> {
|
pub(crate) fn resolve(&self, reference: NodeId) -> Vec<NodeId> {
|
||||||
let Node::Reference(name) = &self.nodes[reference.0] else {
|
let Node::Reference(name) = &self.nodes[reference.0] else {
|
||||||
return Vec::new();
|
return Vec::new();
|
||||||
@@ -144,8 +115,8 @@ impl ScopeGraph {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn add_scope(&mut self, parent: Option<NodeId>, kind: ScopeKind) -> NodeId {
|
fn add_scope(&mut self, parent: Option<NodeId>) -> NodeId {
|
||||||
let scope = self.add_node(Node::Scope(kind));
|
let scope = self.add_node(Node::Scope);
|
||||||
if let Some(parent) = parent {
|
if let Some(parent) = parent {
|
||||||
self.add_edge(scope, parent);
|
self.add_edge(scope, parent);
|
||||||
}
|
}
|
||||||
@@ -192,48 +163,19 @@ impl ScopeGraph {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn is_import(expr: &ast::Expr) -> bool {
|
|
||||||
match unwrap_expr(expr) {
|
|
||||||
ast::Expr::Apply(application) => is_import_callee(&application.lambda),
|
|
||||||
_ => false,
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
fn is_import_callee(expr: &ast::Expr) -> bool {
|
|
||||||
match unwrap_expr(expr) {
|
|
||||||
ast::Expr::Apply(application) => is_import_callee(&application.lambda),
|
|
||||||
ast::Expr::Ident(ident) => ident.name == "import",
|
|
||||||
_ => false,
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
fn unwrap_expr(expr: &ast::Expr) -> &ast::Expr {
|
|
||||||
match expr {
|
|
||||||
ast::Expr::Other(expressions) if expressions.len() == 1 => unwrap_expr(&expressions[0]),
|
|
||||||
_ => expr,
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
enum Node {
|
enum Node {
|
||||||
Scope(ScopeKind),
|
Scope,
|
||||||
Declaration(Declaration),
|
Declaration(Declaration),
|
||||||
Reference(String),
|
Reference(String),
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Node {
|
impl Node {
|
||||||
fn is_scope(&self) -> bool {
|
fn is_scope(&self) -> bool {
|
||||||
matches!(self, Self::Scope(_))
|
matches!(self, Self::Scope)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug)]
|
|
||||||
enum ScopeKind {
|
|
||||||
Root,
|
|
||||||
Let,
|
|
||||||
Lambda,
|
|
||||||
}
|
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
struct Declaration {
|
struct Declaration {
|
||||||
name: String,
|
name: String,
|
||||||
@@ -260,7 +202,5 @@ pub(crate) struct Binding {
|
|||||||
pub(crate) declaration: NodeId,
|
pub(crate) declaration: NodeId,
|
||||||
pub(crate) name: String,
|
pub(crate) name: String,
|
||||||
pub(crate) references: Vec<NodeId>,
|
pub(crate) references: Vec<NodeId>,
|
||||||
pub(crate) is_import: bool,
|
|
||||||
pub(crate) is_lambda: bool,
|
|
||||||
pub(crate) range: ast::SourceRange,
|
pub(crate) range: ast::SourceRange,
|
||||||
}
|
}
|
||||||
|
|||||||
8
src/natu/testdata/clean.nix
vendored
8
src/natu/testdata/clean.nix
vendored
@@ -2,18 +2,18 @@
|
|||||||
|
|
||||||
let
|
let
|
||||||
first = 1;
|
first = 1;
|
||||||
second = first; # NOLINT(single_use_binding)
|
second = first;
|
||||||
in {
|
in {
|
||||||
firstField = first;
|
firstField = first;
|
||||||
secondField = second;
|
secondField = second;
|
||||||
nested = let
|
nested = let
|
||||||
innerSecond = innerFirst; # NOLINT(single_use_binding)
|
innerSecond = innerFirst;
|
||||||
innerFirst = first; # NOLINT(single_use_binding)
|
innerFirst = first;
|
||||||
in {
|
in {
|
||||||
result = innerSecond;
|
result = innerSecond;
|
||||||
};
|
};
|
||||||
parameter = first: let
|
parameter = first: let
|
||||||
usesParameter = first; # NOLINT(single_use_binding)
|
usesParameter = first;
|
||||||
in {
|
in {
|
||||||
result = usesParameter;
|
result = usesParameter;
|
||||||
};
|
};
|
||||||
|
|||||||
8
src/natu/testdata/lint_findings.nix
vendored
8
src/natu/testdata/lint_findings.nix
vendored
@@ -1,9 +1,9 @@
|
|||||||
let
|
let
|
||||||
second = first; # NOLINT(single_use_binding)
|
second = first;
|
||||||
first = 1;
|
first = 1;
|
||||||
nested = let # NOLINT(single_use_binding)
|
nested = let
|
||||||
nestedFirst = first; # NOLINT(single_use_binding)
|
nestedFirst = first;
|
||||||
nestedSecond = nestedFirst; # NOLINT(single_use_binding)
|
nestedSecond = nestedFirst;
|
||||||
in {
|
in {
|
||||||
result = nestedSecond;
|
result = nestedSecond;
|
||||||
};
|
};
|
||||||
|
|||||||
Reference in New Issue
Block a user