From 70d82e92cbbfd981067dc8aa54b9966988ca2584 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:08:48 -0700 Subject: [PATCH 01/24] Move what Violation and Violations share about sources into base classes References will show their sources the way violations do, so the methods that do it move out of Violation and Violations into two abstract classes they now subclass: ConstantUse, for one pack's use of a constant another pack defines, and ConstantUses, for a collection of them. A ConstantUse leaves only one thing to its subclass, how to find its sources in a file. For a Violation that's still wherever the file names its constant, since package_todo.yml records no positions. Nothing changes for callers. Violation#anonymous_sources anonymizes each source itself, rather than through FileCache#get_full_anonymous_sources, which had no other caller and is gone. Violation#count counts the sources the other methods show, which are the constants it counted before. including_files and excluding_files return the class they're called on. sorbet/rbi/todo.rbi stubs ApplicationRecord in ConstantUse, where runtime_keys now names it. Regenerating the file with tapioca would also turn ActionView::ActionViewError back into a module, which Sorbet rejects, so only that line changes. --- lib/query_packwerk.rb | 2 + lib/query_packwerk/constant_use.rb | 216 ++++++++++++++++++++++++ lib/query_packwerk/constant_uses.rb | 244 ++++++++++++++++++++++++++++ lib/query_packwerk/file_cache.rb | 20 --- lib/query_packwerk/violation.rb | 207 ++--------------------- lib/query_packwerk/violations.rb | 229 +------------------------- sorbet/rbi/todo.rbi | 2 +- 7 files changed, 475 insertions(+), 445 deletions(-) create mode 100644 lib/query_packwerk/constant_use.rb create mode 100644 lib/query_packwerk/constant_uses.rb diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index 66e2746..4d96b2e 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -16,6 +16,8 @@ module QueryPackwerk autoload :Console, 'query_packwerk/console' autoload :ConsoleHelpers, 'query_packwerk/console_helpers' autoload :QueryInterface, 'query_packwerk/query_interface' + autoload :ConstantUse, 'query_packwerk/constant_use' + autoload :ConstantUses, 'query_packwerk/constant_uses' autoload :Violations, 'query_packwerk/violations' autoload :Violation, 'query_packwerk/violation' autoload :Packages, 'query_packwerk/packages' diff --git a/lib/query_packwerk/constant_use.rb b/lib/query_packwerk/constant_use.rb new file mode 100644 index 0000000..e817f2e --- /dev/null +++ b/lib/query_packwerk/constant_use.rb @@ -0,0 +1,216 @@ +# typed: strict +# frozen_string_literal: true + +module QueryPackwerk + # A pack's use of a constant that another pack defines, such as a Violation. + # Shows the code that uses the constant: its sources, each the constant with + # any calls chained on it. Subclasses say which files the code is in and how + # to find the sources in each. + class ConstantUse + extend T::Sig + extend T::Helpers + + abstract! + + ALL_CAPS = /\A[A-Z_]+\z/ + + sig { abstract.returns(String) } + def class_name; end + + sig { abstract.returns(T::Array[String]) } + def files; end + + sig { abstract.returns(QueryPackwerk::Package) } + def producing_pack; end + + sig { abstract.returns(QueryPackwerk::Package) } + def consuming_pack; end + + # The sources in one of the files, with the calls chained on each + sig { abstract.params(file_name: String).returns(T::Array[RuboCop::AST::Node]) } + def full_sources_in(file_name); end + + sig { params(file_cache: QueryPackwerk::FileCache).void } + def initialize(file_cache:) + @file_cache = file_cache + @cache_loaded = T.let(false, T::Boolean) + end + + sig { params(headers: T::Boolean).void } + def load_cache!(headers: false) + return true if @cache_loaded + + @file_cache.load!(*T.unsafe(files), headers: headers) + @cache_loaded = true + end + + sig { params(cache: QueryPackwerk::FileCache).void } + def set_cache!(cache) + @cache_loaded = false + @file_cache = cache + end + + sig { returns(Integer) } + def file_count + files.size + end + + # Whether or not any of its files match any provided globs + # + # See Ruby documentation for File.fnmatch for more details. + sig { params(globs: T.any(String, Regexp)).returns(T::Boolean) } + def includes_files?(*globs) + globs.any? do |glob| + files.any? do |file_name| + glob.is_a?(Regexp) ? glob.match?(file_name) : File.fnmatch?(glob, file_name) + end + end + end + + # All sources and their receiver chains across all of its files + sig { returns(T::Array[RuboCop::AST::Node]) } + def sources + load_cache! + + files.flat_map do |file_name| + full_sources_in(file_name) + end + end + + # Adds additional file and line number information to each source + sig { returns(T::Array[T.any(String, T::Array[RuboCop::AST::Node])]) } + def sources_with_locations + load_cache! + + files.flat_map do |file_name| + full_sources_in(file_name).map { |s| ["#{file_name}:#{s.loc.line}", s.source] } + end + end + + # Frequency of which each source occurs + sig { returns(T::Hash[String, Integer]) } + def source_counts + load_cache! + + sources = files.flat_map do |file_name| + full_sources_in(file_name).map(&:source) + end + + sources.tally + end + + # Sources that have had their arguments anonymized + sig { returns(T::Array[String]) } + def anonymous_sources + load_cache! + + files.flat_map do |file_name| + full_sources_in(file_name).map { |s| @file_cache.anonymize_arguments(s.source) } + end + end + + # sig { returns(T::Array[T.any(String, T::Array[String])]) } + sig { returns(T.untyped) } + def anonymous_sources_with_locations + load_cache! + + file_sources = files.flat_map do |file_name| + full_sources_in(file_name).map do |s| + ["#{file_name}:#{s.loc.line}", @file_cache.anonymize_arguments(s.source)] + end + end + + anonymous_source_groups = Hash.new { |h, source| h[source] = [] } + + file_sources.each_with_object(anonymous_source_groups) do |(location, source), groups| + groups[source] << location + end + end + + sig { params(start_offset: Integer, end_offset: Integer).returns(T.untyped) } + def sources_with_contexts(start_offset: 3, end_offset: 3) + load_cache! + + file_sources = files.flat_map do |file_name| + lines = @file_cache.get_file(file_name).lines + + full_sources_in(file_name).map do |s| + line_number = s.loc.line + start_pos = [line_number - start_offset, 1].max + end_pos = [line_number + end_offset, lines.size].min + + location = "#{file_name}:#{s.loc.line} (L#{start_pos}..#{end_pos})" + context = lines[(start_pos - 1)...end_pos] + full_context = unindent((context || ['']).join) + + [@file_cache.anonymize_arguments(s.source), "> #{location}\n\n#{full_context}"] + end + end + + anonymous_source_groups = Hash.new { |h, source| h[source] = [] } + + file_sources.each_with_object(anonymous_source_groups) do |(anonymous_source, full_source), groups| + groups[anonymous_source] << full_source + end + end + + # Like the above source frequency, except grouped by method "shape" rather than + # exact arguments + sig { returns(T::Hash[String, Integer]) } + def anonymous_source_counts + anonymous_sources.tally + end + + # True count of uses, as a file can use the constant more than once. + sig { returns(Integer) } + def count + files.sum do |file_name| + full_sources_in(file_name).size + end + end + + sig do + params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) + end + def runtime_keys(keys) + return {} unless defined?(Rails) + + runtime_values = {} + + return { is_active_record: false, is_constant: false } unless Kernel.const_defined?(class_name) + + if keys.nil? || keys.include?(:is_active_record) + constant = Kernel.const_get(class_name) # rubocop:disable Sorbet/ConstantsFromStrings + + value = @file_cache.set( + :is_active_record, + key: class_name, + value: constant.is_a?(Class) && constant < ApplicationRecord + ) + + runtime_values[:is_active_record] = value + end + + if keys.nil? || keys.include?(:is_constant) + value = @file_cache.set( + :is_constant, + key: class_name, + value: class_name.split('::').last&.match?(ALL_CAPS) + ) + + runtime_values[:is_constant] = value + end + + runtime_values + end + + private + + sig { params(string: String).returns(String) } + def unindent(string) + # Multi-line match, this is intentional + min_space = string.scan(/^\s*/).min_by(&:length) + string.gsub(/^#{min_space}/, '') + end + end +end diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb new file mode 100644 index 0000000..0d1e9a4 --- /dev/null +++ b/lib/query_packwerk/constant_uses.rb @@ -0,0 +1,244 @@ +# typed: strict +# frozen_string_literal: true + +require 'coderay' + +module QueryPackwerk + # A collection of ConstantUses, such as Violations, with methods that group + # their sources by the constant they use, and that say which packs use and + # define those constants. + class ConstantUses + extend T::Sig + extend T::Generic + extend T::Helpers + + abstract! + + Elem = type_member { { upper: QueryPackwerk::ConstantUse } } + + include Enumerable + include QueryInterface + + sig { override.returns(T::Array[Elem]) } + attr_reader :original_collection + + sig do + params( + original_collection: T::Array[Elem], + file_cache: QueryPackwerk::FileCache + ).void + end + def initialize(original_collection, file_cache: QueryPackwerk::FileCache.new) + @original_collection = original_collection + @file_cache = file_cache + + @original_collection.each do |use| + use.set_cache!(file_cache) + end + + @cache_loaded = T.let(false, T::Boolean) + @sources_loaded = T.let(false, T::Boolean) + end + + sig { void } + def load_cache! + return true if @cache_loaded + + warn "Prepopulating AST cache with #{file_count} files: " + start_time = Time.now + + @original_collection.each(&:load_cache!) + + finish_time = Time.now - start_time + warn '', "AST cache loaded in #{finish_time}" + @cache_loaded = true + end + + sig { void } + def load_sources! + return true if @sources_loaded + + unless @cache_loaded + load_cache! + warn + end + + warn "Prepopulating sources cache with #{count} violations: " + start_time = Time.now + + total_sources_loaded = @original_collection.sum do |use| + $stderr.print '.' + use.sources.size + end + + finish_time = Time.now - start_time + warn "Loaded #{total_sources_loaded} full sources in #{finish_time}" + + @sources_loaded = true + end + + sig { returns(Integer) } + def file_count + @original_collection.sum(&:file_count) + end + + sig { returns(T::Boolean) } + def empty? + @original_collection.empty? + end + + sig { returns(T::Boolean) } + def any? + !empty? + end + + # Gets all sources and their receiving chains grouped by the constant they've violated. + sig { returns(T.untyped) } + def raw_sources + load_sources! + + deep_merge_groups(@original_collection) do |v| + [v.class_name, v.sources] + end + end + + # Gets all sources and their receiving chains grouped by the constant they've violated. + sig { returns(T::Hash[String, T::Array[String]]) } + def sources + load_sources! + + deep_merge_groups(@original_collection) { |v| [v.class_name, v.sources.map(&:source)] }.transform_values(&:uniq) + end + + # In addition to the above also provide the file location and line number along with the + # source. + sig { returns(T::Hash[String, T::Array[String]]) } + def sources_with_locations + load_sources! + + deep_merge_groups(@original_collection) { |v| [v.class_name, v.sources_with_locations] } + end + + # Instead of returning all instances of the source, count how often each occurs, with the option to + # provide a threshold to remove lower-occurring items. + sig { params(threshold: Integer).returns(T::Hash[String, T::Hash[String, Integer]]) } + def source_counts(threshold: 0) + load_sources! + + deep_merge_counts(@original_collection, threshold:) { |v| [v.class_name, v.source_counts] } + end + + # "Anonymize" the arguments of sources by replacing all arguments with underscores to get a look + # at the "shape" of a function rather than its exact call (i.e. `test(1, 2, 3)` becomes `test(_, _, _)`). + # + # This also removes extra spacing, line-breaks, cbase constant sigils, and other extra information to + # give a clearer view of a call's "shape". + sig { returns(T::Hash[String, T::Array[String]]) } + def anonymous_sources + load_sources! + + deep_merge_groups(@original_collection) { |v| [v.class_name, v.anonymous_sources] }.transform_values(&:uniq) + end + + sig { returns(T::Hash[String, T::Hash[String, T::Array[String]]]) } + def anonymous_sources_with_locations + load_sources! + + deep_merge_hash_groups(@original_collection) { |v| [v.class_name, v.anonymous_sources_with_locations] } + end + + sig do + params(start_offset: Integer, end_offset: Integer).returns(T::Hash[String, T::Hash[String, T::Array[String]]]) + end + def sources_with_contexts(start_offset: 3, end_offset: 3) + load_sources! + + deep_merge_hash_groups(@original_collection) do |v| + [v.class_name, v.sources_with_contexts(start_offset:, end_offset:)] + end + end + + sig { params(start_offset: Integer, end_offset: Integer).returns(String) } + def sources_with_contexts_report(start_offset: 3, end_offset: 3) + contexts = sources_with_contexts(start_offset:, end_offset:) + output = +'' + + contexts.each do |violated_constant, anonymized_sources| + heavy_underline = '=' * violated_constant.size + output << "#{violated_constant}\n#{heavy_underline}\n\n" + + anonymized_sources.each do |anonymized_source, full_contexts| + light_underline = '-' * anonymized_source.size + output << "#{anonymized_source}\n#{light_underline}\n\n" + + full_contexts.each do |context| + output << highlight_ruby(context) + output << "\n\n" + end + end + end + + output + end + + # Like the above source counts, but uses anonymized sources to give a clearer look at how often each + # "shape" of a method is called across a set of violations. + sig { params(threshold: Integer).returns(T::Hash[String, T::Hash[String, Integer]]) } + def anonymous_source_counts(threshold: 0) + load_sources! + + deep_merge_counts(@original_collection, threshold:) { |v| [v.class_name, v.anonymous_source_counts] } + end + + # Find which packs consume these violations + sig { params(threshold: Integer).returns(T::Hash[String, Integer]) } + def consumers(threshold: 0) + tallies = @original_collection.map { |v| v.consuming_pack.name }.tally + threshold_filter_sort(tallies, threshold:) + end + + # Find which packs produce these violations + sig { params(threshold: Integer).returns(T::Hash[String, Integer]) } + def producers(threshold: 0) + tallies = @original_collection.map { |v| v.producing_pack.name }.tally + threshold_filter_sort(tallies, threshold:) + end + + # Filter for violations which include one of the provided file globs + sig { params(file_globs: T.any(String, Regexp)).returns(T.self_type) } + def including_files(*file_globs) + filtered = @original_collection.select do |use| + T.unsafe(use).includes_files?(*file_globs) # Sorbet hates splats + end + + T.unsafe(self.class).new(filtered) + end + + # Filter for violations which do not include one of the provided file globs + sig { params(file_globs: T.any(String, Regexp)).returns(T.self_type) } + def excluding_files(*file_globs) + filtered = @original_collection.reject do |use| + T.unsafe(use).includes_files?(*file_globs) # Sorbet hates splats + end + + T.unsafe(self.class).new(filtered) + end + + sig { returns(String) } + def inspect + arr = to_a.map(&:inspect) + if arr.empty? + "#<#{self.class.name} []>" + else + "#<#{self.class.name} [\n#{arr.join("\n")}\n]>" + end + end + + private + + sig { params(string: String).returns(String) } + def highlight_ruby(string) + CodeRay.encode(string, :ruby, :terminal) + end + end +end diff --git a/lib/query_packwerk/file_cache.rb b/lib/query_packwerk/file_cache.rb index 0951f11..882f0a9 100644 --- a/lib/query_packwerk/file_cache.rb +++ b/lib/query_packwerk/file_cache.rb @@ -25,9 +25,6 @@ def initialize # { [file_name, const_name] => [AST const nodes with full receiver chains] } @full_source_cache = T.let({}, T::Hash[T::Array[String], T::Array[RuboCop::AST::Node]]) - # { [file_name, const_name] => [anonymized sources] } - @anonymized_source_cache = T.let({}, T::Hash[T::Array[String], T::Array[String]]) - @anonymized_args_cache = T.let({}, T::Hash[String, String]) @is_active_record_cache = T.let({}, T::Hash[String, String]) @@ -122,23 +119,6 @@ def anonymize_arguments(source) .delete("\n").squeeze(' ').delete_prefix('::') end - # Get the full receiver chains on a constant, and anonymize their arguments - sig { params(file_name: String, class_name: String).returns(T::Array[String]) } - def get_full_anonymous_sources(file_name:, class_name:) - const_key = [file_name, class_name] - - return T.must(@anonymized_source_cache[const_key]) if @anonymized_source_cache.key?(const_key) - - @anonymized_source_cache[const_key] = get_full_sources( - file_name: file_name, - class_name: class_name - ).map do |node| - get_full_receiver_chain(node) - .source - .then { |s| anonymize_arguments(s) } - end - end - private # Blanks out everything in an ERB template except its Ruby code, so line numbers still match the template diff --git a/lib/query_packwerk/violation.rb b/lib/query_packwerk/violation.rb index 3daf4e1..971924c 100644 --- a/lib/query_packwerk/violation.rb +++ b/lib/query_packwerk/violation.rb @@ -6,15 +6,13 @@ module QueryPackwerk # Provides methods to analyze violation details including source location, contextual # information, and code patterns. Facilitates both detailed and anonymized views of # dependency violations between packages. - class Violation + class Violation < ConstantUse extend T::Sig - ALL_CAPS = /\A[A-Z_]+\z/ - - sig { returns(QueryPackwerk::Package) } + sig { override.returns(QueryPackwerk::Package) } attr_reader :producing_pack - sig { returns(QueryPackwerk::Package) } + sig { override.returns(QueryPackwerk::Package) } attr_reader :consuming_pack sig do @@ -25,6 +23,7 @@ class Violation ).void end def initialize(original_violation:, consuming_pack:, file_cache: QueryPackwerk::FileCache.new) + super(file_cache:) @original_violation = original_violation @producing_pack = T.let( @@ -36,28 +35,6 @@ def initialize(original_violation:, consuming_pack:, file_cache: QueryPackwerk:: QueryPackwerk::Package.new(original_package: consuming_pack), QueryPackwerk::Package ) - - @file_cache = file_cache - @cache_loaded = T.let(false, T::Boolean) - end - - sig { params(headers: T::Boolean).void } - def load_cache!(headers: false) - return true if @cache_loaded - - @file_cache.load!(*T.unsafe(files), headers: headers) - @cache_loaded = true - end - - sig { params(cache: QueryPackwerk::FileCache).void } - def set_cache!(cache) - @cache_loaded = false - @file_cache = cache - end - - sig { returns(Integer) } - def file_count - files.size end # Forwarding original properties explicitly. @@ -72,174 +49,21 @@ def to_package_name @original_violation.to_package_name end - sig { returns(String) } + sig { override.returns(String) } def class_name @original_violation.class_name end - sig { returns(T::Array[String]) } + sig { override.returns(T::Array[String]) } def files @original_violation.files end - # Addon methods - - # Whether or not the files containing violations match any provided globs - # - # See Ruby documentation for File.fnmatch for more details. - sig { params(globs: T.any(String, Regexp)).returns(T::Boolean) } - def includes_files?(*globs) - globs.any? do |glob| - files.any? do |file_name| - glob.is_a?(Regexp) ? glob.match?(file_name) : File.fnmatch?(glob, file_name) - end - end - end - - # All sources and their receiver chains across all files this violation covers - sig { returns(T::Array[RuboCop::AST::Node]) } - def sources - load_cache! - - files.flat_map do |file_name| - @file_cache.get_full_sources(file_name: file_name, class_name: class_name) - end - end - - # Adds additional file and line number information to each source - sig { returns(T::Array[T.any(String, T::Array[RuboCop::AST::Node])]) } - def sources_with_locations - load_cache! - - files.flat_map do |file_name| - @file_cache - .get_full_sources(file_name: file_name, class_name: class_name) - .map { |s| ["#{file_name}:#{s.loc.line}", s.source] } - end - end - - # Frequency of which each source occurs - sig { returns(T::Hash[String, Integer]) } - def source_counts - load_cache! - - sources = files.flat_map do |file_name| - @file_cache - .get_full_sources(file_name: file_name, class_name: class_name) - .map(&:source) - end - - sources.tally - end - - # Sources that have had their arguments anonymized - sig { returns(T::Array[String]) } - def anonymous_sources - load_cache! - - files.flat_map do |file_name| - @file_cache - .get_full_anonymous_sources(file_name: file_name, class_name: class_name) - end - end - - # sig { returns(T::Array[T.any(String, T::Array[String])]) } - sig { returns(T.untyped) } - def anonymous_sources_with_locations - load_cache! - - file_sources = files.flat_map do |file_name| - @file_cache.get_full_sources(file_name: file_name, class_name: class_name).map do |s| - ["#{file_name}:#{s.loc.line}", @file_cache.anonymize_arguments(s.source)] - end - end - - anonymous_source_groups = Hash.new { |h, source| h[source] = [] } - - file_sources.each_with_object(anonymous_source_groups) do |(location, source), groups| - groups[source] << location - end - end - - sig { params(start_offset: Integer, end_offset: Integer).returns(T.untyped) } - def sources_with_contexts(start_offset: 3, end_offset: 3) - load_cache! - - file_sources = files.flat_map do |file_name| - lines = @file_cache.get_file(file_name).lines - - @file_cache.get_full_sources(file_name: file_name, class_name: class_name).map do |s| - line_number = s.loc.line - start_pos = [line_number - start_offset, 1].max - end_pos = [line_number + end_offset, lines.size].min - - location = "#{file_name}:#{s.loc.line} (L#{start_pos}..#{end_pos})" - context = lines[(start_pos - 1)...end_pos] - full_context = unindent((context || ['']).join) - - [@file_cache.anonymize_arguments(s.source), "> #{location}\n\n#{full_context}"] - end - end - - anonymous_source_groups = Hash.new { |h, source| h[source] = [] } - - file_sources.each_with_object(anonymous_source_groups) do |(anonymous_source, full_source), groups| - groups[anonymous_source] << full_source - end - end - - # Like the above source frequency, except grouped by method "shape" rather than - # exact arguments - sig { returns(T::Hash[String, Integer]) } - def anonymous_source_counts - anonymous_sources.tally - end - - # True count of violations, as there can be multiple of the same violation - # in a file. - sig { returns(Integer) } - def count - files.sum do |file_name| - @file_cache.get_all_const_occurrences( - file_name: file_name, - class_name: class_name - ).size - end - end - - sig do - params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) - end - def runtime_keys(keys) - return {} unless defined?(Rails) - - runtime_values = {} - - return { is_active_record: false, is_constant: false } unless Kernel.const_defined?(class_name) - - if keys.nil? || keys.include?(:is_active_record) - constant = Kernel.const_get(class_name) # rubocop:disable Sorbet/ConstantsFromStrings - - value = @file_cache.set( - :is_active_record, - key: class_name, - value: constant.is_a?(Class) && constant < ApplicationRecord - ) - - runtime_values[:is_active_record] = value - end - - if keys.nil? || keys.include?(:is_constant) - value = @file_cache.set( - :is_constant, - key: class_name, - value: class_name.split('::').last&.match?(ALL_CAPS) - ) - - runtime_values[:is_constant] = value - end - - runtime_values + # package_todo.yml records a violation's files but not where in them it is, + # so its sources are wherever a file names the constant + sig { override.params(file_name: String).returns(T::Array[RuboCop::AST::Node]) } + def full_sources_in(file_name) + @file_cache.get_full_sources(file_name: file_name, class_name: class_name) end sig do @@ -279,14 +103,5 @@ def deconstruct_keys(keys) def inspect "#<#{self.class.name} #{consuming_pack.name} -> #{class_name} (#{type})>" end - - private - - sig { params(string: String).returns(String) } - def unindent(string) - # Multi-line match, this is intentional - min_space = string.scan(/^\s*/).min_by(&:length) - string.gsub(/^#{min_space}/, '') - end end end diff --git a/lib/query_packwerk/violations.rb b/lib/query_packwerk/violations.rb index 0e3ca38..3ecd489 100644 --- a/lib/query_packwerk/violations.rb +++ b/lib/query_packwerk/violations.rb @@ -1,25 +1,17 @@ # typed: strict # frozen_string_literal: true -require 'coderay' - module QueryPackwerk # A collection class for managing and querying sets of Packwerk violations. # Provides aggregation, filtering, and analysis methods for violation data, # including source extraction, contextual reporting, and consumer relationship mapping. # Implements Enumerable and QueryInterface for flexible data manipulation. - class Violations + class Violations < ConstantUses extend T::Sig extend T::Generic Elem = type_member { { fixed: QueryPackwerk::Violation } } - include Enumerable - include QueryInterface - - sig { override.returns(T::Array[QueryPackwerk::Violation]) } - attr_reader :original_collection - @all = T.let(nil, T.nilable(QueryPackwerk::Violations)) class << self @@ -60,224 +52,5 @@ def reload! @all = nil end end - - sig do - params( - original_collection: T::Array[QueryPackwerk::Violation], - file_cache: QueryPackwerk::FileCache - ).void - end - def initialize(original_collection, file_cache: QueryPackwerk::FileCache.new) - @original_collection = original_collection - @file_cache = file_cache - - @original_collection.each do |violation| - violation.set_cache!(file_cache) - end - - @cache_loaded = T.let(false, T::Boolean) - @sources_loaded = T.let(false, T::Boolean) - end - - sig { void } - def load_cache! - return true if @cache_loaded - - warn "Prepopulating AST cache with #{file_count} files: " - start_time = Time.now - - @original_collection.each(&:load_cache!) - - finish_time = Time.now - start_time - warn '', "AST cache loaded in #{finish_time}" - @cache_loaded = true - end - - sig { void } - def load_sources! - return true if @sources_loaded - - unless @cache_loaded - load_cache! - warn - end - - warn "Prepopulating sources cache with #{count} violations: " - start_time = Time.now - - total_sources_loaded = @original_collection.sum do |violation| - $stderr.print '.' - violation.sources.size - end - - finish_time = Time.now - start_time - warn "Loaded #{total_sources_loaded} full sources in #{finish_time}" - - @sources_loaded = true - end - - sig { returns(Integer) } - def file_count - @original_collection.sum(&:file_count) - end - - sig { returns(T::Boolean) } - def empty? - @original_collection.empty? - end - - sig { returns(T::Boolean) } - def any? - !empty? - end - - # Gets all sources and their receiving chains grouped by the constant they've violated. - sig { returns(T.untyped) } - def raw_sources - load_sources! - - deep_merge_groups(@original_collection) do |v| - [v.class_name, v.sources] - end - end - - # Gets all sources and their receiving chains grouped by the constant they've violated. - sig { returns(T::Hash[String, T::Array[String]]) } - def sources - load_sources! - - deep_merge_groups(@original_collection) { |v| [v.class_name, v.sources.map(&:source)] }.transform_values(&:uniq) - end - - # In addition to the above also provide the file location and line number along with the - # source. - sig { returns(T::Hash[String, T::Array[String]]) } - def sources_with_locations - load_sources! - - deep_merge_groups(@original_collection) { |v| [v.class_name, v.sources_with_locations] } - end - - # Instead of returning all instances of the source, count how often each occurs, with the option to - # provide a threshold to remove lower-occurring items. - sig { params(threshold: Integer).returns(T::Hash[String, T::Hash[String, Integer]]) } - def source_counts(threshold: 0) - load_sources! - - deep_merge_counts(@original_collection, threshold:) { |v| [v.class_name, v.source_counts] } - end - - # "Anonymize" the arguments of sources by replacing all arguments with underscores to get a look - # at the "shape" of a function rather than its exact call (i.e. `test(1, 2, 3)` becomes `test(_, _, _)`). - # - # This also removes extra spacing, line-breaks, cbase constant sigils, and other extra information to - # give a clearer view of a call's "shape". - sig { returns(T::Hash[String, T::Array[String]]) } - def anonymous_sources - load_sources! - - deep_merge_groups(@original_collection) { |v| [v.class_name, v.anonymous_sources] }.transform_values(&:uniq) - end - - sig { returns(T::Hash[String, T::Hash[String, T::Array[String]]]) } - def anonymous_sources_with_locations - load_sources! - - deep_merge_hash_groups(@original_collection) { |v| [v.class_name, v.anonymous_sources_with_locations] } - end - - sig do - params(start_offset: Integer, end_offset: Integer).returns(T::Hash[String, T::Hash[String, T::Array[String]]]) - end - def sources_with_contexts(start_offset: 3, end_offset: 3) - load_sources! - - deep_merge_hash_groups(@original_collection) do |v| - [v.class_name, v.sources_with_contexts(start_offset:, end_offset:)] - end - end - - sig { params(start_offset: Integer, end_offset: Integer).returns(String) } - def sources_with_contexts_report(start_offset: 3, end_offset: 3) - contexts = sources_with_contexts(start_offset:, end_offset:) - output = +'' - - contexts.each do |violated_constant, anonymized_sources| - heavy_underline = '=' * violated_constant.size - output << "#{violated_constant}\n#{heavy_underline}\n\n" - - anonymized_sources.each do |anonymized_source, full_contexts| - light_underline = '-' * anonymized_source.size - output << "#{anonymized_source}\n#{light_underline}\n\n" - - full_contexts.each do |context| - output << highlight_ruby(context) - output << "\n\n" - end - end - end - - output - end - - # Like the above source counts, but uses anonymized sources to give a clearer look at how often each - # "shape" of a method is called across a set of violations. - sig { params(threshold: Integer).returns(T::Hash[String, T::Hash[String, Integer]]) } - def anonymous_source_counts(threshold: 0) - load_sources! - - deep_merge_counts(@original_collection, threshold:) { |v| [v.class_name, v.anonymous_source_counts] } - end - - # Find which packs consume these violations - sig { params(threshold: Integer).returns(T::Hash[String, Integer]) } - def consumers(threshold: 0) - tallies = @original_collection.map { |v| v.consuming_pack.name }.tally - threshold_filter_sort(tallies, threshold:) - end - - # Find which packs produce these violations - sig { params(threshold: Integer).returns(T::Hash[String, Integer]) } - def producers(threshold: 0) - tallies = @original_collection.map { |v| v.producing_pack.name }.tally - threshold_filter_sort(tallies, threshold:) - end - - # Filter for violations which include one of the provided file globs - sig { params(file_globs: T.any(String, Regexp)).returns(QueryPackwerk::Violations) } - def including_files(*file_globs) - filtered_violations = @original_collection.select do |violation| - T.unsafe(violation).includes_files?(*file_globs) # Sorbet hates splats - end - - QueryPackwerk::Violations.new(filtered_violations) - end - - # Filter for violations which do not include one of the provided file globs - sig { params(file_globs: T.any(String, Regexp)).returns(QueryPackwerk::Violations) } - def excluding_files(*file_globs) - filtered_violations = @original_collection.reject do |violation| - T.unsafe(violation).includes_files?(*file_globs) # Sorbet hates splats - end - - QueryPackwerk::Violations.new(filtered_violations) - end - - sig { returns(String) } - def inspect - arr = to_a.map(&:inspect) - if arr.empty? - "#<#{self.class.name} []>" - else - "#<#{self.class.name} [\n#{arr.join("\n")}\n]>" - end - end - - private - - sig { params(string: String).returns(String) } - def highlight_ruby(string) - CodeRay.encode(string, :ruby, :terminal) - end end end diff --git a/sorbet/rbi/todo.rbi b/sorbet/rbi/todo.rbi index 3cc0c49..851c88d 100644 --- a/sorbet/rbi/todo.rbi +++ b/sorbet/rbi/todo.rbi @@ -19,4 +19,4 @@ module ActiveSupport::Multibyte::Chars; end module ActiveSupport::SafeBuffer; end module ActiveSupport::StringInquirer; end module ActiveSupport::TimeZone; end -module QueryPackwerk::Violation::ApplicationRecord; end +module QueryPackwerk::ConstantUse::ApplicationRecord; end From 1ae3626736a7af6ba2aa40d615c437c0a7453042 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:14:16 -0700 Subject: [PATCH 02/24] Let where find nothing in an empty collection where checks its keys against the first value in the collection, and with no values it raised NoMethodError on nil, so in an app whose package_todo.yml files record no violations, Violations.where and violations_for raised instead of finding nothing. It now finds nothing, without checking the keys, since nothing could match them. --- lib/query_packwerk/query_interface.rb | 6 +++++- spec/query_packwerk/query_interface_spec.rb | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/query_packwerk/query_interface.rb b/lib/query_packwerk/query_interface.rb index a50330e..3b42d5d 100644 --- a/lib/query_packwerk/query_interface.rb +++ b/lib/query_packwerk/query_interface.rb @@ -99,7 +99,11 @@ def where(**query_params, &_query_fn) query_keys = query_params.keys all_values = all - accepted_keys = all_values.first.deconstruct_keys(nil).keys + # Keys are checked against the first value, and with none, nothing can match + first_value = all_values.first + return [] unless first_value + + accepted_keys = first_value.deconstruct_keys(nil).keys invalid_keys = query_keys - accepted_keys raise ArgumentError, "The following keys are invalid for querying: #{invalid_keys.join(', ')}" if invalid_keys.any? diff --git a/spec/query_packwerk/query_interface_spec.rb b/spec/query_packwerk/query_interface_spec.rb index 9c2213a..320df84 100644 --- a/spec/query_packwerk/query_interface_spec.rb +++ b/spec/query_packwerk/query_interface_spec.rb @@ -105,6 +105,14 @@ class << self it 'can check for multiple potential Array matches' do expect(people.where(hobbies: %w[piano fencing]).count).to eq(2) end + + context 'when there are no people' do + let(:all_people) { [] } + + it 'finds nobody' do + expect(people.where(adult: true).count).to eq(0) + end + end end describe '#count' do From 5771a3d974872288d18218860c9c6134d7c8cd60 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:29:21 -0700 Subject: [PATCH 03/24] Add References, every reference between packs that pks lists package_todo.yml files record only the references that break a pack's rules, so query_packwerk couldn't show how other packs use a pack where a declared dependency or a public constant allows it (#8). pks 0.7.0 lists every reference from one pack to another pack's constants, with the violations each makes. References runs `pks list-references` and has the same methods as Violations: References.where(producing_pack: "packs/foo", allowed: true).anonymous_source_counts QueryPackwerk.references_for and references_from, and Package's incoming_references and outgoing_references, get the references to or from a pack, as violations_for and todos_for do for violations. where can query a reference's constant, file, packs and owners, as for a violation, and also allowed and violation_types. reload! forgets the references too, so the next query runs pks again. query_packwerk runs bin/pks if the app has one, as an app that pins pks's version does, or else pks on the PATH. It checks that the version has list-references, and when pks is missing or too old, the error says where to get it. Violations still come from package_todo.yml files and don't need pks. pks says where each reference is, so a reference finds its source at that line and column rather than by its constant's name. That finds a constant that the code names relatively, as `Bar` inside `module Foo`, which matching names misses, and makes the association call the source of a reference that pks infers from one, as `has_many :tacos` is for `::Taco`. pks counts columns in bytes, where the parser counts characters, so the file cache converts them. pks gives no positions in ERB, and lists a template's references to a constant once, so there the sources are wherever the template names the constant, as for violations. spec/support/referenced_packs.rb writes an app with each kind of reference, along with what pks 0.7.0 lists for it, which the specs use in place of pks. spec/query_packwerk/pks_spec.rb checks that listing against pks when pks is installed, and skips the check when it isn't. --- README.md | 27 ++++- lib/query_packwerk.rb | 22 +++- lib/query_packwerk/console_helpers.rb | 58 +++++++++ lib/query_packwerk/constant_use.rb | 8 +- lib/query_packwerk/constant_uses.rb | 22 ++-- lib/query_packwerk/file_cache.rb | 27 +++++ lib/query_packwerk/package.rb | 14 +++ lib/query_packwerk/pks.rb | 73 ++++++++++++ lib/query_packwerk/reference.rb | 157 +++++++++++++++++++++++++ lib/query_packwerk/references.rb | 78 ++++++++++++ spec/query_packwerk/file_cache_spec.rb | 34 ++++++ spec/query_packwerk/package_spec.rb | 27 +++++ spec/query_packwerk/pks_spec.rb | 110 +++++++++++++++++ spec/query_packwerk/reference_spec.rb | 139 ++++++++++++++++++++++ spec/query_packwerk/references_spec.rb | 133 +++++++++++++++++++++ spec/query_packwerk_spec.rb | 33 ++++++ spec/spec_helper.rb | 1 + spec/support/referenced_packs.rb | 143 ++++++++++++++++++++++ 18 files changed, 1088 insertions(+), 18 deletions(-) create mode 100644 lib/query_packwerk/pks.rb create mode 100644 lib/query_packwerk/reference.rb create mode 100644 lib/query_packwerk/references.rb create mode 100644 spec/query_packwerk/pks_spec.rb create mode 100644 spec/query_packwerk/reference_spec.rb create mode 100644 spec/query_packwerk/references_spec.rb create mode 100644 spec/support/referenced_packs.rb diff --git a/README.md b/README.md index 4ebad31..fcf1929 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # QueryPackwerk QueryPackwerk is a Ruby gem for querying and analyzing [Packwerk](https://github.com/Shopify/packwerk) violations in Ruby applications. -It provides a friendly API for exploring `package.yml` and `package_todo.yml` files, making it easier to manage module boundaries and dependencies in your codebase. +It provides a friendly API for exploring `package.yml` and `package_todo.yml` files, and the references between packs that [pks](https://github.com/rubyatscale/pks) lists, making it easier to manage module boundaries and dependencies in your codebase. ## Installation @@ -80,10 +80,30 @@ query_packwerk:001:0> Violations.all ``` +Explore references, whether or not they break a rule: + +package_todo.yml files record only the references that break a pack's rules. To list every reference from one pack to another pack's constants, including those that a declared dependency or a public constant allows, QueryPackwerk runs [pks](https://github.com/rubyatscale/pks) 0.7.0 or later. It runs `bin/pks` if your app has one, or else `pks` on your `PATH`. See pks's [installation instructions](https://github.com/rubyatscale/pks/blob/main/INSTALLATION.md). + +```ruby +# Get all references to a pack's constants +query_packwerk:001:0> references_for("pack_name") + +# Get the 'shape' of the references to a pack that break no rule +query_packwerk:001:0> References.where(producing_pack: "packs/pack_name", allowed: true).anonymous_source_counts + +# Get all references from a pack to other packs' constants +query_packwerk:001:0> references_from("pack_name") + +# Get all references +query_packwerk:001:0> References.all +``` + +References have the same methods as violations, like `sources_with_locations` and `anonymous_source_counts`. Each one is where pks finds it, so its source is found even where the code names a constant relatively, as `Bar` inside `module Foo`. A reference that pks infers from an association, as `has_many :tacos` does for `Taco`, has the association as its source. pks gives no positions in ERB templates, so there a reference's sources are wherever the template names the constant. Finding sources parses each file they're in, which for a pack that thousands of files reference takes a while. + If you change any of the package.yml or package_todo.yml files, you can reload the running console: ```ruby -# Reset the cache +# Reset the cache, so the next query of references runs pks again query_packwerk:001:0> reload! ``` @@ -102,6 +122,9 @@ consumers = QueryPackwerk.consumers("my_pack") # Get a count of all violation types counts = QueryPackwerk.violation_counts_for("my_pack") + +# Get how other packs use your pack, whether or not they break a rule +QueryPackwerk.references_for("my_pack").anonymous_source_counts ``` ## Examples diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index 4d96b2e..fbe9a38 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -9,7 +9,8 @@ # QueryPackwerk is a tool for querying Packwerk violations. # # It is built on top of ParsePackwerk, and provides a Ruby-friendly API -# for querying package.yml and package_todo.yml files. +# for querying package.yml and package_todo.yml files. It also queries every +# reference between packages, allowed or not, which pks lists. # # See also: https://github.com/rubyatscale/parse_packwerk module QueryPackwerk @@ -20,6 +21,9 @@ module QueryPackwerk autoload :ConstantUses, 'query_packwerk/constant_uses' autoload :Violations, 'query_packwerk/violations' autoload :Violation, 'query_packwerk/violation' + autoload :References, 'query_packwerk/references' + autoload :Reference, 'query_packwerk/reference' + autoload :Pks, 'query_packwerk/pks' autoload :Packages, 'query_packwerk/packages' autoload :Package, 'query_packwerk/package' autoload :RuleRewriter, 'query_packwerk/rule_rewriter' @@ -79,6 +83,21 @@ def consumers(pack_name, threshold: 0) violations_for(pack_name).consumers(threshold: threshold) end + # Get all references from other packages to this package's constants, whether + # or not they break the packages' rules (i.e., how others use this package). + # These come from pks, which must be installed. + sig { params(pack_name: String).returns(QueryPackwerk::References) } + def references_for(pack_name) + QueryPackwerk::References.where(producing_pack: full_name(pack_name)) + end + + # Get all references from this package to other packages' constants, whether + # or not they break the packages' rules (i.e., how this package uses others) + sig { params(pack_name: String).returns(QueryPackwerk::References) } + def references_from(pack_name) + QueryPackwerk::References.where(consuming_pack: full_name(pack_name)) + end + # In case anyone is still using shorthand for pack names sig { params(pack_name: String).returns(String) } def full_name(pack_name) @@ -91,5 +110,6 @@ def full_name(pack_name) def reload! Packages.reload! Violations.reload! + References.reload! end end diff --git a/lib/query_packwerk/console_helpers.rb b/lib/query_packwerk/console_helpers.rb index 4d8bb74..3df60fa 100644 --- a/lib/query_packwerk/console_helpers.rb +++ b/lib/query_packwerk/console_helpers.rb @@ -25,6 +25,10 @@ def help(type = nil) help_violation when 'violations' help_violations + when 'reference' + help_reference + when 'references' + help_references when 'package' help_package when 'packages' @@ -44,6 +48,8 @@ def help_help help # This help help_violation # Help for QueryPackwerk::Violation help_violations # Help for QueryPackwerk::Violations + help_reference # Help for QueryPackwerk::Reference + help_references # Help for QueryPackwerk::References help_package # Help for QueryPackwerk::Package help_packages # Help for QueryPackwerk::Packages HELP @@ -88,6 +94,51 @@ def help_violations HELP end + sig { returns(NilClass) } + def help_reference + puts <<~HELP + QueryPackwerk::Reference (lib/query_packwerk/reference.rb) + ------------------------------------------------ + .constant_name # Returns the referenced constant's name + .file # Returns the file that makes the reference + .line # Returns the line of the reference, or nil in an ERB template + .location # Returns file:line, or the file for an ERB template + .producing_pack # Returns the Package that defines the constant + .consuming_pack # Returns the Package whose file makes the reference + .violations # Returns the rules the reference breaks, with each one's type and whether it's strict and recorded + .violation_types # Returns the types of the rules the reference breaks + .allowed? # Returns whether the reference breaks none of the rules + .sources # Returns array of AST nodes representing the reference + .sources_with_locations # Returns array of [file:line, source] pairs + .anonymous_sources # Returns array of sources with arguments anonymized + .count # Returns how many sources make the reference, which is more than one only in an ERB template + HELP + end + + sig { returns(NilClass) } + def help_references + puts <<~HELP + QueryPackwerk::References (lib/query_packwerk/references.rb) + -------------------------------------------------- + Every reference from a package to another package's constants, whether or not it breaks their rules. + These come from pks 0.7.0 or later (https://github.com/rubyatscale/pks), in bin/pks or on your PATH. + + References.all # Returns all references + References.where(conditions) # Returns new References filtered by conditions, as allowed: true or violation_types: 'privacy' + .sources # Returns hash of constant => source strings for all references + .sources_with_locations # Returns hash of constant => [file:line, source] pairs + .source_counts # Returns hash of constant => {source => count} with occurrence counts + .sources_with_contexts # Returns hash of constant => [file:line, source] pairs for each reference with context + .anonymous_sources # Returns hash of constant => anonymized sources + .anonymous_sources_with_locations # Returns array of [file:line, source] pairs for each reference with arguments anonymized + .anonymous_source_counts # Returns hash of constant => {anonymized source => count} + .consumers(threshold) # Returns hash of consuming package names and reference counts + .producers(threshold) # Returns hash of producing package names and reference counts + .including_files(globs) # Returns new References filtered to include specific files + .excluding_files(globs) # Returns new References filtered to exclude specific files + HELP + end + sig { returns(NilClass) } def help_package puts <<~HELP @@ -104,6 +155,8 @@ def help_package .directory # Returns the package directory path .todos # Returns Violations where this package is the consumer .violations # Returns Violations where this package is the producer + .incoming_references # Returns References from other packages to this package (needs pks) + .outgoing_references # Returns References from this package to other packages (needs pks) .consumers # Returns Packages that consume this package .consumer_names # Returns array of names of consuming packages .consumer_counts # Returns hash of consumer package names and counts @@ -141,7 +194,12 @@ def help_query_packwerk Package Dependencies (how this package uses others): todos_for(pack_name) # Get all todos where this package accesses others + Package References, whether or not they break a rule (needs pks): + references_for(pack_name) # Get all references from others to this package + references_from(pack_name) # Get all references from this package to others + Violations.all # Get all violations + References.all # Get all references (needs pks) HELP end end diff --git a/lib/query_packwerk/constant_use.rb b/lib/query_packwerk/constant_use.rb index e817f2e..e7fabea 100644 --- a/lib/query_packwerk/constant_use.rb +++ b/lib/query_packwerk/constant_use.rb @@ -2,10 +2,10 @@ # frozen_string_literal: true module QueryPackwerk - # A pack's use of a constant that another pack defines, such as a Violation. - # Shows the code that uses the constant: its sources, each the constant with - # any calls chained on it. Subclasses say which files the code is in and how - # to find the sources in each. + # A pack's use of a constant that another pack defines, such as a Violation + # or a Reference. Shows the code that uses the constant: its sources, each the + # constant with any calls chained on it. Subclasses say which files the code + # is in and how to find the sources in each. class ConstantUse extend T::Sig extend T::Helpers diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb index 0d1e9a4..dc9bbf1 100644 --- a/lib/query_packwerk/constant_uses.rb +++ b/lib/query_packwerk/constant_uses.rb @@ -4,9 +4,9 @@ require 'coderay' module QueryPackwerk - # A collection of ConstantUses, such as Violations, with methods that group - # their sources by the constant they use, and that say which packs use and - # define those constants. + # A collection of ConstantUses, such as Violations or References, with + # methods that group their sources by the constant they use, and that say + # which packs use and define those constants. class ConstantUses extend T::Sig extend T::Generic @@ -63,7 +63,7 @@ def load_sources! warn end - warn "Prepopulating sources cache with #{count} violations: " + warn "Prepopulating sources cache with #{count} #{self.class.name.to_s.split('::').last.to_s.downcase}: " start_time = Time.now total_sources_loaded = @original_collection.sum do |use| @@ -92,7 +92,7 @@ def any? !empty? end - # Gets all sources and their receiving chains grouped by the constant they've violated. + # Gets all sources and their receiving chains grouped by the constant they use. sig { returns(T.untyped) } def raw_sources load_sources! @@ -102,7 +102,7 @@ def raw_sources end end - # Gets all sources and their receiving chains grouped by the constant they've violated. + # Gets all sources and their receiving chains grouped by the constant they use. sig { returns(T::Hash[String, T::Array[String]]) } def sources load_sources! @@ -182,7 +182,7 @@ def sources_with_contexts_report(start_offset: 3, end_offset: 3) end # Like the above source counts, but uses anonymized sources to give a clearer look at how often each - # "shape" of a method is called across a set of violations. + # "shape" of a method is called across a set of violations or references. sig { params(threshold: Integer).returns(T::Hash[String, T::Hash[String, Integer]]) } def anonymous_source_counts(threshold: 0) load_sources! @@ -190,21 +190,21 @@ def anonymous_source_counts(threshold: 0) deep_merge_counts(@original_collection, threshold:) { |v| [v.class_name, v.anonymous_source_counts] } end - # Find which packs consume these violations + # Find which packs consume the constants these use sig { params(threshold: Integer).returns(T::Hash[String, Integer]) } def consumers(threshold: 0) tallies = @original_collection.map { |v| v.consuming_pack.name }.tally threshold_filter_sort(tallies, threshold:) end - # Find which packs produce these violations + # Find which packs produce the constants these use sig { params(threshold: Integer).returns(T::Hash[String, Integer]) } def producers(threshold: 0) tallies = @original_collection.map { |v| v.producing_pack.name }.tally threshold_filter_sort(tallies, threshold:) end - # Filter for violations which include one of the provided file globs + # Filter for those in files that match one of the provided file globs sig { params(file_globs: T.any(String, Regexp)).returns(T.self_type) } def including_files(*file_globs) filtered = @original_collection.select do |use| @@ -214,7 +214,7 @@ def including_files(*file_globs) T.unsafe(self.class).new(filtered) end - # Filter for violations which do not include one of the provided file globs + # Filter for those not in files that match one of the provided file globs sig { params(file_globs: T.any(String, Regexp)).returns(T.self_type) } def excluding_files(*file_globs) filtered = @original_collection.reject do |use| diff --git a/lib/query_packwerk/file_cache.rb b/lib/query_packwerk/file_cache.rb index 882f0a9..0acaf43 100644 --- a/lib/query_packwerk/file_cache.rb +++ b/lib/query_packwerk/file_cache.rb @@ -25,6 +25,9 @@ def initialize # { [file_name, const_name] => [AST const nodes with full receiver chains] } @full_source_cache = T.let({}, T::Hash[T::Array[String], T::Array[RuboCop::AST::Node]]) + # { file_name => { line => [AST const and send nodes that start on it] } } + @line_cache = T.let({}, T::Hash[String, T::Hash[Integer, T::Array[RuboCop::AST::Node]]]) + @anonymized_args_cache = T.let({}, T::Hash[String, String]) @is_active_record_cache = T.let({}, T::Hash[String, String]) @@ -111,6 +114,24 @@ def get_full_sources(file_name:, class_name:) end end + # Gets the full source of the reference that pks finds at a line and column. + # pks counts the column in bytes, where the parser counts characters. A + # reference that pks infers from an association, as `has_many :tacos` gives + # `::Taco`, is at the call, so the call is its source. + sig do + params(file_name: String, line: Integer, column: Integer).returns(T.nilable(RuboCop::AST::Node)) + end + def get_full_source_at(file_name:, line:, column:) + nodes = nodes_by_line(file_name).fetch(line, []).select do |node| + range = node.source_range + range.source_line[0, range.column].bytesize == column + end + + # The outermost constant, as `Foo::Bar` rather than the `Foo` in it + node = nodes.find(&:const_type?) || nodes.find(&:send_type?) + node && get_full_receiver_chain(node) + end + # Cleans up and anonymizes a source sig { params(source: String).returns(String) } def anonymize_arguments(source) @@ -121,6 +142,12 @@ def anonymize_arguments(source) private + # Constants and calls by the line they start on, outermost first, as `each_node` visits them + sig { params(file_name: String).returns(T::Hash[Integer, T::Array[RuboCop::AST::Node]]) } + def nodes_by_line(file_name) + @line_cache[file_name] ||= get_file_ast(file_name).each_node(:const, :send).group_by { |node| node.loc.line } + end + # Blanks out everything in an ERB template except its Ruby code, so line numbers still match the template sig { params(file_name: String).returns(String) } def ruby_source(file_name) diff --git a/lib/query_packwerk/package.rb b/lib/query_packwerk/package.rb index 5c89382..5b20d09 100644 --- a/lib/query_packwerk/package.rb +++ b/lib/query_packwerk/package.rb @@ -81,6 +81,20 @@ def violations end alias incoming_violations violations + # Returns references from other packages to this package's constants, + # whether or not they break the packages' rules. These come from pks. + sig { returns(QueryPackwerk::References) } + def incoming_references + QueryPackwerk::References.where(producing_pack: name) + end + + # Returns references from this package to other packages' constants, + # whether or not they break the packages' rules. These come from pks. + sig { returns(QueryPackwerk::References) } + def outgoing_references + QueryPackwerk::References.where(consuming_pack: name) + end + # Returns all packages that consume (depend on) this package sig { returns(QueryPackwerk::Packages) } def consumers diff --git a/lib/query_packwerk/pks.rb b/lib/query_packwerk/pks.rb new file mode 100644 index 0000000..a743299 --- /dev/null +++ b/lib/query_packwerk/pks.rb @@ -0,0 +1,73 @@ +# typed: strict +# frozen_string_literal: true + +require 'json' +require 'open3' + +module QueryPackwerk + # Runs pks (https://github.com/rubyatscale/pks), which lists every reference + # from one pack to a constant that another pack defines, whether or not it + # breaks the packs' rules. package_todo.yml files record only those that do. + module Pks + extend T::Sig + + class Error < StandardError; end + + # The first version with `list-references` + MINIMUM_VERSION = Gem::Version.new('0.7.0') + INSTALLATION_URL = 'https://github.com/rubyatscale/pks/blob/main/INSTALLATION.md' + + @checked_executables = T.let([], T::Array[String]) + + class << self + extend T::Sig + + # The references in the app, as `pks list-references` prints them: + # https://github.com/rubyatscale/pks/blob/main/schema/list-references-output.json + sig { returns(T::Array[T::Hash[String, T.untyped]]) } + def list_references + pks = executable + stdout, stderr, status = Open3.capture3(pks, 'list-references') + raise Error, "`#{pks} list-references` failed:\n#{stderr}" unless status.success? + + # Warnings, as about a file that pks can't parse + $stderr.print(stderr) + JSON.parse(stdout).fetch('references') + end + + # bin/pks, as in an app that pins a version of pks, or else pks on the PATH + sig { returns(String) } + def executable + candidates = ['bin', *ENV.fetch('PATH', '').split(File::PATH_SEPARATOR)].map do |directory| + File.expand_path('pks', directory) + end + executable = candidates.find { |candidate| File.file?(candidate) && File.executable?(candidate) } + unless executable + raise Error, "query_packwerk finds references with pks, which isn't in bin/pks or on your PATH. " \ + "To install it, see #{INSTALLATION_URL}" + end + + check_version(executable) + executable + end + + private + + sig { params(executable: String).void } + def check_version(executable) + return if @checked_executables.include?(executable) + + output, status = Open3.capture2e(executable, '--version') + version = output[/\Apks (\S+)/, 1] + raise Error, "Could not get the version of #{executable}:\n#{output}" unless status.success? && version && Gem::Version.correct?(version) + + if Gem::Version.new(version) < MINIMUM_VERSION + raise Error, "query_packwerk finds references with `pks list-references`, which needs pks #{MINIMUM_VERSION} " \ + "or later, but #{executable} is pks #{version}. To upgrade it, see #{INSTALLATION_URL}" + end + + @checked_executables << executable + end + end + end +end diff --git a/lib/query_packwerk/reference.rb b/lib/query_packwerk/reference.rb new file mode 100644 index 0000000..bfbe611 --- /dev/null +++ b/lib/query_packwerk/reference.rb @@ -0,0 +1,157 @@ +# typed: strict +# frozen_string_literal: true + +module QueryPackwerk + # A reference from one pack to a constant that another pack defines, as pks + # lists it, whether or not it breaks the packs' rules. Unlike a Violation, + # which package_todo.yml records by file, it's at one place in one file, so + # its sources are the code there. + class Reference < ConstantUse + extend T::Sig + + # A rule of the packs that a reference breaks + class Violation < T::Struct + # dependency, privacy, visibility, layer or folder_privacy + const :type, String + + # Whether the pack that enforces the rule is strict about it + const :strict, T::Boolean + + # Whether a package_todo.yml records the violation. `pks check` fails on + # one that isn't recorded. + const :recorded, T::Boolean + end + + sig { override.returns(QueryPackwerk::Package) } + attr_reader :producing_pack + + sig { override.returns(QueryPackwerk::Package) } + attr_reader :consuming_pack + + sig { returns(String) } + attr_reader :file + + # Counted from 1, and nil in ERB, where pks keeps no source positions + sig { returns(T.nilable(Integer)) } + attr_reader :line + + # Counted in bytes from 0, and nil when line is + sig { returns(T.nilable(Integer)) } + attr_reader :column + + sig { returns(T::Array[QueryPackwerk::Reference::Violation]) } + attr_reader :violations + + sig do + params( + constant_name: String, + file: String, + line: T.nilable(Integer), + column: T.nilable(Integer), + producing_pack: QueryPackwerk::Package, + consuming_pack: QueryPackwerk::Package, + violations: T::Array[QueryPackwerk::Reference::Violation], + file_cache: QueryPackwerk::FileCache + ).void + end + def initialize( + constant_name:, + file:, + line:, + column:, + producing_pack:, + consuming_pack:, + violations:, + file_cache: QueryPackwerk::FileCache.new + ) + super(file_cache:) + @constant_name = constant_name + @file = file + @line = line + @column = column + @producing_pack = producing_pack + @consuming_pack = consuming_pack + @violations = violations + end + + # Fully qualified, as `::Foo::Bar`, though the code may name it + # relatively, as `Bar` inside `module Foo` + sig { override.returns(String) } + def class_name + @constant_name + end + alias constant_name class_name + + sig { override.returns(T::Array[String]) } + def files + [file] + end + + # Whether the reference breaks none of the packs' rules + sig { returns(T::Boolean) } + def allowed? + violations.empty? + end + + sig { returns(T::Array[String]) } + def violation_types + violations.map(&:type) + end + + sig { returns(String) } + def location + line ? "#{file}:#{line}" : file + end + + # The code at the reference's line and column. A reference that pks infers + # from an association is at the call, as `has_many :tacos` is for `::Taco`. + # In ERB, pks gives no position and lists a template's references to a + # constant once, so they're wherever the template names it. + sig { override.params(file_name: String).returns(T::Array[RuboCop::AST::Node]) } + def full_sources_in(file_name) + line = @line + column = @column + return @file_cache.get_full_sources(file_name: file_name, class_name: class_name) unless line && column + + [@file_cache.get_full_source_at(file_name: file_name, line: line, column: column)].compact + end + + sig do + params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) + end + def deconstruct_keys(keys) + all_values = { + constant_name: class_name, + pack_name: producing_pack.name, + file: file, + + # Which of the packs' rules the reference breaks, if any + allowed: allowed?, + violation_types: violation_types, + privacy: violation_types.include?('privacy'), + dependency: violation_types.include?('dependency'), + + # Which pack defines the constant, and which pack's code uses it + consuming_pack: consuming_pack.name, + producing_pack: producing_pack.name, + + producing_owner: producing_pack.owner, + consuming_owner: consuming_pack.owner, + + # As for violations, "owner" is the producing pack's + owner: producing_pack.owner, + owned: producing_pack.owner != Packages::UNOWNED, + + **runtime_keys(keys) + } + + keys.nil? ? all_values : all_values.slice(*T.unsafe(keys)) + end + + sig { returns(String) } + def inspect + rules = allowed? ? 'allowed' : violation_types.join(', ') + "#<#{self.class.name} #{consuming_pack.name} -> #{class_name} (#{rules}) at #{location}>" + end + end +end diff --git a/lib/query_packwerk/references.rb b/lib/query_packwerk/references.rb new file mode 100644 index 0000000..8de9c53 --- /dev/null +++ b/lib/query_packwerk/references.rb @@ -0,0 +1,78 @@ +# typed: strict +# frozen_string_literal: true + +module QueryPackwerk + # A collection of the references between packs that pks lists. It includes + # the references that break none of the packs' rules, which package_todo.yml + # files leave out, and has the same methods as Violations. + class References < ConstantUses + extend T::Sig + extend T::Generic + + Elem = type_member { { fixed: QueryPackwerk::Reference } } + + @all = T.let(nil, T.nilable(QueryPackwerk::References)) + + class << self + extend T::Sig + + # Every reference from one pack to a constant that another pack defines. + # The first call runs pks, which reads every file in the app. + sig { returns(QueryPackwerk::References) } + def all + return @all if @all + + file_cache = QueryPackwerk::FileCache.new + packages = T.let({}, T::Hash[String, QueryPackwerk::Package]) + + references = Pks.list_references.map do |reference| + QueryPackwerk::Reference.new( + constant_name: reference.fetch('constant_name'), + file: reference.fetch('file'), + line: reference.fetch('line'), + column: reference.fetch('column'), + producing_pack: packages[reference.fetch('defining_pack_name')] ||= package(reference.fetch('defining_pack_name')), + consuming_pack: packages[reference.fetch('referencing_pack_name')] ||= package(reference.fetch('referencing_pack_name')), + violations: reference.fetch('violations').map do |violation| + QueryPackwerk::Reference::Violation.new( + type: violation.fetch('violation_type'), + strict: violation.fetch('strict'), + recorded: violation.fetch('recorded') + ) + end, + file_cache: file_cache + ) + end + + @all = QueryPackwerk::References.new(references, file_cache: file_cache) + end + + # Wrap the interface `where` with this type + sig do + params( + query_params: T.untyped, # Array, or anything responding to `===`, which can't be typed + query_fn: T.nilable(T.proc.params(arg0: T.untyped).returns(T::Boolean)) + ).returns(QueryPackwerk::References) + end + def where(**query_params, &query_fn) + QueryPackwerk::References.new(super(**query_params, &query_fn)) + end + + # Forgets the references, so the next query runs pks again + sig { void } + def reload! + @all = nil + end + + private + + sig { params(name: String).returns(QueryPackwerk::Package) } + def package(name) + original_package = ParsePackwerk.find(name) + raise Pks::Error, "pks lists references from or to #{name}, which has no package.yml that ParsePackwerk finds" unless original_package + + QueryPackwerk::Package.new(original_package: original_package) + end + end + end +end diff --git a/spec/query_packwerk/file_cache_spec.rb b/spec/query_packwerk/file_cache_spec.rb index 2aae00d..7650c42 100644 --- a/spec/query_packwerk/file_cache_spec.rb +++ b/spec/query_packwerk/file_cache_spec.rb @@ -102,4 +102,38 @@ expect(sources_in('Gemfile.lock')).to eq([]) end end + + describe '#get_full_source_at' do + define_method(:source_at) do |line, column| + file_cache.get_full_source_at(file_name: 'app/models/account.rb', line: line, column: column)&.source + end + + before do + write_file('app/models/account.rb', <<~RUBY) + class Account < ApplicationRecord + has_many :invoices, dependent: :destroy + + def overdue_lines = Billing::Invoice::Line.overdue_for(self) + + def summary = "Échéance ⏰ \#{Billing::Invoice.due_for(self).count}" + end + RUBY + end + + it 'finds the outermost constant that starts there, with the calls chained on it' do + expect(source_at(4, 22)).to eq('Billing::Invoice::Line.overdue_for(self)') + end + + it 'counts the column in bytes, as pks does' do + expect(source_at(6, 34)).to eq('Billing::Invoice.due_for(self).count') + end + + it 'finds the call when no constant starts there, as for an association' do + expect(source_at(2, 2)).to eq('has_many :invoices, dependent: :destroy') + end + + it 'finds nothing when nothing starts there' do + expect(source_at(4, 23)).to be_nil + end + end end diff --git a/spec/query_packwerk/package_spec.rb b/spec/query_packwerk/package_spec.rb index 6129f2f..d6f41d8 100644 --- a/spec/query_packwerk/package_spec.rb +++ b/spec/query_packwerk/package_spec.rb @@ -129,3 +129,30 @@ end end end + +RSpec.describe QueryPackwerk::Package do + include_context 'listed references' + + describe '#incoming_references' do + it "returns references from other packages to this package's constants" do + references = QueryPackwerk.package('packs/honey_production').incoming_references + + expect(references).to be_a(QueryPackwerk::References) + expect(references.count).to eq(8) + end + end + + describe '#outgoing_references' do + it "returns references from this package to other packages' constants" do + references = QueryPackwerk.package('packs/apiary_reports').outgoing_references + + expect(references).to be_a(QueryPackwerk::References) + expect(references.map(&:location)).to eq( + [ + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:4' + ] + ) + end + end +end diff --git a/spec/query_packwerk/pks_spec.rb b/spec/query_packwerk/pks_spec.rb new file mode 100644 index 0000000..216c7b7 --- /dev/null +++ b/spec/query_packwerk/pks_spec.rb @@ -0,0 +1,110 @@ +# typed: false +# frozen_string_literal: true + +require 'shellwords' + +RSpec.describe QueryPackwerk::Pks do + # A stand-in for pks that reports a version and prints references + define_method(:write_pks) do |path, version: '0.7.0', references: [], warning: '', exit_status: 0| + write_file(path, <<~SH) + #!/bin/sh + if [ "$1" = --version ]; then + echo 'pks #{version}' + else + printf '%s' #{Shellwords.escape(warning)} >&2 + echo #{Shellwords.escape(JSON.generate('references' => references))} + exit #{exit_status} + fi + SH + File.chmod(0o755, path) + end + + define_method(:with_path) do |*directories, &block| + original = ENV.fetch('PATH', nil) + ENV['PATH'] = directories.map { |directory| File.expand_path(directory) }.join(File::PATH_SEPARATOR) + block.call + ensure + ENV['PATH'] = original + end + + let(:reference) do + { + 'file' => 'packs/foo/app/services/foo.rb', + 'line' => 3, + 'column' => 4, + 'constant_name' => '::Bar', + 'referencing_pack_name' => 'packs/foo', + 'defining_pack_name' => 'packs/bar', + 'violations' => [] + } + end + + describe '.list_references' do + context 'with pks installed' do + include_context 'referenced packs' + + it 'lists the references that the referenced packs context says it does' do + begin + described_class.executable + rescue QueryPackwerk::Pks::Error => e + skip e.message + end + + expect(described_class.list_references).to eq(listed_references) + end + end + + it 'runs bin/pks, rather than pks on the PATH' do + write_pks('bin/pks', references: [reference]) + write_pks('path/pks', references: []) + + with_path('path') do + expect(described_class.list_references).to eq([reference]) + end + end + + it 'runs pks on the PATH when there is no bin/pks' do + write_pks('path/pks', references: [reference]) + + with_path('empty', 'path') do + expect(described_class.list_references).to eq([reference]) + end + end + + it 'passes on what pks warns about' do + write_pks('bin/pks', warning: "Couldn't parse packs/foo/app/models/foo.rb\n") + + expect { described_class.list_references }.to output("Couldn't parse packs/foo/app/models/foo.rb\n").to_stderr + end + + it 'says how to install pks when there is none' do + with_path('empty') do + expect { described_class.list_references }.to raise_error( + QueryPackwerk::Pks::Error, + "query_packwerk finds references with pks, which isn't in bin/pks or on your PATH. " \ + 'To install it, see https://github.com/rubyatscale/pks/blob/main/INSTALLATION.md' + ) + end + end + + it 'says to upgrade a version of pks without list-references' do + write_pks('bin/pks', version: '0.6.0') + + expect { described_class.list_references }.to raise_error( + QueryPackwerk::Pks::Error, + 'query_packwerk finds references with `pks list-references`, which needs pks 0.7.0 or later, ' \ + "but #{File.expand_path('bin/pks')} is pks 0.6.0. " \ + 'To upgrade it, see https://github.com/rubyatscale/pks/blob/main/INSTALLATION.md' + ) + end + + it 'says what went wrong when pks fails' do + write_pks('bin/pks', warning: "Error: No pack found 'packs/foo'", exit_status: 2) + + expect { described_class.list_references }.to raise_error( + QueryPackwerk::Pks::Error, + "`#{File.expand_path('bin/pks')} list-references` failed:\nError: No pack found 'packs/foo'" + ) + end + end +end diff --git a/spec/query_packwerk/reference_spec.rb b/spec/query_packwerk/reference_spec.rb new file mode 100644 index 0000000..591a159 --- /dev/null +++ b/spec/query_packwerk/reference_spec.rb @@ -0,0 +1,139 @@ +# typed: false +# frozen_string_literal: true + +RSpec.describe QueryPackwerk::Reference do + include_context 'listed references' + + define_method(:reference_at) do |location| + QueryPackwerk::References.all.find { |reference| reference.location == location } + end + + describe 'a reference that pks lists' do + subject(:reference) { reference_at('packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:4') } + + it 'says where it is and which packs it is between' do + expect(reference).to have_attributes( + constant_name: '::HoneyProductionNectar', + class_name: '::HoneyProductionNectar', + file: 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb', + files: ['packs/hive_dashboard/app/services/beekeeping/apiary_status.rb'], + line: 4, + column: 6 + ) + expect(reference.consuming_pack.name).to eq('packs/hive_dashboard') + expect(reference.producing_pack.name).to eq('packs/honey_production') + end + + it 'says which rules it breaks' do + expect(reference.violations).to contain_exactly( + an_instance_of(described_class::Violation).and(have_attributes(type: 'privacy', strict: false, recorded: true)) + ) + expect(reference.violation_types).to eq(['privacy']) + expect(reference).not_to be_allowed + end + + it 'finds its source where pks says it is' do + expect(reference.sources_with_locations).to eq( + [['packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:4', '::HoneyProductionNectar.as_of_now.find_by(apiary_id: 1)']] + ) + expect(reference.anonymous_sources).to eq(['HoneyProductionNectar.as_of_now.find_by(apiary_id: _)']) + expect(reference.count).to eq(1) + end + + it 'can be queried' do + expect(reference.deconstruct_keys(nil)).to include( + constant_name: '::HoneyProductionNectar', + pack_name: 'packs/honey_production', + file: 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb', + allowed: false, + violation_types: ['privacy'], + privacy: true, + dependency: false, + consuming_pack: 'packs/hive_dashboard', + producing_pack: 'packs/honey_production', + producing_owner: 'Honey', + consuming_owner: 'Unowned', + owner: 'Honey', + owned: true + ) + end + + it 'can be inspected' do + expect(reference.inspect).to eq( + '# ::HoneyProductionNectar (privacy) ' \ + 'at packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:4>' + ) + end + end + + it 'is allowed when it breaks no rule' do + reference = reference_at('packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12') + + expect(reference).to be_allowed + expect(reference.inspect).to eq( + '# ::Beekeeping::Nectars (allowed) ' \ + 'at packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12>' + ) + end + + it 'finds a constant that the code names relatively' do + reference = reference_at('packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:8') + + expect(reference.constant_name).to eq('::Beekeeping::NectarSource') + expect(reference.sources.map(&:source)).to eq(['NectarSource.where(apiary_id: 1).pluck(:name)']) + end + + it 'finds a constant after multibyte characters on its line, where pks counts bytes' do + reference = reference_at('packs/apiary_reports/app/services/apiary_report.rb:4') + + expect(reference.column).to eq(31) + expect(reference.sources.map(&:source)).to eq(['HoneyProductionNectar.model_name.human']) + end + + it 'finds the association that pks infers a reference from' do + reference = reference_at('packs/hive_dashboard/app/models/hive.rb:2') + + expect(reference.sources.map(&:source)).to eq(['has_many :honey_production_nectars, dependent: :destroy']) + expect(reference.anonymous_sources).to eq(['has_many _, dependent: _']) + end + + it 'finds nothing when the code at its position has changed since pks ran' do + write_file('packs/hive_dashboard/app/models/hive.rb', "class Hive < ApplicationRecord\nend\n") + + reference = reference_at('packs/hive_dashboard/app/models/hive.rb:2') + + expect(reference.sources).to eq([]) + expect(reference.count).to eq(0) + end + + context 'when the reference is in an ERB template' do + subject(:reference) { reference_at('packs/hive_dashboard/app/views/apiaries/show.html.erb') } + + it 'has no position' do + expect(reference).to have_attributes(line: nil, column: nil, location: 'packs/hive_dashboard/app/views/apiaries/show.html.erb') + end + + it 'finds every place the template names the constant, which pks lists once' do + expect(reference.sources_with_locations).to eq( + [ + ['packs/hive_dashboard/app/views/apiaries/show.html.erb:1', 'HoneyProductionNectar.as_of_now.exists?(apiary_id: @apiary.id)'], + ['packs/hive_dashboard/app/views/apiaries/show.html.erb:2', 'HoneyProductionNectar.as_of_now.find_by(apiary_id: @apiary.id).volume'] + ] + ) + expect(reference.count).to eq(2) + end + + it 'shows context from the template' do + expect(reference.sources_with_contexts(start_offset: 0, end_offset: 0)).to eq( + 'HoneyProductionNectar.as_of_now.exists?(apiary_id: _)' => [ + "> packs/hive_dashboard/app/views/apiaries/show.html.erb:1 (L1..1)\n\n" \ + "<% if HoneyProductionNectar.as_of_now.exists?(apiary_id: @apiary.id) %>\n" + ], + 'HoneyProductionNectar.as_of_now.find_by(apiary_id: _).volume' => [ + "> packs/hive_dashboard/app/views/apiaries/show.html.erb:2 (L2..2)\n\n" \ + "

