From 0f0379d7deb57cd605bc5a786a8244923a3c422b Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 26 Sep 2026 20:07:40 -0700 Subject: [PATCH 1/2] Move `strict` from ViolationIdentifier onto Violation `package_todo.yml` can't record `strict`, so recorded violations were rebuilt with `strict: false` while found violations in a strict pack had `true`. Every comparison with the recorded set had to go through `recorded_key()` to zero the flag, and a comparison that missed it was #41. `strict` says how to treat a violation, not which violation it is, so it now lives on `Violation`. The identifier compares directly, `recorded_key()` is gone, and the found-violation set in `build_stale_violations` borrows identifiers instead of cloning one per violation. The `Violation` doc comment's claim that references to the same constant in the same file are one violation is corrected: they're separate violations that share an identifier. No behaviour change. Text, JSON and CSV output, exit codes, and the files `update` writes all match main on every fixture, comparing JSON and CSV entries order-insensitively since they come from a HashSet. --- src/packs/checker.rs | 51 +++++++++++-------------------- src/packs/checker/common_test.rs | 4 +-- src/packs/checker/pack_checker.rs | 2 +- src/packs/csv.rs | 4 +-- src/packs/json.rs | 4 +-- src/packs/pack.rs | 3 -- src/packs/package_todo.rs | 5 ++- src/packs/text.rs | 2 +- tests/check_test.rs | 15 +++++---- 9 files changed, 34 insertions(+), 56 deletions(-) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 903d34c7..11d194a2 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -33,40 +33,31 @@ use super::reference_extractor::get_all_references; #[derive(PartialEq, Clone, Eq, Hash, Debug)] pub struct ViolationIdentifier { pub violation_type: CheckerType, - pub strict: bool, pub file: String, pub constant_name: String, pub referencing_pack_name: String, pub defining_pack_name: String, } -impl ViolationIdentifier { - /// `strict` describes how a violation should be treated, not which violation - /// it is, and `package_todo.yml` has nowhere to record it, so recorded - /// violations are always rebuilt with `strict: false`. Compare through this - /// so a violation in a strict pack can still match its recorded entry. - pub(crate) fn recorded_key(&self) -> Self { - Self { - strict: false, - ..self.clone() - } - } -} - /// A violation combines an identifier with display metadata. /// /// `source_location` is intentionally separate from `ViolationIdentifier` because: -/// - The identifier defines "sameness" for deduplication and comparison with -/// recorded violations in `package_todo.yml`, which doesn't store line/column -/// - Multiple references to the same constant in the same file are considered -/// one violation, even if they occur at different lines +/// - The identifier defines "sameness" for comparison with recorded violations +/// in `package_todo.yml`, which doesn't store line/column +/// - Violations at different lines stay distinct, but those of the same type on +/// the same constant in the same file share an identifier, so one recorded +/// entry covers them all /// - Keeping line/column out of the identity makes violations stable across /// minor code movements that shift line numbers /// +/// `strict` is kept out of the identifier too: it says how to treat the +/// violation, not which violation it is, and `package_todo.yml` cannot record it. +/// /// Violations store only data - template expansion happens in formatters. #[derive(PartialEq, Clone, Eq, Hash, Debug)] pub struct Violation { pub identifier: ViolationIdentifier, + pub strict: bool, pub source_location: SourceLocation, // Additional data for template expansion: pub referencing_pack_relative_yml: String, @@ -156,10 +147,7 @@ impl<'a> CheckAllBuilder<'a> { self.found_violations .violations .iter() - .filter(|v| { - !recorded_violations - .contains(&v.identifier.recorded_key()) - }) + .filter(|v| !recorded_violations.contains(&v.identifier)) .collect() }; reportable_violations @@ -169,11 +157,11 @@ impl<'a> CheckAllBuilder<'a> { &mut self, recorded_violations: &'a HashSet, ) -> anyhow::Result> { - let found_violation_identifiers: HashSet = self + let found_violation_identifiers: HashSet<&ViolationIdentifier> = self .found_violations .violations .par_iter() - .map(|v| v.identifier.recorded_key()) + .map(|v| &v.identifier) .collect(); let relative_files = self .found_violations @@ -213,13 +201,9 @@ impl<'a> CheckAllBuilder<'a> { Ok(stale_violations) } - /// `found_violation_identifiers` is keyed by [`ViolationIdentifier::recorded_key`]. - /// `todo_violation_identifier` needs no such normalization: it comes from - /// `pack_set.all_violations`, which rebuilds every recorded violation with - /// `strict: false` already, so it is its own recorded key. fn is_stale_violation( relative_files: &HashSet<&str>, - found_violation_identifiers: &HashSet, + found_violation_identifiers: &HashSet<&ViolationIdentifier>, todo_violation_identifier: &ViolationIdentifier, ) -> bool { let violation_path_exists = @@ -242,11 +226,10 @@ impl<'a> CheckAllBuilder<'a> { self.found_violations .violations .iter() - .filter(|v| v.identifier.strict) + .filter(|v| v.strict) .filter(|v| { self.configuration.ignore_recorded_violations - || !recorded_violations - .contains(&v.identifier.recorded_key()) + || !recorded_violations.contains(&v.identifier) }) .cloned() .collect() @@ -346,8 +329,8 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { // as packwerk's `unlisted_strict_mode_violations`. let unlisted_strict_violations = &violations .iter() - .filter(|v| v.identifier.strict) - .filter(|v| !recorded_violations.contains(&v.identifier.recorded_key())) + .filter(|v| v.strict) + .filter(|v| !recorded_violations.contains(&v.identifier)) .collect::>(); if !unlisted_strict_violations.is_empty() { for violation in unlisted_strict_violations { diff --git a/src/packs/checker/common_test.rs b/src/packs/checker/common_test.rs index af53195c..44620679 100644 --- a/src/packs/checker/common_test.rs +++ b/src/packs/checker/common_test.rs @@ -46,12 +46,12 @@ pub mod tests { Violation { identifier: ViolationIdentifier { violation_type, - strict, file: String::from("packs/foo/app/services/foo.rb"), constant_name: String::from("::Bar"), referencing_pack_name: String::from("packs/foo"), defining_pack_name: String::from("packs/bar"), }, + strict, source_location: SourceLocation { line: 3, column: 1 }, referencing_pack_relative_yml: String::from( "packs/foo/package.yml", @@ -69,12 +69,12 @@ pub mod tests { Violation { identifier: ViolationIdentifier { violation_type, - strict, file: String::from("packs/foo/app/services/foo.rb"), constant_name, referencing_pack_name: String::from("packs/foo"), defining_pack_name: String::from("packs/bar"), }, + strict, source_location: SourceLocation { line: 3, column: 1 }, referencing_pack_relative_yml: String::from( "packs/foo/package.yml", diff --git a/src/packs/checker/pack_checker.rs b/src/packs/checker/pack_checker.rs index c99ef41f..72f2a6f2 100644 --- a/src/packs/checker/pack_checker.rs +++ b/src/packs/checker/pack_checker.rs @@ -163,6 +163,7 @@ impl<'a> PackChecker<'a> { }; Ok(Some(Violation { identifier: self.violation_identifier(), + strict: self.is_strict(), source_location: self.reference.source_location.clone(), referencing_pack_relative_yml: self .referencing_pack @@ -177,7 +178,6 @@ impl<'a> PackChecker<'a> { pub fn violation_identifier(&self) -> ViolationIdentifier { ViolationIdentifier { violation_type: self.checker_type.clone(), - strict: self.is_strict(), file: self.reference.relative_referencing_file.clone(), constant_name: self.reference.constant_name.clone(), referencing_pack_name: self.referencing_pack.name.clone(), diff --git a/src/packs/csv.rs b/src/packs/csv.rs index 5193141e..b0704353 100644 --- a/src/packs/csv.rs +++ b/src/packs/csv.rs @@ -9,7 +9,7 @@ use super::Configuration; /// Build message from violation using template expansion. /// For CSV, reference_location uses the default plain format. fn build_message(v: &Violation, config: &Configuration) -> String { - if v.identifier.strict { + if v.strict { build_strict_violation_message(&v.identifier) } else { let checker_config = @@ -50,7 +50,7 @@ pub fn write_csv( let message = build_message(violation, config); wtr.serialize(( identifier.violation_type.to_string(), - &identifier.strict, + &violation.strict, &identifier.file, &identifier.constant_name, &identifier.referencing_pack_name, diff --git a/src/packs/json.rs b/src/packs/json.rs index 228dfbc3..26127236 100644 --- a/src/packs/json.rs +++ b/src/packs/json.rs @@ -53,7 +53,7 @@ struct JsonSummary { /// Build message from violation using template expansion. /// For JSON, reference_location is cleared (location is in separate fields). fn build_message(v: &Violation, config: &Configuration) -> String { - if v.identifier.strict { + if v.strict { build_strict_violation_message(&v.identifier) } else { let checker_config = @@ -87,7 +87,7 @@ pub fn write_json( constant_name: &v.identifier.constant_name, referencing_pack_name: &v.identifier.referencing_pack_name, defining_pack_name: &v.identifier.defining_pack_name, - strict: v.identifier.strict, + strict: v.strict, message: build_message(v, config), }) .collect(); diff --git a/src/packs/pack.rs b/src/packs/pack.rs index 6c1fd2d6..f62a91d2 100644 --- a/src/packs/pack.rs +++ b/src/packs/pack.rs @@ -192,7 +192,6 @@ impl Pack { for file in &violation_group.files { let identifier = ViolationIdentifier { violation_type: violation_type.clone(), - strict: false, file: file.clone(), constant_name: constant_name.clone(), referencing_pack_name: self.name.clone(), @@ -793,7 +792,6 @@ ignored_private_constants: let expected = vec![ ViolationIdentifier { violation_type: CheckerType::Dependency, - strict: false, file: "packs/foo/app/services/foo.rb".to_string(), constant_name: "::Bar".to_string(), referencing_pack_name: "packs/foo".to_string(), @@ -801,7 +799,6 @@ ignored_private_constants: }, ViolationIdentifier { violation_type: CheckerType::Dependency, - strict: false, file: "packs/foo/app/services/other_foo.rb".to_string(), constant_name: "::Bar".to_string(), referencing_pack_name: "packs/foo".to_string(), diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 79dfce19..94afedb4 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -151,9 +151,8 @@ pub fn write_violations_to_disk( // build green and fail the next `check` with no source change in // between. packwerk keeps the entry for the same reason, in // `OffenseCollection#add_offense`. - if violation.identifier.strict - && !recorded_violations - .contains(&violation.identifier.recorded_key()) + if violation.strict + && !recorded_violations.contains(&violation.identifier) { continue; } diff --git a/src/packs/text.rs b/src/packs/text.rs index 97360987..45aab4cd 100644 --- a/src/packs/text.rs +++ b/src/packs/text.rs @@ -99,12 +99,12 @@ mod tests { Violation { identifier: ViolationIdentifier { violation_type: CheckerType::Privacy, - strict: false, file: "foo/bar/file.rb".to_string(), constant_name: "Foo".to_string(), referencing_pack_name: "bar".to_string(), defining_pack_name: "foo".to_string(), }, + strict: false, source_location: SourceLocation { line: 10, column: 5, diff --git a/tests/check_test.rs b/tests/check_test.rs index 7172ba26..a2315899 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -455,16 +455,15 @@ fn test_check_with_single_recorded_violation_type_in_strict_pack( // privacy and dependencies. Recording one type must tolerate only that type, // so the dependency violation still fails the run. // - // This pins `violation_type` inside the recorded-comparison key. A refactor - // that normalized it away, the way `strict` is normalized by - // `ViolationIdentifier::recorded_key`, would silence both types from a - // single-type entry. + // This pins `violation_type` inside `ViolationIdentifier`, the key compared + // with recorded violations. A refactor that moved it onto `Violation` + // alongside `strict` would silence both types from a single-type entry. // - // Measured, because the obvious way to say this overstates it: collapsing + // Measured, because the obvious way to say this overstates it: ignoring // `violation_type` in the strict filter *specifically* fails exactly this - // test and nothing else in the suite. Collapsing it everywhere in - // `recorded_key` fails eight tests, since it also breaks reportable and - // stale comparisons, so that coarser mutation proves nothing about this + // test and nothing else in the suite. Ignoring it in every comparison with + // the recorded set fails other tests too, since it also breaks reportable + // and stale comparisons, so that coarser mutation proves nothing about this // one. Every other strict fixture records privacy and dependency together, // which is why the narrow case needs its own coverage. cargo_bin_cmd!("pks") From 91c03cff6712bf495b2d1074a97aa988fe880492 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sun, 27 Sep 2026 00:03:43 -0700 Subject: [PATCH 2/2] Test update with both strict and non-strict violations Every strict fixture is strict throughout, so dropping the `strict` filter from `update`'s report of unlisted strict violations passed the whole suite, and dropping it from `write_violations_to_disk`'s skip was caught only by tests of ordinary non-strict recording. The new fixture has one unlisted strict violation and one ordinary one. The test pins `update`'s whole stdout and the `package_todo.yml` it writes, and fails under either mutation. --- .../package.yml | 0 .../packs/bar/app/services/bar.rb | 2 + .../packs/bar/package.yml | 1 + .../packs/baz/app/services/baz.rb | 2 + .../packs/baz/package.yml | 1 + .../packs/foo/app/services/foo.rb | 9 ++++ .../packs/foo/package.yml | 4 ++ .../packwerk.yml | 23 ++++++++ tests/update_test.rs | 53 +++++++++++++++++++ 9 files changed, 95 insertions(+) create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/package.yml create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/app/services/bar.rb create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/package.yml create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/app/services/baz.rb create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/package.yml create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/app/services/foo.rb create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/package.yml create mode 100644 tests/fixtures/contains_strict_and_non_strict_violations/packwerk.yml diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/package.yml b/tests/fixtures/contains_strict_and_non_strict_violations/package.yml new file mode 100644 index 00000000..e69de29b diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/app/services/bar.rb b/tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/app/services/bar.rb new file mode 100644 index 00000000..50031503 --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/package.yml b/tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/package.yml new file mode 100644 index 00000000..827dac40 --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/package.yml @@ -0,0 +1 @@ +enforce_privacy: strict diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/app/services/baz.rb b/tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/app/services/baz.rb new file mode 100644 index 00000000..dbe89a2b --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/app/services/baz.rb @@ -0,0 +1,2 @@ +module Baz +end diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/package.yml b/tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/package.yml new file mode 100644 index 00000000..a473d63d --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/package.yml @@ -0,0 +1 @@ +enforce_privacy: true diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/app/services/foo.rb b/tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/app/services/foo.rb new file mode 100644 index 00000000..e7dfbc51 --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/app/services/foo.rb @@ -0,0 +1,9 @@ +module Foo + def calls_bar + Bar + end + + def calls_baz + Baz + end +end diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/package.yml b/tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/package.yml new file mode 100644 index 00000000..10482952 --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/package.yml @@ -0,0 +1,4 @@ +enforce_dependencies: true +dependencies: + - packs/bar + - packs/baz diff --git a/tests/fixtures/contains_strict_and_non_strict_violations/packwerk.yml b/tests/fixtures/contains_strict_and_non_strict_violations/packwerk.yml new file mode 100644 index 00000000..51f2f3b4 --- /dev/null +++ b/tests/fixtures/contains_strict_and_non_strict_violations/packwerk.yml @@ -0,0 +1,23 @@ +# See: Setting up the configuration file +# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file + +# List of patterns for folder paths to include +# include: +# - "**/*.{rb,rake,erb}" + +# List of patterns for folder paths to exclude +# exclude: +# - "{bin,node_modules,script,tmp,vendor}/**/*" + +# Patterns to find package configuration files +# package_paths: "**/" + +# List of custom associations, if any +# custom_associations: +# - "cache_belongs_to" + +# Whether or not you want the cache enabled (disabled by default) +cache: false + +# Where you want the cache to be stored (default below) +# cache_directory: 'tmp/cache/packwerk' diff --git a/tests/update_test.rs b/tests/update_test.rs index 5ec0ecf5..ac25f8da 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -343,3 +343,56 @@ fn test_update_with_strict_violations() -> anyhow::Result<()> { ); Ok(()) } + +#[test] +// `packs/bar` enforces privacy strictly and `packs/baz` doesn't, so `foo.rb` +// has one unlisted strict violation and one ordinary one. Every other strict +// fixture is strict throughout, so dropping the `strict` filter from `update`'s +// report, or from the check `write_violations_to_disk` uses to skip unlisted +// strict violations, passed the rest of the suite. +fn test_update_with_strict_and_non_strict_violations() -> anyhow::Result<()> { + let fixture = + common::Fixture::new("contains_strict_and_non_strict_violations"); + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("update") + .assert() + .success() + .get_output() + .stdout + .clone(); + + assert_eq!( + String::from_utf8(output)?, + "\ +packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file +1 strict mode violation(s) detected. These violations must be fixed for `check` to succeed. +Successfully updated package_todo.yml files! +" + ); + + let actual = + std::fs::read_to_string(fixture.path("packs/foo/package_todo.yml"))?; + assert_eq!( + actual, + "\ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/baz: + \"::Baz\": + violations: + - privacy + files: + - packs/foo/app/services/foo.rb +" + ); + Ok(()) +}