Detect deleted files as stale violations

lib/packwerk/formatters/default_offenses_formatter.rb

@@ -20,9 +20,9 @@ def show_offenses(offenses)
         EOS
       end
 
-      sig { override.params(offense_collection: OffenseCollection, fileset: T::Set[String]).returns(String) }
-      def show_stale_violations(offense_collection, fileset)
-        if offense_collection.stale_violations?(fileset)
+      sig { override.params(offense_collection: OffenseCollection, file_set: T::Set[String]).returns(String) }
+      def show_stale_violations(offense_collection, file_set)
+        if offense_collection.stale_violations?(file_set)
           "There were stale violations found, please run `packwerk update-todo`"
         else
           "No stale violations detected"

lib/packwerk/package_todo.rb

@@ -7,24 +7,28 @@ module Packwerk
   class PackageTodo
     extend T::Sig
 
-    EntriesType = T.type_alias do
-      T::Hash[String, T.untyped]
+    PackageName = T.type_alias { String }
+    ConstantName = T.type_alias { String }
+    FilePath = T.type_alias { String }
+    Entry = T.type_alias { T::Hash[ConstantName, T::Hash[ConstantName, T::Array[FilePath]]] }
+    Entries = T.type_alias do
+      T::Hash[PackageName, Entry]
     end
 
-    sig { params(package: Packwerk::Package, filepath: String).void }
-    def initialize(package, filepath)
+    sig { params(package: Packwerk::Package, path: String).void }
+    def initialize(package, path)
       @package = package
-      @filepath = filepath
-      @new_entries = T.let({}, EntriesType)
-      @todo_list = T.let(nil, T.nilable(EntriesType))
+      @path = path
+      @new_entries = T.let({}, Entries)
+      @old_entries = T.let(nil, T.nilable(Entries))
     end
 
     sig do
       params(reference: Packwerk::Reference, violation_type: String)
         .returns(T::Boolean)
     end
     def listed?(reference, violation_type:)
-      violated_constants_found = todo_list.dig(reference.constant.package.name, reference.constant.name)
+      violated_constants_found = old_entries.dig(reference.constant.package.name, reference.constant.name)
       return false unless violated_constants_found
 
       violated_constant_in_file = violated_constants_found.fetch("files", []).include?(reference.relative_path)
@@ -37,61 +41,39 @@ def listed?(reference, violation_type:)
       params(reference: Packwerk::Reference, violation_type: String).returns(T::Boolean)
     end
     def add_entries(reference, violation_type)
-      package_violations = @new_entries.fetch(reference.constant.package.name, {})
+      package_violations = new_entries.fetch(reference.constant.package.name, {})
       entries_for_constant = package_violations[reference.constant.name] ||= {}
 
       entries_for_constant["violations"] ||= []
-      entries_for_constant["violations"] << violation_type
+      entries_for_constant.fetch("violations") << violation_type
 
       entries_for_constant["files"] ||= []
-      entries_for_constant["files"] << reference.relative_path.to_s
+      entries_for_constant.fetch("files") << reference.relative_path.to_s
 
-      @new_entries[reference.constant.package.name] = package_violations
+      new_entries[reference.constant.package.name] = package_violations
       listed?(reference, violation_type: violation_type)
     end
 
     sig { params(for_files: T::Set[String]).returns(T::Boolean) }
     def stale_violations?(for_files)
       prepare_entries_for_dump
 
-      todo_list.any? do |package, package_violations|
-        package_violations_for_files = {}
-        package_violations.each do |constant_name, entries_for_constant|
-          entries_for_files = for_files & entries_for_constant["files"]
-          next if entries_for_files.none?
-
-          package_violations_for_files[constant_name] = {
-            "violations" => entries_for_constant["violations"],
-            "files" => entries_for_files.to_a,
-          }
-        end
+      old_entries.any? do |package, violations|
+        files = for_files + deleted_files_for(package)
+        violations_for_files = package_violations_for(violations, files: files)
 
         # We `next false` because if we cannot find existing violations for `for_files` within
         # the `package_todo.yml` file, then there are no violations that
         # can be considered stale.
-        next false if package_violations_for_files.empty?
-
-        package_violations_for_files.any? do |constant_name, entries_for_constant|
-          new_entries_violation_types = @new_entries.dig(package, constant_name, "violations")
-          # If there are no NEW entries that match the old entries `for_files`,
-          # @new_entries is from the list of violations we get when we check this file.
-          # If this list is empty, we also must have stale violations.
-          next true if new_entries_violation_types.nil?
-
-          if entries_for_constant["violations"].all? { |type| new_entries_violation_types.include?(type) }
-            stale_violations =
-              entries_for_constant["files"] - Array(@new_entries.dig(package, constant_name, "files"))
-            stale_violations.any?
-          else
-            return true
-          end
-        end
+        next false if violations_for_files.empty?
+
+        stale_violation_for_package?(package, violations: violations_for_files)
       end
     end
 
     sig { void }
     def dump
-      if @new_entries.empty?
+      if new_entries.empty?
         delete_if_exists
       else
         prepare_entries_for_dump
@@ -104,45 +86,90 @@ def dump
           #
           # bin/packwerk update-todo
         MESSAGE
-        File.open(@filepath, "w") do |f|
+        File.open(@path, "w") do |f|
           f.write(message)
-          f.write(@new_entries.to_yaml)
+          f.write(new_entries.to_yaml)
         end
       end
     end
 
     sig { void }
     def delete_if_exists
