Move strict from ViolationIdentifier onto Violation

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<ViolationIdentifier>,
     ) -> anyhow::Result<Vec<&'a ViolationIdentifier>> {
-        let found_violation_identifiers: HashSet<ViolationIdentifier> = 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<ViolationIdentifier>,
+        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::<Vec<&Violation>>();
     if !unlisted_strict_violations.is_empty() {
         for violation in unlisted_strict_violations {

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",

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(),

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<W: std::io::Write>(
             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,

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<W: std::io::Write>(
             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();

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,15 +792,13 @@ 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(),
                 defining_pack_name: "packs/bar".to_string(),
             },
             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(),

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;
         }

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,

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")

tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/app/services/bar.rb

@@ -0,0 +1,2 @@
+module Bar
+end

tests/fixtures/contains_strict_and_non_strict_violations/packs/bar/package.yml

@@ -0,0 +1 @@
+enforce_privacy: strict

tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/app/services/baz.rb

@@ -0,0 +1,2 @@
+module Baz
+end

tests/fixtures/contains_strict_and_non_strict_violations/packs/baz/package.yml

@@ -0,0 +1 @@
+enforce_privacy: true

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

tests/fixtures/contains_strict_and_non_strict_violations/packs/foo/package.yml

@@ -0,0 +1,4 @@
+enforce_dependencies: true
+dependencies:
+  - packs/bar
+  - packs/baz

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'

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(())
+}