Tolerate recorded violations in strict mode (Part B of #43)

CHANGELOG.md

@@ -4,6 +4,33 @@
 
 ### Breaking Changes
 
+#### Strict mode tolerates violations already recorded in `package_todo.yml`
+
+`enforce_privacy: strict` and `enforce_dependencies: strict` now fail only on
+references that are **not** already recorded in a `package_todo.yml`. This matches
+packwerk's `unlisted_strict_mode_violations` (Shopify/packwerk#368).
+
+**Who is affected:** any project with a strict pack whose existing violations are
+recorded in todo files. Previously `pks check` failed on every one of them, so a
+strict pack could only be green with an empty todo list.
+
+**What changes:** pks silently produces different (smaller) results without any
+configuration change. Strict packs that were red because of grandfathered
+violations go green. New references into a strict pack still fail, and `pks update`
+still refuses to record an unrecorded strict violation, so strict mode cannot be
+silenced by running it.
+
+**Opt out:** there is no config flag, matching packwerk. To see everything the todo
+files are grandfathering, run:
+
+```
+pks check --ignore-recorded-violations
+```
+
+## 0.4.0
+
+### Breaking Changes
+
 #### `respect_gitignore` defaults to `true`
 
 pks now respects `.gitignore` files by default. Files and directories matched by

CHECKERS.md

@@ -13,9 +13,11 @@ enforce_privacy: true
 
 Setting `enforce_privacy` to `true` will make all references to private constants in your package a violation.
 
-Setting `enforce_privacy` to `strict` will forbid all references to private constants in your package. **This includes violations that have been added to other packages' `package_todo.yml` files.**
+Setting `enforce_privacy` to `strict` will forbid *new* references to private constants in your package. **Violations already recorded in another package's `package_todo.yml` are tolerated**, so strict mode stops the list growing rather than requiring it to be empty.
 
-Note: You will need to remove all existing privacy violations before setting `enforce_privacy` to `strict`.
+Note: you do not need to remove existing privacy violations before setting `enforce_privacy` to `strict`. Turn it on, and any reference that is not already recorded will fail the check. To see everything the todo files are currently grandfathering, run `pks check --ignore-recorded-violations`.
+
+Running `pks update` will not silence strict mode either: an unrecorded strict violation is never written to a `package_todo.yml`, so it keeps failing until the reference itself is dealt with.
 
 ### Using public folders
 You may enforce privacy either way mentioned above and still expose a public API for your package by placing constants in the public folder, which by default is `app/public`. The constants in the public folder will be made available for use by the rest of the application.
@@ -99,8 +101,7 @@ end => Ideal solution. No exceptions from rubocop and very low risk of the magic
 Sometimes it is desirable to only enforce privacy on a subset of constants in a package. You can do so by defining a `private_constants` list in your package.yml. Note that `enforce_privacy` must be set to `true` or `'strict'` for this to work.
 
 ### Ignore strict mode for violation coming from specific path patterns
-If you want to activate `'strict'` mode on your package but have a few privacy violations you know you will deal with later,
-you can set a list of patterns to exclude.
+You do not need this to adopt `'strict'` mode on a package that already has violations you will deal with later: violations recorded in a `package_todo.yml` are tolerated by default. Reach for these patterns when you want to exempt a **path** instead of a recorded list.
 
 ```yaml
 enforce_privacy: strict
@@ -110,6 +111,8 @@ strict_privacy_ignored_patterns:
 
 In this example, violations on constants of your engine referenced in those files `engines/another_engine/test/**/*` will not fail Packwerk checks.
 
+The difference matters. A `package_todo.yml` entry grandfathers one `(constant, file)` pair, so a *new* reference from the same file still fails. A pattern here exempts the path outright, so anything those files reference later is ignored too. Prefer the todo file unless you genuinely want the whole path exempt.
+
 ### Package Privacy violation
 Packwerk thinks something is a privacy violation if you're referencing a constant, class, or module defined in the private implementation (i.e. not the public folder) of another package. We care about these because we want to make sure we only use parts of a package that have been exposed as public API.
 

src/packs/checker.rs

@@ -39,6 +39,20 @@ pub struct ViolationIdentifier {
     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 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:
@@ -124,7 +138,7 @@ impl<'a> CheckAllBuilder<'a> {
                 .cloned()
                 .collect(),
             strict_mode_violations: self
-                .build_strict_mode_violations()
+                .build_strict_mode_violations(recorded_violations)
                 .into_iter()
                 .collect(),
         })
@@ -142,7 +156,10 @@ impl<'a> CheckAllBuilder<'a> {
                 self.found_violations
                     .violations
                     .iter()
-                    .filter(|v| !recorded_violations.contains(&v.identifier))
+                    .filter(|v| {
+                        !recorded_violations
+                            .contains(&v.identifier.recorded_key())
+                    })
                     .collect()
             };
         reportable_violations
@@ -152,11 +169,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)
+            .map(|v| v.identifier.recorded_key())
             .collect();
         let relative_files = self
             .found_violations
@@ -196,9 +213,13 @@ 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 =
@@ -210,11 +231,23 @@ impl<'a> CheckAllBuilder<'a> {
         }
     }
 
-    fn build_strict_mode_violations(&self) -> Vec<Violation> {
+    /// Strict mode reports violations that are not already recorded in a
+    /// `package_todo.yml`, matching packwerk's `unlisted_strict_mode_violations`
+    /// (Shopify/packwerk#368). Turning strict on therefore blocks new violations
+    /// without also requiring every recorded one to be fixed first.
+    fn build_strict_mode_violations(
+        &self,
+        recorded_violations: &HashSet<ViolationIdentifier>,
+    ) -> Vec<Violation> {
         self.found_violations
             .violations
             .iter()
             .filter(|v| v.identifier.strict)
+            .filter(|v| {
+                self.configuration.ignore_recorded_violations
+                    || !recorded_violations
+                        .contains(&v.identifier.recorded_key())
+            })
             .cloned()
             .collect()
     }
@@ -302,22 +335,33 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> {
         &checkers,
     )?;
 
-    let strict_violations = &violations
+    let recorded_violations = &configuration.pack_set.all_violations;
+
+    // Only *unlisted* strict violations make `check` fail, so only those are
+    // worth reporting here. Reporting recorded ones too claimed `check` would
+    // fail when it succeeds. Same filter as `build_strict_mode_violations`, and
+    // 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()))
         .collect::<Vec<&Violation>>();
-    if !strict_violations.is_empty() {
-        for violation in strict_violations {
+    if !unlisted_strict_violations.is_empty() {
+        for violation in unlisted_strict_violations {
             let strict_message =
                 build_strict_violation_message(&violation.identifier);
             println!("{}", strict_message);
         }
         println!(
             "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.",
-            &strict_violations.len()
+            &unlisted_strict_violations.len()
         );
     }
-    package_todo::write_violations_to_disk(configuration, violations);
+    package_todo::write_violations_to_disk(
+        configuration,
+        violations,
+        recorded_violations,
+    );
     println!("Successfully updated package_todo.yml files!");
 
     Ok(())

src/packs/package_todo.rs

@@ -3,6 +3,7 @@ use serde::{ser::SerializeMap, Deserialize, Serialize, Serializer};
 use std::collections::{BTreeMap, HashMap, HashSet};
 use tracing::debug;
 
+use super::checker::ViolationIdentifier;
 use super::{pack::Pack, Configuration, Violation};
 
 #[derive(PartialEq, Debug, Eq, Deserialize, Serialize, Default, Clone)]
@@ -133,6 +134,7 @@ pub fn package_todos_for_pack_name(
 pub fn write_violations_to_disk(
     configuration: &Configuration,
     violations: HashSet<Violation>,
+    recorded_violations: &HashSet<ViolationIdentifier>,
 ) {
     debug!("Starting writing violations to disk");
     // First we need to group the violations by the responsible pack, which today is always the referencing pack
@@ -141,7 +143,18 @@ pub fn write_violations_to_disk(
     let mut violations_by_responsible_pack: HashMap<String, Vec<Violation>> =
         HashMap::new();
     for violation in violations {
-        if violation.identifier.strict {
+        // An *unlisted* strict violation is never recorded, so `update` cannot
+        // be used to silence strict mode. An already-recorded one has to be
+        // re-written, because `check` now tolerates recorded violations in
+        // strict packs and `PackageTodo` is dumped wholesale from these
+        // entries — dropping it here would delete the record that made the
+        // 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())
+        {
             continue;
         }
         let referencing_pack_name =

tests/check_test.rs

@@ -320,12 +320,34 @@ fn test_check_without_stale_violations() -> Result<(), Box<dyn Error>> {
 }
 
 #[test]
-fn test_check_with_strict_mode() -> Result<(), Box<dyn Error>> {
+fn test_check_with_recorded_strict_mode_violation() -> Result<(), Box<dyn Error>>
+{
+    // The violation is already recorded in packs/foo/package_todo.yml, so strict
+    // mode tolerates it and only blocks new ones. Matches packwerk's
+    // `unlisted_strict_mode_violations` (Shopify/packwerk#368).
     cargo_bin_cmd!("pks")
         .arg("--project-root")
         .arg("tests/fixtures/uses_strict_mode")
         .arg("check")
         .assert()
+        .code(0)
+        .stdout(predicate::str::contains("No violations detected!"));
+
+    common::teardown();
+    Ok(())
+}
+
+#[test]
+fn test_check_with_recorded_strict_mode_violation_ignoring_todo(
+) -> Result<(), Box<dyn Error>> {
+    // `--ignore-recorded-violations` is the escape hatch: it still surfaces
+    // everything the todo file is grandfathering.
+    cargo_bin_cmd!("pks")
+        .arg("--project-root")
+        .arg("tests/fixtures/uses_strict_mode")
+        .arg("check")
+        .arg("--ignore-recorded-violations")
+        .assert()
         .code(1)
         .stdout(predicate::str::contains(
             "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",
@@ -338,18 +360,74 @@ fn test_check_with_strict_mode() -> Result<(), Box<dyn Error>> {
     Ok(())
 }
 
+#[test]
+fn test_check_with_unrecorded_strict_mode_violation(
+) -> Result<(), Box<dyn Error>> {
+    // No package_todo.yml entry for this one, so strict mode must still fail.
+    cargo_bin_cmd!("pks")
+        .arg("--project-root")
+        .arg("tests/fixtures/contains_strict_violations")
+        .arg("check")
+        .assert()
+        .code(1)
+        .stdout(predicate::str::contains(
+            "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",
+        ));
+
+    common::teardown();
+    Ok(())
+}
+
+#[test]
+fn test_check_with_partially_recorded_strict_mode_violations(
+) -> Result<(), Box<dyn Error>> {
+    // The case that makes strict mode adoptable, and the one nothing else
+    // covers: one recorded violation (::Bar) and one unrecorded (::Baz) in the
+    // same strict pack, in the same run. Only the unrecorded one is reported,
+    // and the run still fails because of it.
+    cargo_bin_cmd!("pks")
+        .arg("--project-root")
+        .arg("tests/fixtures/uses_strict_mode_partially_recorded")
+        .arg("check")
+        .assert()
+        .code(1)
+        .stdout(predicate::str::contains(
+            "packs/foo cannot have privacy violations on packs/baz because strict mode is enabled for privacy violations in the enforcing pack's package.yml file",
+        ))
+        .stdout(predicate::str::contains(
+            "packs/foo cannot have dependency violations on packs/baz because strict mode is enabled for dependency violations in the enforcing pack's package.yml file",
+        ))
+        .stdout(predicate::str::contains("::Baz"))
+        // The recorded one stays silent: no strict message, no new-violation
+        // report, no stale-todo line.
+        .stdout(predicate::str::contains("packs/bar").not())
+        .stdout(predicate::str::contains("::Bar").not())
+        .stdout(
+            predicate::str::contains(
+                "There were stale violations found, please run `packs update`",
+            )
+            .not(),
+        );
+
+    common::teardown();
+    Ok(())
+}
+
 #[test]
 fn test_check_with_strict_mode_output_csv() -> Result<(), Box<dyn Error>> {
+    // Uses `contains_strict_violations` rather than `uses_strict_mode`: the
+    // latter's violation is recorded, so there is nothing left to assert against
+    // in the CSV. The duplicate assertion this used to carry was byte-identical
+    // to the one below it, so nothing is lost by dropping it.
     cargo_bin_cmd!("pks")
         .arg("--project-root")
-        .arg("tests/fixtures/uses_strict_mode")
+        .arg("tests/fixtures/contains_strict_violations")
         .arg("check")
         .arg("-o")
         .arg("csv")
         .assert()
         .code(1)
         .stdout(predicate::str::contains("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message"))
-        .stdout(predicate::str::contains("privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,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"))
         .stdout(predicate::str::contains(
             "privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,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",
         ));

tests/common/mod.rs

@@ -55,6 +55,38 @@ pub fn delete_foobar_app_with_custom_readme() {
     }
 }
 
+// Restores the round-trip strict-mode fixture. Its todo file records a strict
+// violation, which is the state `check` tolerance depends on, so any test that
+// runs `update` against it has to put it back.
+#[allow(dead_code)]
+pub fn set_up_uses_strict_mode_round_trip_fixture() {
+    let package_todo = String::from(
+        "\
+# 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/bar:
+  \"::Bar\":
+    violations:
+    - privacy
+    - dependency
+    files:
+    - packs/foo/app/services/foo.rb
+",
+    );
+
+    fs::write(
+        "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml",
+        package_todo,
+    )
+    .unwrap();
+}
+
 // In case we want our tests to call `update` or otherwise mutate the file system
 #[allow(dead_code)]
 pub fn set_up_fixtures() {

tests/fixtures/uses_strict_mode_partially_recorded/package.yml

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

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

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

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

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