diff --git a/lib/packwerk.rb b/lib/packwerk.rb index eedaf4734..89c439979 100644 --- a/lib/packwerk.rb +++ b/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 diff --git a/lib/packwerk/checker.rb b/lib/packwerk/checker.rb deleted file mode 100644 index 78ace63c0..000000000 --- a/lib/packwerk/checker.rb +++ /dev/null @@ -1,17 +0,0 @@ -# typed: true -# frozen_string_literal: true - -module Packwerk - 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 diff --git a/lib/packwerk/dependency_checker.rb b/lib/packwerk/dependency_checker.rb deleted file mode 100644 index f96fb8058..000000000 --- a/lib/packwerk/dependency_checker.rb +++ /dev/null @@ -1,26 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -module Packwerk - 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 diff --git a/lib/packwerk/file_processor.rb b/lib/packwerk/file_processor.rb index 4b9f2023c..2ccccd808 100644 --- a/lib/packwerk/file_processor.rb +++ b/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 diff --git a/lib/packwerk/node_processor.rb b/lib/packwerk/node_processor.rb index 26d4fb212..18da923ab 100644 --- a/lib/packwerk/node_processor.rb +++ b/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 diff --git a/lib/packwerk/node_processor_factory.rb b/lib/packwerk/node_processor_factory.rb index 0a467c290..e29f616d8 100644 --- a/lib/packwerk/node_processor_factory.rb +++ b/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 diff --git a/lib/packwerk/node_visitor.rb b/lib/packwerk/node_visitor.rb index 5ab20216d..cc1527eba 100644 --- a/lib/packwerk/node_visitor.rb +++ b/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| diff --git a/lib/packwerk/privacy_checker.rb b/lib/packwerk/privacy_checker.rb deleted file mode 100644 index 23eadc59b..000000000 --- a/lib/packwerk/privacy_checker.rb +++ /dev/null @@ -1,53 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -module Packwerk - class PrivacyChecker - extend T::Sig - include Checker - - sig { override.returns(Packwerk::ViolationType) } - def violation_type - ViolationType::Privacy - end - - sig do - override - .params(reference: Packwerk::Reference) - .returns(T::Boolean) - end - def invalid_reference?(reference) - return false if reference.constant.public? - - privacy_option = reference.constant.package.enforce_privacy - return false if enforcement_disabled?(privacy_option) - - return false unless privacy_option == true || - explicitly_private_constant?(reference.constant, explicitly_private_constants: privacy_option) - - true - end - - private - - sig do - params( - constant: ConstantDiscovery::ConstantContext, - explicitly_private_constants: T::Array[String] - ).returns(T::Boolean) - end - def explicitly_private_constant?(constant, explicitly_private_constants:) - explicitly_private_constants.include?(constant.name) || - # nested constants - explicitly_private_constants.any? { |epc| constant.name.start_with?(epc + "::") } - end - - sig do - params(privacy_option: T.nilable(T.any(T::Boolean, T::Array[String]))) - .returns(T::Boolean) - end - def enforcement_disabled?(privacy_option) - [false, nil].include?(privacy_option) - end - end -end diff --git a/lib/packwerk/reference.rb b/lib/packwerk/reference.rb index 0155f4580..b47b4490f 100644 --- a/lib/packwerk/reference.rb +++ b/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 diff --git a/lib/packwerk/reference_checking/checkers/checker.rb b/lib/packwerk/reference_checking/checkers/checker.rb new file mode 100644 index 000000000..590763baa --- /dev/null +++ b/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 diff --git a/lib/packwerk/reference_checking/checkers/dependency_checker.rb b/lib/packwerk/reference_checking/checkers/dependency_checker.rb new file mode 100644 index 000000000..f44c7aed2 --- /dev/null +++ b/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 diff --git a/lib/packwerk/reference_checking/checkers/privacy_checker.rb b/lib/packwerk/reference_checking/checkers/privacy_checker.rb new file mode 100644 index 000000000..f870b213d --- /dev/null +++ b/lib/packwerk/reference_checking/checkers/privacy_checker.rb @@ -0,0 +1,57 @@ +# typed: strict +# frozen_string_literal: true + +module Packwerk + module ReferenceChecking + module Checkers + class PrivacyChecker + extend T::Sig + include Checker + + sig { override.returns(Packwerk::ViolationType) } + def violation_type + ViolationType::Privacy + end + + sig do + override + .params(reference: Packwerk::Reference) + .returns(T::Boolean) + end + def invalid_reference?(reference) + return false if reference.constant.public? + + privacy_option = reference.constant.package.enforce_privacy + return false if enforcement_disabled?(privacy_option) + + return false unless privacy_option == true || + explicitly_private_constant?(reference.constant, explicitly_private_constants: privacy_option) + + true + end + + private + + sig do + params( + constant: ConstantDiscovery::ConstantContext, + explicitly_private_constants: T::Array[String] + ).returns(T::Boolean) + end + def explicitly_private_constant?(constant, explicitly_private_constants:) + explicitly_private_constants.include?(constant.name) || + # nested constants + explicitly_private_constants.any? { |epc| constant.name.start_with?(epc + "::") } + end + + sig do + params(privacy_option: T.nilable(T.any(T::Boolean, T::Array[String]))) + .returns(T::Boolean) + end + def enforcement_disabled?(privacy_option) + [false, nil].include?(privacy_option) + end + end + end + end +end diff --git a/lib/packwerk/reference_checking/reference_checker.rb b/lib/packwerk/reference_checking/reference_checker.rb new file mode 100644 index 000000000..4e4863165 --- /dev/null +++ b/lib/packwerk/reference_checking/reference_checker.rb @@ -0,0 +1,33 @@ +# typed: true +# frozen_string_literal: true + +module Packwerk + module ReferenceChecking + class ReferenceChecker + extend T::Sig + + def initialize(checkers) + @checkers = checkers + end + + sig do + params( + reference: T.any(Packwerk::Reference, Packwerk::Offense) + ).returns(T::Array[Packwerk::Offense]) + end + def call(reference) + return [reference] if reference.is_a?(Packwerk::Offense) + + @checkers.each_with_object([]) do |checker, violations| + next unless checker.invalid_reference?(reference) + offense = Packwerk::ReferenceOffense.new( + location: reference.source_location, + reference: reference, + violation_type: checker.violation_type + ) + violations << offense + end + end + end + end +end diff --git a/lib/packwerk/reference_extractor.rb b/lib/packwerk/reference_extractor.rb index 7df132c3d..532529bae 100644 --- a/lib/packwerk/reference_extractor.rb +++ b/lib/packwerk/reference_extractor.rb @@ -59,7 +59,7 @@ def reference_from_constant(constant_name, node:, ancestors:, file_path:) return if source_package == constant.package - Reference.new(source_package, relative_path, constant) + Reference.new(source_package, relative_path, constant, Node.location(node)) end def local_reference?(constant_name, name_location, namespace_path) diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 6d5179d7d..8f36345b0 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -17,8 +17,8 @@ class RunContext ) DEFAULT_CHECKERS = [ - ::Packwerk::DependencyChecker, - ::Packwerk::PrivacyChecker, + ::Packwerk::ReferenceChecking::Checkers::DependencyChecker, + ::Packwerk::ReferenceChecking::Checkers::PrivacyChecker, ] class << self @@ -50,9 +50,12 @@ def initialize( @checker_classes = checker_classes end - sig { params(file: String).returns(T::Array[T.nilable(::Packwerk::Offense)]) } + sig { params(file: String).returns(T::Array[Packwerk::Offense]) } def process_file(file:) - file_processor.call(file) + references = file_processor.call(file) + + reference_checker = ReferenceChecking::ReferenceChecker.new(checkers) + references.flat_map { |reference| reference_checker.call(reference) } end private @@ -66,7 +69,6 @@ def file_processor def node_processor_factory NodeProcessorFactory.new( context_provider: context_provider, - checkers: checkers, root_path: root_path, constant_name_inspectors: constant_name_inspectors ) @@ -94,7 +96,7 @@ def package_set ::Packwerk::PackageSet.load_all_from(root_path, package_pathspec: package_paths) end - sig { returns(T::Array[Checker]) } + sig { returns(T::Array[ReferenceChecking::Checkers::Checker]) } def checkers checker_classes.map(&:new) end diff --git a/test/support/factory_helper.rb b/test/support/factory_helper.rb index 160d71bff..97a78a831 100644 --- a/test/support/factory_helper.rb +++ b/test/support/factory_helper.rb @@ -7,7 +7,8 @@ def build_reference( destination_package: Packwerk::Package.new(name: "components/destination", config: {}), path: "some/path.rb", constant_name: "::SomeName", - public_constant: false + public_constant: false, + source_location: Packwerk::Node::Location.new(2, 12) ) constant = Packwerk::ConstantDiscovery::ConstantContext.new( constant_name, @@ -15,6 +16,6 @@ def build_reference( destination_package, public_constant ) - Packwerk::Reference.new(source_package, path, constant) + Packwerk::Reference.new(source_package, path, constant, source_location) end end diff --git a/test/unit/dependency_checker_test.rb b/test/unit/dependency_checker_test.rb index ebcd11046..79b2737d5 100644 --- a/test/unit/dependency_checker_test.rb +++ b/test/unit/dependency_checker_test.rb @@ -37,7 +37,7 @@ class DependencyCheckerTest < Minitest::Test private def dependency_checker - DependencyChecker.new + ReferenceChecking::Checkers::DependencyChecker.new end end end diff --git a/test/unit/file_processor_test.rb b/test/unit/file_processor_test.rb index a0568cb79..7a945d14e 100644 --- a/test/unit/file_processor_test.rb +++ b/test/unit/file_processor_test.rb @@ -5,52 +5,52 @@ module Packwerk class FileProcessorTest < Minitest::Test + include FactoryHelper + setup do @node_processor_factory = mock @node_processor = mock @file_processor = ::Packwerk::FileProcessor.new(node_processor_factory: @node_processor_factory) end - test "#call visits all nodes in a file path with no offenses" do + test "#call visits all nodes in a file path with no references" do @node_processor_factory.expects(:for).returns(@node_processor) - @node_processor.expects(:call).twice.returns([]) + @node_processor.expects(:call).twice.returns(nil) - offenses = tempfile(name: "foo", content: "def food_bar; end") do |file_path| + references = tempfile(name: "foo", content: "def food_bar; end") do |file_path| @file_processor.call(file_path) end - assert_empty offenses + assert_empty references end - test "#call visits a node in file path with an offense" do - location = mock - location.stubs(line: 3, column: 22) + test "#call visits a node in file path with an reference" do + reference = build_reference(path: "tempfile", source_location: Packwerk::Node::Location.new(3, 22)) - offense = stub(location: location, file: "tempfile", message: "Use of unassigned variable") @node_processor_factory.expects(:for).returns(@node_processor) - @node_processor.expects(:call).returns([offense]) + @node_processor.expects(:call).returns(reference) - offenses = tempfile(name: "foo", content: "a_variable_name") do |file_path| + references = tempfile(name: "foo", content: "a_variable_name") do |file_path| @file_processor.call(file_path) end - assert_equal 1, offenses.length + assert_equal 1, references.length + + reference = references.first - offense = offenses.first - assert_equal "tempfile", offense.file - assert_equal 3, offense.location.line - assert_equal 22, offense.location.column - assert_equal "Use of unassigned variable", offense.message + assert_equal "tempfile", reference.relative_path + assert_equal 3, reference.source_location.line + assert_equal 22, reference.source_location.column end test "#call provides node processor with the correct ancestors" do - offense = mock + reference = mock @node_processor_factory.expects(:for).returns(@node_processor) @node_processor.expects(:call).with do |node, ancestors| Node.class?(node) && # class Hello; world; end Node.class_or_module_name(node) == "Hello" && ancestors.empty? - end.returns([]) + end.returns(nil) @node_processor.expects(:call).with do |node, ancestors| parent = ancestors.first # class Hello; world; end Node.constant?(node) && # Hello @@ -58,7 +58,7 @@ class FileProcessorTest < Minitest::Test ancestors.length == 1 && Node.class?(parent) && Node.class_or_module_name(parent) == "Hello" - end.returns([]) + end.returns(nil) @node_processor.expects(:call).with do |node, ancestors| parent = ancestors.first # class Hello; world; end Node.method_call?(node) && # world @@ -66,21 +66,21 @@ class FileProcessorTest < Minitest::Test ancestors.length == 1 && Node.class?(parent) && Node.class_or_module_name(parent) == "Hello" - end.returns([offense]) + end.returns(reference) - offenses = tempfile(name: "hello_world", content: "class Hello; world; end") do |file_path| + references = tempfile(name: "hello_world", content: "class Hello; world; end") do |file_path| @file_processor.call(file_path) end - assert_equal [offense], offenses + assert_equal [reference], references end - test "#call reports no offenses for an empty file" do - offenses = tempfile(name: "foo", content: "# no fun") do |file_path| + test "#call reports no references for an empty file" do + references = tempfile(name: "foo", content: "# no fun") do |file_path| @file_processor.call(file_path) end - assert_empty offenses + assert_empty references end test "#call with an invalid syntax doesn't parse node" do diff --git a/test/unit/privacy_checker_test.rb b/test/unit/privacy_checker_test.rb index 76d73c7fb..30230de70 100644 --- a/test/unit/privacy_checker_test.rb +++ b/test/unit/privacy_checker_test.rb @@ -76,7 +76,7 @@ class PrivacyCheckerTest < Minitest::Test private def privacy_checker - PrivacyChecker.new + ReferenceChecking::Checkers::PrivacyChecker.new end end end diff --git a/test/unit/reference_checking/reference_checker_test.rb b/test/unit/reference_checking/reference_checker_test.rb new file mode 100644 index 000000000..cc05c8454 --- /dev/null +++ b/test/unit/reference_checking/reference_checker_test.rb @@ -0,0 +1,55 @@ +# typed: false +# frozen_string_literal: true + +require "test_helper" + +module Packwerk + class FileProcessorTest < Minitest::Test + include FactoryHelper + + class StubChecker + include ReferenceChecking::Checkers::Checker + + def initialize(**options) + @is_invalid_reference = options[:invalid_reference?] + @violation_type = options[:violation_type] + end + + def violation_type + @violation_type || ViolationType::Dependency + end + + def invalid_reference?(_reference) + @is_invalid_reference + end + end + + test "#call early returns with offense if it is not a reference type" do + input_offense = FileProcessor::UnknownFileTypeResult.new(file: "tempfile") + offenses = reference_checker.call(input_offense) + + assert_equal 1, offenses.length + assert_equal input_offense, offenses.first + end + + test "#call enumerates the list of checkers to create ReferenceOffense objects" do + instance = reference_checker([StubChecker.new( + invalid_reference?: true, + violation_type: ViolationType::Privacy + )]) + input_reference = build_reference + offenses = instance.call(input_reference) + + assert_equal 1, offenses.length + + offense = offenses.first + assert_equal input_reference.relative_path, offense.file + assert_equal input_reference.source_location, offense.location + assert offense.message.start_with?("Privacy violation") + end + + def reference_checker(checkers = [StubChecker.new]) + Packwerk::ReferenceChecking::ReferenceChecker.new(checkers) + end + end +end