Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow fetching all references for a file, or all files, using public API #397

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/packwerk.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module Packwerk
autoload :Reference
autoload :ReferenceOffense
autoload :Validator
autoload :ReferencesFromFile

module OutputStyles
extend ActiveSupport::Autoload
Expand Down
41 changes: 41 additions & 0 deletions lib/packwerk/references_from_file.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# typed: strict
# frozen_string_literal: true

module Packwerk
# Extracts all static constant references between Ruby files.
class ReferencesFromFile
extend T::Sig

class FileParserError < RuntimeError
extend T::Sig

sig { params(file: String, offenses: T::Array[Packwerk::Offense]).void }
def initialize(file:, offenses:)
super("Errors while parsing #{file}: #{offenses.map(&:to_s).join("\n")}")
end
end

sig { params(config: Packwerk::Configuration).void }
def initialize(config = Configuration.from_path)
@config = config
@run_context = T.let(RunContext.from_configuration(@config), RunContext)
end

sig { params(relative_file_paths: T::Array[String]).returns(T::Array[Packwerk::Reference]) }
def list_for_all(relative_file_paths: [])
files = FilesForProcessing.fetch(relative_file_paths: relative_file_paths, configuration: @config).files
files.flat_map { |file| list_for_file(file) }
end

sig { params(relative_file: String).returns(T::Array[Packwerk::Reference]) }
def list_for_file(relative_file)
references_result = @run_context.references_from_file(relative_file: relative_file)

if references_result.file_offenses.present?
raise FileParserError.new(file: relative_file, offenses: references_result.file_offenses)
end

references_result.references
end
end
end
18 changes: 16 additions & 2 deletions lib/packwerk/run_context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,29 @@ def initialize(

sig { params(relative_file: String).returns(T::Array[Packwerk::Offense]) }
def process_file(relative_file:)
reference_checker = ReferenceChecking::ReferenceChecker.new(@checkers)

references_result = references_from_file(relative_file: relative_file)

references_result.file_offenses +
references_result.references.flat_map { |reference| reference_checker.call(reference) }
end

class FileReferencesResult < T::Struct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe T::Struct is slower than a normal struct. Please provide a benchmark, or switch to a plain old Struct (I think we only use Struct in other files).

Copy link
Contributor Author

@exterm exterm Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 4 other uses of T::Struct around the codebase and 4 uses of Struct.

I did not think about performance differences and chose T::Struct because it's typed, and I appreciate the added documentation and type safety, especially within this class that is a little messy and doesn't have great tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick benchmark shows that access is about the same, while instantiation is ~5x slower for T::Struct. I was still able to instantiate 380k T::Struct within a second on my laptop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess you were looking for benchmarks of the whole thing, running packwerk on a real code base. Will do. I'm curious myself.

Copy link
Contributor Author

@exterm exterm Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, on this codebase I'm currently looking at: 800k lines of Ruby according to cloc, 164 packages. Tested on a github codespace.

There is no measurable difference between uncached packwerk check runs on this branch vs on Shopify/packwerk main. Packwerk reports ~18s, real is ~20s.

I also tested with cache, and there is no measurable difference either. Packwerk reports ~3.6s, real is ~6.5s.

const :references, T::Array[Packwerk::Reference]
const :file_offenses, T::Array[Packwerk::Offense]
end

sig { params(relative_file: String).returns(FileReferencesResult) }
def references_from_file(relative_file:)
processed_file = file_processor.call(relative_file)

references = ReferenceExtractor.get_fully_qualified_references_from(
processed_file.unresolved_references,
context_provider
)
reference_checker = ReferenceChecking::ReferenceChecker.new(@checkers)

processed_file.offenses + references.flat_map { |reference| reference_checker.call(reference) }
FileReferencesResult.new(references: references, file_offenses: processed_file.offenses)
end

sig { returns(PackageSet) }
Expand Down
61 changes: 61 additions & 0 deletions test/unit/packwerk/references_from_file_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# typed: true
# frozen_string_literal: true

require "test_helper"

module Packwerk
class ReferencesFromFileTest < Minitest::Test
setup do
@config = Configuration.new
@config.stubs(:load_paths).returns({})
@run_context = RunContext.from_configuration(@config)
RunContext.stubs(:from_configuration).with(@config).returns(@run_context)
@referencer = ReferencesFromFile.new(@config)
end

test "raises on parser error" do
offense = Offense.new(file: "something.rb", message: "yo")
@run_context.stubs(:references_from_file).returns(
RunContext::FileReferencesResult.new(file_offenses: [offense], references: [])
)

assert_raises ReferencesFromFile::FileParserError do
@referencer.list_for_file("lib/something.rb")
end
end

test "fetches violations for all files from run context" do
references = {
"lib/something.rb" => [
make_fake_reference,
],
"components/ice_cream_sales/app/models/scoop.rb" => [
make_fake_reference,
],
}
ffp_mock = mock("FilesForProcessing instance")
ffp_mock.stubs(:files).returns(references.keys)
FilesForProcessing.stubs(:fetch).with(relative_file_paths: [], configuration: @config).returns(ffp_mock)

references.each do |file, references|
@run_context.stubs(:references_from_file).with(relative_file: file).returns(
RunContext::FileReferencesResult.new(file_offenses: [], references: references)
)
end

assert_equal Set.new(references.values.flatten), Set.new(@referencer.list_for_all)
end

private

def make_fake_reference
package_name = Array("ilikeletters".chars.sample(5)).join
Copy link
Contributor Author

@exterm exterm Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array call put in to satisfy sorbet.

Reference.new(
package: Package.new(name: package_name),
relative_path: package_name,
constant: ConstantContext.new,
source_location: nil
)
end
end
end
Loading