feat(natu): report expected let order #102
@@ -49,6 +49,7 @@ pub enum Expr {
|
|||||||
Ident(Ident),
|
Ident(Ident),
|
||||||
Select(Select),
|
Select(Select),
|
||||||
Let {
|
Let {
|
||||||
|
range: SourceRange,
|
||||||
bindings: Vec<Binding>,
|
bindings: Vec<Binding>,
|
||||||
body: Box<Expr>,
|
body: Box<Expr>,
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -1,3 +1,5 @@
|
|||||||
|
use std::collections::HashSet;
|
||||||
|
|
||||||
mod ast;
|
mod ast;
|
||||||
mod file_store;
|
mod file_store;
|
||||||
mod parser;
|
mod parser;
|
||||||
@@ -23,7 +25,45 @@ impl Report {
|
|||||||
|
|
||||||
impl std::fmt::Display for Report {
|
impl std::fmt::Display for Report {
|
||||||
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||||
|
let mut reported_let_order_files = HashSet::new();
|
||||||
|
|
||||||
for diagnostic in &self.diagnostics {
|
for diagnostic in &self.diagnostics {
|
||||||
|
if diagnostic.finding.rule_id == "let_order" {
|
||||||
|
if !reported_let_order_files.insert(diagnostic.file_id) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
let file = self.files.get(diagnostic.file_id);
|
||||||
|
writeln!(
|
||||||
|
formatter,
|
||||||
|
"{} [let_order] Let bindings must follow dependency order",
|
||||||
|
file.path.display()
|
||||||
|
)?;
|
||||||
|
|
||||||
|
let mut findings = self
|
||||||
|
.diagnostics
|
||||||
|
.iter()
|
||||||
|
.filter(|candidate| {
|
||||||
|
candidate.file_id == diagnostic.file_id
|
||||||
|
&& candidate.finding.rule_id == "let_order"
|
||||||
|
})
|
||||||
|
.collect::<Vec<_>>();
|
||||||
|
findings.sort_by_key(|finding| finding.finding.range.start);
|
||||||
|
|
||||||
|
for finding in findings {
|
||||||
|
let (start, _) = line_range(&file.source, finding.finding.range);
|
||||||
|
let expected_order =
|
||||||
|
finding.finding.expected_order.as_ref().expect("let order");
|
||||||
|
writeln!(
|
||||||
|
formatter,
|
||||||
|
" let at line {start}: {}",
|
||||||
|
expected_order.join(", ")
|
||||||
|
)?;
|
||||||
|
}
|
||||||
|
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
let file = self.files.get(diagnostic.file_id);
|
let file = self.files.get(diagnostic.file_id);
|
||||||
let (start, end) = line_range(&file.source, diagnostic.finding.range);
|
let (start, end) = line_range(&file.source, diagnostic.finding.range);
|
||||||
let location = if start == end {
|
let location = if start == end {
|
||||||
|
|||||||
@@ -50,7 +50,18 @@ fn natu_reports_out_of_order_let_bindings() -> googletest::Result<()> {
|
|||||||
.iter()
|
.iter()
|
||||||
.filter(|diagnostic| diagnostic.finding.rule_id == "let_order")
|
.filter(|diagnostic| diagnostic.finding.rule_id == "let_order")
|
||||||
.count();
|
.count();
|
||||||
expect_that!(let_order_count, eq(4));
|
expect_that!(let_order_count, eq(2));
|
||||||
|
expect_that!(
|
||||||
|
report.to_string(),
|
||||||
|
eq(concat!(
|
||||||
|
"testdata/lint_findings.nix:11 [manual_inherit] Use `inherit` for this assignment\n",
|
||||||
|
"testdata/lint_findings.nix:12 [manual_inherit_from] Use `inherit` for this selected attribute\n",
|
||||||
|
"testdata/lint_findings.nix:13 [unquoted_uri] URI literals should be quoted\n",
|
||||||
|
"testdata/lint_findings.nix [let_order] Let bindings must follow dependency order\n",
|
||||||
|
" let at line 1: first, second, nested\n",
|
||||||
|
" let at line 4: nestedSecond, nestedFirst\n",
|
||||||
|
))
|
||||||
|
);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -43,6 +43,7 @@ fn convert_expr(expr: rnix::ast::Expr) -> ast::Expr {
|
|||||||
match expr {
|
match expr {
|
||||||
rnix::ast::Expr::Ident(ident) => ast::Expr::Ident(ident.into()),
|
rnix::ast::Expr::Ident(ident) => ast::Expr::Ident(ident.into()),
|
||||||
rnix::ast::Expr::LetIn(let_in) => ast::Expr::Let {
|
rnix::ast::Expr::LetIn(let_in) => ast::Expr::Let {
|
||||||
|
range: let_in.syntax().text_range().into(),
|
||||||
bindings: let_in
|
bindings: let_in
|
||||||
.attrpath_values()
|
.attrpath_values()
|
||||||
.filter_map(convert_binding_ast)
|
.filter_map(convert_binding_ast)
|
||||||
|
|||||||
@@ -22,6 +22,7 @@ pub struct Finding {
|
|||||||
pub range: ast::SourceRange,
|
pub range: ast::SourceRange,
|
||||||
pub message: &'static str,
|
pub message: &'static str,
|
||||||
pub binding: Option<String>,
|
pub binding: Option<String>,
|
||||||
|
pub expected_order: Option<Vec<String>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl std::fmt::Display for Diagnostic {
|
impl std::fmt::Display for Diagnostic {
|
||||||
@@ -79,7 +80,8 @@ pub fn run_rules(file: &ast::ParsedFile, graph: &ScopeGraph) -> Vec<Diagnostic>
|
|||||||
rule_id: "let_order",
|
rule_id: "let_order",
|
||||||
range: finding.range,
|
range: finding.range,
|
||||||
message: "Let bindings must follow dependency order",
|
message: "Let bindings must follow dependency order",
|
||||||
binding: Some(finding.binding),
|
binding: None,
|
||||||
|
expected_order: Some(finding.expected_order),
|
||||||
});
|
});
|
||||||
|
|
||||||
node_diagnostics
|
node_diagnostics
|
||||||
@@ -104,6 +106,7 @@ where
|
|||||||
range: node.range,
|
range: node.range,
|
||||||
message,
|
message,
|
||||||
binding: None,
|
binding: None,
|
||||||
|
expected_order: None,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -119,5 +122,6 @@ where
|
|||||||
range: token.range,
|
range: token.range,
|
||||||
message,
|
message,
|
||||||
binding: None,
|
binding: None,
|
||||||
|
expected_order: None,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -5,7 +5,7 @@ use crate::{
|
|||||||
|
|
||||||
pub struct Finding {
|
pub struct Finding {
|
||||||
pub range: ast::SourceRange,
|
pub range: ast::SourceRange,
|
||||||
pub binding: String,
|
pub expected_order: Vec<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn check(graph: &ScopeGraph) -> Vec<Finding> {
|
pub fn check(graph: &ScopeGraph) -> Vec<Finding> {
|
||||||
@@ -14,26 +14,26 @@ pub fn check(graph: &ScopeGraph) -> Vec<Finding> {
|
|||||||
.iter()
|
.iter()
|
||||||
.flat_map(|let_scope| {
|
.flat_map(|let_scope| {
|
||||||
let expected = expected_order(graph, let_scope);
|
let expected = expected_order(graph, let_scope);
|
||||||
let positions = expected
|
let is_ordered = expected.iter().enumerate().all(|(position, declaration)| {
|
||||||
.iter()
|
let_scope
|
||||||
.enumerate()
|
.bindings
|
||||||
.map(|(position, declaration)| (*declaration, position))
|
.get(position)
|
||||||
.collect::<Vec<_>>();
|
.is_some_and(|binding| binding.declaration == *declaration)
|
||||||
|
});
|
||||||
|
|
||||||
let_scope
|
(!is_ordered).then(|| Finding {
|
||||||
.bindings
|
range: let_scope.range,
|
||||||
.iter()
|
expected_order: expected
|
||||||
.enumerate()
|
.iter()
|
||||||
.filter_map(move |(position, binding)| {
|
.filter_map(|declaration| {
|
||||||
positions
|
let_scope
|
||||||
.iter()
|
.bindings
|
||||||
.find(|(declaration, _)| *declaration == binding.declaration)
|
.iter()
|
||||||
.filter(|(_, expected_position)| position != *expected_position)
|
.find(|binding| binding.declaration == *declaration)
|
||||||
.map(|_| Finding {
|
.map(|binding| binding.name.clone())
|
||||||
range: binding.range,
|
})
|
||||||
binding: binding.name.clone(),
|
.collect(),
|
||||||
})
|
})
|
||||||
})
|
|
||||||
})
|
})
|
||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -26,7 +26,11 @@ impl ScopeGraph {
|
|||||||
fn build_expr(&mut self, expr: &ast::Expr, scope: NodeId) -> Vec<NodeId> {
|
fn build_expr(&mut self, expr: &ast::Expr, scope: NodeId) -> Vec<NodeId> {
|
||||||
match expr {
|
match expr {
|
||||||
ast::Expr::Ident(ident) => vec![self.add_reference(&ident.name, scope)],
|
ast::Expr::Ident(ident) => vec![self.add_reference(&ident.name, scope)],
|
||||||
ast::Expr::Let { bindings, body } => self.build_let(bindings, body, scope),
|
ast::Expr::Let {
|
||||||
|
range,
|
||||||
|
bindings,
|
||||||
|
body,
|
||||||
|
} => self.build_let(*range, bindings, body, scope),
|
||||||
ast::Expr::AttrSet(bindings) => bindings
|
ast::Expr::AttrSet(bindings) => bindings
|
||||||
.iter()
|
.iter()
|
||||||
.flat_map(|binding| self.build_expr(&binding.value, scope))
|
.flat_map(|binding| self.build_expr(&binding.value, scope))
|
||||||
@@ -48,6 +52,7 @@ impl ScopeGraph {
|
|||||||
|
|
||||||
fn build_let(
|
fn build_let(
|
||||||
&mut self,
|
&mut self,
|
||||||
|
range: ast::SourceRange,
|
||||||
bindings: &[ast::Binding],
|
bindings: &[ast::Binding],
|
||||||
body: &ast::Expr,
|
body: &ast::Expr,
|
||||||
parent: NodeId,
|
parent: NodeId,
|
||||||
@@ -59,7 +64,6 @@ impl ScopeGraph {
|
|||||||
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(),
|
||||||
range: binding.range,
|
|
||||||
})
|
})
|
||||||
.collect::<Vec<_>>();
|
.collect::<Vec<_>>();
|
||||||
|
|
||||||
@@ -74,6 +78,7 @@ impl ScopeGraph {
|
|||||||
.chain(entry_references.iter().copied())
|
.chain(entry_references.iter().copied())
|
||||||
.collect();
|
.collect();
|
||||||
self.lets.push(LetScope {
|
self.lets.push(LetScope {
|
||||||
|
range,
|
||||||
bindings: graph_bindings,
|
bindings: graph_bindings,
|
||||||
entry_references,
|
entry_references,
|
||||||
});
|
});
|
||||||
@@ -181,6 +186,7 @@ struct Edge {
|
|||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub(crate) struct LetScope {
|
pub(crate) struct LetScope {
|
||||||
|
pub(crate) range: ast::SourceRange,
|
||||||
pub(crate) bindings: Vec<Binding>,
|
pub(crate) bindings: Vec<Binding>,
|
||||||
pub(crate) entry_references: Vec<NodeId>,
|
pub(crate) entry_references: Vec<NodeId>,
|
||||||
}
|
}
|
||||||
@@ -190,5 +196,4 @@ 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) range: ast::SourceRange,
|
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user