<%= HoneyProductionNectar.as_of_now.find_by(apiary_id: @apiary.id).volume %>

\n" + ] + ) + end + end +end diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb new file mode 100644 index 0000000..78a6f05 --- /dev/null +++ b/spec/query_packwerk/references_spec.rb @@ -0,0 +1,133 @@ +# typed: false +# frozen_string_literal: true + +RSpec.describe QueryPackwerk::References do + include_context 'listed references' + + describe '.all' do + it 'has each reference that pks lists' do + expect(described_class.all.map(&:location)).to eq( + [ + 'app/services/apiary_summary.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:4', + 'packs/hive_dashboard/app/models/hive.rb:2', + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:4', + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:8', + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12', + 'packs/hive_dashboard/app/views/apiaries/show.html.erb' + ] + ) + end + + it 'runs pks once' do + 2.times { described_class.all } + + expect(QueryPackwerk::Pks).to have_received(:list_references).once + end + + it 'runs pks again after a reload' do + described_class.all + described_class.reload! + described_class.all + + expect(QueryPackwerk::Pks).to have_received(:list_references).twice + end + + it 'says when pks lists a pack that ParsePackwerk does not find' do + listed_references.first['referencing_pack_name'] = 'packs/gone' + + expect { described_class.all }.to raise_error( + QueryPackwerk::Pks::Error, + 'pks lists references from or to packs/gone, which has no package.yml that ParsePackwerk finds' + ) + end + end + + describe '.where' do + it 'finds the references that break no rule' do + expect(described_class.where(allowed: true).map(&:location)).to eq( + [ + 'app/services/apiary_summary.rb:2', + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12' + ] + ) + end + + it 'finds the references that break a rule' do + expect(described_class.where(violation_types: 'dependency').map(&:location)).to eq( + [ + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:4' + ] + ) + end + + it 'finds the references from a pack' do + expect(described_class.where(consuming_pack: '.').map(&:constant_name)).to eq(['::Beekeeping::Nectars']) + end + + it 'finds nothing when pks lists no references' do + listed_references.clear + + expect(described_class.where(producing_pack: 'packs/honey_production')).to be_empty + end + end + + describe '#sources_with_locations' do + it 'gets the sources of each constant, where pks finds them' do + expect(described_class.all.sources_with_locations).to eq( + { + '::Beekeeping::Nectars' => [ + ['app/services/apiary_summary.rb:2', 'Beekeeping::Nectars.status_for(apiary_id: 1)'], + ['packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12', 'Nectars.status_for(apiary_id: 1)'] + ], + '::HoneyProductionNectar' => [ + ['packs/apiary_reports/app/services/apiary_report.rb:2', 'HoneyProductionNectar.count'], + ['packs/apiary_reports/app/services/apiary_report.rb:4', 'HoneyProductionNectar.model_name.human'], + ['packs/hive_dashboard/app/models/hive.rb:2', 'has_many :honey_production_nectars, dependent: :destroy'], + ['packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:4', '::HoneyProductionNectar.as_of_now.find_by(apiary_id: 1)'], + ['packs/hive_dashboard/app/views/apiaries/show.html.erb:1', 'HoneyProductionNectar.as_of_now.exists?(apiary_id: @apiary.id)'], + ['packs/hive_dashboard/app/views/apiaries/show.html.erb:2', 'HoneyProductionNectar.as_of_now.find_by(apiary_id: @apiary.id).volume'] + ], + '::Beekeeping::NectarSource' => [ + ['packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:8', 'NectarSource.where(apiary_id: 1).pluck(:name)'] + ] + } + ) + end + end + + describe '#anonymous_source_counts' do + it "shows how a pack's constants are used where the rules allow it" do + allowed = described_class.where(producing_pack: 'packs/honey_production', allowed: true) + + expect(allowed.anonymous_source_counts).to eq( + '::Beekeeping::Nectars' => { + 'Beekeeping::Nectars.status_for(apiary_id: _)' => 1, + 'Nectars.status_for(apiary_id: _)' => 1 + } + ) + end + end + + describe '#consumers' do + it 'counts the references from each pack' do + expect(described_class.all.consumers).to eq('packs/hive_dashboard' => 5, 'packs/apiary_reports' => 2, '.' => 1) + end + end + + describe '#including_files' do + it 'finds the references in files that match' do + references = described_class.all.including_files('packs/apiary_reports/**') + + expect(references).to be_a(described_class) + expect(references.map(&:location)).to eq( + [ + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:4' + ] + ) + end + end +end diff --git a/spec/query_packwerk_spec.rb b/spec/query_packwerk_spec.rb index c96bf31..1f160ce 100644 --- a/spec/query_packwerk_spec.rb +++ b/spec/query_packwerk_spec.rb @@ -144,3 +144,36 @@ end end end + +RSpec.describe QueryPackwerk do + include_context 'listed references' + + describe '#references_for' do + it "gets all references from other packs to a pack's constants" do + references = described_class.references_for('honey_production') + + expect(references).to be_a(QueryPackwerk::References) + expect(references.count).to eq(8) + end + end + + describe '#references_from' do + it "gets all references from a pack to other packs' constants" do + expect(described_class.references_from('hive_dashboard').map(&:constant_name).tally).to eq( + '::HoneyProductionNectar' => 3, + '::Beekeeping::NectarSource' => 1, + '::Beekeeping::Nectars' => 1 + ) + end + end + + describe '#reload!' do + it 'runs pks again for the next query' do + described_class.references_for('honey_production') + described_class.reload! + described_class.references_for('honey_production') + + expect(QueryPackwerk::Pks).to have_received(:list_references).twice + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b011a67..89de8c9 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -5,6 +5,7 @@ require 'query_packwerk' require_relative 'support/pack_helpers' require_relative 'support/pseudo_packs' +require_relative 'support/referenced_packs' require 'rubocop/rspec/support' require 'packs/rspec/support' require 'packs-specification' diff --git a/spec/support/referenced_packs.rb b/spec/support/referenced_packs.rb new file mode 100644 index 0000000..237b5a6 --- /dev/null +++ b/spec/support/referenced_packs.rb @@ -0,0 +1,143 @@ +# typed: false +# frozen_string_literal: true + +# An app whose packs reference each other's constants in each way that pks +# lists: allowed or not, recorded in a package_todo.yml or not, by a relative +# name, through an association, in ERB, and after multibyte characters. +RSpec.shared_context 'referenced packs' do + # ParsePackwerk and QueryPackwerk keep the packs they find, so they'd keep + # these from, or for, specs that write other packs + around do |example| + ParsePackwerk.bust_cache! + QueryPackwerk.reload! + example.run + ensure + ParsePackwerk.bust_cache! + QueryPackwerk.reload! + end + + before do + write_file('packwerk.yml', "cache: false\n") + write_package_yml('.', enforce_privacy: false, enforce_dependencies: false) + write_package_yml('packs/honey_production', config: { 'owner' => 'Honey' }) + write_package_yml('packs/hive_dashboard', dependencies: ['packs/honey_production']) + write_package_yml('packs/apiary_reports') + + write_package_todo( + pack_name: 'packs/hive_dashboard', + references: { + 'packs/honey_production' => { + '::HoneyProductionNectar' => { + 'violations' => ['privacy'], + 'files' => [ + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb', + 'packs/hive_dashboard/app/views/apiaries/show.html.erb' + ] + } + } + } + ) + write_package_todo( + pack_name: 'packs/apiary_reports', + references: { + 'packs/honey_production' => { + '::HoneyProductionNectar' => { + 'violations' => ['dependency'], + 'files' => ['packs/apiary_reports/app/services/apiary_report.rb'] + } + } + } + ) + + app_files.each { |file_name, source| write_file(file_name, source) } + end + + let(:app_files) do + { + 'packs/honey_production/app/models/honey_production_nectar.rb' => <<~RUBY, + class HoneyProductionNectar < ApplicationRecord + end + RUBY + 'packs/honey_production/app/models/beekeeping/nectar_source.rb' => <<~RUBY, + module Beekeeping + class NectarSource < ApplicationRecord + end + end + RUBY + 'packs/honey_production/app/public/beekeeping/nectars.rb' => <<~RUBY, + module Beekeeping + module Nectars + def self.status_for(apiary_id:); end + end + end + RUBY + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb' => <<~RUBY, + module Beekeeping + class ApiaryStatus + def nectar + ::HoneyProductionNectar.as_of_now.find_by(apiary_id: 1) + end + + def sources + NectarSource.where(apiary_id: 1).pluck(:name) + end + + def status + Nectars.status_for(apiary_id: 1) + end + end + end + RUBY + 'packs/hive_dashboard/app/models/hive.rb' => <<~RUBY, + class Hive < ApplicationRecord + has_many :honey_production_nectars, dependent: :destroy + end + RUBY + 'packs/hive_dashboard/app/views/apiaries/show.html.erb' => <<~ERB, + <% if HoneyProductionNectar.as_of_now.exists?(apiary_id: @apiary.id) %> +

