diff --git a/Cargo.lock b/Cargo.lock index 86462e9..1375ba1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -159,6 +159,7 @@ name = "nix-style-linter" version = "0.1.0" dependencies = [ "rnix", + "rowan", ] [[package]] diff --git a/src/nix-style-linter/BUILD b/src/nix-style-linter/BUILD index 31ba571..02bf38a 100644 --- a/src/nix-style-linter/BUILD +++ b/src/nix-style-linter/BUILD @@ -4,7 +4,10 @@ rust_library( name = "nix_style_linter_lib", crate_name = "nix_style_linter", srcs = ["lib.rs"], - deps = ["//third_party/crates:rnix"], + deps = [ + "//third_party/crates:rnix", + "//third_party/crates:rowan", + ], ) rust_binary( diff --git a/src/nix-style-linter/Cargo.toml b/src/nix-style-linter/Cargo.toml index 5dc87c8..3856d77 100644 --- a/src/nix-style-linter/Cargo.toml +++ b/src/nix-style-linter/Cargo.toml @@ -16,3 +16,4 @@ path = "lib_test.rs" [dependencies] rnix = "0.14.0" +rowan = "0.16.1" diff --git a/src/nix-style-linter/lib.rs b/src/nix-style-linter/lib.rs index 76ebb5e..3ef7f07 100644 --- a/src/nix-style-linter/lib.rs +++ b/src/nix-style-linter/lib.rs @@ -1,9 +1,17 @@ +use std::collections::HashSet; use std::ffi::OsStr; use std::fmt; use std::fs; use std::io::{self, Write}; use std::path::{Path, PathBuf}; +use rnix::SyntaxKind; +use rnix::ast::{self, HasEntry}; +use rowan::ast::AstNode; + +const PARSE_ERROR_RULE: &str = "parse-error"; +const LET_ORDER_RULE: &str = "let-order"; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RunStatus { Clean, @@ -58,7 +66,8 @@ pub fn run( let mut found_tool_errors = false; for path in paths { - match collect_nix_files(path) { + let path = resolve_workspace_path(path); + match collect_nix_files(&path) { Ok(files) => { for file in files { match lint_file(&file) { @@ -95,15 +104,36 @@ pub fn run( } } -pub fn lint_file(path: &Path) -> io::Result> { - let source = fs::read_to_string(path)?; - Ok(parse_diagnostics(path, &source)) +fn resolve_workspace_path(path: &Path) -> PathBuf { + if path.is_absolute() { + return path.to_path_buf(); + } + + if let Some(workspace) = std::env::var_os("BUILD_WORKSPACE_DIRECTORY") { + return PathBuf::from(workspace).join(path); + } + + path.to_path_buf() } -fn parse_diagnostics(path: &Path, source: &str) -> Vec { - let parse = rnix::Root::parse(source); - parse - .errors() +pub fn lint_file(path: &Path) -> io::Result> { + let source = fs::read_to_string(path)?; + let parse = rnix::Root::parse(&source); + let parse_diagnostics = parse_diagnostics(path, &source, parse.errors()); + + if !parse_diagnostics.is_empty() { + return Ok(parse_diagnostics); + } + + Ok(let_order_diagnostics(path, &source, &parse.tree())) +} + +fn parse_diagnostics( + path: &Path, + source: &str, + errors: &[rnix::ParseError], +) -> Vec { + errors .iter() .map(|error| { let (line, column) = line_column(source, parse_error_offset(error, source.len())); @@ -111,13 +141,274 @@ fn parse_diagnostics(path: &Path, source: &str) -> Vec { path: path.to_path_buf(), line, column, - rule: "parse-error", + rule: PARSE_ERROR_RULE, message: error.to_string(), } }) .collect() } +fn let_order_diagnostics(path: &Path, source: &str, root: &ast::Root) -> Vec { + root.syntax() + .descendants() + .filter_map(ast::LetIn::cast) + .flat_map(|let_in| let_order_diagnostics_for_let(path, source, &let_in)) + .collect() +} + +fn let_order_diagnostics_for_let( + path: &Path, + source: &str, + let_in: &ast::LetIn, +) -> Vec { + let bindings: Vec<_> = let_in + .attrpath_values() + .filter_map(|binding| plain_binding(source, &binding)) + .collect(); + let names: Vec<_> = bindings + .iter() + .map(|binding| binding.name.clone()) + .collect(); + let mut diagnostics = Vec::new(); + + for (index, binding) in bindings.iter().enumerate() { + let later_names: HashSet<_> = names.iter().skip(index + 1).cloned().collect(); + if let Some(referenced_name) = + first_forward_reference(binding.value.syntax(), &later_names, &mut HashSet::new()) + { + if is_suppressed(source, binding.line, LET_ORDER_RULE) { + continue; + } + + diagnostics.push(Diagnostic { + path: path.to_path_buf(), + line: binding.line, + column: binding.column, + rule: LET_ORDER_RULE, + message: format!( + "`{binding}` references later let binding `{referenced}`", + binding = binding.name, + referenced = referenced_name + ), + }); + } + } + + diagnostics +} + +#[derive(Debug, Clone)] +struct Binding { + name: String, + line: usize, + column: usize, + value: ast::Expr, +} + +fn plain_binding(source: &str, binding: &ast::AttrpathValue) -> Option { + let attr = plain_attr(binding.attrpath()?)?; + let value = binding.value()?; + let offset = usize::from(attr.text_range().start()); + let (line, column) = line_column(source, offset); + + Some(Binding { + name: attr.text().to_string(), + line, + column, + value, + }) +} + +fn plain_attrpath_name(binding: &ast::AttrpathValue) -> Option { + let attr = plain_attr(binding.attrpath()?)?; + Some(attr.text().to_string()) +} + +fn plain_attr(attrpath: ast::Attrpath) -> Option { + let mut attrs = attrpath.attrs(); + let attr = attrs.next()?; + + if attrs.next().is_some() { + return None; + } + + match attr { + ast::Attr::Ident(ident) => ident.ident_token(), + ast::Attr::Dynamic(_) | ast::Attr::Str(_) => None, + } +} + +fn first_forward_reference( + node: &rnix::SyntaxNode, + later_names: &HashSet, + shadowed: &mut HashSet, +) -> Option { + match node.kind() { + SyntaxKind::NODE_LAMBDA => { + return with_shadowed(shadowed, lambda_bound_names(node), |shadowed| { + first_forward_reference_in_children(node, later_names, shadowed) + }); + } + SyntaxKind::NODE_LET_IN => { + return with_shadowed(shadowed, plain_let_binding_names(node), |shadowed| { + first_forward_reference_in_children(node, later_names, shadowed) + }); + } + SyntaxKind::NODE_ATTR_SET if rec_attrset_binds_names(node) => { + return with_shadowed(shadowed, plain_attrset_binding_names(node), |shadowed| { + first_forward_reference_in_children(node, later_names, shadowed) + }); + } + _ => {} + } + + if is_reference_ident(node) { + if let Some(name) = ident_name(node) { + if later_names.contains(&name) && !shadowed.contains(&name) { + return Some(name); + } + } + } + + first_forward_reference_in_children(node, later_names, shadowed) +} + +fn first_forward_reference_in_children( + node: &rnix::SyntaxNode, + later_names: &HashSet, + shadowed: &mut HashSet, +) -> Option { + node.children() + .find_map(|child| first_forward_reference(&child, later_names, shadowed)) +} + +fn with_shadowed( + shadowed: &mut HashSet, + names: Vec, + action: impl FnOnce(&mut HashSet) -> R, +) -> R { + let inserted: Vec<_> = names + .into_iter() + .filter(|name| shadowed.insert(name.clone())) + .collect(); + let result = action(shadowed); + + for name in inserted { + shadowed.remove(&name); + } + + result +} + +fn is_reference_ident(node: &rnix::SyntaxNode) -> bool { + if node.kind() != SyntaxKind::NODE_IDENT { + return false; + } + + !matches!( + node.parent().map(|parent| parent.kind()), + Some( + SyntaxKind::NODE_ATTRPATH + | SyntaxKind::NODE_IDENT_PARAM + | SyntaxKind::NODE_PAT_BIND + | SyntaxKind::NODE_PAT_ENTRY + ) + ) +} + +fn ident_name(node: &rnix::SyntaxNode) -> Option { + ast::Ident::cast(node.clone()) + .and_then(|ident| ident.ident_token()) + .map(|token| token.text().to_string()) +} + +fn lambda_bound_names(node: &rnix::SyntaxNode) -> Vec { + let Some(lambda) = ast::Lambda::cast(node.clone()) else { + return Vec::new(); + }; + let Some(param) = lambda.param() else { + return Vec::new(); + }; + + match param { + ast::Param::IdentParam(param) => param + .ident() + .and_then(|ident| ident.ident_token()) + .map(|token| vec![token.text().to_string()]) + .unwrap_or_default(), + ast::Param::Pattern(pattern) => { + let mut names: Vec<_> = pattern + .pat_entries() + .filter_map(|entry| entry.ident()) + .filter_map(|ident| ident.ident_token()) + .map(|token| token.text().to_string()) + .collect(); + + if let Some(name) = pattern + .pat_bind() + .and_then(|bind| bind.ident()) + .and_then(|ident| ident.ident_token()) + .map(|token| token.text().to_string()) + { + names.push(name); + } + + names + } + } +} + +fn plain_let_binding_names(node: &rnix::SyntaxNode) -> Vec { + ast::LetIn::cast(node.clone()) + .map(|let_in| { + let_in + .attrpath_values() + .filter_map(|binding| plain_attrpath_name(&binding)) + .collect() + }) + .unwrap_or_default() +} + +fn rec_attrset_binds_names(node: &rnix::SyntaxNode) -> bool { + ast::AttrSet::cast(node.clone()) + .and_then(|attrset| attrset.rec_token()) + .is_some() +} + +fn plain_attrset_binding_names(node: &rnix::SyntaxNode) -> Vec { + ast::AttrSet::cast(node.clone()) + .map(|attrset| { + attrset + .attrpath_values() + .filter_map(|binding| plain_attrpath_name(&binding)) + .collect() + }) + .unwrap_or_default() +} + +fn is_suppressed(source: &str, line: usize, rule: &str) -> bool { + let Some(previous_line_index) = line.checked_sub(2) else { + return false; + }; + let Some(previous_line) = source.lines().nth(previous_line_index) else { + return false; + }; + let Some(comment) = previous_line.trim_start().strip_prefix('#') else { + return false; + }; + let Some(rest) = comment.trim().strip_prefix("nix-style-linter:") else { + return false; + }; + let Some((directive, reason)) = rest.trim().split_once("--") else { + return false; + }; + let mut words = directive.split_whitespace(); + + words.next() == Some("disable-next-line") + && words.any(|word| word == rule) + && !reason.trim().is_empty() +} + fn collect_nix_files(path: &Path) -> Result, InputError> { let metadata = fs::metadata(path).map_err(|source| InputError::Metadata { path: path.to_path_buf(), diff --git a/src/nix-style-linter/lib_test.rs b/src/nix-style-linter/lib_test.rs index 3ca553b..a76df4f 100644 --- a/src/nix-style-linter/lib_test.rs +++ b/src/nix-style-linter/lib_test.rs @@ -19,6 +19,154 @@ fn malformed_file_reports_parse_error() { "{}:1:9: parse-error: unexpected TOKEN_SEMICOLON at 8..9", workspace.path("broken.nix").display() ))); + assert!(!output.stdout.contains("let-order")); +} + +#[test] +fn forward_reference_reports_let_order() { + let workspace = TestWorkspace::new("forward_reference_reports_let_order"); + let path = workspace.write( + "forward.nix", + "let\n app = package;\n package = \"hello\";\nin app\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Diagnostics); + assert_eq!(output.stderr, ""); + assert_eq!( + output.stdout, + format!( + "{}:2:3: let-order: `app` references later let binding `package`\n", + workspace.path("forward.nix").display() + ) + ); +} + +#[test] +fn dependency_ordered_bindings_exit_clean() { + let workspace = TestWorkspace::new("dependency_ordered_bindings_exit_clean"); + let path = workspace.write( + "ordered.nix", + "let\n package = \"hello\";\n app = package;\nin app\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); +} + +#[test] +fn outer_scope_reference_exits_clean() { + let workspace = TestWorkspace::new("outer_scope_reference_exits_clean"); + let path = workspace.write( + "outer.nix", + "let\n package = \"hello\";\nin\nlet\n app = package;\nin app\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); +} + +#[test] +fn nested_lambda_parameter_shadows_later_binding() { + let workspace = TestWorkspace::new("nested_lambda_parameter_shadows_later_binding"); + let path = workspace.write( + "lambda.nix", + "let\n call = package: package;\n package = \"hello\";\nin call\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); +} + +#[test] +fn nested_let_binding_shadows_later_binding() { + let workspace = TestWorkspace::new("nested_let_binding_shadows_later_binding"); + let path = workspace.write( + "nested-let.nix", + "let\n app = let package = \"inner\"; in package;\n package = \"outer\";\nin app\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); +} + +#[test] +fn earlier_sibling_constant_shared_by_multiple_bindings_exits_clean() { + let workspace = TestWorkspace::new("earlier_sibling_constant_shared_by_multiple_bindings"); + let path = workspace.write( + "shared.nix", + "let\n package = \"hello\";\n app = package;\n shell = package;\nin [ app shell ]\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); +} + +#[test] +fn previous_line_suppression_disables_let_order() { + let workspace = TestWorkspace::new("previous_line_suppression_disables_let_order"); + let path = workspace.write( + "suppressed.nix", + "let\n # nix-style-linter: disable-next-line let-order -- intentional recursion\n app = package;\n package = app;\nin app\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); +} + +#[test] +fn malformed_suppression_does_not_disable_let_order() { + let workspace = TestWorkspace::new("malformed_suppression_does_not_disable_let_order"); + let path = workspace.write( + "unsuppressed.nix", + "let\n # nix-style-linter: disable-next-line let-order\n app = package;\n package = \"hello\";\nin app\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Diagnostics); + assert_eq!( + output.stdout, + format!( + "{}:3:3: let-order: `app` references later let binding `package`\n", + workspace.path("unsuppressed.nix").display() + ) + ); + assert_eq!(output.stderr, ""); +} + +#[test] +fn unsupported_binding_syntax_is_skipped() { + let workspace = TestWorkspace::new("unsupported_binding_syntax_is_skipped"); + let path = workspace.write( + "unsupported.nix", + "let\n attrs.value = package;\n inherit package;\n package = \"hello\";\nin attrs\n", + ); + + let output = run_linter(&[path]); + + assert_eq!(output.status, RunStatus::Clean); + assert_eq!(output.stdout, ""); + assert_eq!(output.stderr, ""); } #[test] diff --git a/src/skills/nix-style/rules/let.md b/src/skills/nix-style/rules/let.md index 7012562..7a3d576 100644 --- a/src/skills/nix-style/rules/let.md +++ b/src/skills/nix-style/rules/let.md @@ -15,17 +15,3 @@ Do not apply this rule when a binding names a local constant used to keep siblin Shared convenience bindings hide dependency direction. Explicit references show which value owns a concept and which value consumes it. - -## Order `let` bindings by dependency - -### Rule - -Define a `let` binding before other bindings that reference it. -Use forward references only for intentional recursion or fixed-point wiring. - -### Rationale - -Nix permits recursive `let` bindings. -Top-down dependency order makes ordinary wiring easier to read. -This rule can become a linter rule because binding references are mechanically detectable. - diff --git a/third_party/crates/BUILD.bazel b/third_party/crates/BUILD.bazel index f06367d..660512f 100644 --- a/third_party/crates/BUILD.bazel +++ b/third_party/crates/BUILD.bazel @@ -55,6 +55,18 @@ alias( tags = ["manual"], ) +alias( + name = "rowan-0.16.1", + actual = "//third_party/crates/rowan-0.16.1:rowan", + tags = ["manual"], +) + +alias( + name = "rowan", + actual = "//third_party/crates/rowan-0.16.1:rowan", + tags = ["manual"], +) + alias( name = "serde-1.0.229", actual = "//third_party/crates/serde-1.0.229:serde", diff --git a/third_party/crates/crates.bzl b/third_party/crates/crates.bzl index 70ede6a..821af61 100644 --- a/third_party/crates/crates.bzl +++ b/third_party/crates/crates.bzl @@ -334,6 +334,7 @@ _NORMAL_DEPENDENCIES = { "src/nix-style-linter": { _COMMON_CONDITION: { "rnix": Label("//third_party/crates/rnix-0.14.0:rnix"), + "rowan": Label("//third_party/crates/rowan-0.16.1:rowan"), }, }, }