From 1fc9f87d7eaa01a633086464e9087b78502dd930 Mon Sep 17 00:00:00 2001 From: Jakob Guldberg Aaes Date: Wed, 22 Apr 2026 15:09:58 +0200 Subject: [PATCH] Group repeated keys into attrset fixes --- bin/tests/_utils.rs | 33 ++- bin/tests/repeated_keys.rs | 52 ++++ ...4aee37038d8367b6bc7f5b43ce051c24908e5.snap | 15 +- ...0281713b79ec2d69e2c65ddd0bc22b96fb21c.snap | 26 ++ ...38519eded4ba2078838f788b3e769db564123.snap | 21 +- ...0281713b79ec2d69e2c65ddd0bc22b96fb21c.snap | 18 ++ lib/src/lints/repeated_keys.rs | 255 ++++++++++++++---- lib/src/make.rs | 4 + 8 files changed, 369 insertions(+), 55 deletions(-) create mode 100644 bin/tests/snapshots/repeated_keys__fix_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap create mode 100644 bin/tests/snapshots/repeated_keys__lint_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap diff --git a/bin/tests/_utils.rs b/bin/tests/_utils.rs index 6dcc2a7f..5b931e21 100644 --- a/bin/tests/_utils.rs +++ b/bin/tests/_utils.rs @@ -1,22 +1,47 @@ -use std::{io::Write, process::Command}; +use std::{fs, io::Write, process::Command}; use tempfile::NamedTempFile; -pub fn test_cli(expression: &str, args: &[&str]) -> anyhow::Result { +fn write_fixture(expression: &str) -> anyhow::Result { let mut fixture = NamedTempFile::with_suffix(".nix")?; fixture.write_all(expression.as_bytes())?; fixture.write_all(b"\n")?; // otherwise diff says there's no newline at end of file + Ok(fixture) +} + +fn run_cli(path: &std::path::Path, args: &[&str]) -> anyhow::Result { let output = Command::new("cargo") .arg("run") .arg("--") .args(args) - .arg(fixture.path()) + .arg(path) .output()?; let stdout = strip_ansi_escapes::strip(output.stdout)?; let stdout = String::from_utf8(stdout)?; - let stdout = stdout.replace(fixture.path().to_str().unwrap(), ""); + let stdout = stdout.replace(path.to_str().unwrap(), ""); Ok(stdout) } + +pub fn test_cli(expression: &str, args: &[&str]) -> anyhow::Result { + let fixture = write_fixture(expression)?; + run_cli(fixture.path(), args) +} + +#[allow(dead_code)] +pub fn apply_and_check( + expression: &str, + fix_args: &[&str], + check_args: &[&str], +) -> anyhow::Result<(String, String, String)> { + let fixture = write_fixture(expression)?; + let path = fixture.path(); + + let fix_stdout = run_cli(path, fix_args)?; + let contents = fs::read_to_string(path)?; + let check_stdout = run_cli(path, check_args)?; + + Ok((fix_stdout, contents, check_stdout)) +} diff --git a/bin/tests/repeated_keys.rs b/bin/tests/repeated_keys.rs index 70a1f65f..b22ff473 100644 --- a/bin/tests/repeated_keys.rs +++ b/bin/tests/repeated_keys.rs @@ -38,5 +38,57 @@ generate_tests! { foo.baz.bar5 = 5; } "}, + + // non-contiguous entries should still be grouped into a fix + indoc! {" + { + hardware.bluetooth = { + enable = true; + }; + networking.hostName = \"nixbox\"; + hardware.nvidia-container-toolkit.enable = true; + hardware.nvidia = { + modesetting.enable = true; + }; + } + "}, ], } + +#[test] +fn repeated_keys_fix_removes_nested_repeated_key_warnings() { + let expression = indoc! {r#" + { + services.pcscd.enable = true; + services.xserver.xkb = { + layout = "us"; + variant = ""; + }; + services.swapspace.enable = true; + services.xserver.videoDrivers = [ "nvidia" ]; + services.xserver.enable = true; + + environment.plasma6.excludePackages = [ ]; + environment.etc."environment.d/desktop-environment.conf".text = '' + DESKTOP_SESSION=plasma + ''; + environment.variables.KWIN_DRM_PREFER_COLOR_DEPTH = "24"; + + programs.dconf.enable = true; + programs.nix-ld.enable = true; + programs.nix-ld.libraries = [ ]; + + security.rtkit.enable = true; + security.polkit.enable = true; + security.sudo.package = pkgs.sudo; + } + "#}; + + let (_, fixed, check_output) = + _utils::apply_and_check(expression, &["fix"], &["check"]).unwrap(); + + assert!( + !check_output.contains("Avoid repeated keys in attribute sets"), + "repeated_keys warning remained after fix\nfixed:\n{fixed}\ncheck output:\n{check_output}", + ); +} diff --git a/bin/tests/snapshots/repeated_keys__fix_107c49dd2c0b84878c81852acfd4aee37038d8367b6bc7f5b43ce051c24908e5.snap b/bin/tests/snapshots/repeated_keys__fix_107c49dd2c0b84878c81852acfd4aee37038d8367b6bc7f5b43ce051c24908e5.snap index 2b28053c..9f6c0d7a 100644 --- a/bin/tests/snapshots/repeated_keys__fix_107c49dd2c0b84878c81852acfd4aee37038d8367b6bc7f5b43ce051c24908e5.snap +++ b/bin/tests/snapshots/repeated_keys__fix_107c49dd2c0b84878c81852acfd4aee37038d8367b6bc7f5b43ce051c24908e5.snap @@ -1,5 +1,18 @@ --- source: bin/tests/repeated_keys.rs +assertion_line: 7 expression: "\"{\\n foo.bar = 1;\\n foo.bar.\\\"hello\\\" = 1;\\n foo.again = 1;\\n}\\n\"" --- - +--- ++++ [fixed] +@@ -1,6 +1,8 @@ + { +- foo.bar = 1; +- foo.bar."hello" = 1; +- foo.again = 1; ++ foo = { ++ bar = 1; ++ bar."hello" = 1; ++ again = 1; ++ }; + } diff --git a/bin/tests/snapshots/repeated_keys__fix_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap b/bin/tests/snapshots/repeated_keys__fix_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap new file mode 100644 index 00000000..d4a8d126 --- /dev/null +++ b/bin/tests/snapshots/repeated_keys__fix_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap @@ -0,0 +1,26 @@ +--- +source: bin/tests/repeated_keys.rs +assertion_line: 7 +expression: "\"{\\n hardware.bluetooth = {\\n enable = true;\\n };\\n networking.hostName = \\\"nixbox\\\";\\n hardware.nvidia-container-toolkit.enable = true;\\n hardware.nvidia = {\\n modesetting.enable = true;\\n };\\n}\\n\"" +--- +--- ++++ [fixed] +@@ -1,11 +1,13 @@ + { +- hardware.bluetooth = { +- enable = true; ++ hardware = { ++ bluetooth = { ++ enable = true; ++ }; ++ nvidia-container-toolkit.enable = true; ++ nvidia = { ++ modesetting.enable = true; ++ }; + }; + networking.hostName = "nixbox"; +- hardware.nvidia-container-toolkit.enable = true; +- hardware.nvidia = { +- modesetting.enable = true; +- }; + } diff --git a/bin/tests/snapshots/repeated_keys__fix_653aae0c9fb8b448e30e62c7daa38519eded4ba2078838f788b3e769db564123.snap b/bin/tests/snapshots/repeated_keys__fix_653aae0c9fb8b448e30e62c7daa38519eded4ba2078838f788b3e769db564123.snap index 22e3cd97..d28226ea 100644 --- a/bin/tests/snapshots/repeated_keys__fix_653aae0c9fb8b448e30e62c7daa38519eded4ba2078838f788b3e769db564123.snap +++ b/bin/tests/snapshots/repeated_keys__fix_653aae0c9fb8b448e30e62c7daa38519eded4ba2078838f788b3e769db564123.snap @@ -1,5 +1,24 @@ --- source: bin/tests/repeated_keys.rs +assertion_line: 7 expression: "\"{\\n foo.baz.bar1 = 1;\\n foo.baz.bar2 = 2;\\n foo.baz.bar3 = 3;\\n foo.baz.bar4 = 4;\\n foo.baz.bar5 = 5;\\n}\\n\"" --- - +--- ++++ [fixed] +@@ -1,8 +1,12 @@ + { +- foo.baz.bar1 = 1; +- foo.baz.bar2 = 2; +- foo.baz.bar3 = 3; +- foo.baz.bar4 = 4; +- foo.baz.bar5 = 5; ++ foo = { ++ baz = { ++ bar1 = 1; ++ bar2 = 2; ++ bar3 = 3; ++ bar4 = 4; ++ bar5 = 5; ++ }; ++ }; + } diff --git a/bin/tests/snapshots/repeated_keys__lint_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap b/bin/tests/snapshots/repeated_keys__lint_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap new file mode 100644 index 00000000..effa1c56 --- /dev/null +++ b/bin/tests/snapshots/repeated_keys__lint_404a292d1811899300d6466e6c10281713b79ec2d69e2c65ddd0bc22b96fb21c.snap @@ -0,0 +1,18 @@ +--- +source: bin/tests/repeated_keys.rs +assertion_line: 7 +expression: "\"{\\n hardware.bluetooth = {\\n enable = true;\\n };\\n networking.hostName = \\\"nixbox\\\";\\n hardware.nvidia-container-toolkit.enable = true;\\n hardware.nvidia = {\\n modesetting.enable = true;\\n };\\n}\\n\"" +--- +[W20] Warning: Avoid repeated keys in attribute sets + ╭─[:2:3] + │ + 2 │ hardware.bluetooth = { + · ─────────┬──────── + · ╰────────── The key hardware is first assigned here ... + 6 │ hardware.nvidia-container-toolkit.enable = true; + · ────────────────────┬─────────────────── + · ╰───────────────────── ... repeated here ... + 7 │ hardware.nvidia = { + · ───────┬─────── + · ╰───────── ... and here. Try hardware = { bluetooth=...; nvidia-container-toolkit.enable=...; nvidia=...; } instead. +───╯ diff --git a/lib/src/lints/repeated_keys.rs b/lib/src/lints/repeated_keys.rs index 6425a580..559d35a1 100644 --- a/lib/src/lints/repeated_keys.rs +++ b/lib/src/lints/repeated_keys.rs @@ -1,10 +1,10 @@ use std::fmt::Write as _; -use crate::{Metadata, Report, Rule}; +use crate::{Metadata, Report, Rule, Suggestion, make}; use macros::lint; use rnix::{ - NodeOrToken, SyntaxElement, SyntaxKind, + NodeOrToken, SyntaxElement, SyntaxKind, TextRange, ast::{Attr, AttrSet, AttrpathValue, Entry, HasEntry as _}, }; use rowan::ast::AstNode as _; @@ -44,6 +44,178 @@ use rowan::ast::AstNode as _; )] struct RepeatedKeys; +struct Occurrence { + attrpath_range: TextRange, + entry_range: TextRange, + subkey: String, + nested_entry_text: String, +} + +fn relative_range(parent_start: usize, range: TextRange) -> (usize, usize) { + ( + usize::from(range.start()) - parent_start, + usize::from(range.end()) - parent_start, + ) +} + +fn find_entry_indent(parent_text: &str, first_entry_start: usize) -> &str { + let line_start = parent_text[..first_entry_start] + .rfind('\n') + .map_or(0, |offset| offset + 1); + &parent_text[line_start..first_entry_start] +} + +fn indent_nested_entry(text: &str, entry_indent: &str) -> String { + let mut lines = text.lines(); + let first_line = lines.next().unwrap_or_default(); + let nested_indent = format!("{entry_indent} "); + + let mut indented = format!("{nested_indent}{first_line}"); + for line in lines { + indented.push('\n'); + indented.push_str(entry_indent); + indented.push_str(line); + } + + indented +} + +fn build_grouped_entry( + first_component: &str, + occurrences: &[Occurrence], + entry_indent: &str, +) -> String { + let nested_entries = occurrences + .iter() + .map(|occurrence| indent_nested_entry(&occurrence.nested_entry_text, entry_indent)) + .collect::>() + .join("\n"); + format!("{first_component} = {{\n{nested_entries}\n{entry_indent}}};") +} + +fn collect_occurrences(parent_attr_set: &AttrSet, first_component: &str) -> Vec { + parent_attr_set + .entries() + .filter_map(|entry| { + let Entry::AttrpathValue(attrpath_value) = entry else { + return None; + }; + + let attrpath = attrpath_value.attrpath()?; + let mut components = attrpath.attrs(); + let first_attr = components.next()?; + + let Attr::Ident(ident) = first_attr else { + return None; + }; + + if ident.to_string() != first_component { + return None; + } + + let entry_text = attrpath_value.syntax().to_string(); + let nested_entry_text = entry_text + .strip_prefix(&format!("{first_component}.")) + .map(str::to_owned)?; + + Some(Occurrence { + attrpath_range: attrpath.syntax().text_range(), + entry_range: attrpath_value.syntax().text_range(), + subkey: components + .map(|component| component.to_string()) + .collect::>() + .join("."), + nested_entry_text, + }) + }) + .collect() +} + +fn build_third_message( + first_component: &str, + first_subkey: &str, + second_subkey: &str, + third_subkey: &str, + remaining_occurrences: usize, +) -> String { + let mut message = match remaining_occurrences { + 0 => "... and here.".to_string(), + 1 => "... and here (`1` occurrence omitted).".to_string(), + n => format!("... and here (`{n}` occurrences omitted)."), + }; + write!( + message, + " Try `{first_component} = {{ {first_subkey}=...; {second_subkey}=...; {third_subkey}=...; }}` instead." + ) + .unwrap(); + message +} + +fn build_fix( + parent_attr_set: &AttrSet, + first_component: &str, + occurrences: &[Occurrence], +) -> Option { + let parent_range = parent_attr_set.syntax().text_range(); + let first_entry_range = occurrences.first()?.entry_range; + let last_entry_range = occurrences.last()?.entry_range; + let parent_text = parent_attr_set.syntax().to_string(); + let parent_start = usize::from(parent_range.start()); + let first_entry_start = relative_range(parent_start, first_entry_range).0; + let entry_indent = find_entry_indent(&parent_text, first_entry_start); + let grouped_entry = build_grouped_entry(first_component, occurrences, entry_indent); + + let rewritten_middle = { + let mut rewritten = String::new(); + let mut cursor = first_entry_start; + + for entry in parent_attr_set.entries() { + let entry_range = entry.syntax().text_range(); + if entry_range.end() <= first_entry_range.start() + || entry_range.start() >= last_entry_range.end() + { + continue; + } + + let (entry_start, entry_end) = relative_range(parent_start, entry_range); + if entry_range == first_entry_range { + rewritten.push_str(&parent_text[cursor..entry_start]); + rewritten.push_str(&grouped_entry); + } else if occurrences + .iter() + .any(|occurrence| occurrence.entry_range == entry_range) + { + rewritten.push_str( + parent_text[cursor..entry_start].trim_end_matches(char::is_whitespace), + ); + } else { + rewritten.push_str(&parent_text[cursor..entry_start]); + rewritten.push_str(&parent_text[entry_start..entry_end]); + } + + cursor = entry_end; + } + + let last_entry_end = relative_range(parent_start, last_entry_range).1; + rewritten.push_str(&parent_text[cursor..last_entry_end]); + rewritten + }; + + let last_entry_end = relative_range(parent_start, last_entry_range).1; + let rewritten_parent = format!( + "{}{}{}", + &parent_text[..first_entry_start], + rewritten_middle, + &parent_text[last_entry_end..] + ); + let replacement = make::attrset_from_text(&rewritten_parent); + + Some(Suggestion::with_replacement( + parent_range, + replacement.syntax().clone(), + )) +} + impl Rule for RepeatedKeys { fn validate(&self, node: &SyntaxElement) -> Option { let NodeOrToken::Node(node) = node else { @@ -69,36 +241,10 @@ impl Rule for RepeatedKeys { return None; } - let occurrences = parent_attr_set - .entries() - .filter_map(|kv_scrutinee| { - let Entry::AttrpathValue(kv_scrutinee) = kv_scrutinee else { - return None; - }; - - let scrutinee_key = kv_scrutinee.attrpath()?; - let mut kv_scrutinee_components = scrutinee_key.attrs(); - let kv_scrutinee_first_component = kv_scrutinee_components.next()?; - - let Attr::Ident(kv_scrutinee_ident) = kv_scrutinee_first_component else { - return None; - }; - - if kv_scrutinee_ident.to_string() != first_component_ident.to_string() { - return None; - } - - Some(( - scrutinee_key.syntax().text_range(), - kv_scrutinee_components - .map(|n| n.to_string()) - .collect::>() - .join("."), - )) - }) - .collect::>(); + let first_component_ident_text = first_component_ident.to_string(); + let occurrences = collect_occurrences(&parent_attr_set, &first_component_ident_text); - if occurrences.first()?.0 != attrpath.syntax().text_range() { + if occurrences.first()?.attrpath_range != attrpath.syntax().text_range() { return None; } @@ -106,33 +252,44 @@ impl Rule for RepeatedKeys { return None; } + let fix = build_fix(&parent_attr_set, &first_component_ident_text, &occurrences); let mut iter = occurrences.into_iter(); - let (first_annotation, first_subkey) = iter.next().unwrap(); + let Occurrence { + attrpath_range: first_annotation, + subkey: first_subkey, + .. + } = iter.next().unwrap(); let first_message = format!("The key `{first_component_ident}` is first assigned here ..."); - let (second_annotation, second_subkey) = iter.next().unwrap(); + let Occurrence { + attrpath_range: second_annotation, + subkey: second_subkey, + .. + } = iter.next().unwrap(); let second_message = "... repeated here ..."; - let (third_annotation, third_subkey) = iter.next().unwrap(); - let third_message = { - let remaining_occurrences = iter.count(); - let mut message = match remaining_occurrences { - 0 => "... and here.".to_string(), - 1 => "... and here (`1` occurrence omitted).".to_string(), - n => format!("... and here (`{n}` occurrences omitted)."), - }; - write!( - message, - " Try `{first_component_ident} = {{ {first_subkey}=...; {second_subkey}=...; {third_subkey}=...; }}` instead." - ) - .unwrap(); - message + let Occurrence { + attrpath_range: third_annotation, + subkey: third_subkey, + .. + } = iter.next().unwrap(); + let third_message = build_third_message( + &first_component_ident_text, + &first_subkey, + &second_subkey, + &third_subkey, + iter.count(), + ); + + let report = if let Some(fix) = fix { + self.report().suggest(first_annotation, first_message, fix) + } else { + self.report().diagnostic(first_annotation, first_message) }; Some( - self.report() - .diagnostic(first_annotation, first_message) + report .diagnostic(second_annotation, second_message) .diagnostic(third_annotation, third_message), ) diff --git a/lib/src/make.rs b/lib/src/make.rs index b0a62449..78e1547e 100644 --- a/lib/src/make.rs +++ b/lib/src/make.rs @@ -76,6 +76,10 @@ pub fn attrset( ast_from_text(&buffer) } +pub fn attrset_from_text(text: &str) -> ast::AttrSet { + ast_from_text(text) +} + pub fn select(set: &SyntaxNode, index: &SyntaxNode) -> ast::Select { ast_from_text(&format!("{set}.{index}")) }