-      File.delete(@filepath) if File.exist?(@filepath)
+      File.delete(@path) if File.exist?(@path)
     end
 
     private
 
-    sig { returns(EntriesType) }
+    sig { returns(Entries) }
+    attr_reader(:new_entries)
+
+    sig { params(package: String).returns(T::Array[String]) }
+    def deleted_files_for(package)
+      old_files = old_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
+      new_files = new_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
+      old_files - new_files
+    end
+
+    sig { params(package: String, violations: Entry).returns(T::Boolean) }
+    def stale_violation_for_package?(package, violations:)
+      violations.any? do |constant_name, entries_for_constant|
+        new_entries_violation_types = T.cast(
+          new_entries.dig(package, constant_name, "violations"),
+          T.nilable(T::Array[String]),
+        )
+        # If there are no NEW entries that match the old entries `for_files`,
+        # new_entries is from the list of violations we get when we check this file.
+        # If this list is empty, we also must have stale violations.
+        next true if new_entries_violation_types.nil?
+
+        if entries_for_constant.fetch("violations").all? { |type| new_entries_violation_types.include?(type) }
+          stale_violations =
+            entries_for_constant.fetch("files") - Array(new_entries.dig(package, constant_name, "files"))
+          stale_violations.any?
+        else
+          return true
+        end
+      end
+    end
+
+    sig { params(package_violations: Entry, files: T::Set[String]).returns(Entry) }
+    def package_violations_for(package_violations, files:)
+      {}.tap do |package_violations_for_files|
+        package_violations_for_files = T.cast(package_violations_for_files, Entry)
+
+        package_violations.each do |constant_name, entries_for_constant|
+          entries_for_files = files & entries_for_constant.fetch("files")
+          next if entries_for_files.none?
+
+          package_violations_for_files[constant_name] = {
+            "violations" => entries_for_constant["violations"],
+            "files" => entries_for_files.to_a,
+          }
+        end
+      end
+    end
+
+    sig { returns(Entries) }
     def prepare_entries_for_dump
-      @new_entries.each do |package_name, package_violations|
+      new_entries.each do |package_name, package_violations|
         package_violations.each do |_, entries_for_constant|
-          entries_for_constant["violations"].sort!.uniq!
-          entries_for_constant["files"].sort!.uniq!
+          entries_for_constant.fetch("violations").sort!.uniq!
+          entries_for_constant.fetch("files").sort!.uniq!
         end
-        @new_entries[package_name] = package_violations.sort.to_h
+        new_entries[package_name] = package_violations.sort.to_h
       end
 
-      @new_entries = @new_entries.sort.to_h
+      @new_entries = new_entries.sort.to_h
     end
 
-    sig { returns(EntriesType) }
-    def todo_list
-      @todo_list ||= if File.exist?(@filepath)
-        load_yaml(@filepath)
-      else
-        {}
-      end
+    sig { returns(Entries) }
+    def old_entries
+      @old_entries ||= load_yaml_file(@path)
     end
 
-    sig { params(filepath: String).returns(EntriesType) }
-    def load_yaml(filepath)
-      YAML.load_file(filepath) || {}
+    sig { params(path: String).returns(Entries) }
+    def load_yaml_file(path)
+      File.exist?(path) && YAML.load_file(path) || {}
     rescue Psych::Exception
       {}
     end

test/fixtures/package_todo.yml

@@ -5,4 +5,3 @@ buyers:
     - some_checker_type
     files:
     - orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb
-    - orders/app/models/orders/services/adjustment_engine.rb

test/unit/packwerk/offense_collection_test.rb

@@ -27,19 +27,23 @@ class OffenseCollectionTest < Minitest::Test
 
     test "#stale_violations? returns true if there are stale violations" do
       @offense_collection.add_offense(@offense)
+      file_set = Set.new
 
       Packwerk::PackageTodo.any_instance
         .expects(:stale_violations?)
+        .with(file_set)
         .returns(true)
 
-      assert @offense_collection.stale_violations?(Set.new)
+      assert @offense_collection.stale_violations?(file_set)
     end
 
     test "#stale_violations? returns false if no stale violations" do
       @offense_collection.add_offense(@offense)
+      file_set = Set.new
 
       Packwerk::PackageTodo.any_instance
         .expects(:stale_violations?)
+        .with(file_set)
         .returns(false)
 
       refute @offense_collection.stale_violations?(Set.new)

test/unit/packwerk/package_todo_test.rb

@@ -123,6 +123,15 @@ class PackageTodoTest < Minitest::Test
       )
     end
 
+    test "#stale_violations? returns true when deleted files are present" do
+      package = Package.new(name: "buyers", config: { "enforce_dependencies" => true })
+
+      package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml")
+      assert package_todo.stale_violations?(
+        Set.new(["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"])
+      )
+    end
+
     test "#listed? returns false if constant is not violated" do
       reference = build_reference(destination_package: destination_package)
       package_todo = PackageTodo.new(destination_package, "test/fixtures/package_todo.yml")

test/unit/packwerk/parse_run_test.rb

@@ -328,11 +328,11 @@ def something
 
       expected_message = <<~EOS
         No offenses detected
-        No stale violations detected
+        There were stale violations found, please run `packwerk update-todo`
       EOS
       assert_equal expected_message, result.message
 
-      assert result.status
+      refute result.status
     end
 
     test "#check lists out violations of strict mode" do
@@ -435,7 +435,7 @@ def something
 
         1 offense detected
 
-        No stale violations detected
+        There were stale violations found, please run `packwerk update-todo`
       EOS
 
       assert_equal expected_message, result.message