From 1df54a4df96958a49d36c7692c1ed42336389b66 Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Tue, 3 Nov 2026 15:52:07 -0800 Subject: [PATCH 1/2] Preserve comments when auto-correcting unused dependencies `check-unused-dependencies --auto-correct` rewrote every package.yml it touched by serializing the pack, which deleted its comments and reordered its keys. It now removes the unused entries from the `dependencies:` list as text and leaves the rest of the file as it was, line endings included. A comment covers the entries below it, up to the next blank line or comment, and is removed only when all of those entries are. A commented-out entry, and any comment directly above one, is always kept. When no entries remain, the `dependencies:` key stays, with nothing under it, while a comment is on it, directly above it or still under it; otherwise the whole block is removed. pks and packwerk both read a key with no entries as an empty list. Removing entries one at a time gives the same file as removing them together, which a test checks for every subset of every list of up to five lines. The edited text must parse back to the original pack minus exactly the removed dependencies. When it doesn't, as for a flow-style list, the file is rewritten as before and a warning names the file and the dependencies removed, in sorted order. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 22 + src/packs/checker.rs | 47 +- src/packs/checker/dependency_removal.rs | 696 ++++++++++++++++++ tests/check_unused_dependencies.rs | 154 +++- tests/common/mod.rs | 5 +- .../packs/foo/package.yml | 5 +- 6 files changed, 900 insertions(+), 29 deletions(-) create mode 100644 src/packs/checker/dependency_removal.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 389898a7..9afb7fcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,28 @@ So `pks check` may report a violation it didn't, or no longer report one, and a `package_todo.yml` entry for one it no longer reports is stale until you run `pks update`. +#### `check-unused-dependencies --auto-correct` keeps comments in `package.yml` + +Auto-correct used to rewrite every `package.yml` it touched from scratch, which +deleted all of its comments and reordered its keys. It now removes the unused +entries from the `dependencies:` list and leaves the rest of the file as it +was, line endings included. A comment covers the entries below it, up to the +next blank line or comment, and is removed only when all of those entries are. +A commented-out entry such as `# - packs/a` is always kept, along with any +comment directly above it. If no dependencies remain, the `dependencies:` key +is kept, with no entries under it, while a comment is on it, directly above it +or still under it; otherwise the whole block is removed. Removing entries one at +a time gives the same file as removing them together. + +If a list is written in a form that cannot be edited this way, such as +`dependencies: [packs/a, packs/b]`, the file is rewritten as before and a +warning names the file and the dependencies removed. + +Only this command keeps comments. `lint-package-yml-files`, `add-dependency`, +`create`, `rm` and `update-dependencies-for-constant` still rewrite the +`package.yml` they change and drop its comments, so running +`lint-package-yml-files` afterwards removes the comments kept here. + ## 0.7.0 ### Features diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 7cf22bdc..624188c0 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -3,6 +3,7 @@ mod dependency; pub(crate) mod layer; mod common_test; +mod dependency_removal; mod folder_privacy; pub(crate) mod pack_checker; mod privacy; @@ -606,15 +607,43 @@ fn remove_reference_to_dependency( pack: &Pack, dependency_names: &[String], ) -> anyhow::Result<()> { - let without_dependency = pack - .dependencies - .iter() - .filter(|dependency| !dependency_names.contains(dependency)); - let updated_pack = Pack { - dependencies: without_dependency.cloned().collect(), - ..pack.clone() - }; - write_pack_to_disk(&updated_pack)?; + let contents = std::fs::read_to_string(&pack.yml).map_err(|e| { + anyhow::Error::new(e) + .context(format!("Failed to read pack {:?}", pack.yml)) + })?; + + match dependency_removal::remove_dependencies(&contents, dependency_names) { + Some(updated) => std::fs::write(&pack.yml, updated).map_err(|e| { + anyhow::Error::new(e) + .context(format!("Failed to write pack to disk {:?}", pack.yml)) + })?, + None => { + let mut sorted_names: Vec<&String> = + dependency_names.iter().collect(); + sorted_names.sort(); + eprintln!( + "Warning: could not edit the dependencies list in `{}` in \ + place to remove {}, so the file was rewritten: its comments \ + were not preserved and its keys may be reordered.", + pack.relative_yml().display(), + sorted_names + .iter() + .map(|name| format!("`{name}`")) + .collect::>() + .join(", "), + ); + let without_dependency = pack + .dependencies + .iter() + .filter(|dependency| !dependency_names.contains(dependency)); + let updated_pack = Pack { + dependencies: without_dependency.cloned().collect(), + ..pack.clone() + }; + write_pack_to_disk(&updated_pack)?; + } + } + Ok(()) } // Note: Display impl was removed from CheckAllResult. Use write_text() directly with Configuration. diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/checker/dependency_removal.rs new file mode 100644 index 00000000..cdeecf32 --- /dev/null +++ b/src/packs/checker/dependency_removal.rs @@ -0,0 +1,696 @@ +use crate::packs::pack::Pack; + +/// Returns `contents` with `names` removed from its top-level `dependencies:` +/// list, edited as text so comments, key order and line endings survive. +/// +/// Returns `None` when the edited text does not parse back to the original +/// pack minus exactly `names`, which happens when the list uses a layout the +/// line matcher does not recognise (flow style, multi-line scalars, ...). +pub(super) fn remove_dependencies( + contents: &str, + names: &[String], +) -> Option { + let edited = remove_dependency_lines(contents, names); + is_exact_removal(contents, &edited, names).then_some(edited) +} + +fn is_exact_removal(original: &str, edited: &str, names: &[String]) -> bool { + let (Ok(mut expected), Ok(actual)) = ( + yaml_serde::from_str::(original), + yaml_serde::from_str::(edited), + ) else { + return false; + }; + expected.dependencies.retain(|name| !names.contains(name)); + expected == actual +} + +/// Drops each matching item. A comment covers the items below it, up to the +/// next blank line or comment, and is dropped only when all of them are. A +/// commented-out entry, and any comment directly above one, is kept. If no +/// items are left, the key stays while any comment is on it, directly above +/// it or left under it, so no comment is ever lost with the block; otherwise +/// the whole block goes, as serializing the pack would do. Everything else is +/// verbatim. +fn remove_dependency_lines(contents: &str, names: &[String]) -> String { + let lines: Vec<&str> = contents.split_inclusive('\n').collect(); + let Some(key) = lines.iter().position(|line| is_dependencies_key(line)) + else { + return contents.to_string(); + }; + let mut block_end = key + 1; + while lines.get(block_end).is_some_and(|line| { + let trimmed = line.trim_start(); + trimmed.is_empty() + || trimmed.starts_with('#') + || trimmed.starts_with("- ") + }) { + block_end += 1; + } + + let (block, kept_items) = edit_block(&lines[key + 1..block_end], names); + let keep_key = kept_items > 0 + || has_comment(lines[key]) + || key > 0 && lines[key - 1].trim_start().starts_with('#') + || block.iter().any(|line| line.trim_start().starts_with('#')); + let mut out: Vec<&str> = lines[..key].to_vec(); + if keep_key { + out.push(lines[key]); + out.extend(block); + } + out.extend(&lines[block_end..]); + out.concat() +} + +/// The block's lines with the named items removed, and how many items are +/// left. +fn edit_block<'a>( + lines: &[&'a str], + names: &[String], +) -> (Vec<&'a str>, usize) { + let mut out: Vec<&'a str> = Vec::new(); + let mut group = Group::default(); + let mut kept_items = 0; + + for &line in lines { + let trimmed = line.trim_start(); + if trimmed.starts_with("- ") { + group.items.push(line); + continue; + } + if trimmed.starts_with('#') && group.items.is_empty() { + group.comments.push(line); + continue; + } + let vanished = group.flush(&mut out, names, &mut kept_items); + if trimmed.starts_with('#') { + group.comments.push(line); + } else if !(vanished && out.last().is_none_or(|l| l.trim().is_empty())) + { + // Skipped when a removed group sat between a blank line and + // another blank line or the start of the list. + out.push(line); + } + } + let vanished = group.flush(&mut out, names, &mut kept_items); + if vanished && out.last().is_some_and(|l| l.trim().is_empty()) { + out.pop(); + } + (out, kept_items) +} + +/// A run of comment lines and the items below them. +#[derive(Default)] +struct Group<'a> { + comments: Vec<&'a str>, + items: Vec<&'a str>, +} + +impl<'a> Group<'a> { + /// Moves the group's surviving lines to `out`, returning whether it had + /// lines and none survived. + fn flush( + &mut self, + out: &mut Vec<&'a str>, + names: &[String], + kept_items: &mut usize, + ) -> bool { + let had_lines = !self.comments.is_empty() || !self.items.is_empty(); + let had_items = !self.items.is_empty(); + let kept: Vec<&'a str> = self + .items + .drain(..) + .filter(|line| !is_named_item(line, names)) + .collect(); + *kept_items += kept.len(); + if had_items && kept.is_empty() { + // Keep commented-out entries and the comments above them. + let last_entry = self + .comments + .iter() + .rposition(|line| is_commented_out_item(line)); + self.comments + .truncate(last_entry.map_or(0, |index| index + 1)); + } + let vanished = had_lines && self.comments.is_empty() && kept.is_empty(); + out.append(&mut self.comments); + out.extend(kept); + vanished + } +} + +fn is_named_item(line: &str, names: &[String]) -> bool { + let item = &line.trim_start()["- ".len()..]; + names.iter().any(|name| name == item_value(item)) +} + +fn is_commented_out_item(line: &str) -> bool { + line.trim_start() + .strip_prefix('#') + .is_some_and(|rest| rest.trim_start().starts_with("- ")) +} + +/// Matches the top-level key only: a nested `dependencies:` is indented. +fn is_dependencies_key(line: &str) -> bool { + strip_comment(line).trim_end() == "dependencies:" +} + +fn has_comment(line: &str) -> bool { + strip_comment(line).len() < line.len() +} + +/// The scalar value of a block list item, without quotes or a trailing +/// comment. Escapes inside quotes are not decoded; the parse check in +/// `remove_dependencies` catches any item that this misreads. +fn item_value(item: &str) -> &str { + let item = item.trim(); + for quote in ['"', '\''] { + if let Some(rest) = item.strip_prefix(quote) { + return rest.find(quote).map_or(item, |end| &rest[..end]); + } + } + strip_comment(item).trim_end() +} + +/// In YAML, `#` starts a comment only at the start of a line or after +/// whitespace, so `packs/a#b` is a plain value. +fn strip_comment(text: &str) -> &str { + text.match_indices('#') + .find(|(index, _)| *index == 0 || text[..*index].ends_with([' ', '\t'])) + .map_or(text, |(index, _)| &text[..index]) +} + +#[cfg(test)] +mod tests { + use super::remove_dependencies; + use pretty_assertions::assert_eq; + + fn remove(contents: &str, names: &[&str]) -> Option { + let names: Vec = names.iter().map(|n| n.to_string()).collect(); + remove_dependencies(contents, &names) + } + + fn assert_removes(before: &str, names: &[&str], after: &str) { + assert_eq!(remove(before, names).as_deref(), Some(after)); + } + + #[test] + fn removes_first_middle_and_last_items() { + let before = "dependencies:\n- packs/a\n- packs/b\n- packs/c\n"; + assert_removes( + before, + &["packs/a"], + "dependencies:\n- packs/b\n- packs/c\n", + ); + assert_removes( + before, + &["packs/b"], + "dependencies:\n- packs/a\n- packs/c\n", + ); + assert_removes( + before, + &["packs/c"], + "dependencies:\n- packs/a\n- packs/b\n", + ); + assert_removes( + before, + &["packs/a", "packs/c"], + "dependencies:\n- packs/b\n", + ); + } + + #[test] + fn removes_the_key_when_no_items_remain() { + assert_removes( + "enforce_dependencies: true\ndependencies:\n- packs/a\n- packs/b\nlayer: utilities\n", + &["packs/a", "packs/b"], + "enforce_dependencies: true\nlayer: utilities\n", + ); + assert_removes("dependencies:\n- packs/a\n", &["packs/a"], ""); + assert_removes( + "# Header.\n\ndependencies:\n- packs/a\n", + &["packs/a"], + "# Header.\n\n", + ); + } + + #[test] + fn handles_dependencies_as_the_last_key() { + assert_removes( + "enforce_privacy: true\ndependencies:\n- packs/a\n- packs/b\n", + &["packs/b"], + "enforce_privacy: true\ndependencies:\n- packs/a\n", + ); + assert_removes( + "enforce_privacy: true\ndependencies:\n- packs/a\n- packs/b", + &["packs/b"], + "enforce_privacy: true\ndependencies:\n- packs/a\n", + ); + } + + #[test] + fn handles_indented_lists() { + assert_removes( + "dependencies:\n - packs/a\n - packs/b\nlayer: utilities\n", + &["packs/a"], + "dependencies:\n - packs/b\nlayer: utilities\n", + ); + } + + #[test] + fn handles_quoted_items() { + assert_removes( + "dependencies:\n- \"packs/a\"\n- 'packs/b'\n- packs/c\n", + &["packs/a", "packs/b"], + "dependencies:\n- packs/c\n", + ); + } + + #[test] + fn handles_a_trailing_comment_on_an_item() { + assert_removes( + "dependencies:\n- packs/a # used\n- packs/b # unused\n", + &["packs/b"], + "dependencies:\n- packs/a # used\n", + ); + } + + #[test] + fn keeps_a_hash_that_is_part_of_the_value() { + assert_removes( + "dependencies:\n- packs/a#b\n- packs/a\n", + &["packs/a"], + "dependencies:\n- packs/a#b\n", + ); + } + + #[test] + fn handles_a_comment_on_the_key_line() { + assert_removes( + "dependencies: # keep sorted\n- packs/a\n- packs/b\n", + &["packs/b"], + "dependencies: # keep sorted\n- packs/a\n", + ); + } + + #[test] + fn handles_inline_comments_on_keys_and_values() { + let before = "\ +enforce_privacy: true # why +dependencies: # deps + - packs/a # used by A + - packs/b # unused +ignored_dependencies: # note + - packs/b # would cycle +layer: utilities # layer note +"; + assert_removes( + before, + &["packs/b"], + "\ +enforce_privacy: true # why +dependencies: # deps + - packs/a # used by A +ignored_dependencies: # note + - packs/b # would cycle +layer: utilities # layer note +", + ); + assert_removes( + before, + &["packs/a", "packs/b"], + "\ +enforce_privacy: true # why +dependencies: # deps +ignored_dependencies: # note + - packs/b # would cycle +layer: utilities # layer note +", + ); + } + + #[test] + fn ignores_names_and_syntax_inside_inline_comments() { + assert_removes( + "dependencies: # a: b, [c]\n- packs/a # packs/b\n- packs/b # - packs/c\n- packs/c\n", + &["packs/b"], + "dependencies: # a: b, [c]\n- packs/a # packs/b\n- packs/c\n", + ); + assert_removes( + "dependencies:\n- \"packs/a\" # see \"packs/b\"\n- 'packs/b' # don't\n", + &["packs/b"], + "dependencies:\n- \"packs/a\" # see \"packs/b\"\n", + ); + assert_removes( + "dependencies: #\n- packs/a #\n- packs/b #\n", + &["packs/a"], + "dependencies: #\n- packs/b #\n", + ); + } + + #[test] + fn handles_inline_comments_after_tabs_and_before_crlf() { + assert_removes( + "dependencies:\t# k\n- packs/a\t# a\n- packs/b\t# b\n", + &["packs/b"], + "dependencies:\t# k\n- packs/a\t# a\n", + ); + assert_removes( + "dependencies: # k\r\n- packs/a # a\r\n- packs/b # b\r\n", + &["packs/b"], + "dependencies: # k\r\n- packs/a # a\r\n", + ); + } + + #[test] + fn handles_blank_lines_inside_the_list() { + assert_removes( + "dependencies:\n- packs/a\n\n- packs/b\n\nlayer: utilities\n", + &["packs/b"], + "dependencies:\n- packs/a\n\nlayer: utilities\n", + ); + } + + #[test] + fn removes_the_comment_directly_above_a_removed_item() { + assert_removes( + "dependencies:\n# Needed for A.\n- packs/a\n# Needed for B.\n- packs/b\n", + &["packs/a"], + "dependencies:\n# Needed for B.\n- packs/b\n", + ); + } + + #[test] + fn keeps_a_comment_separated_from_a_removed_item_by_a_blank_line() { + assert_removes( + "dependencies:\n# Keep sorted.\n\n- packs/a\n- packs/b\n", + &["packs/a"], + "dependencies:\n# Keep sorted.\n\n- packs/b\n", + ); + } + + #[test] + fn keeps_a_comment_between_the_list_and_the_next_key() { + assert_removes( + "dependencies:\n- packs/a\n- packs/b\n# About the layer.\nlayer: utilities\n", + &["packs/b"], + "dependencies:\n- packs/a\n# About the layer.\nlayer: utilities\n", + ); + } + + #[test] + fn leaves_ignored_dependencies_and_nested_keys_alone() { + assert_removes( + "\ +metadata: + dependencies: + - packs/a +dependencies: +- packs/a +- packs/b +ignored_dependencies: +# Deliberately ignored: a would cycle. +- packs/a +", + &["packs/a"], + "\ +metadata: + dependencies: + - packs/a +dependencies: +- packs/b +ignored_dependencies: +# Deliberately ignored: a would cycle. +- packs/a +", + ); + } + + #[test] + fn preserves_crlf_line_endings() { + assert_removes( + "# Header.\r\ndependencies:\r\n- packs/a\r\n- packs/b\r\n", + &["packs/b"], + "# Header.\r\ndependencies:\r\n- packs/a\r\n", + ); + } + + #[test] + fn keeps_a_comment_while_any_item_it_covers_remains() { + assert_removes( + "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n", + &["packs/baz"], + "dependencies:\n# Keep this list sorted.\n- packs/bar\n", + ); + assert_removes( + "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n\n- packs/bop\n", + &["packs/baz", "packs/bar"], + "dependencies:\n- packs/bop\n", + ); + } + + #[test] + fn keeps_a_commented_out_entry() { + assert_removes( + "dependencies:\n- packs/bar\n# - packs/bop # re-add after the migration\n- packs/baz\n", + &["packs/baz"], + "dependencies:\n- packs/bar\n# - packs/bop # re-add after the migration\n", + ); + } + + #[test] + fn removes_the_whole_block_when_no_comment_remains() { + assert_removes( + "enforce_dependencies: true\ndependencies:\n - packs/a\n\nenforce_privacy: true\n", + &["packs/a"], + "enforce_dependencies: true\nenforce_privacy: true\n", + ); + assert_removes( + "metadata:\n notes: |\n Some notes.\ndependencies:\n # Needed for A.\n - packs/a\n\n # Needed for B.\n - packs/b\nlayer: utilities\n", + &["packs/a", "packs/b"], + "metadata:\n notes: |\n Some notes.\nlayer: utilities\n", + ); + } + + #[test] + fn keeps_the_key_when_a_comment_is_on_it_or_directly_above_it() { + assert_removes( + "enforce_dependencies: true\ndependencies: # keep sorted\n - packs/a\n # Keep sorted.\n\nenforce_privacy: true\n", + &["packs/a"], + "enforce_dependencies: true\ndependencies: # keep sorted\n # Keep sorted.\n\nenforce_privacy: true\n", + ); + assert_removes( + "\ +# A list of this package's dependencies +# Note that packages in this list require their own `package.yml` file +dependencies: + - packs/baz + +# Specify which packages are allowed to see this one +visible_to: + - packs/bar +", + &["packs/baz"], + "\ +# A list of this package's dependencies +# Note that packages in this list require their own `package.yml` file +dependencies: +# Specify which packages are allowed to see this one +visible_to: + - packs/bar +", + ); + } + + #[test] + fn keeps_the_key_while_a_comment_under_it_remains() { + assert_removes( + "dependencies:\n# - packs/bop # re-add after the migration\n- packs/baz\nlayer: utilities\n", + &["packs/baz"], + "dependencies:\n# - packs/bop # re-add after the migration\nlayer: utilities\n", + ); + assert_removes( + "dependencies:\n- packs/baz\n# Layers, lowest first:\n# - utilities may not depend on services\nlayer: utilities\n", + &["packs/baz"], + "dependencies:\n# Layers, lowest first:\n# - utilities may not depend on services\nlayer: utilities\n", + ); + assert_removes( + "dependencies:\n- packs/a\n# - packs/old\nlayer: utilities\n", + &["packs/a"], + "dependencies:\n# - packs/old\nlayer: utilities\n", + ); + assert_removes( + "dependencies:\n - packs/a\n # - packs/old\n", + &["packs/a"], + "dependencies:\n # - packs/old\n", + ); + assert_removes( + "dependencies:\n- packs/a\n# Footer.\n", + &["packs/a"], + "dependencies:\n# Footer.\n", + ); + } + + #[test] + fn declines_layouts_it_cannot_edit() { + assert_eq!( + remove("dependencies: [packs/a, packs/b]\n", &["packs/b"]), + None + ); + assert_eq!( + remove("dependencies:\n- \"packs/\\u0061\"\n", &["packs/a"]), + None + ); + assert_eq!( + remove( + "dependencies: &deps # anchored\n- packs/a\n- packs/b\n", + &["packs/b"] + ), + None + ); + assert_eq!( + remove( + "dependencies: !!seq # tagged\n- packs/a\n- packs/b\n", + &["packs/b"] + ), + None + ); + } + + #[test] + fn keeps_a_comment_directly_above_a_commented_out_entry() { + assert_removes( + "dependencies:\n- packs/a\n# Re-add after the migration:\n# - packs/old\n- packs/b\n", + &["packs/b"], + "dependencies:\n- packs/a\n# Re-add after the migration:\n# - packs/old\n", + ); + } + + #[test] + fn a_commented_out_entry_does_not_keep_a_comment_below_it() { + assert_removes( + "dependencies:\n- packs/a\n# - packs/old\n# Needed for B.\n- packs/b\n", + &["packs/b"], + "dependencies:\n- packs/a\n# - packs/old\n", + ); + } + + #[test] + fn removes_a_blank_line_left_at_either_end_of_the_list() { + assert_removes( + "dependencies:\n- packs/a\n\n- packs/b\n", + &["packs/a"], + "dependencies:\n- packs/b\n", + ); + assert_removes( + "dependencies:\n- packs/a\n\n- packs/b\nlayer: utilities\n", + &["packs/b"], + "dependencies:\n- packs/a\nlayer: utilities\n", + ); + } + + #[test] + fn leaves_document_markers_alone() { + assert_removes( + "---\ndependencies:\n- packs/a\n- packs/b\n...\n", + &["packs/a"], + "---\ndependencies:\n- packs/b\n...\n", + ); + } + + #[test] + fn removing_one_at_a_time_matches_removing_together() { + let before = "\ +enforce_dependencies: true +dependencies: +# Keep sorted. +- packs/a +- packs/b +# - packs/old +# Needed for C. +- packs/c # trailing + +# Needed for D and E. +- packs/d +- packs/e +# Re-add after the migration: +# - packs/older +- packs/f +# About the layer. +layer: utilities +"; + let all = [ + "packs/a", "packs/b", "packs/c", "packs/d", "packs/e", "packs/f", + ]; + for mask in 1..1u32 << all.len() { + let names: Vec<&str> = (0..all.len()) + .filter(|bit| mask & (1 << bit) != 0) + .map(|bit| all[bit]) + .collect(); + let together = remove(before, &names).unwrap(); + for order in [names.clone(), names.iter().rev().copied().collect()] + { + let one_at_a_time = + order.iter().fold(before.to_string(), |text, name| { + remove(&text, &[name]).unwrap() + }); + assert_eq!(one_at_a_time, together, "removing {order:?}"); + } + } + } + + /// Every list of up to five lines drawn from an entry, a comment, a + /// commented-out entry and a blank line, with and without a comment + /// above the key. + #[test] + fn removal_order_never_matters_for_generated_layouts() { + const KINDS: [&str; 4] = ["item", "# note\n", "# - packs/old\n", "\n"]; + for prefix in ["", "# Header.\n"] { + for len in 1..=5u32 { + for code in 0..4usize.pow(len) { + let mut block = String::new(); + let mut items: Vec = Vec::new(); + for position in 0..len { + let kind = KINDS[code / 4usize.pow(position) % 4]; + if kind == "item" { + let name = format!("packs/p{position}"); + block.push_str(&format!("- {name}\n")); + items.push(name); + } else { + block.push_str(kind); + } + } + let before = format!( + "{prefix}dependencies:\n{block}layer: utilities\n" + ); + assert_removal_order_never_matters(&before, &items); + } + } + } + } + + fn assert_removal_order_never_matters(before: &str, items: &[String]) { + let names = |mask: u32| -> Vec<&str> { + (0..items.len()) + .filter(|bit| mask & (1 << bit) != 0) + .map(|bit| items[bit].as_str()) + .collect() + }; + for mask in 1..1u32 << items.len() { + let names = names(mask); + let together = remove(before, &names) + .unwrap_or_else(|| panic!("declined {before:?}")); + for order in [names.clone(), names.iter().rev().copied().collect()] + { + let one_at_a_time = + order.iter().fold(before.to_string(), |text, name| { + remove(&text, &[name]).unwrap() + }); + assert_eq!( + one_at_a_time, together, + "removing {order:?} from {before:?}" + ); + } + } + } +} diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 77be8bf3..b8fa9f5b 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -1,6 +1,6 @@ use assert_cmd::cargo::cargo_bin_cmd; use predicates::prelude::*; -use std::{error::Error, fs}; +use std::{error::Error, fs, path::Path}; mod common; fn assert_check_unused_dependencies(cmd: &str) -> Result<(), Box> { @@ -39,15 +39,16 @@ fn assert_auto_correct_unused_dependencies( ) -> Result<(), Box> { common::set_up_fixtures(); - let expected_before_autocorrect = [ - "enforce_dependencies: true", - "enforce_privacy: true", - "layer: technical_services", - "dependencies:", - "- packs/bar", - "- packs/baz\n", - ] - .join("\n"); + let expected_before_autocorrect = r#"# Header comment: proves comments above the first key survive. +enforce_dependencies: true +enforce_privacy: true +dependencies: +# Comment inside the dependencies block. +- packs/bar +- packs/baz +# Trailing comment, after the list and before another key. +layer: technical_services +"#; let foo_package_yml = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap(); assert_eq!(foo_package_yml, expected_before_autocorrect); @@ -60,20 +61,54 @@ fn assert_auto_correct_unused_dependencies( .assert() .success(); - let expected_autocorrect = [ - "enforce_privacy: true", - "layer: technical_services", - "enforce_dependencies: true", - "dependencies:", - "- packs/bar\n", - ] - .join("\n"); + // Comments in all three positions survive, and the key order is unchanged: + // the correction deletes the one list item and nothing else. + let expected_autocorrect = r#"# Header comment: proves comments above the first key survive. +enforce_dependencies: true +enforce_privacy: true +dependencies: +# Comment inside the dependencies block. +- packs/bar +# Trailing comment, after the list and before another key. +layer: technical_services +"#; let after_autocorrect = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap(); assert_eq!(after_autocorrect, expected_autocorrect); + assert_no_unused_dependencies(Path::new( + "tests/fixtures/app_with_unnecessary_dependencies", + )); + Ok(()) } +fn assert_no_unused_dependencies(project_root: &Path) { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(project_root) + .arg("check-unused-dependencies") + .assert() + .success(); +} + +fn auto_correct_foo_package_yml(before: &str) -> (String, String) { + let fixture = common::Fixture::new("app_with_unnecessary_dependencies"); + let package_yml = fixture.path("packs/foo/package.yml"); + fs::write(&package_yml, before).unwrap(); + + let output = + common::pks(&fixture, &["check-unused-dependencies", "--auto-correct"]) + .success() + .get_output() + .clone(); + + assert_no_unused_dependencies(fixture.root()); + ( + fs::read_to_string(&package_yml).unwrap(), + String::from_utf8(output.stderr).unwrap(), + ) +} + #[test] fn test_auto_correct_unnecessary_dependencies() -> Result<(), Box> { assert_auto_correct_unused_dependencies( @@ -103,3 +138,86 @@ fn test_check_unnecessary_dependencies_no_issue() -> Result<(), Box> .success(); Ok(()) } + +#[test] +fn test_auto_correct_preserves_comments_in_hand_edited_layouts() { + let before = "\ +# Header comment. +enforce_dependencies: true +enforce_privacy: true +dependencies: # keep sorted + # Comment inside the dependencies block. + - \"packs/bar\" + + - packs/baz # unused +ignored_dependencies: + # Deliberately ignored: bop would cycle. + - packs/bop +"; + let expected = "\ +# Header comment. +enforce_dependencies: true +enforce_privacy: true +dependencies: # keep sorted + # Comment inside the dependencies block. + - \"packs/bar\" +ignored_dependencies: + # Deliberately ignored: bop would cycle. + - packs/bop +"; + + let (after, stderr) = auto_correct_foo_package_yml(before); + assert_eq!(after, expected); + assert_eq!(stderr, ""); +} + +#[test] +fn test_auto_correct_preserves_inline_comments_on_keys_and_values() { + let before = "\ +enforce_dependencies: true # enforced since the split +enforce_privacy: true # see the privacy docs +dependencies: # keep sorted +- packs/bar # used by Foo +- packs/baz # unused: was used by Foo's old job +ignored_dependencies: # cycles we accept for now +- packs/bop # would cycle with packs/baz +"; + let expected = "\ +enforce_dependencies: true # enforced since the split +enforce_privacy: true # see the privacy docs +dependencies: # keep sorted +- packs/bar # used by Foo +ignored_dependencies: # cycles we accept for now +- packs/bop # would cycle with packs/baz +"; + + let (after, stderr) = auto_correct_foo_package_yml(before); + assert_eq!(after, expected); + assert_eq!(stderr, ""); +} + +#[test] +fn test_auto_correct_falls_back_to_rewriting_layouts_it_cannot_edit() { + let before = "\ +# This comment is lost by the rewrite. +enforce_dependencies: true +enforce_privacy: true +dependencies: [packs/bar, packs/baz] +"; + + let (after, stderr) = auto_correct_foo_package_yml(before); + let expected = "\ +enforce_privacy: true +enforce_dependencies: true +dependencies: +- packs/bar +"; + assert_eq!(after, expected); + assert_eq!( + stderr, + "Warning: could not edit the dependencies list in \ + `packs/foo/package.yml` in place to remove `packs/baz`, so the file \ + was rewritten: its comments were not preserved and its keys may be \ + reordered.\n" + ); +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 8fcf4485..e1090563 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -343,12 +343,15 @@ enforce_dependencies: true ); let pack_yml_contents = String::from( "\ +# Header comment: proves comments above the first key survive. enforce_dependencies: true enforce_privacy: true -layer: technical_services dependencies: +# Comment inside the dependencies block. - packs/bar - packs/baz +# Trailing comment, after the list and before another key. +layer: technical_services ", ); diff --git a/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml b/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml index bdb0edeb..ad9202cf 100644 --- a/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml +++ b/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml @@ -1,6 +1,9 @@ +# Header comment: proves comments above the first key survive. enforce_dependencies: true enforce_privacy: true -layer: technical_services dependencies: +# Comment inside the dependencies block. - packs/bar - packs/baz +# Trailing comment, after the list and before another key. +layer: technical_services From 83eba89cff8ea9a54e2038b99295f625164fdc90 Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Tue, 3 Nov 2026 17:00:43 -0800 Subject: [PATCH 2/2] Add end-to-end flows for check-unused-dependencies --auto-correct Each test builds a small project in a temporary directory, runs pks the way a developer would, and compares every file before and after, so it fails both when an intended edit is missing and when any other file or line changes. The flows cover a documented package.yml, several packs in one run, a second run, emptying a list with and without comments, removing over several runs, lists that name the same packs under other keys, CRLF and a missing final newline, the flow-style fallback, a check without --auto-correct, and a pack whose dependencies are all used. After each correction the project must pass check-unused-dependencies, check and validate. Co-Authored-By: Claude Opus 5.5 --- ..._correct_unused_dependencies_flows_test.rs | 567 ++++++++++++++++++ 1 file changed, 567 insertions(+) create mode 100644 tests/auto_correct_unused_dependencies_flows_test.rs diff --git a/tests/auto_correct_unused_dependencies_flows_test.rs b/tests/auto_correct_unused_dependencies_flows_test.rs new file mode 100644 index 00000000..aaf3606b --- /dev/null +++ b/tests/auto_correct_unused_dependencies_flows_test.rs @@ -0,0 +1,567 @@ +//! End-to-end flows for `check-unused-dependencies --auto-correct`. +//! +//! Each test builds a small project in a temporary directory, runs `pks` the +//! way a developer would, and compares every file in the project before and +//! after: the edits we intend must happen, and no other file may change. + +use assert_cmd::cargo::cargo_bin_cmd; +use pretty_assertions::assert_eq; +use std::{collections::BTreeMap, fs, path::Path}; +use tempfile::TempDir; + +const LIBRARIES: [&str; 4] = ["alpha", "beta", "gamma", "delta"]; + +/// A project with one library pack per name in `LIBRARIES`, each defining a +/// module of the same name, plus the consumer packs given to `new`. +struct Project { + dir: TempDir, +} + +impl Project { + fn new() -> Project { + let project = Project { + dir: TempDir::new().unwrap(), + }; + project.write("packwerk.yml", "cache: false\n"); + project.write("package.yml", "enforce_dependencies: true\n"); + for name in LIBRARIES { + project.write( + &format!("packs/{name}/package.yml"), + "enforce_dependencies: true\n", + ); + project.write( + &format!("packs/{name}/app/services/{name}.rb"), + &format!("module {}\nend\n", module_name(name)), + ); + } + project + } + + /// Adds a pack whose code references the modules of `uses`. + fn consumer(self, name: &str, package_yml: &str, uses: &[&str]) -> Self { + self.write(&format!("packs/{name}/package.yml"), package_yml); + self.uses(name, uses); + self + } + + /// Replaces the consumer's code so that it references only `uses`. + fn uses(&self, name: &str, uses: &[&str]) { + let body: String = uses + .iter() + .map(|library| format!(" {}\n", module_name(library))) + .collect(); + self.write( + &format!("packs/{name}/app/services/{name}_service.rb"), + &format!( + "class {}Service\n def call\n{body} end\nend\n", + module_name(name) + ), + ); + } + + fn write(&self, relative: &str, contents: &str) { + let path = self.dir.path().join(relative); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, contents).unwrap(); + } + + fn read(&self, relative: &str) -> String { + fs::read_to_string(self.dir.path().join(relative)).unwrap() + } + + /// Every file in the project except pks's cache, keyed by relative path. + fn snapshot(&self) -> BTreeMap { + let mut files = BTreeMap::new(); + collect(self.dir.path(), self.dir.path(), &mut files); + files + } + + fn pks(&self, args: &[&str]) -> assert_cmd::assert::Assert { + cargo_bin_cmd!("pks") + .env_remove("RUST_LOG") + .arg("--project-root") + .arg(self.dir.path()) + .args(args) + .assert() + } + + /// Runs auto-correct and returns its stderr. + fn auto_correct(&self) -> String { + let output = self + .pks(&["check-unused-dependencies", "--auto-correct"]) + .success() + .get_output() + .clone(); + String::from_utf8(output.stderr).unwrap() + } + + /// The project is consistent: nothing unused, nothing missing, and every + /// package.yml loads. + fn assert_clean(&self) { + self.pks(&["check-unused-dependencies"]).success(); + self.pks(&["check"]).success(); + self.pks(&["validate"]).success(); + } +} + +fn collect(root: &Path, dir: &Path, files: &mut BTreeMap) { + for entry in fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + let relative = path.strip_prefix(root).unwrap().to_str().unwrap(); + if relative == "tmp" { + continue; + } + if path.is_dir() { + collect(root, &path, files); + } else { + files.insert( + relative.to_string(), + fs::read_to_string(&path).unwrap(), + ); + } + } +} + +fn module_name(pack: &str) -> String { + let mut chars = pack.chars(); + let first = chars.next().unwrap().to_ascii_uppercase(); + format!("{first}{}", chars.as_str()) +} + +/// Asserts that exactly the files in `expected` changed, to those contents. +fn assert_changes( + before: &BTreeMap, + after: &BTreeMap, + expected: &[(&str, &str)], +) { + let mut want = before.clone(); + for (path, contents) in expected { + assert!(want.contains_key(*path), "{path} is not in the project"); + want.insert(path.to_string(), contents.to_string()); + } + assert_eq!(after, &want); +} + +/// The layout `packwerk init` and `pks create` start from, after a developer +/// has filled in a few dependencies and commented on them. +#[test] +fn removes_one_unused_dependency_from_a_documented_package_yml() { + let before_yml = "\ +# Turn on dependency checks for this package +enforce_dependencies: true + +# Turn on privacy checks for this package +enforce_privacy: true + +# A list of this package's dependencies +# Note that packages in this list require their own `package.yml` file +dependencies: + # Shared models. + - packs/alpha + # Kept until the beta migration lands. + - packs/beta + - packs/gamma # used by the importer + +# Specify which packages are allowed to see this one +visible_to: + - packs/delta +"; + let project = + Project::new().consumer("app", before_yml, &["alpha", "gamma"]); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + // The beta comment stays: it covers every entry down to the next blank + // line or comment, and gamma is still one of them. + let after_yml = "\ +# Turn on dependency checks for this package +enforce_dependencies: true + +# Turn on privacy checks for this package +enforce_privacy: true + +# A list of this package's dependencies +# Note that packages in this list require their own `package.yml` file +dependencies: + # Shared models. + - packs/alpha + # Kept until the beta migration lands. + - packs/gamma # used by the importer + +# Specify which packages are allowed to see this one +visible_to: + - packs/delta +"; + assert_changes( + &before, + &project.snapshot(), + &[("packs/app/package.yml", after_yml)], + ); + project.assert_clean(); +} + +#[test] +fn removes_a_comment_with_the_only_entry_it_covers() { + let project = Project::new().consumer( + "app", + "dependencies:\n - packs/alpha\n\n # Kept until the beta migration lands.\n - packs/beta\n\n - packs/gamma\n", + &["alpha", "gamma"], + ); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + assert_changes( + &before, + &project.snapshot(), + &[( + "packs/app/package.yml", + "dependencies:\n - packs/alpha\n\n - packs/gamma\n", + )], + ); + project.assert_clean(); +} + +#[test] +fn edits_every_pack_with_unused_dependencies_and_no_other_file() { + let project = Project::new() + .consumer( + "orders", + "dependencies:\n# Keep sorted.\n- packs/alpha\n- packs/beta\n", + &["beta"], + ) + .consumer( + "billing", + "enforce_dependencies: true # strict soon\ndependencies:\n- packs/gamma\n- packs/delta\n", + &["gamma"], + ) + .consumer( + "shipping", + "# Already tidy.\ndependencies:\n- packs/alpha\n", + &["alpha"], + ); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + assert_changes( + &before, + &project.snapshot(), + &[ + ( + "packs/orders/package.yml", + "dependencies:\n# Keep sorted.\n- packs/beta\n", + ), + ( + "packs/billing/package.yml", + "enforce_dependencies: true # strict soon\ndependencies:\n- packs/gamma\n", + ), + ], + ); + project.assert_clean(); +} + +#[test] +fn a_second_run_changes_nothing() { + let project = Project::new().consumer( + "app", + "dependencies:\n- packs/alpha\n# - packs/old\n- packs/beta\n", + &["alpha"], + ); + project.auto_correct(); + let after_first = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + assert_eq!(project.snapshot(), after_first); + assert_eq!( + project.read("packs/app/package.yml"), + "dependencies:\n- packs/alpha\n# - packs/old\n" + ); +} + +#[test] +fn removing_the_last_dependency_keeps_the_key_while_comments_remain() { + let before_yml = "\ +enforce_dependencies: true +dependencies: +# - packs/delta # re-add after the migration +- packs/alpha +- packs/beta +# Visibility, narrowest first: +# - only the delta pack may see this one +visible_to: +- packs/delta +"; + let project = Project::new().consumer("app", before_yml, &[]); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + let after_yml = "\ +enforce_dependencies: true +dependencies: +# - packs/delta # re-add after the migration +# Visibility, narrowest first: +# - only the delta pack may see this one +visible_to: +- packs/delta +"; + assert_changes( + &before, + &project.snapshot(), + &[("packs/app/package.yml", after_yml)], + ); + project.assert_clean(); +} + +#[test] +fn removing_the_last_dependency_removes_an_uncommented_block() { + let project = Project::new().consumer( + "app", + "enforce_dependencies: true\ndependencies:\n - packs/alpha\n - packs/beta\n\nenforce_privacy: true\n", + &[], + ); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + assert_changes( + &before, + &project.snapshot(), + &[( + "packs/app/package.yml", + "enforce_dependencies: true\nenforce_privacy: true\n", + )], + ); + project.assert_clean(); +} + +/// A developer drops one usage, runs auto-correct, then drops another and runs +/// it again. The file must end up the same as when both go in one run. +#[test] +fn removing_dependencies_over_several_runs_matches_one_run() { + let before_yml = "\ +enforce_dependencies: true +dependencies: +# Core. +- packs/alpha +- packs/beta +# note + +- packs/gamma +# - packs/old +- packs/delta +enforce_privacy: true +"; + let incremental = Project::new().consumer( + "app", + before_yml, + &["alpha", "beta", "gamma", "delta"], + ); + let mut steps = vec![]; + for uses in [ + &["alpha", "beta", "gamma"][..], + &["beta", "gamma"], + &["beta"], + &[], + ] { + incremental.uses("app", uses); + assert_eq!(incremental.auto_correct(), ""); + incremental.assert_clean(); + steps.push(incremental.read("packs/app/package.yml")); + } + + for (index, uses) in [ + &["alpha", "beta", "gamma"][..], + &["beta", "gamma"], + &["beta"], + &[], + ] + .iter() + .enumerate() + { + let batch = Project::new().consumer("app", before_yml, uses); + assert_eq!(batch.auto_correct(), ""); + assert_eq!( + batch.read("packs/app/package.yml"), + steps[index], + "after removing all but {uses:?}" + ); + } + assert_eq!( + steps.last().unwrap(), + "\ +enforce_dependencies: true +dependencies: +# note + +# - packs/old +enforce_privacy: true +" + ); +} + +#[test] +fn leaves_other_lists_that_name_the_same_packs_alone() { + let before_yml = "\ +metadata: + owner: team-a + dependencies: + - packs/beta +dependencies: +- packs/alpha +- packs/beta +ignored_dependencies: +# Would cycle. +- packs/beta +visible_to: +- packs/beta +"; + let project = Project::new().consumer("app", before_yml, &["alpha"]); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + assert_changes( + &before, + &project.snapshot(), + &[( + "packs/app/package.yml", + "\ +metadata: + owner: team-a + dependencies: + - packs/beta +dependencies: +- packs/alpha +ignored_dependencies: +# Would cycle. +- packs/beta +visible_to: +- packs/beta +", + )], + ); +} + +#[test] +fn preserves_crlf_line_endings_and_a_missing_final_newline() { + let project = Project::new() + .consumer( + "windows", + "# Edited on Windows.\r\ndependencies:\r\n- packs/alpha\r\n- packs/beta\r\n", + &["alpha"], + ) + .consumer( + "unterminated", + "dependencies:\n- packs/alpha\n- packs/beta\nenforce_privacy: true", + &["alpha"], + ); + let before = project.snapshot(); + + assert_eq!(project.auto_correct(), ""); + + assert_changes( + &before, + &project.snapshot(), + &[ + ( + "packs/windows/package.yml", + "# Edited on Windows.\r\ndependencies:\r\n- packs/alpha\r\n", + ), + ( + "packs/unterminated/package.yml", + "dependencies:\n- packs/alpha\nenforce_privacy: true", + ), + ], + ); + project.assert_clean(); +} + +/// A list the text edit can't handle is rewritten as before, with a warning, +/// and the other packs in the same run are still edited in place. +#[test] +fn falls_back_to_rewriting_only_the_files_it_cannot_edit() { + let project = Project::new() + .consumer( + "flow", + "# Lost by the rewrite.\nenforce_privacy: true\ndependencies: [packs/alpha, packs/gamma, packs/beta]\n", + &["alpha"], + ) + .consumer( + "block", + "# Kept.\ndependencies:\n- packs/alpha\n- packs/beta\n", + &["alpha"], + ); + let before = project.snapshot(); + + let stderr = project.auto_correct(); + + assert_eq!( + stderr, + "Warning: could not edit the dependencies list in \ + `packs/flow/package.yml` in place to remove `packs/beta`, \ + `packs/gamma`, so the file was rewritten: its comments were not \ + preserved and its keys may be reordered.\n" + ); + assert_changes( + &before, + &project.snapshot(), + &[ + ( + "packs/flow/package.yml", + "enforce_privacy: true\ndependencies:\n- packs/alpha\n", + ), + ( + "packs/block/package.yml", + "# Kept.\ndependencies:\n- packs/alpha\n", + ), + ], + ); + project.assert_clean(); +} + +#[test] +fn without_auto_correct_reports_and_writes_nothing() { + let project = Project::new().consumer( + "app", + "dependencies:\n- packs/alpha\n- packs/beta\n", + &["alpha"], + ); + let before = project.snapshot(); + + project + .pks(&["check-unused-dependencies"]) + .failure() + .stdout(predicates::str::contains( + "packs/app depends on packs/beta but does not use it", + )); + + assert_eq!(project.snapshot(), before); +} + +#[test] +fn never_removes_a_dependency_that_is_used() { + let before_yml = "\ +dependencies: +- packs/alpha +- \"packs/beta\" +- 'packs/gamma' # quoted +- packs/delta +"; + let project = Project::new().consumer( + "app", + before_yml, + &["alpha", "beta", "gamma", "delta"], + ); + let before = project.snapshot(); + + project + .pks(&["check-unused-dependencies", "--auto-correct"]) + .success(); + + assert_eq!(project.snapshot(), before); + project.assert_clean(); +}