<%= HoneyProductionNectar.as_of_now.find_by(apiary_id: @apiary.id).volume %>

+ <% end %> + ERB + 'packs/apiary_reports/app/services/apiary_report.rb' => <<~'RUBY', + class ApiaryReport + def nectar_count = HoneyProductionNectar.count + + def title = "Récolte 🍯 #{HoneyProductionNectar.model_name.human}" + end + RUBY + 'app/services/apiary_summary.rb' => <<~RUBY + class ApiarySummary + def status = Beekeeping::Nectars.status_for(apiary_id: 1) + end + RUBY + } + end + + # What `pks list-references` prints for the app. spec/query_packwerk/pks_spec.rb + # checks this against pks itself, when it's installed. + let(:listed_references) do + JSON.parse(<<~JSON).fetch('references') + { + "references": [ + {"file": "app/services/apiary_summary.rb", "line": 2, "column": 15, "constant_name": "::Beekeeping::Nectars", "referencing_pack_name": ".", "defining_pack_name": "packs/honey_production", "violations": []}, + {"file": "packs/apiary_reports/app/services/apiary_report.rb", "line": 2, "column": 21, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/apiary_reports", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "dependency", "strict": false, "recorded": true}, {"violation_type": "privacy", "strict": false, "recorded": false}]}, + {"file": "packs/apiary_reports/app/services/apiary_report.rb", "line": 4, "column": 31, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/apiary_reports", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "dependency", "strict": false, "recorded": true}, {"violation_type": "privacy", "strict": false, "recorded": false}]}, + {"file": "packs/hive_dashboard/app/models/hive.rb", "line": 2, "column": 2, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": false}]}, + {"file": "packs/hive_dashboard/app/services/beekeeping/apiary_status.rb", "line": 4, "column": 6, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": true}]}, + {"file": "packs/hive_dashboard/app/services/beekeeping/apiary_status.rb", "line": 8, "column": 6, "constant_name": "::Beekeeping::NectarSource", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": false}]}, + {"file": "packs/hive_dashboard/app/services/beekeeping/apiary_status.rb", "line": 12, "column": 6, "constant_name": "::Beekeeping::Nectars", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": []}, + {"file": "packs/hive_dashboard/app/views/apiaries/show.html.erb", "line": null, "column": null, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": true}]} + ] + } + JSON + end +end + +# Stands in for pks, with what it lists for the referenced packs +RSpec.shared_context 'listed references' do + include_context 'referenced packs' + + before do + allow(QueryPackwerk::Pks).to receive(:list_references).and_return(listed_references) + end +end From 3db056dd2b2d124898c567e9e5f71ae9d89c07d3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:39:20 -0700 Subject: [PATCH 04/24] Keep a source to the code that uses its constant A source is a constant with the calls chained on it, and finding those calls climbed through any parent whose receiver equaled the node. That compared nodes by structure, and blocks and singleton method definitions have receivers too, so in `Foo.bar(Foo)` the second `Foo`'s source was the whole call, in `Foo.each { Foo }` the inner one's was the whole block, and in `def Foo.patched` it was the whole method. Now only a call on the node itself extends it. Violations share the code, so they counted such a call twice, once for each `Foo`, and now don't. Finding a constant by name also matched the `Foo` in `Foo::Bar`, which neither packwerk nor pks counts as a use of `::Foo`, since both resolve `Foo::Bar` by its whole name. So a template that names `Foo::Bar` gave a reference to `::Foo` an extra source, as any file did a violation of `::Foo`. A file that FileCache can't read or parse stood in as `x`, which is a call, so a reference at line 1, column 0 of such a file got `x` as its source. It stands in as `nil` now, which has no constant or call. --- lib/query_packwerk/file_cache.rb | 17 ++++++---- spec/query_packwerk/file_cache_spec.rb | 45 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/lib/query_packwerk/file_cache.rb b/lib/query_packwerk/file_cache.rb index 0acaf43..c588e87 100644 --- a/lib/query_packwerk/file_cache.rb +++ b/lib/query_packwerk/file_cache.rb @@ -68,8 +68,9 @@ def get_file_ast(file_name) source = processed_source(ruby_source(file_name)) warn_unparseable(file_name, source) unless source.valid_syntax? - # The AST is also nil for source with no code, like a template that's only HTML - source.ast || ast_from('x') + # The AST is also nil for source with no code, like a template that's only HTML. + # `nil` stands in, as it has no constant or call that a reference's position could match. + source.ast || ast_from('nil') end end @@ -78,7 +79,7 @@ def get_file(file_name) @file_cache[file_name] ||= if (RUBY_FILE.match?(file_name) || ERB_FILE.match?(file_name)) && File.exist?(file_name) File.read(file_name) else - 'x' + '' end end @@ -93,7 +94,8 @@ def get_all_const_occurrences(file_name:, class_name:) relative_const_node = ast_from(class_name.delete_prefix('::')) @file_const_cache[const_key] = get_file_ast(file_name).each_node.select do |node| - next false unless node.const_type? + # Not the `Foo` in `Foo::Bar`, which packwerk and pks count only as a use of `Foo::Bar` + next false unless node.const_type? && !node.parent&.const_type? node == absolute_const_node || node == relative_const_node end @@ -177,8 +179,9 @@ def ast_from(string) end # We can find a constant, but by going up its parents we can find the full call chain. - # We check if each parent is a receiver of the child, giving us method calls and - # arguments on a constant as well as where it occurred. + # We check that each parent is a call on the child, giving us method calls and + # arguments on a constant as well as where it occurred. The receiver must be the + # child itself, not an equal node, as the first `Foo` is to the second in `Foo.bar(Foo)`. sig { params(node: RuboCop::AST::Node).returns(RuboCop::AST::Node) } def get_full_receiver_chain(node) return node unless node.const_type? @@ -186,7 +189,7 @@ def get_full_receiver_chain(node) current_node = T.let(node, RuboCop::AST::Node) while (parent_node = current_node.parent) - break unless parent_node.receiver == current_node + break unless parent_node.call_type? && parent_node.receiver.equal?(current_node) current_node = parent_node end diff --git a/spec/query_packwerk/file_cache_spec.rb b/spec/query_packwerk/file_cache_spec.rb index 7650c42..364eaaf 100644 --- a/spec/query_packwerk/file_cache_spec.rb +++ b/spec/query_packwerk/file_cache_spec.rb @@ -101,6 +101,37 @@ expect(sources_in('Gemfile.lock')).to eq([]) end + + it 'gets the calls on each use of the constant, rather than on an equal use around it' do + write_file('app/models/account.rb', <<~RUBY) + class Account < ApplicationRecord + def merge = Billing::Invoice.merge(Billing::Invoice) + + def latest = Billing::Invoice.latest.then { Billing::Invoice.latest } + end + RUBY + + expect(sources_in('app/models/account.rb')).to eq( + [ + [2, 'Billing::Invoice.merge(Billing::Invoice)'], + [2, 'Billing::Invoice'], + [4, 'Billing::Invoice.latest.then'], + [4, 'Billing::Invoice.latest'] + ] + ) + end + + it 'finds no use of a constant where it is only the namespace of another' do + write_file('app/models/account.rb', <<~RUBY) + class Account < ApplicationRecord + def billing = Billing.for(self) + + def invoice = Billing::Invoice.for(self) + end + RUBY + + expect(sources_in('app/models/account.rb', class_name: '::Billing')).to eq([[2, 'Billing.for(self)']]) + end end describe '#get_full_source_at' do @@ -135,5 +166,19 @@ def summary = "Échéance ⏰ \#{Billing::Invoice.due_for(self).count}" it 'finds nothing when nothing starts there' do expect(source_at(4, 23)).to be_nil end + + it 'finds the calls on the constant there, rather than on an equal one around it' do + write_file('app/models/account.rb', "def merge = Billing::Invoice.merge(Billing::Invoice)\n") + + expect(source_at(1, 35)).to eq('Billing::Invoice') + end + + it 'finds nothing in a file that does not exist or does not parse' do + write_file('app/models/broken.rb', "Billing::Invoice.find(\n") + + expect(file_cache.get_full_source_at(file_name: 'app/models/missing.rb', line: 1, column: 0)).to be_nil + expect { expect(file_cache.get_full_source_at(file_name: 'app/models/broken.rb', line: 1, column: 0)).to be_nil } + .to output(%r{\ACould not parse app/models/broken\.rb}).to_stderr + end end end From 84f91de94beab227d926d8739e99be35eb0bd3a5 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:41:55 -0700 Subject: [PATCH 05/24] Anonymize a source with a heredoc argument without raising A source is the code of its node, and a heredoc's body isn't in that code, since it's on the lines after the call, as in Foo.execute(1, <<~SQL) SELECT 1 SQL So `Foo.execute(1, <<~SQL)` doesn't parse, and anonymizing it raised NoMethodError on the nil AST, which failed anonymous_sources, anonymous_source_counts, anonymous_sources_with_locations and sources_with_contexts for every violation or reference in a collection. That was so before References, but References include every use of a pack's constants, so one with a heredoc is likelier. Such a source now keeps its arguments as they are. --- lib/query_packwerk/rule_rewriter/rule_set_rewriter.rb | 6 ++++-- spec/query_packwerk/rule_rewriter_spec.rb | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/query_packwerk/rule_rewriter/rule_set_rewriter.rb b/lib/query_packwerk/rule_rewriter/rule_set_rewriter.rb index 6fdcc03..c610b73 100644 --- a/lib/query_packwerk/rule_rewriter/rule_set_rewriter.rb +++ b/lib/query_packwerk/rule_rewriter/rule_set_rewriter.rb @@ -13,7 +13,7 @@ class RuleSetRewriter sig { returns(RuboCop::ProcessedSource) } attr_reader :source - sig { returns(RuboCop::AST::Node) } + sig { returns(T.nilable(RuboCop::AST::Node)) } attr_reader :ast sig { returns(Parser::Source::TreeRewriter) } @@ -35,7 +35,9 @@ def initialize(string, rules: RULES) def process @rules.each do |rule_class| rule = rule_class.new(@rewriter) - @ast.each_node { |node| rule.process(node) } + # nil for a call with a heredoc argument, which doesn't parse without the + # heredoc's body on the lines after it, so its arguments are left as they are + @ast&.each_node { |node| rule.process(node) } end @rewriter diff --git a/spec/query_packwerk/rule_rewriter_spec.rb b/spec/query_packwerk/rule_rewriter_spec.rb index ca4c5c3..64aad99 100644 --- a/spec/query_packwerk/rule_rewriter_spec.rb +++ b/spec/query_packwerk/rule_rewriter_spec.rb @@ -66,4 +66,10 @@ RUBY end end + + context 'With a heredoc argument' do + it 'leaves the source as it is, since it does not parse without the heredoc body after it' do + expect(described_class.rewrite('Model.execute(1, <<~SQL)')).to eq('Model.execute(1, <<~SQL)') + end + end end From 12f66f2f6752871e2c171bb3c2d1d833e69d0903 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:45:29 -0700 Subject: [PATCH 06/24] Let reload! read changed packs, and check pks's version again ParsePackwerk keeps the packs it reads, with the violations in their package_todo.yml files, and reload! didn't make it forget them. So after a change to a package.yml or package_todo.yml file, reload! still gave the violations from before, though the README says it picks up such changes. With References that got worse: pks lists the packs as they are now, so a pack added or renamed since the packs were read made every query of references raise Pks::Error until the console restarted. reload! busts ParsePackwerk's cache now too. pks's version was checked once for each path, so after checking out a branch whose bin/pks is older, the next query failed on `list-references` itself rather than saying to upgrade. The version is checked each time pks runs now, which is once until the next reload!. --- lib/query_packwerk.rb | 2 ++ lib/query_packwerk/pks.rb | 7 +------ spec/query_packwerk/pks_spec.rb | 8 ++++++++ spec/query_packwerk_spec.rb | 32 ++++++++++++++++++++++++++++++++ spec/support/referenced_packs.rb | 6 ++---- 5 files changed, 45 insertions(+), 10 deletions(-) diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index fbe9a38..7753bb7 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -108,6 +108,8 @@ def full_name(pack_name) sig { void } def reload! + # ParsePackwerk keeps the packs it reads, along with their package_todo.yml files + ParsePackwerk.bust_cache! Packages.reload! Violations.reload! References.reload! diff --git a/lib/query_packwerk/pks.rb b/lib/query_packwerk/pks.rb index a743299..3aaf8e4 100644 --- a/lib/query_packwerk/pks.rb +++ b/lib/query_packwerk/pks.rb @@ -17,8 +17,6 @@ class Error < StandardError; end MINIMUM_VERSION = Gem::Version.new('0.7.0') INSTALLATION_URL = 'https://github.com/rubyatscale/pks/blob/main/INSTALLATION.md' - @checked_executables = T.let([], T::Array[String]) - class << self extend T::Sig @@ -53,10 +51,9 @@ def executable private + # Checked each time pks runs, since checking out another branch can change bin/pks sig { params(executable: String).void } def check_version(executable) - return if @checked_executables.include?(executable) - output, status = Open3.capture2e(executable, '--version') version = output[/\Apks (\S+)/, 1] raise Error, "Could not get the version of #{executable}:\n#{output}" unless status.success? && version && Gem::Version.correct?(version) @@ -65,8 +62,6 @@ def check_version(executable) raise Error, "query_packwerk finds references with `pks list-references`, which needs pks #{MINIMUM_VERSION} " \ "or later, but #{executable} is pks #{version}. To upgrade it, see #{INSTALLATION_URL}" end - - @checked_executables << executable end end end diff --git a/spec/query_packwerk/pks_spec.rb b/spec/query_packwerk/pks_spec.rb index 216c7b7..0b8bf97 100644 --- a/spec/query_packwerk/pks_spec.rb +++ b/spec/query_packwerk/pks_spec.rb @@ -98,6 +98,14 @@ ) end + it 'checks the version each time it runs pks, as bin/pks can change with the branch' do + write_pks('bin/pks') + described_class.list_references + write_pks('bin/pks', version: '0.6.0') + + expect { described_class.list_references }.to raise_error(QueryPackwerk::Pks::Error, /is pks 0\.6\.0\./) + end + it 'says what went wrong when pks fails' do write_pks('bin/pks', warning: "Error: No pack found 'packs/foo'", exit_status: 2) diff --git a/spec/query_packwerk_spec.rb b/spec/query_packwerk_spec.rb index 1f160ce..08b6eea 100644 --- a/spec/query_packwerk_spec.rb +++ b/spec/query_packwerk_spec.rb @@ -175,5 +175,37 @@ expect(QueryPackwerk::Pks).to have_received(:list_references).twice end + + it 'reads the package_todo.yml files again' do + expect(described_class.todos_for('apiary_reports').count).to eq(1) + + write_package_todo(pack_name: 'packs/apiary_reports', references: {}) + described_class.reload! + + expect(described_class.todos_for('apiary_reports').count).to eq(0) + end + + it 'finds a pack added since, which pks lists references from' do + described_class.references_for('honey_production') + + write_package_yml('packs/wax_reports') + write_file('packs/wax_reports/app/services/wax_report.rb', <<~RUBY) + class WaxReport + def nectar_count = HoneyProductionNectar.count + end + RUBY + listed_references << { + 'file' => 'packs/wax_reports/app/services/wax_report.rb', 'line' => 2, 'column' => 21, + 'constant_name' => '::HoneyProductionNectar', + 'referencing_pack_name' => 'packs/wax_reports', 'defining_pack_name' => 'packs/honey_production', + 'violations' => [ + { 'violation_type' => 'dependency', 'strict' => false, 'recorded' => false }, + { 'violation_type' => 'privacy', 'strict' => false, 'recorded' => false } + ] + } + described_class.reload! + + expect(described_class.references_for('honey_production').consumers).to include('packs/wax_reports' => 1) + end end end diff --git a/spec/support/referenced_packs.rb b/spec/support/referenced_packs.rb index 237b5a6..cc49e23 100644 --- a/spec/support/referenced_packs.rb +++ b/spec/support/referenced_packs.rb @@ -5,14 +5,12 @@ # lists: allowed or not, recorded in a package_todo.yml or not, by a relative # name, through an association, in ERB, and after multibyte characters. RSpec.shared_context 'referenced packs' do - # ParsePackwerk and QueryPackwerk keep the packs they find, so they'd keep - # these from, or for, specs that write other packs + # QueryPackwerk keeps the packs it finds, so it'd keep these from, or for, + # specs that write other packs around do |example| - ParsePackwerk.bust_cache! QueryPackwerk.reload! example.run ensure - ParsePackwerk.bust_cache! QueryPackwerk.reload! end From 5efecb8e03f43c4fb7ab62a7c0542cd23f3e6f29 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:50:25 -0700 Subject: [PATCH 07/24] Run pks, and read what it lists, in more of the ways apps have it A DotSlash file saved from a pks release, which is how an app pins pks's version in bin/pks, isn't executable until it's made so, and query_packwerk then ran pks on the PATH instead, or said there was no bin/pks. It says to `chmod +x bin/pks` now. The version check read pks's stdout and stderr together and expected the version at the start, so a wrapper that warns first failed it. It reads only stdout now. pks prints UTF-8, but Open3 gives what it prints in the locale's encoding, so with LANG=C, a file or constant with a character outside ASCII made JSON.parse raise Encoding::InvalidByteSequenceError. The output is read as UTF-8 now. Its strings are also frozen, so each one that repeats, as a file's name does for each of its references, is kept once. pks follows a symlink to a pack, but ParsePackwerk's default `**/` glob doesn't, so a symlinked pack made every query of references raise Pks::Error. A pack that ParsePackwerk doesn't find is read from its package.yml now, and only a pack without one raises. The fake pks in the specs prints its JSON with printf, since echo in dash, Ubuntu's /bin/sh, reads the backslashes in it as escapes. --- lib/query_packwerk/pks.rb | 14 +++++--- lib/query_packwerk/references.rb | 6 ++-- spec/query_packwerk/pks_spec.rb | 45 +++++++++++++++++++++++--- spec/query_packwerk/references_spec.rb | 15 +++++++-- 4 files changed, 68 insertions(+), 12 deletions(-) diff --git a/lib/query_packwerk/pks.rb b/lib/query_packwerk/pks.rb index 3aaf8e4..cfd0b87 100644 --- a/lib/query_packwerk/pks.rb +++ b/lib/query_packwerk/pks.rb @@ -30,12 +30,17 @@ def list_references # Warnings, as about a file that pks can't parse $stderr.print(stderr) - JSON.parse(stdout).fetch('references') + # pks prints UTF-8, whatever the locale. Freezing shares the strings of each file and constant. + JSON.parse(stdout.force_encoding(Encoding::UTF_8), freeze: true).fetch('references') end # bin/pks, as in an app that pins a version of pks, or else pks on the PATH sig { returns(String) } def executable + # A DotSlash file saved from a release isn't executable at first. Running + # pks on the PATH instead would ignore the version the app pins. + raise Error, "bin/pks isn't executable. To run it, `chmod +x bin/pks`" if File.file?('bin/pks') && !File.executable?('bin/pks') + candidates = ['bin', *ENV.fetch('PATH', '').split(File::PATH_SEPARATOR)].map do |directory| File.expand_path('pks', directory) end @@ -54,9 +59,10 @@ def executable # Checked each time pks runs, since checking out another branch can change bin/pks sig { params(executable: String).void } def check_version(executable) - output, status = Open3.capture2e(executable, '--version') - version = output[/\Apks (\S+)/, 1] - raise Error, "Could not get the version of #{executable}:\n#{output}" unless status.success? && version && Gem::Version.correct?(version) + # Only stdout, as a wrapper may warn on stderr first + stdout, stderr, status = Open3.capture3(executable, '--version') + version = stdout[/^pks (\S+)$/, 1] + raise Error, "Could not get the version of #{executable}:\n#{stdout}#{stderr}" unless status.success? && version && Gem::Version.correct?(version) if Gem::Version.new(version) < MINIMUM_VERSION raise Error, "query_packwerk finds references with `pks list-references`, which needs pks #{MINIMUM_VERSION} " \ diff --git a/lib/query_packwerk/references.rb b/lib/query_packwerk/references.rb index 8de9c53..7f74e2c 100644 --- a/lib/query_packwerk/references.rb +++ b/lib/query_packwerk/references.rb @@ -68,8 +68,10 @@ def reload! sig { params(name: String).returns(QueryPackwerk::Package) } def package(name) - original_package = ParsePackwerk.find(name) - raise Pks::Error, "pks lists references from or to #{name}, which has no package.yml that ParsePackwerk finds" unless original_package + yml = Pathname.new(name).join(ParsePackwerk::PACKAGE_YML_NAME) + # pks follows a symlink to a pack, where ParsePackwerk's default `**/` glob doesn't + original_package = ParsePackwerk.find(name) || (ParsePackwerk::Package.from(yml) if yml.file?) + raise Pks::Error, "pks lists references from or to #{name}, which has no package.yml" unless original_package QueryPackwerk::Package.new(original_package: original_package) end diff --git a/spec/query_packwerk/pks_spec.rb b/spec/query_packwerk/pks_spec.rb index 0b8bf97..b70c09c 100644 --- a/spec/query_packwerk/pks_spec.rb +++ b/spec/query_packwerk/pks_spec.rb @@ -5,14 +5,14 @@ RSpec.describe QueryPackwerk::Pks do # A stand-in for pks that reports a version and prints references - define_method(:write_pks) do |path, version: '0.7.0', references: [], warning: '', exit_status: 0| + define_method(:write_pks) do |path, version: '0.7.0', version_output: "pks #{version}", references: [], warning: '', exit_status: 0| write_file(path, <<~SH) #!/bin/sh + printf '%s' #{Shellwords.escape(warning)} >&2 if [ "$1" = --version ]; then - echo 'pks #{version}' + printf '%s\\n' #{Shellwords.escape(version_output)} else - printf '%s' #{Shellwords.escape(warning)} >&2 - echo #{Shellwords.escape(JSON.generate('references' => references))} + printf '%s\\n' #{Shellwords.escape(JSON.generate('references' => references))} exit #{exit_status} fi SH @@ -71,12 +71,40 @@ end end + it 'says to make bin/pks executable, rather than run pks on the PATH' do + write_file('bin/pks', "#!/bin/sh\n") + write_pks('path/pks', references: [reference]) + + with_path('path') do + expect { described_class.list_references }.to raise_error( + QueryPackwerk::Pks::Error, "bin/pks isn't executable. To run it, `chmod +x bin/pks`" + ) + end + end + it 'passes on what pks warns about' do write_pks('bin/pks', warning: "Couldn't parse packs/foo/app/models/foo.rb\n") expect { described_class.list_references }.to output("Couldn't parse packs/foo/app/models/foo.rb\n").to_stderr end + it 'gets the version from what pks prints, rather than from what it warns about' do + write_pks('bin/pks', references: [reference], warning: "warning: running pks from ~/.cargo/bin\n") + + expect { expect(described_class.list_references).to eq([reference]) }.to output.to_stderr + end + + it 'reads what pks prints as UTF-8, whatever the locale' do + reference['file'] = 'packs/foo/app/services/récolte.rb' + write_pks('bin/pks', references: [reference]) + original = Encoding.default_external + Encoding.default_external = Encoding::US_ASCII + + expect(described_class.list_references).to eq([reference]) + ensure + Encoding.default_external = original + end + it 'says how to install pks when there is none' do with_path('empty') do expect { described_class.list_references }.to raise_error( @@ -98,6 +126,15 @@ ) end + it 'says when it cannot get the version of pks' do + write_pks('bin/pks', version_output: "error: unexpected argument '--version'") + + expect { described_class.list_references }.to raise_error( + QueryPackwerk::Pks::Error, + "Could not get the version of #{File.expand_path('bin/pks')}:\nerror: unexpected argument '--version'\n" + ) + end + it 'checks the version each time it runs pks, as bin/pks can change with the branch' do write_pks('bin/pks') described_class.list_references diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb index 78a6f05..420a493 100644 --- a/spec/query_packwerk/references_spec.rb +++ b/spec/query_packwerk/references_spec.rb @@ -34,14 +34,25 @@ expect(QueryPackwerk::Pks).to have_received(:list_references).twice end - it 'says when pks lists a pack that ParsePackwerk does not find' do + it 'says when pks lists a pack that has no package.yml' do listed_references.first['referencing_pack_name'] = 'packs/gone' expect { described_class.all }.to raise_error( QueryPackwerk::Pks::Error, - 'pks lists references from or to packs/gone, which has no package.yml that ParsePackwerk finds' + 'pks lists references from or to packs/gone, which has no package.yml' ) end + + it 'finds a pack that a symlink points to, which ParsePackwerk does not' do + Dir.mktmpdir do |shared_pack| + File.write(File.join(shared_pack, 'package.yml'), "enforce_dependencies: true\n") + File.symlink(shared_pack, 'packs/wax_reports') + listed_references.first['referencing_pack_name'] = 'packs/wax_reports' + + expect(ParsePackwerk.find('packs/wax_reports')).to be_nil + expect(described_class.all.first.consuming_pack.name).to eq('packs/wax_reports') + end + end end describe '.where' do From cadeeed511ce9792c6e9484fbd01a4a797722d3b Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:54:46 -0700 Subject: [PATCH 08/24] Parse each file once for every query, until reload! A query, as with where, including_files or excluding_files, built its result with a new FileCache, and pointed the violations or references in it, which all keeps too, at that cache. So asking again, or asking about some of the same code, parsed every file again, and the earlier results' caches stayed in memory. References made that worse, since a pack's references are in more files than its violations are, and each reference is in two packs' results. A result shares all's cache now, so a file is parsed once until reload!, and setting a use's cache to the one it has leaves it loaded. The progress line counted a file once for each use in it, and printed a dot for each, though the file is parsed once. It counts, and dots, each file once now. Finding the node at pks's line and column converted the column of every node on the line from characters to bytes, which on a line with thousands of references, as in a generated list of constants, took minutes. pks's column is converted to characters once instead. --- lib/query_packwerk/constant_use.rb | 3 +++ lib/query_packwerk/constant_uses.rb | 10 +++++++--- lib/query_packwerk/file_cache.rb | 13 +++++++++---- lib/query_packwerk/references.rb | 3 ++- lib/query_packwerk/violations.rb | 3 ++- spec/query_packwerk/file_cache_spec.rb | 8 ++++++++ spec/query_packwerk/references_spec.rb | 14 ++++++++++++++ spec/query_packwerk/violations_spec.rb | 4 ++++ 8 files changed, 49 insertions(+), 9 deletions(-) diff --git a/lib/query_packwerk/constant_use.rb b/lib/query_packwerk/constant_use.rb index e7fabea..90bbd28 100644 --- a/lib/query_packwerk/constant_use.rb +++ b/lib/query_packwerk/constant_use.rb @@ -46,6 +46,9 @@ def load_cache!(headers: false) sig { params(cache: QueryPackwerk::FileCache).void } def set_cache!(cache) + # Each query sets the cache again, and a cache it already has is still loaded + return if cache.equal?(@file_cache) + @cache_loaded = false @file_cache = cache end diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb index dc9bbf1..6d1e4ea 100644 --- a/lib/query_packwerk/constant_uses.rb +++ b/lib/query_packwerk/constant_uses.rb @@ -22,6 +22,9 @@ class ConstantUses sig { override.returns(T::Array[Elem]) } attr_reader :original_collection + sig { returns(QueryPackwerk::FileCache) } + attr_reader :file_cache + sig do params( original_collection: T::Array[Elem], @@ -79,7 +82,8 @@ def load_sources! sig { returns(Integer) } def file_count - @original_collection.sum(&:file_count) + # Once for each file, which is parsed once however many uses are in it + @original_collection.flat_map(&:files).uniq.size end sig { returns(T::Boolean) } @@ -211,7 +215,7 @@ def including_files(*file_globs) T.unsafe(use).includes_files?(*file_globs) # Sorbet hates splats end - T.unsafe(self.class).new(filtered) + self.class.new(filtered, file_cache: @file_cache) end # Filter for those not in files that match one of the provided file globs @@ -221,7 +225,7 @@ def excluding_files(*file_globs) T.unsafe(use).includes_files?(*file_globs) # Sorbet hates splats end - T.unsafe(self.class).new(filtered) + self.class.new(filtered, file_cache: @file_cache) end sig { returns(String) } diff --git a/lib/query_packwerk/file_cache.rb b/lib/query_packwerk/file_cache.rb index c588e87..114f72a 100644 --- a/lib/query_packwerk/file_cache.rb +++ b/lib/query_packwerk/file_cache.rb @@ -41,6 +41,9 @@ def load!(*file_names, headers: true) warn "Prepopulating AST cache with #{file_count} files: " if headers file_names.each do |f| + # A dot for each file parsed, rather than for each use of one + next if @file_ast_cache.key?(f) + get_file_ast(f) $stderr.print '.' end @@ -124,10 +127,12 @@ def get_full_sources(file_name:, class_name:) params(file_name: String, line: Integer, column: Integer).returns(T.nilable(RuboCop::AST::Node)) end def get_full_source_at(file_name:, line:, column:) - nodes = nodes_by_line(file_name).fetch(line, []).select do |node| - range = node.source_range - range.source_line[0, range.column].bytesize == column - end + source_line = get_file_ast(file_name).source_range.source_buffer.source_lines[line - 1] + return unless source_line + + # Converted once, rather than for each node, as on a line with thousands of references + character_column = source_line.byteslice(0, column).to_s.length + nodes = nodes_by_line(file_name).fetch(line, []).select { |node| node.source_range.column == character_column } # The outermost constant, as `Foo::Bar` rather than the `Foo` in it node = nodes.find(&:const_type?) || nodes.find(&:send_type?) diff --git a/lib/query_packwerk/references.rb b/lib/query_packwerk/references.rb index 7f74e2c..76164da 100644 --- a/lib/query_packwerk/references.rb +++ b/lib/query_packwerk/references.rb @@ -55,7 +55,8 @@ def all ).returns(QueryPackwerk::References) end def where(**query_params, &query_fn) - QueryPackwerk::References.new(super(**query_params, &query_fn)) + # all's file cache, so a file is parsed once for every query until reload! + QueryPackwerk::References.new(super(**query_params, &query_fn), file_cache: all.file_cache) end # Forgets the references, so the next query runs pks again diff --git a/lib/query_packwerk/violations.rb b/lib/query_packwerk/violations.rb index 3ecd489..77664c4 100644 --- a/lib/query_packwerk/violations.rb +++ b/lib/query_packwerk/violations.rb @@ -44,7 +44,8 @@ def all ).returns(QueryPackwerk::Violations) end def where(**query_params, &query_fn) - QueryPackwerk::Violations.new(super(**query_params, &query_fn)) + # all's file cache, so a file is parsed once for every query until reload! + QueryPackwerk::Violations.new(super(**query_params, &query_fn), file_cache: all.file_cache) end sig { void } diff --git a/spec/query_packwerk/file_cache_spec.rb b/spec/query_packwerk/file_cache_spec.rb index 364eaaf..cf94c30 100644 --- a/spec/query_packwerk/file_cache_spec.rb +++ b/spec/query_packwerk/file_cache_spec.rb @@ -10,6 +10,14 @@ .map { |node| [node.loc.line, node.source] } end + describe '#load!' do + it 'prints a dot for each file it parses, and none for a file it has' do + write_file('app/models/account.rb', "class Account < ApplicationRecord\nend\n") + + expect { file_cache.load!('app/models/account.rb', 'app/models/account.rb', headers: false) }.to output('.').to_stderr + end + end + describe '#get_full_sources' do it 'finds references in the Ruby code of an ERB template, on their template lines' do write_file('app/views/invoices/show.html.erb', <<~ERB) diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb index 420a493..890945a 100644 --- a/spec/query_packwerk/references_spec.rb +++ b/spec/query_packwerk/references_spec.rb @@ -83,6 +83,20 @@ expect(described_class.where(producing_pack: 'packs/honey_production')).to be_empty end + + it 'keeps the files that all parses, so each query parses a file only once' do + all = described_class.all + + expect(described_class.where(allowed: true).file_cache).to equal(all.file_cache) + expect(all.including_files('app/**').file_cache).to equal(all.file_cache) + expect(all.excluding_files('app/**').file_cache).to equal(all.file_cache) + end + end + + describe '#file_count' do + it 'counts each file once, however many references are in it' do + expect(described_class.all.file_count).to eq(5) + end end describe '#sources_with_locations' do diff --git a/spec/query_packwerk/violations_spec.rb b/spec/query_packwerk/violations_spec.rb index 3a812d3..ca29889 100644 --- a/spec/query_packwerk/violations_spec.rb +++ b/spec/query_packwerk/violations_spec.rb @@ -16,6 +16,10 @@ expect(violations).to eq(['::Beekeeping::HoneyProduction::Nectars']) end + + it 'keeps the files that all parses, so each query parses a file only once' do + expect(described_class.where(privacy: true).file_cache).to equal(described_class.all.file_cache) + end end describe '#empty? and #any?' do From d04a81bc0ad4a03d7308b363c7d6ce257aab323c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:55:45 -0700 Subject: [PATCH 09/24] Take the name of any pack in violations_for, references_for and the rest full_name puts packs/ before any name that doesn't start with packs/ or components/, so violations_for, todos_for, references_for, references_from and the methods that use them found nothing for the root pack, ".", or for a pack elsewhere, as engines/wax. The root pack is often where references come from, as for code in app/. A name that ParsePackwerk finds is kept as it is now. --- lib/query_packwerk.rb | 2 +- spec/query_packwerk_spec.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index 7753bb7..f589bb4 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -101,7 +101,7 @@ def references_from(pack_name) # In case anyone is still using shorthand for pack names sig { params(pack_name: String).returns(String) } def full_name(pack_name) - return pack_name if pack_name.match?(%r{\A(packs|components)/}) + return pack_name if pack_name.match?(%r{\A(packs|components)/}) || ParsePackwerk.find(pack_name) "packs/#{pack_name}" end diff --git a/spec/query_packwerk_spec.rb b/spec/query_packwerk_spec.rb index 08b6eea..9c941c4 100644 --- a/spec/query_packwerk_spec.rb +++ b/spec/query_packwerk_spec.rb @@ -165,6 +165,10 @@ '::Beekeeping::Nectars' => 1 ) end + + it 'takes the name of a pack outside packs/, as the root pack' do + expect(described_class.references_from('.').map(&:location)).to eq(['app/services/apiary_summary.rb:2']) + end end describe '#reload!' do From 6edfbfa0cd88c1a77ddfb1cef1d7b4bc09657fbe Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:58:16 -0700 Subject: [PATCH 10/24] Correct the help text, the README and some comments help_violations and help_references, which copied it, said that sources_with_contexts and anonymous_sources_with_locations return arrays of [file:line, source] pairs, but they return hashes of each constant's anonymized sources, and showed consumers and producers taking a threshold that is a keyword. help_reference lists column, source_counts and anonymous_sources_with_locations too now, and help_references lists raw_sources, as for violations. The README says that reload! is for changes to code too, since a reference's position is where pks found it, and that the parsed files stay in memory until then. A file that doesn't parse is said to have no sources, rather than no violations, since references have them too, and the comments on Reference and ConstantUses say what they are more exactly. --- README.md | 4 ++-- lib/query_packwerk.rb | 3 ++- lib/query_packwerk/console_helpers.rb | 22 +++++++++++++--------- lib/query_packwerk/constant_uses.rb | 6 +++--- lib/query_packwerk/file_cache.rb | 2 +- lib/query_packwerk/reference.rb | 2 +- spec/query_packwerk/file_cache_spec.rb | 2 +- 7 files changed, 23 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index fcf1929..a6abef9 100644 --- a/README.md +++ b/README.md @@ -98,9 +98,9 @@ query_packwerk:001:0> references_from("pack_name") query_packwerk:001:0> References.all ``` -References have the same methods as violations, like `sources_with_locations` and `anonymous_source_counts`. Each one is where pks finds it, so its source is found even where the code names a constant relatively, as `Bar` inside `module Foo`. A reference that pks infers from an association, as `has_many :tacos` does for `Taco`, has the association as its source. pks gives no positions in ERB templates, so there a reference's sources are wherever the template names the constant. Finding sources parses each file they're in, which for a pack that thousands of files reference takes a while. +References have the same methods as violations, like `sources_with_locations` and `anonymous_source_counts`. Each one is where pks finds it, so its source is found even where the code names a constant relatively, as `Bar` inside `module Foo`. A reference that pks infers from an association, as `has_many :tacos` does for `Taco`, has the association as its source. pks gives no positions in ERB templates, so there a reference's sources are wherever the template names the constant. Finding sources parses each file they're in, which for a pack that thousands of files reference takes a while, and keeps each one in memory, about 200 KB for a file of 100 lines, until `reload!`. -If you change any of the package.yml or package_todo.yml files, you can reload the running console: +If you change your code or any of the package.yml or package_todo.yml files, you can reload the running console: ```ruby # Reset the cache, so the next query of references runs pks again diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index f589bb4..f79e14f 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -92,7 +92,8 @@ def references_for(pack_name) end # Get all references from this package to other packages' constants, whether - # or not they break the packages' rules (i.e., how this package uses others) + # or not they break the packages' rules (i.e., how this package uses others). + # These come from pks too. sig { params(pack_name: String).returns(QueryPackwerk::References) } def references_from(pack_name) QueryPackwerk::References.where(consuming_pack: full_name(pack_name)) diff --git a/lib/query_packwerk/console_helpers.rb b/lib/query_packwerk/console_helpers.rb index 3df60fa..43e95e2 100644 --- a/lib/query_packwerk/console_helpers.rb +++ b/lib/query_packwerk/console_helpers.rb @@ -69,7 +69,7 @@ def help_violation .sources_with_locations # Returns array of [file:line, source] pairs for each violation .source_counts # Returns hash of how often each violation source occurs .anonymous_sources # Returns array of violation sources with arguments anonymized - .anonymous_sources_with_locations # Returns array of [file:line, source] pairs for each violation with arguments anonymized + .anonymous_sources_with_locations # Returns hash of anonymized source => [file:line] .count # Returns total number of violation occurrences HELP end @@ -84,11 +84,11 @@ def help_violations .sources # Returns hash of constant => source strings for all violations .sources_with_locations # Returns hash of constant => [file:line, source] pairs .source_counts # Returns hash of constant => {source => count} with occurrence counts - .sources_with_contexts # Returns hash of constant => [file:line, source] pairs for each violation with context + .sources_with_contexts # Returns hash of constant => {anonymized source => [file:line with the lines around it]} .anonymous_sources # Returns hash of constant => anonymized sources - .anonymous_sources_with_locations # Returns array of [file:line, source] pairs for each violation with arguments anonymized - .consumers(threshold) # Returns hash of consuming package names and violation counts - .producers(threshold) # Returns hash of producing package names and violation counts + .anonymous_sources_with_locations # Returns hash of constant => {anonymized source => [file:line]} + .consumers(threshold: 0) # Returns hash of consuming package names and violation counts + .producers(threshold: 0) # Returns hash of producing package names and violation counts .including_files(globs) # Returns new Violations filtered to include specific files .excluding_files(globs) # Returns new Violations filtered to exclude specific files HELP @@ -102,6 +102,7 @@ def help_reference .constant_name # Returns the referenced constant's name .file # Returns the file that makes the reference .line # Returns the line of the reference, or nil in an ERB template + .column # Returns the column of the reference in bytes from 0, or nil in an ERB template .location # Returns file:line, or the file for an ERB template .producing_pack # Returns the Package that defines the constant .consuming_pack # Returns the Package whose file makes the reference @@ -110,7 +111,9 @@ def help_reference .allowed? # Returns whether the reference breaks none of the rules .sources # Returns array of AST nodes representing the reference .sources_with_locations # Returns array of [file:line, source] pairs + .source_counts # Returns hash of how often each source occurs .anonymous_sources # Returns array of sources with arguments anonymized + .anonymous_sources_with_locations # Returns hash of anonymized source => [file:line] .count # Returns how many sources make the reference, which is more than one only in an ERB template HELP end @@ -125,15 +128,16 @@ def help_references References.all # Returns all references References.where(conditions) # Returns new References filtered by conditions, as allowed: true or violation_types: 'privacy' + .raw_sources # Returns hash of constant => AST nodes for all references .sources # Returns hash of constant => source strings for all references .sources_with_locations # Returns hash of constant => [file:line, source] pairs .source_counts # Returns hash of constant => {source => count} with occurrence counts - .sources_with_contexts # Returns hash of constant => [file:line, source] pairs for each reference with context + .sources_with_contexts # Returns hash of constant => {anonymized source => [file:line with the lines around it]} .anonymous_sources # Returns hash of constant => anonymized sources - .anonymous_sources_with_locations # Returns array of [file:line, source] pairs for each reference with arguments anonymized + .anonymous_sources_with_locations # Returns hash of constant => {anonymized source => [file:line]} .anonymous_source_counts # Returns hash of constant => {anonymized source => count} - .consumers(threshold) # Returns hash of consuming package names and reference counts - .producers(threshold) # Returns hash of producing package names and reference counts + .consumers(threshold: 0) # Returns hash of consuming package names and reference counts + .producers(threshold: 0) # Returns hash of producing package names and reference counts .including_files(globs) # Returns new References filtered to include specific files .excluding_files(globs) # Returns new References filtered to exclude specific files HELP diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb index 6d1e4ea..4d81b64 100644 --- a/lib/query_packwerk/constant_uses.rb +++ b/lib/query_packwerk/constant_uses.rb @@ -4,9 +4,9 @@ require 'coderay' module QueryPackwerk - # A collection of ConstantUses, such as Violations or References, with - # methods that group their sources by the constant they use, and that say - # which packs use and define those constants. + # The base of Violations and References, which are collections of ConstantUse + # objects, with methods that group their sources by the constant they use, + # and that say which packs use and define those constants. class ConstantUses extend T::Sig extend T::Generic diff --git a/lib/query_packwerk/file_cache.rb b/lib/query_packwerk/file_cache.rb index 114f72a..e0f9394 100644 --- a/lib/query_packwerk/file_cache.rb +++ b/lib/query_packwerk/file_cache.rb @@ -174,7 +174,7 @@ def warn_unparseable(file_name, source) error = source.diagnostics.find { |diagnostic| diagnostic.level == :error } location = error ? "#{file_name}:#{error.location.line} (#{error.message})" : file_name - warn "Could not parse #{location}, so its violations are not included" + warn "Could not parse #{location}, so its sources are not included" end # Turns a string into a Ruby AST diff --git a/lib/query_packwerk/reference.rb b/lib/query_packwerk/reference.rb index bfbe611..90ae785 100644 --- a/lib/query_packwerk/reference.rb +++ b/lib/query_packwerk/reference.rb @@ -5,7 +5,7 @@ module QueryPackwerk # A reference from one pack to a constant that another pack defines, as pks # lists it, whether or not it breaks the packs' rules. Unlike a Violation, # which package_todo.yml records by file, it's at one place in one file, so - # its sources are the code there. + # its source is the code there, except in ERB, where pks gives no place. class Reference < ConstantUse extend T::Sig diff --git a/spec/query_packwerk/file_cache_spec.rb b/spec/query_packwerk/file_cache_spec.rb index cf94c30..abf489a 100644 --- a/spec/query_packwerk/file_cache_spec.rb +++ b/spec/query_packwerk/file_cache_spec.rb @@ -89,7 +89,7 @@ expect do sources_in('app/views/invoices/show.html.erb') sources_in('app/views/invoices/show.html.erb', class_name: '::Billing::Payment') - end.to output(%r{\ACould not parse app/views/invoices/show\.html\.erb:4 \(.+\), so its violations are not included\n\z}) + end.to output(%r{\ACould not parse app/views/invoices/show\.html\.erb:4 \(.+\), so its sources are not included\n\z}) .to_stderr expect(sources_in('app/views/invoices/show.html.erb')).to eq([]) end From 12d14f10628234c27646512c932e98822b3fa8ae Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:02:38 -0700 Subject: [PATCH 11/24] Check more of what a reference finds where pks lists it A reference finds its sources where pks says it is, but only sources, sources_with_locations and anonymous_sources were checked that way, and on references whose files name their constants once. So count, source_counts, anonymous_sources_with_locations and sources_with_contexts could have found sources by name and passed. They're checked on apiary_report.rb:2 now, whose file names its constant again on line 4, as are its two violations, one that a package_todo.yml records and one that it doesn't. Every reference in the specs' app was to packs/honey_production, so references_for and incoming_references were checked against the count of all references, and producers and owned: false weren't checked. The app has a reference to a pack without an owner now, as pks 0.7.0 lists it. The specs also check a constant with no call on it, as an argument or a superclass, a strict pack, and excluding_files. --- spec/query_packwerk/file_cache_spec.rb | 14 +++++++++++ spec/query_packwerk/reference_spec.rb | 32 +++++++++++++++++++++++ spec/query_packwerk/references_spec.rb | 35 +++++++++++++++++++++++--- spec/query_packwerk_spec.rb | 3 ++- spec/support/referenced_packs.rb | 13 +++++++++- 5 files changed, 92 insertions(+), 5 deletions(-) diff --git a/spec/query_packwerk/file_cache_spec.rb b/spec/query_packwerk/file_cache_spec.rb index abf489a..a20ea5c 100644 --- a/spec/query_packwerk/file_cache_spec.rb +++ b/spec/query_packwerk/file_cache_spec.rb @@ -167,6 +167,20 @@ def summary = "Échéance ⏰ \#{Billing::Invoice.due_for(self).count}" expect(source_at(6, 34)).to eq('Billing::Invoice.due_for(self).count') end + it 'finds a constant that no call is chained on, as an argument or a superclass' do + write_file('app/models/account.rb', <<~RUBY) + class Account < ApplicationRecord + def latest_number = Billing::Invoice.last&.number + + def invoice?(record) = record.is_a?(Billing::Invoice) + end + RUBY + + expect(source_at(1, 16)).to eq('ApplicationRecord') + expect(source_at(2, 22)).to eq('Billing::Invoice.last&.number') + expect(source_at(4, 38)).to eq('Billing::Invoice') + end + it 'finds the call when no constant starts there, as for an association' do expect(source_at(2, 2)).to eq('has_many :invoices, dependent: :destroy') end diff --git a/spec/query_packwerk/reference_spec.rb b/spec/query_packwerk/reference_spec.rb index 591a159..ce338fe 100644 --- a/spec/query_packwerk/reference_spec.rb +++ b/spec/query_packwerk/reference_spec.rb @@ -66,6 +66,38 @@ end end + describe 'a reference in a file that names its constant elsewhere too' do + subject(:reference) { reference_at('packs/apiary_reports/app/services/apiary_report.rb:2') } + + it 'has only the source where pks says it is' do + expect(reference.count).to eq(1) + expect(reference.source_counts).to eq('HoneyProductionNectar.count' => 1) + expect(reference.anonymous_sources_with_locations).to eq( + 'HoneyProductionNectar.count' => ['packs/apiary_reports/app/services/apiary_report.rb:2'] + ) + expect(reference.sources_with_contexts(start_offset: 0, end_offset: 0)).to eq( + 'HoneyProductionNectar.count' => [ + "> packs/apiary_reports/app/services/apiary_report.rb:2 (L2..2)\n\ndef nectar_count = HoneyProductionNectar.count\n" + ] + ) + end + + it 'says which rules it breaks, and which of them a package_todo.yml records' do + expect(reference.violations.map { |violation| [violation.type, violation.recorded] }).to eq( + [['dependency', true], ['privacy', false]] + ) + expect(reference.deconstruct_keys(%i[violation_types dependency privacy])).to eq( + violation_types: %w[dependency privacy], dependency: true, privacy: true + ) + end + end + + it 'says whether the pack that enforces a rule is strict about it' do + listed_references.first['violations'] = [{ 'violation_type' => 'privacy', 'strict' => true, 'recorded' => false }] + + expect(reference_at('app/services/apiary_summary.rb:2').violations.map(&:strict)).to eq([true]) + end + it 'is allowed when it breaks no rule' do reference = reference_at('packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12') diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb index 890945a..405c14d 100644 --- a/spec/query_packwerk/references_spec.rb +++ b/spec/query_packwerk/references_spec.rb @@ -15,6 +15,7 @@ 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:4', 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:8', 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12', + 'packs/hive_dashboard/app/services/report_link.rb:2', 'packs/hive_dashboard/app/views/apiaries/show.html.erb' ] ) @@ -69,11 +70,16 @@ expect(described_class.where(violation_types: 'dependency').map(&:location)).to eq( [ 'packs/apiary_reports/app/services/apiary_report.rb:2', - 'packs/apiary_reports/app/services/apiary_report.rb:4' + 'packs/apiary_reports/app/services/apiary_report.rb:4', + 'packs/hive_dashboard/app/services/report_link.rb:2' ] ) end + it 'finds the references to a pack without an owner' do + expect(described_class.where(owned: false).map(&:constant_name)).to eq(['::ApiaryReportCard']) + end + it 'finds the references from a pack' do expect(described_class.where(consuming_pack: '.').map(&:constant_name)).to eq(['::Beekeeping::Nectars']) end @@ -95,7 +101,7 @@ describe '#file_count' do it 'counts each file once, however many references are in it' do - expect(described_class.all.file_count).to eq(5) + expect(described_class.all.file_count).to eq(6) end end @@ -117,6 +123,9 @@ ], '::Beekeeping::NectarSource' => [ ['packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:8', 'NectarSource.where(apiary_id: 1).pluck(:name)'] + ], + '::ApiaryReportCard' => [ + ['packs/hive_dashboard/app/services/report_link.rb:2', 'ApiaryReportCard.new'] ] } ) @@ -138,7 +147,13 @@ describe '#consumers' do it 'counts the references from each pack' do - expect(described_class.all.consumers).to eq('packs/hive_dashboard' => 5, 'packs/apiary_reports' => 2, '.' => 1) + expect(described_class.all.consumers).to eq('packs/hive_dashboard' => 6, 'packs/apiary_reports' => 2, '.' => 1) + end + end + + describe '#producers' do + it 'counts the references to each pack' do + expect(described_class.all.producers).to eq('packs/honey_production' => 8, 'packs/apiary_reports' => 1) end end @@ -155,4 +170,18 @@ ) end end + + describe '#excluding_files' do + it 'finds the references in files that do not match' do + references = described_class.all.excluding_files('packs/hive_dashboard/**', 'app/**') + + expect(references).to be_a(described_class) + expect(references.map(&:location)).to eq( + [ + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:4' + ] + ) + end + end end diff --git a/spec/query_packwerk_spec.rb b/spec/query_packwerk_spec.rb index 9c941c4..b4f02c7 100644 --- a/spec/query_packwerk_spec.rb +++ b/spec/query_packwerk_spec.rb @@ -162,7 +162,8 @@ expect(described_class.references_from('hive_dashboard').map(&:constant_name).tally).to eq( '::HoneyProductionNectar' => 3, '::Beekeeping::NectarSource' => 1, - '::Beekeeping::Nectars' => 1 + '::Beekeeping::Nectars' => 1, + '::ApiaryReportCard' => 1 ) end diff --git a/spec/support/referenced_packs.rb b/spec/support/referenced_packs.rb index cc49e23..de2fdf7 100644 --- a/spec/support/referenced_packs.rb +++ b/spec/support/referenced_packs.rb @@ -3,7 +3,8 @@ # An app whose packs reference each other's constants in each way that pks # lists: allowed or not, recorded in a package_todo.yml or not, by a relative -# name, through an association, in ERB, and after multibyte characters. +# name, through an association, in ERB, after multibyte characters, and to a +# pack without an owner. RSpec.shared_context 'referenced packs' do # QueryPackwerk keeps the packs it finds, so it'd keep these from, or for, # specs that write other packs @@ -91,6 +92,11 @@ class Hive < ApplicationRecord has_many :honey_production_nectars, dependent: :destroy end RUBY + 'packs/hive_dashboard/app/services/report_link.rb' => <<~RUBY, + class ReportLink + def card = ApiaryReportCard.new + end + RUBY 'packs/hive_dashboard/app/views/apiaries/show.html.erb' => <<~ERB, <% if HoneyProductionNectar.as_of_now.exists?(apiary_id: @apiary.id) %>

