Merge pull request #147 from Shopify/yuta/simplify-orchestration

Refactor - simplify orchestration layer

- Extract checkers into its own namespace / module.
- Decouple reference extraction from offence checks.

lib/packwerk.rb

@@ -12,13 +12,11 @@ module Packwerk
   autoload :ApplicationValidator
   autoload :AssociationInspector
   autoload :OffenseCollection
-  autoload :Checker
   autoload :Cli
   autoload :Configuration
   autoload :ConstantDiscovery
   autoload :ConstantNameInspector
   autoload :ConstNodeInspector
-  autoload :DependencyChecker
   autoload :DeprecatedReferences
   autoload :FileProcessor
   autoload :FilesForProcessing
@@ -36,7 +34,6 @@ module Packwerk
   autoload :ParsedConstantDefinitions
   autoload :Parsers
   autoload :ParseRun
-  autoload :PrivacyChecker
   autoload :Reference
   autoload :ReferenceExtractor
   autoload :ReferenceOffense
@@ -77,4 +74,18 @@ module Generators
     autoload :InflectionsFile
     autoload :RootPackage
   end
+
+  module ReferenceChecking
+    extend ActiveSupport::Autoload
+
+    autoload :ReferenceChecker
+
+    module Checkers
+      extend ActiveSupport::Autoload
+
+      autoload :Checker
+      autoload :DependencyChecker
+      autoload :PrivacyChecker
+    end
+  end
 end

lib/packwerk/file_processor.rb

@@ -1,10 +1,12 @@
-# typed: false
+# typed: true
 # frozen_string_literal: true
 
 require "ast/node"
 
 module Packwerk
   class FileProcessor
+    extend T::Sig
+
     class UnknownFileTypeResult < Offense
       def initialize(file:)
         super(file: file, message: "unknown file type")
@@ -16,24 +18,47 @@ def initialize(node_processor_factory:, parser_factory: nil)
       @parser_factory = parser_factory || Packwerk::Parsers::Factory.instance
     end
 
+    sig do
+      params(file_path: String).returns(
+        T::Array[
+          T.any(
+            Packwerk::Reference,
+            Packwerk::Offense,
+          )
+        ]
+      )
+    end
     def call(file_path)
-      parser = @parser_factory.for_path(file_path)
-      return [UnknownFileTypeResult.new(file: file_path)] if parser.nil?
+      return [UnknownFileTypeResult.new(file: file_path)] if parser_for(file_path).nil?
 
-      node = File.open(file_path, "r", external_encoding: Encoding::UTF_8) do |file|
-        parser.call(io: file, file_path: file_path)
-      rescue Parsers::ParseError => e
-        return [e.result]
-      end
+      node = parse_into_ast(file_path)
+      return [] unless node
+
+      references_from_ast(node, file_path)
+    rescue Parsers::ParseError => e
+      [e.result]
+    end
+
+    private
 
-      result = []
-      if node
-        node_processor = @node_processor_factory.for(filename: file_path, node: node)
-        node_visitor = Packwerk::NodeVisitor.new(node_processor: node_processor)
+    def references_from_ast(node, file_path)
+      references = []
 
-        node_visitor.visit(node, ancestors: [], result: result)
+      node_processor = @node_processor_factory.for(filename: file_path, node: node)
+      node_visitor = Packwerk::NodeVisitor.new(node_processor: node_processor)
+      node_visitor.visit(node, ancestors: [], result: references)
+
+      references
+    end
+
+    def parse_into_ast(file_path)
+      File.open(file_path, "r", nil, external_encoding: Encoding::UTF_8) do |file|
+        parser_for(file_path).call(io: file, file_path: file_path)
       end
-      result
+    end
+
+    def parser_for(file_path)
+      @parser_factory.for_path(file_path)
     end
   end
 end

lib/packwerk/node_processor.rb

@@ -9,35 +9,22 @@ class NodeProcessor
       params(
         reference_extractor: ReferenceExtractor,
         filename: String,
-        checkers: T::Array[Checker]
       ).void
     end
