diff --git a/src/packs/checker.rs b/src/packs/checker.rs index ba9feff..b84cc29 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; @@ -475,15 +476,36 @@ 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) if updated == contents => {} + 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 => { + eprintln!( + "Warning: could not edit the dependencies list in {} in \ + place, so the file was rewritten and its comments were \ + not preserved.", + pack.yml.display() + ); + 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 0000000..49e6a85 --- /dev/null +++ b/src/packs/checker/dependency_removal.rs @@ -0,0 +1,303 @@ +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, along with any comment lines directly above it. +/// Other comments, blank lines and blocks are kept verbatim. If no items are +/// left, the `dependencies:` key goes too, as serializing the pack would do. +fn remove_dependency_lines(contents: &str, names: &[String]) -> String { + let mut out: Vec<&str> = Vec::new(); + // Comments seen inside the block, held until we know whether the item + // below them is kept. + let mut pending_comments: Vec<&str> = Vec::new(); + let mut key_index = None; + let mut in_block = false; + let mut kept_items = 0; + + for line in contents.split_inclusive('\n') { + if in_block { + let trimmed = line.trim_start(); + if let Some(item) = trimmed.strip_prefix("- ") { + if names.iter().any(|name| name == item_value(item)) { + pending_comments.clear(); + } else { + out.append(&mut pending_comments); + out.push(line); + kept_items += 1; + } + continue; + } + if trimmed.starts_with('#') { + pending_comments.push(line); + continue; + } + if trimmed.is_empty() { + out.append(&mut pending_comments); + out.push(line); + continue; + } + // A comment between the last item and the next key belongs to + // the key. + out.append(&mut pending_comments); + in_block = false; + } else if key_index.is_none() && is_dependencies_key(line) { + key_index = Some(out.len()); + in_block = true; + } + out.push(line); + } + out.append(&mut pending_comments); + + if let Some(index) = key_index.filter(|_| kept_items == 0) { + out.remove(index); + } + out.concat() +} + +/// Matches the top-level key only: a nested `dependencies:` is indented. +fn is_dependencies_key(line: &str) -> bool { + strip_comment(line).trim_end() == "dependencies:" +} + +/// 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.\ndependencies:\n- packs/a\n", + &["packs/a"], + "# Header.\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_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\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 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 + ); + } +} diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 77be8bf..193788f 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,58 @@ 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 = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check-unused-dependencies") + .arg("--auto-correct") + .assert() + .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 +142,54 @@ 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_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); + assert!(!after.contains("packs/baz"), "{after}"); + assert!(after.contains("- packs/bar"), "{after}"); + assert!( + stderr.contains("could not edit the dependencies list"), + "{stderr}" + ); +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 7abbeb5..ada9a2f 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -225,12 +225,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 bdb0ede..ad9202c 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