<%= HoneyProductionNectar.as_of_now.find_by(apiary_id: @apiary.id).volume %>

@@ -103,6 +109,10 @@ def nectar_count = HoneyProductionNectar.count def title = "Récolte 🍯 #{HoneyProductionNectar.model_name.human}" end RUBY + 'packs/apiary_reports/app/public/apiary_report_card.rb' => <<~RUBY, + class ApiaryReportCard + end + RUBY 'app/services/apiary_summary.rb' => <<~RUBY class ApiarySummary def status = Beekeeping::Nectars.status_for(apiary_id: 1) @@ -124,6 +134,7 @@ def status = Beekeeping::Nectars.status_for(apiary_id: 1) {"file": "packs/hive_dashboard/app/services/beekeeping/apiary_status.rb", "line": 4, "column": 6, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": true}]}, {"file": "packs/hive_dashboard/app/services/beekeeping/apiary_status.rb", "line": 8, "column": 6, "constant_name": "::Beekeeping::NectarSource", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": false}]}, {"file": "packs/hive_dashboard/app/services/beekeeping/apiary_status.rb", "line": 12, "column": 6, "constant_name": "::Beekeeping::Nectars", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": []}, + {"file": "packs/hive_dashboard/app/services/report_link.rb", "line": 2, "column": 13, "constant_name": "::ApiaryReportCard", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/apiary_reports", "violations": [{"violation_type": "dependency", "strict": false, "recorded": false}]}, {"file": "packs/hive_dashboard/app/views/apiaries/show.html.erb", "line": null, "column": null, "constant_name": "::HoneyProductionNectar", "referencing_pack_name": "packs/hive_dashboard", "defining_pack_name": "packs/honey_production", "violations": [{"violation_type": "privacy", "strict": false, "recorded": true}]} ] } From 0f28b6bce5950fce44f7bb6b16d479ba41623b34 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:30:46 -0700 Subject: [PATCH 12/24] Let count and where take a block along with the rest count was an alias of size, so it ignored a block or an item, as in `References.all.count(&:allowed?)`, and gave how many references there are in all. It counts what they match now, as Enumerable does. where gave its block the place of its keys, so `References.where(allowed: true) { |reference| ... }` found what the block matched, allowed or not. The block narrows what the keys find now. --- lib/query_packwerk/query_interface.rb | 18 ++++++++++++++++-- spec/query_packwerk/query_interface_spec.rb | 13 +++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/query_packwerk/query_interface.rb b/lib/query_packwerk/query_interface.rb index 3b42d5d..0853267 100644 --- a/lib/query_packwerk/query_interface.rb +++ b/lib/query_packwerk/query_interface.rb @@ -48,7 +48,19 @@ def size end alias length size - alias count size + + # Not an alias of size, so that an item or a block counts what it matches, as in Enumerable + sig do + params( + args: T.untyped, + block: T.nilable(T.proc.params(arg0: Elem).returns(T.untyped)) + ).returns(Integer) + end + def count(*args, &block) + return size if args.empty? && block.nil? + + T.unsafe(original_collection).count(*args, &block) + end # Extend the class with class extensions whenever this module is # included so we get both singleton and instance methods we need for @@ -109,7 +121,9 @@ def where(**query_params, &_query_fn) raise ArgumentError, "The following keys are invalid for querying: #{invalid_keys.join(', ')}" if invalid_keys.any? all_values.select do |value| - next yield(value) if block_given? + # A block narrows what the keys find, rather than taking their place + next false if block_given? && !yield(value) + next true if query_keys.empty? object_params = value.deconstruct_keys(query_keys) diff --git a/spec/query_packwerk/query_interface_spec.rb b/spec/query_packwerk/query_interface_spec.rb index 320df84..29f7004 100644 --- a/spec/query_packwerk/query_interface_spec.rb +++ b/spec/query_packwerk/query_interface_spec.rb @@ -106,6 +106,14 @@ class << self expect(people.where(hobbies: %w[piano fencing]).count).to eq(2) end + it 'can narrow what conditions find with a block' do + expect(people.where(adult: true) { |a_person| a_person.sex == 'male' }.map(&:name)).to eq(['Bob']) + end + + it 'can find people with only a block' do + expect(people.where { |a_person| a_person.sex == 'male' }.map(&:name)).to eq(%w[Bob Kai]) + end + context 'when there are no people' do let(:all_people) { [] } @@ -119,6 +127,11 @@ class << self it 'can get the count/size/length of a query' do expect(people.all.count).to eq(4) end + + it 'counts what an item or a block matches, as Enumerable does' do + expect(people.all.count { |a_person| a_person.age >= 18 }).to eq(3) + expect(people.all.count(all_people.first)).to eq(1) + end end describe '#each' do From a6d46fc17ec9e55d63189b43c95910771dd6dc29 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:32:29 -0700 Subject: [PATCH 13/24] Give is_active_record false for a class that isn't a model In a Rails app, is_active_record was `constant < ApplicationRecord` for a class, and Class#< gives nil, not false, for a class that isn't related to ApplicationRecord. So `where(is_active_record: false)` found modules and constants that aren't loaded, but no classes. It's false for such a class now. --- lib/query_packwerk/constant_use.rb | 3 ++- spec/query_packwerk/reference_spec.rb | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/query_packwerk/constant_use.rb b/lib/query_packwerk/constant_use.rb index 90bbd28..7d938b2 100644 --- a/lib/query_packwerk/constant_use.rb +++ b/lib/query_packwerk/constant_use.rb @@ -188,7 +188,8 @@ def runtime_keys(keys) value = @file_cache.set( :is_active_record, key: class_name, - value: constant.is_a?(Class) && constant < ApplicationRecord + # Class#< gives nil, not false, for a class that isn't related + value: constant.is_a?(Class) && (constant < ApplicationRecord || false) ) runtime_values[:is_active_record] = value diff --git a/spec/query_packwerk/reference_spec.rb b/spec/query_packwerk/reference_spec.rb index ce338fe..232f028 100644 --- a/spec/query_packwerk/reference_spec.rb +++ b/spec/query_packwerk/reference_spec.rb @@ -98,6 +98,25 @@ expect(reference_at('app/services/apiary_summary.rb:2').violations.map(&:strict)).to eq([true]) end + context 'when the app is loaded with Rails' do + before do + stub_const('Rails', Module.new) + application_record = stub_const('ApplicationRecord', Class.new) + stub_const('HoneyProductionNectar', Class.new(application_record)) + stub_const('ApiaryReportCard', Class.new) + end + + it 'says whether its constant is an Active Record model' do + expect( + [ + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/hive_dashboard/app/services/report_link.rb:2', + 'app/services/apiary_summary.rb:2' + ].map { |location| reference_at(location).deconstruct_keys([:is_active_record]).fetch(:is_active_record) } + ).to eq([true, false, false]) + end + end + it 'is allowed when it breaks no rule' do reference = reference_at('packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12') From 79ce9243504e5d2e13b602210712d67e61cb123e Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:33:30 -0700 Subject: [PATCH 14/24] Type the source methods as what they return sources_with_locations gives [file:line, source] pairs, but its sig said an array of strings, or of strings and arrays of nodes, and violation_sources_for's said the same. So Sorbet let a caller treat each pair as a string, which raised at runtime, since sorbet-runtime doesn't check what's in an array. anonymous_sources_with_locations, sources_with_contexts and raw_sources returned T.untyped. Each says what it returns now. --- lib/query_packwerk.rb | 2 +- lib/query_packwerk/constant_use.rb | 7 +++---- lib/query_packwerk/constant_uses.rb | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index f79e14f..7349ced 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -54,7 +54,7 @@ def todos_for(pack_name) end # Get where the violations occurred (where other packages access this package) - sig { params(pack_name: String).returns(T::Hash[String, T::Array[String]]) } + sig { params(pack_name: String).returns(T::Hash[String, T::Array[[String, String]]]) } def violation_sources_for(pack_name) violations_for(pack_name).sources_with_locations end diff --git a/lib/query_packwerk/constant_use.rb b/lib/query_packwerk/constant_use.rb index 7d938b2..de4acd6 100644 --- a/lib/query_packwerk/constant_use.rb +++ b/lib/query_packwerk/constant_use.rb @@ -81,7 +81,7 @@ def sources end # Adds additional file and line number information to each source - sig { returns(T::Array[T.any(String, T::Array[RuboCop::AST::Node])]) } + sig { returns(T::Array[[String, String]]) } def sources_with_locations load_cache! @@ -112,8 +112,7 @@ def anonymous_sources end end - # sig { returns(T::Array[T.any(String, T::Array[String])]) } - sig { returns(T.untyped) } + sig { returns(T::Hash[String, T::Array[String]]) } def anonymous_sources_with_locations load_cache! @@ -130,7 +129,7 @@ def anonymous_sources_with_locations end end - sig { params(start_offset: Integer, end_offset: Integer).returns(T.untyped) } + sig { params(start_offset: Integer, end_offset: Integer).returns(T::Hash[String, T::Array[String]]) } def sources_with_contexts(start_offset: 3, end_offset: 3) load_cache! diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb index 4d81b64..1e828a9 100644 --- a/lib/query_packwerk/constant_uses.rb +++ b/lib/query_packwerk/constant_uses.rb @@ -97,7 +97,7 @@ def any? end # Gets all sources and their receiving chains grouped by the constant they use. - sig { returns(T.untyped) } + sig { returns(T::Hash[String, T::Array[RuboCop::AST::Node]]) } def raw_sources load_sources! @@ -116,7 +116,7 @@ def sources # In addition to the above also provide the file location and line number along with the # source. - sig { returns(T::Hash[String, T::Array[String]]) } + sig { returns(T::Hash[String, T::Array[[String, String]]]) } def sources_with_locations load_sources! From 132d72fbd60ae8ca236df62a8d050d4dafb8f6b3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:38:27 -0700 Subject: [PATCH 15/24] Read only the keys that where asks for where asks each violation or reference in a collection for the keys it queries, and deconstruct_keys built all of them before slicing out those, so a query of one key cost what a query of all of them did. In a Rails app, that included looking up every reference's constant, for is_active_record and is_constant, though no query asked about them, which can load the constant. Violation and Reference each keep a table of how to read a key now, and ConstantUse's deconstruct_keys reads only the keys asked for, looking constants up only for those two. On an app with 61,000 references, a query of the references to one pack took 0.29 seconds and takes 0.12 now. A query of violations took 0.13 seconds and takes 0.08. --- lib/query_packwerk/constant_use.rb | 22 ++++++++++++++ lib/query_packwerk/reference.rb | 43 +++++++++++++-------------- lib/query_packwerk/violation.rb | 41 +++++++++++++------------ spec/query_packwerk/reference_spec.rb | 8 +++++ 4 files changed, 71 insertions(+), 43 deletions(-) diff --git a/lib/query_packwerk/constant_use.rb b/lib/query_packwerk/constant_use.rb index de4acd6..afc6b3f 100644 --- a/lib/query_packwerk/constant_use.rb +++ b/lib/query_packwerk/constant_use.rb @@ -13,6 +13,7 @@ class ConstantUse abstract! ALL_CAPS = /\A[A-Z_]+\z/ + RUNTIME_KEYS = %i[is_active_record is_constant].freeze sig { abstract.returns(String) } def class_name; end @@ -30,6 +31,10 @@ def consuming_pack; end sig { abstract.params(file_name: String).returns(T::Array[RuboCop::AST::Node]) } def full_sources_in(file_name); end + # The keys that where and pattern matching can ask for, with how to read each + sig { abstract.returns(T::Hash[Symbol, T.proc.params(arg0: T.untyped).returns(T.untyped)]) } + def query_keys; end + sig { params(file_cache: QueryPackwerk::FileCache).void } def initialize(file_cache:) @file_cache = file_cache @@ -171,11 +176,28 @@ def count end end + # Reads only the keys asked for, since where asks every use in a collection + sig do + params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) + end + def deconstruct_keys(keys) + readers = query_keys + values = (keys || readers.keys).each_with_object({}) do |key, key_values| + reader = readers[key] + key_values[key] = reader.call(self) if reader + end + + runtime_values = runtime_keys(keys) + values.merge(keys.nil? ? runtime_values : runtime_values.slice(*T.unsafe(keys))) + end + sig do params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) end def runtime_keys(keys) return {} unless defined?(Rails) + # These load the constant, so only when asked for + return {} unless keys.nil? || keys.intersect?(RUNTIME_KEYS) runtime_values = {} diff --git a/lib/query_packwerk/reference.rb b/lib/query_packwerk/reference.rb index 90ae785..7de14e4 100644 --- a/lib/query_packwerk/reference.rb +++ b/lib/query_packwerk/reference.rb @@ -116,36 +116,35 @@ def full_sources_in(file_name) [@file_cache.get_full_source_at(file_name: file_name, line: line, column: column)].compact end - sig do - params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) - end - def deconstruct_keys(keys) - all_values = { - constant_name: class_name, - pack_name: producing_pack.name, - file: file, + QUERY_KEYS = T.let( + { + constant_name: lambda(&:class_name), + pack_name: ->(reference) { reference.producing_pack.name }, + file: lambda(&:file), # Which of the packs' rules the reference breaks, if any - allowed: allowed?, - violation_types: violation_types, - privacy: violation_types.include?('privacy'), - dependency: violation_types.include?('dependency'), + allowed: lambda(&:allowed?), + violation_types: lambda(&:violation_types), + privacy: ->(reference) { reference.violation_types.include?('privacy') }, + dependency: ->(reference) { reference.violation_types.include?('dependency') }, # Which pack defines the constant, and which pack's code uses it - consuming_pack: consuming_pack.name, - producing_pack: producing_pack.name, + consuming_pack: ->(reference) { reference.consuming_pack.name }, + producing_pack: ->(reference) { reference.producing_pack.name }, - producing_owner: producing_pack.owner, - consuming_owner: consuming_pack.owner, + producing_owner: ->(reference) { reference.producing_pack.owner }, + consuming_owner: ->(reference) { reference.consuming_pack.owner }, # As for violations, "owner" is the producing pack's - owner: producing_pack.owner, - owned: producing_pack.owner != Packages::UNOWNED, - - **runtime_keys(keys) - } + owner: ->(reference) { reference.producing_pack.owner }, + owned: ->(reference) { reference.producing_pack.owner != Packages::UNOWNED } + }.freeze, + T::Hash[Symbol, T.proc.params(arg0: QueryPackwerk::Reference).returns(T.untyped)] + ) - keys.nil? ? all_values : all_values.slice(*T.unsafe(keys)) + sig { override.returns(T::Hash[Symbol, T.proc.params(arg0: QueryPackwerk::Reference).returns(T.untyped)]) } + def query_keys + QUERY_KEYS end sig { returns(String) } diff --git a/lib/query_packwerk/violation.rb b/lib/query_packwerk/violation.rb index 971924c..cc19b3a 100644 --- a/lib/query_packwerk/violation.rb +++ b/lib/query_packwerk/violation.rb @@ -66,37 +66,36 @@ def full_sources_in(file_name) @file_cache.get_full_sources(file_name: file_name, class_name: class_name) end - sig do - params(keys: T.nilable(T::Array[Symbol])).returns(T::Hash[Symbol, T.untyped]) - end - def deconstruct_keys(keys) - all_values = { - constant_name: class_name, - pack_name: to_package_name, + QUERY_KEYS = T.let( + { + constant_name: lambda(&:class_name), + pack_name: lambda(&:to_package_name), # Type related properties, including convenience boolean handlers - type: type, - privacy: type == 'privacy', - dependency: type == 'dependency', + type: lambda(&:type), + privacy: ->(violation) { violation.type == 'privacy' }, + dependency: ->(violation) { violation.type == 'dependency' }, # Reaching into which pack produced the violated constant, and # which consumes the violated constant. - consuming_pack: consuming_pack.name, - producing_pack: producing_pack.name, + consuming_pack: ->(violation) { violation.consuming_pack.name }, + producing_pack: ->(violation) { violation.producing_pack.name }, # Same, except for owners - producing_owner: producing_pack.owner, - consuming_owner: consuming_pack.owner, + producing_owner: ->(violation) { violation.producing_pack.owner }, + consuming_owner: ->(violation) { violation.consuming_pack.owner }, # So why is this "owner" implying producer? Because the # owner field of the violation is producer-oriented. - owner: producing_pack.owner, - owned: producing_pack.owner.nil?, - - **runtime_keys(keys) - } - - keys.nil? ? all_values : all_values.slice(*T.unsafe(keys)) + owner: ->(violation) { violation.producing_pack.owner }, + owned: ->(violation) { violation.producing_pack.owner.nil? } + }.freeze, + T::Hash[Symbol, T.proc.params(arg0: QueryPackwerk::Violation).returns(T.untyped)] + ) + + sig { override.returns(T::Hash[Symbol, T.proc.params(arg0: QueryPackwerk::Violation).returns(T.untyped)]) } + def query_keys + QUERY_KEYS end sig { returns(String) } diff --git a/spec/query_packwerk/reference_spec.rb b/spec/query_packwerk/reference_spec.rb index 232f028..d2a52cc 100644 --- a/spec/query_packwerk/reference_spec.rb +++ b/spec/query_packwerk/reference_spec.rb @@ -115,6 +115,14 @@ ].map { |location| reference_at(location).deconstruct_keys([:is_active_record]).fetch(:is_active_record) } ).to eq([true, false, false]) end + + it 'looks its constant up only for a query that asks about it' do + allow(Kernel).to receive(:const_defined?).and_call_original + + expect(QueryPackwerk::References.where(allowed: true).count).to eq(2) + # Once, as where checks the query's keys against the first reference's + expect(Kernel).to have_received(:const_defined?).once + end end it 'is allowed when it breaks no rule' do From 703eabbc229c13f6808f561bbd1eb414b1751185 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:40:08 -0700 Subject: [PATCH 16/24] Let where find references that break a rule no package_todo.yml records A reference's violations say whether a package_todo.yml records each one, but where couldn't ask, so finding what `pks check` reports took a block. `where(unrecorded: true)` finds the references with such a violation now, and `where(unrecorded_violation_types: 'privacy')` those with one of a type, from Reference#unrecorded_violation_types: References.where(producing_pack: "packs/foo", unrecorded: true).anonymous_source_counts A violation's strictness is left out, as it's how the pack is set up, and a recorded violation of a strict rule isn't what `pks check` reports. --- README.md | 3 +++ lib/query_packwerk/console_helpers.rb | 3 ++- lib/query_packwerk/reference.rb | 10 ++++++++++ spec/query_packwerk/reference_spec.rb | 1 + spec/query_packwerk/references_spec.rb | 15 +++++++++++++++ 5 files changed, 31 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index a6abef9..6ff21bd 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,9 @@ query_packwerk:001:0> references_for("pack_name") # Get the 'shape' of the references to a pack that break no rule query_packwerk:001:0> References.where(producing_pack: "packs/pack_name", allowed: true).anonymous_source_counts +# Get the references to a pack that break a rule no package_todo.yml records, which `pks check` reports +query_packwerk:001:0> References.where(producing_pack: "packs/pack_name", unrecorded: true) + # Get all references from a pack to other packs' constants query_packwerk:001:0> references_from("pack_name") diff --git a/lib/query_packwerk/console_helpers.rb b/lib/query_packwerk/console_helpers.rb index 43e95e2..164c243 100644 --- a/lib/query_packwerk/console_helpers.rb +++ b/lib/query_packwerk/console_helpers.rb @@ -108,6 +108,7 @@ def help_reference .consuming_pack # Returns the Package whose file makes the reference .violations # Returns the rules the reference breaks, with each one's type and whether it's strict and recorded .violation_types # Returns the types of the rules the reference breaks + .unrecorded_violation_types # Returns the types of those rules that no package_todo.yml records, which pks check reports .allowed? # Returns whether the reference breaks none of the rules .sources # Returns array of AST nodes representing the reference .sources_with_locations # Returns array of [file:line, source] pairs @@ -127,7 +128,7 @@ def help_references These come from pks 0.7.0 or later (https://github.com/rubyatscale/pks), in bin/pks or on your PATH. References.all # Returns all references - References.where(conditions) # Returns new References filtered by conditions, as allowed: true or violation_types: 'privacy' + References.where(conditions) # Returns new References filtered by conditions, as allowed: true, violation_types: 'privacy' or unrecorded: true .raw_sources # Returns hash of constant => AST nodes for all references .sources # Returns hash of constant => source strings for all references .sources_with_locations # Returns hash of constant => [file:line, source] pairs diff --git a/lib/query_packwerk/reference.rb b/lib/query_packwerk/reference.rb index 7de14e4..1767c1b 100644 --- a/lib/query_packwerk/reference.rb +++ b/lib/query_packwerk/reference.rb @@ -98,6 +98,12 @@ def violation_types violations.map(&:type) end + # The types of the rules it breaks that no package_todo.yml records, which `pks check` reports + sig { returns(T::Array[String]) } + def unrecorded_violation_types + violations.reject(&:recorded).map(&:type) + end + sig { returns(String) } def location line ? "#{file}:#{line}" : file @@ -128,6 +134,10 @@ def full_sources_in(file_name) privacy: ->(reference) { reference.violation_types.include?('privacy') }, dependency: ->(reference) { reference.violation_types.include?('dependency') }, + # Which of those no package_todo.yml records + unrecorded: ->(reference) { reference.unrecorded_violation_types.any? }, + unrecorded_violation_types: lambda(&:unrecorded_violation_types), + # Which pack defines the constant, and which pack's code uses it consuming_pack: ->(reference) { reference.consuming_pack.name }, producing_pack: ->(reference) { reference.producing_pack.name }, diff --git a/spec/query_packwerk/reference_spec.rb b/spec/query_packwerk/reference_spec.rb index d2a52cc..096529c 100644 --- a/spec/query_packwerk/reference_spec.rb +++ b/spec/query_packwerk/reference_spec.rb @@ -86,6 +86,7 @@ expect(reference.violations.map { |violation| [violation.type, violation.recorded] }).to eq( [['dependency', true], ['privacy', false]] ) + expect(reference.unrecorded_violation_types).to eq(['privacy']) expect(reference.deconstruct_keys(%i[violation_types dependency privacy])).to eq( violation_types: %w[dependency privacy], dependency: true, privacy: true ) diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb index 405c14d..f8d8497 100644 --- a/spec/query_packwerk/references_spec.rb +++ b/spec/query_packwerk/references_spec.rb @@ -76,6 +76,21 @@ ) end + it 'finds the references that break a rule no package_todo.yml records' do + expect(described_class.where(unrecorded: true).map(&:location)).to eq( + [ + 'packs/apiary_reports/app/services/apiary_report.rb:2', + 'packs/apiary_reports/app/services/apiary_report.rb:4', + 'packs/hive_dashboard/app/models/hive.rb:2', + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:8', + 'packs/hive_dashboard/app/services/report_link.rb:2' + ] + ) + expect(described_class.where(unrecorded_violation_types: 'dependency').map(&:location)).to eq( + ['packs/hive_dashboard/app/services/report_link.rb:2'] + ) + end + it 'finds the references to a pack without an owner' do expect(described_class.where(owned: false).map(&:constant_name)).to eq(['::ApiaryReportCard']) end From 82ee789239b13adb1975294ff7f7285fdd472ae0 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:46:32 -0700 Subject: [PATCH 17/24] Print a dot for each file parsed, rather than for each use too Loading a collection's sources printed a dot for each violation or reference in it, after the dots for the files it parsed, and did it for every new query, though the sources load quickly once the files are parsed. For a pack with 24,000 references, that was 24,000 dots each time, even when nothing was parsed. Only the files parsed get a dot now, so a repeated query prints a few lines. --- lib/query_packwerk/constant_uses.rb | 8 +++----- spec/query_packwerk/references_spec.rb | 10 ++++++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb index 1e828a9..2a8834c 100644 --- a/lib/query_packwerk/constant_uses.rb +++ b/lib/query_packwerk/constant_uses.rb @@ -66,13 +66,11 @@ def load_sources! warn end - warn "Prepopulating sources cache with #{count} #{self.class.name.to_s.split('::').last.to_s.downcase}: " + warn "Prepopulating sources cache with #{count} #{self.class.name.to_s.split('::').last.to_s.downcase}" start_time = Time.now - total_sources_loaded = @original_collection.sum do |use| - $stderr.print '.' - use.sources.size - end + # No dot for each use: parsing, which load_cache! dots, is the slow part + total_sources_loaded = @original_collection.sum { |use| use.sources.size } finish_time = Time.now - start_time warn "Loaded #{total_sources_loaded} full sources in #{finish_time}" diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb index f8d8497..730d926 100644 --- a/spec/query_packwerk/references_spec.rb +++ b/spec/query_packwerk/references_spec.rb @@ -114,6 +114,16 @@ end end + describe '#load_sources!' do + it 'prints a dot for each file it parses, rather than for each reference' do + references = described_class.where(producing_pack: 'packs/honey_production') + + expect { references.load_sources! }.to output( + /\APrepopulating AST cache with 5 files: \n\.{5}\nAST cache loaded in \S+\nPrepopulating sources cache with 8 references\nLoaded 9 full sources in \S+\n\z/ + ).to_stderr + end + end + describe '#file_count' do it 'counts each file once, however many references are in it' do expect(described_class.all.file_count).to eq(6) From a2258ccec192e799a585152ba9008a9afd737c8c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 18:50:32 -0700 Subject: [PATCH 18/24] Check the specs' references against pks in CI Every spec of references uses what spec/support/referenced_packs.rb says pks lists for its app, and pks_spec.rb checks that against pks, but only where pks is installed, which CI wasn't. So a change to the app or to that listing that pks wouldn't agree with passed in CI. bin/install-pks downloads pks 0.7.0, the oldest version that query_packwerk supports, into tmp/pks, and checks the archive's sha256 before unpacking it. CI runs it before the specs and puts tmp/pks on the PATH, and the check fails in CI rather than skipping when pks isn't there. It covers Linux on x86_64, as in CI, and macOS on Apple Silicon. --- .github/workflows/ci.yml | 3 +++ README.md | 2 ++ bin/install-pks | 34 +++++++++++++++++++++++++++++++++ spec/query_packwerk/pks_spec.rb | 5 ++++- 4 files changed, 43 insertions(+), 1 deletion(-) create mode 100755 bin/install-pks diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index feed61c..3591ab1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,3 +12,6 @@ jobs: contents: read uses: rubyatscale/shared-config/.github/workflows/ci.yml@main # zizmor: ignore[unpinned-uses,secrets-inherit] internal reusable workflow tracked at @main by convention so shared-config updates propagate automatically; its ci.yml reads SLACK_WEBHOOK_URL but declares no workflow_call secrets, so inherit is the only way to pass it secrets: inherit + with: + # With pks, spec/query_packwerk/pks_spec.rb checks the references that the specs use against what pks lists + test-command: bin/install-pks && PATH="$PWD/tmp/pks:$PATH" bundle exec rspec diff --git a/README.md b/README.md index 6ff21bd..a2943a6 100644 --- a/README.md +++ b/README.md @@ -150,6 +150,8 @@ QueryPackwerk.anonymous_violation_counts_for("my_pack", threshold: 3) After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. +The specs for references use what pks lists for an app they write, and one spec checks that against pks itself. It runs where pks is on your `PATH`, as in CI. To install the version it checks against, run `bin/install-pks` and add `tmp/pks` to your `PATH`. + To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, bump `VERSION` in `lib/query_packwerk/version.rb` in your PR and run `bundle install` so `Gemfile.lock` matches. When the PR is merged and CI passes on `main`, the gem is tagged and published to RubyGems automatically. Merges that don't change the version don't publish anything. ## Contributing diff --git a/bin/install-pks b/bin/install-pks new file mode 100755 index 0000000..5042a35 --- /dev/null +++ b/bin/install-pks @@ -0,0 +1,34 @@ +#!/usr/bin/env bash +# Installs pks in tmp/pks, as CI does, so pks_spec.rb checks the specs' listing against pks. +# Keep version at QueryPackwerk::Pks::MINIMUM_VERSION, with each archive's sha256. +set -euo pipefail + +version=0.7.0 +case "$(uname -sm)" in + 'Linux x86_64') + target=x86_64-unknown-linux-gnu + sha256=51b8fb321a463769d59b99a1be2832fca035c92729e0bc26ecaca88c0dc9baa7 + ;; + 'Darwin arm64') + target=aarch64-apple-darwin + sha256=0c360f17e1d4e617711d9dce5869e2034841fa584a911b1d7b831ce536ed500e + ;; + *) + echo "bin/install-pks has no pks $version for $(uname -sm)" >&2 + exit 1 + ;; +esac + +archive="tmp/pks/pks-$target.tar.xz" +mkdir -p tmp/pks +curl --proto '=https' --tlsv1.2 -fsSL -o "$archive" \ + "https://github.com/rubyatscale/pks/releases/download/v$version/pks-$target.tar.xz" + +actual=$(shasum -a 256 "$archive" 2> /dev/null || sha256sum "$archive") +if [ "${actual%% *}" != "$sha256" ]; then + echo "$archive's sha256 is ${actual%% *}, not $sha256" >&2 + exit 1 +fi + +tar -xJf "$archive" -C tmp/pks --strip-components 1 "pks-$target/pks" +tmp/pks/pks --version diff --git a/spec/query_packwerk/pks_spec.rb b/spec/query_packwerk/pks_spec.rb index b70c09c..cd18553 100644 --- a/spec/query_packwerk/pks_spec.rb +++ b/spec/query_packwerk/pks_spec.rb @@ -47,7 +47,10 @@ begin described_class.executable rescue QueryPackwerk::Pks::Error => e - skip e.message + # CI installs pks with bin/install-pks, so there the check runs rather than skips + raise if ENV['CI'] + + skip "#{e.message}\nTo install it here, run bin/install-pks and put tmp/pks on your PATH." end expect(described_class.list_references).to eq(listed_references) From 8c065bcb4edec3aabfc380bc07c17ffd5ce0ae84 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 19:09:51 -0700 Subject: [PATCH 19/24] Give owned true for a violation of a pack with an owner A violation's owned compared its producing pack's owner with nil, but Package#owner gives 'Unowned' rather than nil for a pack without one, so owned was always false: `where(owned: true)` found no violations, and `where(owned: false)` found them all. It compares the owner with Packages::UNOWNED now, as Package's and Reference's owned do. --- lib/query_packwerk/violation.rb | 2 +- spec/query_packwerk/violations_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/query_packwerk/violation.rb b/lib/query_packwerk/violation.rb index cc19b3a..9d2735d 100644 --- a/lib/query_packwerk/violation.rb +++ b/lib/query_packwerk/violation.rb @@ -88,7 +88,7 @@ def full_sources_in(file_name) # So why is this "owner" implying producer? Because the # owner field of the violation is producer-oriented. owner: ->(violation) { violation.producing_pack.owner }, - owned: ->(violation) { violation.producing_pack.owner.nil? } + owned: ->(violation) { violation.producing_pack.owner != Packages::UNOWNED } }.freeze, T::Hash[Symbol, T.proc.params(arg0: QueryPackwerk::Violation).returns(T.untyped)] ) diff --git a/spec/query_packwerk/violations_spec.rb b/spec/query_packwerk/violations_spec.rb index ca29889..131b893 100644 --- a/spec/query_packwerk/violations_spec.rb +++ b/spec/query_packwerk/violations_spec.rb @@ -101,3 +101,16 @@ def up end end end + +RSpec.describe QueryPackwerk::Violations do + include_context 'referenced packs' + + describe '.where' do + it 'finds the violations of a pack with an owner' do + expect(described_class.where(owned: true).map { |violation| violation.consuming_pack.name }).to contain_exactly( + 'packs/hive_dashboard', 'packs/apiary_reports' + ) + expect(described_class.where(owned: false)).to be_empty + end + end +end From eeb73a03c2f5c95899524323005f7b5143b7eaec Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 19:17:55 -0700 Subject: [PATCH 20/24] Find each pack that pks names through one small lambda References.all kept each pack it found in a hash, so a pack is looked up once however many references name it, but each of the two keyword arguments did that inline, naming its key twice in a line of more than 100 characters. A lambda does it now, so each argument reads as the pack it is. --- lib/query_packwerk/references.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/query_packwerk/references.rb b/lib/query_packwerk/references.rb index 76164da..a4cf4ce 100644 --- a/lib/query_packwerk/references.rb +++ b/lib/query_packwerk/references.rb @@ -24,6 +24,7 @@ def all file_cache = QueryPackwerk::FileCache.new packages = T.let({}, T::Hash[String, QueryPackwerk::Package]) + pack = ->(name) { packages[name] ||= package(name) } references = Pks.list_references.map do |reference| QueryPackwerk::Reference.new( @@ -31,8 +32,8 @@ def all file: reference.fetch('file'), line: reference.fetch('line'), column: reference.fetch('column'), - producing_pack: packages[reference.fetch('defining_pack_name')] ||= package(reference.fetch('defining_pack_name')), - consuming_pack: packages[reference.fetch('referencing_pack_name')] ||= package(reference.fetch('referencing_pack_name')), + producing_pack: pack.call(reference.fetch('defining_pack_name')), + consuming_pack: pack.call(reference.fetch('referencing_pack_name')), violations: reference.fetch('violations').map do |violation| QueryPackwerk::Reference::Violation.new( type: violation.fetch('violation_type'), From 62b8abcbdd4a9d59556d4751fcb2a51aa33da0bc Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 19:55:56 -0700 Subject: [PATCH 21/24] Find a constant that where names without its leading :: package_todo.yml files and pks name each constant from the top, as ::Foo::Bar, so `where(constant_name: 'Foo::Bar')` found nothing, though that's how code names it. Violations and References qualify a constant name that where is given, alone or in an array, before they match it, and leave a pattern such as a Regexp as it is. --- lib/query_packwerk/constant_uses.rb | 29 ++++++++++++++++++++++++++ spec/query_packwerk/references_spec.rb | 10 +++++++++ spec/query_packwerk/violations_spec.rb | 6 ++++++ 3 files changed, 45 insertions(+) diff --git a/lib/query_packwerk/constant_uses.rb b/lib/query_packwerk/constant_uses.rb index 2a8834c..19c7b15 100644 --- a/lib/query_packwerk/constant_uses.rb +++ b/lib/query_packwerk/constant_uses.rb @@ -19,6 +19,35 @@ class ConstantUses include Enumerable include QueryInterface + class << self + extend T::Sig + + # package_todo.yml and pks name a constant from the top, as ::Foo::Bar, so + # `constant_name: 'Foo::Bar'` would find nothing unless it's qualified too + sig do + params( + query_params: T.untyped, + query_fn: T.nilable(T.proc.params(arg0: T.untyped).returns(T::Boolean)) + ).returns(T.untyped) + end + def where(**query_params, &query_fn) + query_params[:constant_name] = qualify(query_params[:constant_name]) if query_params.key?(:constant_name) + + super + end + + private + + sig { params(name: T.untyped).returns(T.untyped) } + def qualify(name) + case name + when String then name.start_with?('::') ? name : "::#{name}" + when Array then name.map { |each_name| qualify(each_name) } + else name + end + end + end + sig { override.returns(T::Array[Elem]) } attr_reader :original_collection diff --git a/spec/query_packwerk/references_spec.rb b/spec/query_packwerk/references_spec.rb index 730d926..96db39c 100644 --- a/spec/query_packwerk/references_spec.rb +++ b/spec/query_packwerk/references_spec.rb @@ -95,6 +95,16 @@ expect(described_class.where(owned: false).map(&:constant_name)).to eq(['::ApiaryReportCard']) end + it 'finds the references to constants named with or without their leading ::' do + expect(described_class.where(constant_name: %w[Beekeeping::Nectars ::ApiaryReportCard]).map(&:location)).to eq( + [ + 'app/services/apiary_summary.rb:2', + 'packs/hive_dashboard/app/services/beekeeping/apiary_status.rb:12', + 'packs/hive_dashboard/app/services/report_link.rb:2' + ] + ) + end + it 'finds the references from a pack' do expect(described_class.where(consuming_pack: '.').map(&:constant_name)).to eq(['::Beekeeping::Nectars']) end diff --git a/spec/query_packwerk/violations_spec.rb b/spec/query_packwerk/violations_spec.rb index 131b893..28a01a1 100644 --- a/spec/query_packwerk/violations_spec.rb +++ b/spec/query_packwerk/violations_spec.rb @@ -17,6 +17,12 @@ expect(violations).to eq(['::Beekeeping::HoneyProduction::Nectars']) end + it 'finds a constant named without its leading ::' do + violations = described_class.where(constant_name: 'Beekeeping::HoneyProduction::Nectars').map(&:class_name) + + expect(violations).to eq(['::Beekeeping::HoneyProduction::Nectars']) + end + it 'keeps the files that all parses, so each query parses a file only once' do expect(described_class.where(privacy: true).file_cache).to equal(described_class.all.file_cache) end From 542f3fe4aae379c8908ec05a87ab778decd0e80f Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 19:56:42 -0700 Subject: [PATCH 22/24] Let package take a pack's short name, as violations_for does violations_for, references_for and the rest take a name in packs/ as honey_production, through full_name, but package didn't, so `package("honey_production")`, as the README writes it, gave nil, and `package("honey_production").incoming_references` raised NoMethodError. package goes through full_name too now. --- lib/query_packwerk.rb | 2 +- spec/query_packwerk_spec.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index 7349ced..8419550 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -36,7 +36,7 @@ module QueryPackwerk sig { params(name: String).returns(T.nilable(QueryPackwerk::Package)) } def package(name) - Packages.where(name: name).first + Packages.where(name: full_name(name)).first end # Get all violations where other packages access code from this package diff --git a/spec/query_packwerk_spec.rb b/spec/query_packwerk_spec.rb index b4f02c7..38496fe 100644 --- a/spec/query_packwerk_spec.rb +++ b/spec/query_packwerk_spec.rb @@ -9,6 +9,10 @@ expect(described_class.package(package_name)).to be_a(QueryPackwerk::Package) end + it 'gets a package by its name in packs/, as violations_for does' do + expect(described_class.package('honey_production')&.name).to eq('packs/honey_production') + end + it 'returns nil if the package does not exist' do expect(described_class.package('not_here')).to be_nil end From 45f03b257ead73591b3669698a6da5a479531b10 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 19:57:52 -0700 Subject: [PATCH 23/24] Return nil from reload!, rather than void, which the console shows reload! and each collection's reload! had `void` sigs, and sorbet-runtime makes a void method return T::Private::Types::Void::VOID, so after `reload!` the console printed `# => T::Private::Types::Void::VOID`. They return nil now, as the console's help methods do, so it prints `# => nil`. --- lib/query_packwerk.rb | 4 +++- lib/query_packwerk/packages.rb | 2 +- lib/query_packwerk/references.rb | 2 +- lib/query_packwerk/violations.rb | 2 +- spec/query_packwerk_spec.rb | 5 +++++ 5 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/query_packwerk.rb b/lib/query_packwerk.rb index 8419550..d94eade 100644 --- a/lib/query_packwerk.rb +++ b/lib/query_packwerk.rb @@ -107,12 +107,14 @@ def full_name(pack_name) "packs/#{pack_name}" end - sig { void } + # nil rather than void, which the console shows as T::Private::Types::Void::VOID + sig { returns(NilClass) } def reload! # ParsePackwerk keeps the packs it reads, along with their package_todo.yml files ParsePackwerk.bust_cache! Packages.reload! Violations.reload! References.reload! + nil end end diff --git a/lib/query_packwerk/packages.rb b/lib/query_packwerk/packages.rb index c6cbd8d..7f9299a 100644 --- a/lib/query_packwerk/packages.rb +++ b/lib/query_packwerk/packages.rb @@ -45,7 +45,7 @@ def where(**query_params, &query_fn) QueryPackwerk::Packages.new(super) end - sig { void } + sig { returns(NilClass) } def reload! @all = nil end diff --git a/lib/query_packwerk/references.rb b/lib/query_packwerk/references.rb index a4cf4ce..89cad3a 100644 --- a/lib/query_packwerk/references.rb +++ b/lib/query_packwerk/references.rb @@ -61,7 +61,7 @@ def where(**query_params, &query_fn) end # Forgets the references, so the next query runs pks again - sig { void } + sig { returns(NilClass) } def reload! @all = nil end diff --git a/lib/query_packwerk/violations.rb b/lib/query_packwerk/violations.rb index 77664c4..64b5c3b 100644 --- a/lib/query_packwerk/violations.rb +++ b/lib/query_packwerk/violations.rb @@ -48,7 +48,7 @@ def where(**query_params, &query_fn) QueryPackwerk::Violations.new(super(**query_params, &query_fn), file_cache: all.file_cache) end - sig { void } + sig { returns(NilClass) } def reload! @all = nil end diff --git a/spec/query_packwerk_spec.rb b/spec/query_packwerk_spec.rb index 38496fe..0f6d8f5 100644 --- a/spec/query_packwerk_spec.rb +++ b/spec/query_packwerk_spec.rb @@ -185,6 +185,11 @@ expect(QueryPackwerk::Pks).to have_received(:list_references).twice end + it 'returns nil, which the console shows plainly, as do the reload! of each collection' do + expect(described_class.reload!).to be_nil + expect([QueryPackwerk::Packages, QueryPackwerk::Violations, QueryPackwerk::References].map(&:reload!)).to eq([nil, nil, nil]) + end + it 'reads the package_todo.yml files again' do expect(described_class.todos_for('apiary_reports').count).to eq(1) From 3749b94233a6c4f00c2671a2ff8c6e105f9d6eb0 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 20:08:55 -0700 Subject: [PATCH 24/24] Read pks's version without requiring the line to end there 5efecb8 read the version from a line of pks's stdout that is only `pks `, so a build that prints more after it, as `pks 0.7.0 (abc1234)`, would fail the check, though it's new enough. The version is the word after `pks ` at the start of a line now, as before 5efecb8, which anchored it at the start of the output. --- lib/query_packwerk/pks.rb | 2 +- spec/query_packwerk/pks_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/query_packwerk/pks.rb b/lib/query_packwerk/pks.rb index cfd0b87..41c8ebb 100644 --- a/lib/query_packwerk/pks.rb +++ b/lib/query_packwerk/pks.rb @@ -61,7 +61,7 @@ def executable def check_version(executable) # Only stdout, as a wrapper may warn on stderr first stdout, stderr, status = Open3.capture3(executable, '--version') - version = stdout[/^pks (\S+)$/, 1] + version = stdout[/^pks (\S+)/, 1] raise Error, "Could not get the version of #{executable}:\n#{stdout}#{stderr}" unless status.success? && version && Gem::Version.correct?(version) if Gem::Version.new(version) < MINIMUM_VERSION diff --git a/spec/query_packwerk/pks_spec.rb b/spec/query_packwerk/pks_spec.rb index cd18553..5b068c1 100644 --- a/spec/query_packwerk/pks_spec.rb +++ b/spec/query_packwerk/pks_spec.rb @@ -129,6 +129,12 @@ ) end + it 'gets the version when pks prints more after it, as a build might' do + write_pks('bin/pks', version_output: 'pks 0.7.0 (abc1234)', references: [reference]) + + expect(described_class.list_references).to eq([reference]) + end + it 'says when it cannot get the version of pks' do write_pks('bin/pks', version_output: "error: unexpected argument '--version'")