-    def initialize(reference_extractor:, filename:, checkers:)
+    def initialize(reference_extractor:, filename:)
       @reference_extractor = reference_extractor
       @filename = filename
-      @checkers = checkers
     end
 
-    sig { params(node: Parser::AST::Node, ancestors: T::Array[Parser::AST::Node]).returns(T::Array[Offense]) }
-    def call(node, ancestors)
-      return [] unless Node.method_call?(node) || Node.constant?(node)
-      reference = @reference_extractor.reference_from_node(node, ancestors: ancestors, file_path: @filename)
-      check_reference(reference, node)
+    sig do
+      params(
+        node: Parser::AST::Node,
+        ancestors: T::Array[Parser::AST::Node]
+      ).returns(T.nilable(Packwerk::Reference))
     end
-
-    private
-
-    def check_reference(reference, node)
-      return [] unless reference
-      @checkers.each_with_object([]) do |checker, violations|
-        next unless checker.invalid_reference?(reference)
-        offense = Packwerk::ReferenceOffense.new(
-          location: Node.location(node),
-          reference: reference,
-          violation_type: checker.violation_type
-        )
-        violations << offense
-      end
+    def call(node, ancestors)
+      return unless Node.method_call?(node) || Node.constant?(node)
+      @reference_extractor.reference_from_node(node, ancestors: ancestors, file_path: @filename)
     end
   end
 end

lib/packwerk/node_processor_factory.rb

@@ -8,14 +8,12 @@ class NodeProcessorFactory < T::Struct
     const :root_path, String
     const :context_provider, Packwerk::ConstantDiscovery
     const :constant_name_inspectors, T::Array[ConstantNameInspector]
-    const :checkers, T::Array[Checker]
 
     sig { params(filename: String, node: AST::Node).returns(NodeProcessor) }
     def for(filename:, node:)
       ::Packwerk::NodeProcessor.new(
         reference_extractor: reference_extractor(node: node),
         filename: filename,
-        checkers: checkers,
       )
     end
 

lib/packwerk/node_visitor.rb

@@ -8,7 +8,8 @@ def initialize(node_processor:)
     end
 
     def visit(node, ancestors:, result:)
-      result.concat(@node_processor.call(node, ancestors))
+      reference = @node_processor.call(node, ancestors)
+      result << reference if reference
 
       child_ancestors = [node] + ancestors
       Node.each_child(node) do |child|

lib/packwerk/reference.rb

@@ -2,5 +2,5 @@
 # frozen_string_literal: true
 
 module Packwerk
-  Reference = Struct.new(:source_package, :relative_path, :constant)
+  Reference = Struct.new(:source_package, :relative_path, :constant, :source_location)
 end

lib/packwerk/reference_checking/checkers/checker.rb

@@ -0,0 +1,21 @@
+# typed: true
+# frozen_string_literal: true
+
+module Packwerk
+  module ReferenceChecking
+    module Checkers
+      module Checker
+        extend T::Sig
+        extend T::Helpers
+
+        interface!
+
+        sig { returns(ViolationType).abstract }
+        def violation_type; end
+
+        sig { params(reference: Reference).returns(T::Boolean).abstract }
+        def invalid_reference?(reference); end
+      end
+    end
+  end
+end

lib/packwerk/reference_checking/checkers/dependency_checker.rb

@@ -0,0 +1,30 @@
+# typed: strict
+# frozen_string_literal: true
+
+module Packwerk
+  module ReferenceChecking
+    module Checkers
+      class DependencyChecker
+        extend T::Sig
+        include Checker
+
+        sig { override.returns(ViolationType) }
+        def violation_type
+          ViolationType::Dependency
+        end
+
+        sig do
+          override
+            .params(reference: Packwerk::Reference)
+            .returns(T::Boolean)
+        end
+        def invalid_reference?(reference)
+          return false unless reference.source_package
+          return false unless reference.source_package.enforce_dependencies?
+          return false if reference.source_package.dependency?(reference.constant.package)
+          true
+        end
+      end
+    end
+  end
+end