diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fd0c11b5..2e5926a82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,6 +67,10 @@ jobs: - name: Run Ruby tests run: bundle exec rake test + - name: Run Rubydex linter + if: matrix.os == 'ubuntu-latest' && matrix.ruby == '4.0' + run: bundle exec rdx lint . + - name: Save Rust compile cache id: rust-compile-cache-save uses: actions/cache/save@v6 diff --git a/ext/rubydex/config.c b/ext/rubydex/config.c index 44043ec71..dd080714e 100644 --- a/ext/rubydex/config.c +++ b/ext/rubydex/config.c @@ -1,4 +1,5 @@ #include "config.h" +#include "diagnostic.h" #include "rustbindings.h" #include "utils.h" @@ -29,9 +30,19 @@ static VALUE config_linter_build(VALUE opaque_rule_array) { for (size_t i = 0; i < rule_array->len; i++) { CLinterRule rule = rule_array->items[i]; VALUE rule_name = rb_str_freeze(rb_utf8_str_new(rule.name, (long)rule.name_length)); - VALUE argv[] = {rule_name, rule.enabled ? Qtrue : Qfalse}; - - rb_hash_aset(rules, rule_name, rb_class_new_instance(2, argv, cRuleConfig)); + VALUE exclude_patterns = rb_ary_new_capa((long)rule.exclude_patterns_length); + for (size_t j = 0; j < rule.exclude_patterns_length; j++) { + CConfigString pattern = rule.exclude_patterns[j]; + rb_ary_push(exclude_patterns, rb_str_freeze(rb_utf8_str_new(pattern.data, (long)pattern.length))); + } + rb_obj_freeze(exclude_patterns); + + VALUE severity = rule.severity == NULL + ? Qnil + : rdxi_build_diagnostic_severity_value(mRubydex, *rule.severity); + VALUE argv[] = {rule_name, rule.enabled ? Qtrue : Qfalse, exclude_patterns, severity}; + + rb_hash_aset(rules, rule_name, rb_class_new_instance(4, argv, cRuleConfig)); } return rb_class_new_instance(1, &rules, cLinterConfig); diff --git a/lib/ruby_lsp/rubydex/addon.rb b/lib/ruby_lsp/rubydex/addon.rb new file mode 100644 index 000000000..6bc0782d3 --- /dev/null +++ b/lib/ruby_lsp/rubydex/addon.rb @@ -0,0 +1,210 @@ +# frozen_string_literal: true + +require "rubydex/linter" + +# This add-on is only supported by the beta version of the LSP. We don't want to keep showing window dialogs to users of +# the stable version until the v0.27 series is stable, so here we are hand-rolling our own `depend_on_ruby_lsp!` method +# to avoid notifying them. +lsp_version = Gem::Version.new(RubyLsp::VERSION) +return unless [">= 0.27.0.beta4", "< 0.28"].all? { |constraint| Gem::Requirement.new(constraint).satisfied_by?(lsp_version) } + +module Rubydex + module Linter + module RubyLsp + class Addon < ::RubyLsp::Addon + CONFIGURATION_FILE = "rubydex.toml" #: String + + # @override + #: () -> String + def name + "Rubydex Linter" + end + + # @override + #: () -> String + def version + ::Rubydex::VERSION + end + + # @override + #: (::RubyLsp::GlobalState, Thread::Queue) -> void + def activate(global_state, outgoing_queue) + @outgoing_queue = outgoing_queue #: Thread::Queue? + @linter = Linter.new(global_state) #: Linter? + + global_state.register_formatter( + "rubydex", + @linter, #: as !nil + ) + + register_additional_file_watchers(global_state, outgoing_queue) + end + + # @override + #: () -> void + def deactivate; end + + #: (Array[{ uri: String, type: Integer }]) -> void + def workspace_did_change_watched_files(changes) + return unless @linter && @outgoing_queue + + @linter.reload_configuration if changes.any? { |change| configuration_change?(change) } + @linter.lint! + + @linter.diagnostics_to_clear.each do |uri| + @outgoing_queue << ::RubyLsp::Notification.publish_diagnostics(uri, []) + end + + @linter.current_diagnostics.each do |uri, diagnostics| + @outgoing_queue << ::RubyLsp::Notification.publish_diagnostics(uri, diagnostics) + end + end + + private + + #: (::RubyLsp::GlobalState, Thread::Queue) -> void + def register_additional_file_watchers(global_state, outgoing_queue) + return unless global_state.client_capabilities.supports_watching_files + + outgoing_queue << ::RubyLsp::Request.new( + id: "rubydex-linter-file-watcher", + method: "client/registerCapability", + params: ::RubyLsp::Interface::RegistrationParams.new( + registrations: [ + ::RubyLsp::Interface::Registration.new( + id: "workspace/didChangeWatchedFilesRubydexLinter", + method: "workspace/didChangeWatchedFiles", + register_options: ::RubyLsp::Interface::DidChangeWatchedFilesRegistrationOptions.new( + watchers: [ + ::RubyLsp::Interface::FileSystemWatcher.new( + glob_pattern: ::RubyLsp::Interface::RelativePattern.new( + base_uri: global_state.workspace_uri.to_s, + pattern: CONFIGURATION_FILE, + ), + kind: ::RubyLsp::Constant::WatchKind::CREATE | ::RubyLsp::Constant::WatchKind::CHANGE, + ), + ], + ), + ), + ], + ), + ) + end + + # The Ruby LSP forwards every watched file change to every add-on, including the ones registered by other add-ons, + # so we have to check that the change is actually about our configuration file. + #: ({ uri: String, type: Integer }) -> bool + def configuration_change?(change) + path = URI(change[:uri]).full_path + return false unless path + + File.basename(path) == CONFIGURATION_FILE + end + end + + class Linter + include ::RubyLsp::Requests::Support::Formatter + + DIAGNOSTIC_SEVERITIES = { + ::Rubydex::Severity::Error => ::RubyLsp::Constant::DiagnosticSeverity::ERROR, + ::Rubydex::Severity::Warning => ::RubyLsp::Constant::DiagnosticSeverity::WARNING, + ::Rubydex::Severity::Information => ::RubyLsp::Constant::DiagnosticSeverity::INFORMATION, + ::Rubydex::Severity::Hint => ::RubyLsp::Constant::DiagnosticSeverity::HINT, + }.freeze #: Hash[singleton(::Rubydex::Severity::Base), Integer] + + #: Hash[String, Array[::RubyLsp::Interface::Diagnostic]] + attr_reader :current_diagnostics + + #: Array[String] + attr_reader :diagnostics_to_clear + + #: (::RubyLsp::GlobalState) -> void + def initialize(global_state) + @graph = global_state.graph #: ::Rubydex::Graph + @workspace_path = global_state.workspace_path #: String + @runner = build_runner #: ::Rubydex::Linter::Runner + + @current_diagnostics = {} #: Hash[String, Array[::RubyLsp::Interface::Diagnostic]] + @diagnostics_to_clear = [] #: Array[String] + end + + # @override + #: (URI::Generic, ::RubyLsp::Document[untyped]) -> Array[::RubyLsp::Interface::Diagnostic]? + def run_diagnostic(uri, _document) + @current_diagnostics[uri.to_s] + end + + # @override + #: (URI::Generic, ::RubyLsp::RubyDocument) -> String? + def run_formatting(uri, document); end + + # @override + #: (URI::Generic, String, Integer) -> String? + def run_range_formatting(uri, source, base_indentation); end + + #: () -> void + def lint! + @diagnostics_to_clear = @current_diagnostics.keys + @current_diagnostics.clear + + @runner.run.diagnostics.each do |diagnostic| + uri = diagnostic.location.uri + (@current_diagnostics[uri] ||= []) << to_lsp_diagnostic(diagnostic) + end + + @diagnostics_to_clear -= @current_diagnostics.keys + end + + #: () -> void + def reload_configuration + @runner = build_runner + end + + private + + #: () -> ::Rubydex::Linter::Runner + def build_runner + config = ::Rubydex::Config.load(@workspace_path) + rules = ::Rubydex::Linter::RuleLoader.load(@workspace_path) + ::Rubydex::Linter::Runner.new(@graph, rules:, config: config.linter) + end + + #: (::Rubydex::Diagnostic) -> ::RubyLsp::Interface::Diagnostic + def to_lsp_diagnostic(diagnostic) + location = diagnostic.location + + ::RubyLsp::Interface::Diagnostic.new( + message: diagnostic.message, + source: "Rubydex", + code: diagnostic.rule, + severity: DIAGNOSTIC_SEVERITIES.fetch(diagnostic.severity), + range: lsp_range(location), + related_information: diagnostic.related_information.map do |information| + ::RubyLsp::Interface::DiagnosticRelatedInformation.new( + location: ::RubyLsp::Interface::Location.new( + uri: information.location.uri, + range: lsp_range(information.location), + ), + message: information.message, + ) + end, + ) + end + + #: (::Rubydex::Location) -> ::RubyLsp::Interface::Range + def lsp_range(location) + ::RubyLsp::Interface::Range.new( + start: ::RubyLsp::Interface::Position.new( + line: location.start_line, + character: location.start_column, + ), + end: ::RubyLsp::Interface::Position.new( + line: location.end_line, + character: location.end_column, + ), + ) + end + end + end + end +end diff --git a/lib/rubydex/cli/command/explain.rb b/lib/rubydex/cli/command/explain.rb new file mode 100644 index 000000000..a723784a2 --- /dev/null +++ b/lib/rubydex/cli/command/explain.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require "rubydex/cli/command" + +module Rubydex + module CLI + # `rdx explain [PATH]` — prints documentation for every discovered rule with that name. + class Command + class Explain < Command + command "explain" + arguments " [PATH]" + summary "Print complete documentation for a linter rule" + + #: -> void + def run + parse_options! + + rule_name = argv.shift + abort_with_usage("`explain` requires a rule name argument") unless rule_name + + workspace_path = File.expand_path(argv.shift || Dir.pwd) + abort_with_usage("unexpected argument: #{argv.first}") unless argv.empty? + abort_with_usage("workspace is not a directory: #{workspace_path}") unless File.directory?(workspace_path) + + require "rubydex/linter" + + rules = load_linter_rules(workspace_path).select { |rule_class| rule_class.rule_name == rule_name } + abort("Rule does not exist: #{rule_name}") if rules.empty? + + graph = Rubydex::Graph.configure_for_workspace(workspace_path) + rule_files = rules.map do |rule_class| + Object.const_source_location(rule_class.name).fetch(0) + end + graph.index_all([File.expand_path("../../linter/rule.rb", __dir__), *rule_files]) + graph.resolve + + puts(rules.sort_by(&:name).map { |rule_class| documentation_for(rule_class, graph) }.join("\n")) + end + + private + + #: (String workspace_path) -> Array[singleton(Rubydex::Linter::Rule)] + def load_linter_rules(workspace_path) + Rubydex::Linter::RuleLoader.load(workspace_path) + rescue Rubydex::Linter::RuleLoadError => error + abort(error.message) + end + + #: (singleton(Rubydex::Linter::Rule) rule_class, Graph graph) -> String + def documentation_for(rule_class, graph) + rule_name = rule_class.name #: as !nil + declaration = graph[rule_name] #: as !nil + documentation = declaration.definitions.flat_map do |definition| + definition.comments.map { |comment| comment.string.gsub(/^#\s*/, "") } + end.join("\n") + + return "#{rule_name}: no documentation available." if documentation.empty? + + <<~DOCUMENTATION + #{rule_name} + + #{documentation} + DOCUMENTATION + end + end + end + end +end diff --git a/lib/rubydex/cli/command/lint.rb b/lib/rubydex/cli/command/lint.rb index d58a7abe9..1d9267de9 100644 --- a/lib/rubydex/cli/command/lint.rb +++ b/lib/rubydex/cli/command/lint.rb @@ -7,8 +7,6 @@ module CLI # `rdx lint [PATH]` — discovers project and dependency rules and runs them against a workspace. class Command class Lint < Command - RULE_GLOB = "rubydex_linter/rules/**/*.rb" #: String - command "lint" arguments "[PATH]" summary "Run semantic lint rules against a workspace" @@ -32,7 +30,12 @@ def run graph = build_graph($stderr, workspace_path:, config:, fail_on_index_errors: true) result = Rubydex::Linter::Runner.new(graph, rules:, config: config.linter).run - result.diagnostics.each { |diagnostic| puts(format_linter_diagnostic(diagnostic)) } + if result.diagnostics.empty? + print_summary(graph.documents.count, result.diagnostics) + return + end + + print_offenses(result.diagnostics, graph) exit(1) unless result.success? end @@ -51,48 +54,103 @@ def warn_unknown_rules(config, known_rule_classes) ) end - #: (String workspace_path) -> Array[singleton(Linter::Rule)] + #: (String workspace_path) -> Array[singleton(Rubydex::Linter::Rule)] def load_linter_rules(workspace_path) - existing_rules = Rubydex::Linter::Rule.subclasses - rule_files = Dir.glob(RULE_GLOB, base: workspace_path).map do |rule_file| - File.expand_path(rule_file, workspace_path) - end - if ENV["BUNDLE_GEMFILE"] - rule_files.concat(Gem.find_latest_files(RULE_GLOB)) - end + Rubydex::Linter::RuleLoader.load(workspace_path) + rescue Rubydex::Linter::RuleLoadError => error + abort(error.message) + end - rule_files.each do |rule_file| - require rule_file - rescue LoadError, SyntaxError => error - abort("Unable to load linter rules from #{rule_file}: #{error.message}") + #: (Array[Diagnostic] diagnostics, Graph graph) -> void + def print_offenses(diagnostics, graph) + puts("Offenses:") + puts + + diagnostics.each do |diagnostic| + puts(format_linter_diagnostic(diagnostic, workspace_path: graph.workspace_path)) + print_source_excerpt(diagnostic.location) + puts end - Rubydex::Linter::Rule.subclasses - existing_rules + print_summary(graph.documents.count, diagnostics) + puts("For more information about a rule, run `rdx explain RuleName`.") end - #: (Location location) -> String - def format_linter_location(location) + #: (Location location, workspace_path: String) -> String + def format_linter_location(location, workspace_path:) display_location = location.to_display - path = begin - display_location.to_file_path - rescue Rubydex::Location::NotFileUriError - display_location.uri - end + path = Rubydex::Linter::Helpers::PathHelpers.display_path(display_location, workspace: workspace_path) "#{path}:#{display_location.start_line}:#{display_location.start_column}" end - #: (Diagnostic diagnostic) -> String - def format_linter_diagnostic(diagnostic) - content = +"#{format_linter_location(diagnostic.location)}: " \ + #: (Diagnostic diagnostic, workspace_path: String) -> String + def format_linter_diagnostic(diagnostic, workspace_path:) + content = +"#{format_linter_location(diagnostic.location, workspace_path:)}: " \ "#{diagnostic.severity.value}: #{diagnostic.rule}: #{diagnostic.message}" diagnostic.related_information.each do |information| - content << "\n #{format_linter_location(information.location)}: #{information.message}" + content << "\n #{format_linter_location(information.location, workspace_path:)}: #{information.message}" end content end + + #: (Location location) -> void + def print_source_excerpt(location) + line = source_line_for(location) + return unless line + + end_column = location.end_line == location.start_line ? location.end_column : line.length + carets = "^" * [end_column - location.start_column, 1].max + + puts + puts(line) + puts("#{" " * location.start_column}#{carets}") + end + + #: (Location location) -> String? + def source_line_for(location) + source_lines_for(location.to_file_path)[location.start_line] + rescue Errno::ENOENT, Errno::EACCES, Rubydex::Location::NotFileUriError + nil + end + + #: (String path) -> Array[String] + def source_lines_for(path) + @source_lines_cache ||= {} #: Hash[String, Array[String]]? + @source_lines_cache[path] ||= File.readlines(path, chomp: true) + end + + #: (Integer file_count, Array[Diagnostic] diagnostics) -> void + def print_summary(file_count, diagnostics) + if diagnostics.empty? + puts("#{file_count} #{pluralize("file", file_count)} inspected, no offenses detected") + return + end + + severity_counts = diagnostics.map(&:severity).tally + offense_count = diagnostics.length + error_count = severity_counts.fetch(Rubydex::Severity::Error, 0) + warning_count = severity_counts.fetch(Rubydex::Severity::Warning, 0) + hint_count = severity_counts.fetch(Rubydex::Severity::Hint, 0) + severity_summary = [ + "#{error_count} #{pluralize("error", error_count)}", + "#{warning_count} #{pluralize("warning", warning_count)}", + "#{severity_counts.fetch(Rubydex::Severity::Information, 0)} info", + "#{hint_count} #{pluralize("hint", hint_count)}", + ].join(", ") + + puts( + "#{file_count} #{pluralize("file", file_count)} inspected, " \ + "#{offense_count} #{pluralize("offense", offense_count)} detected: #{severity_summary}", + ) + end + + #: (String word, Integer count) -> String + def pluralize(word, count) + count == 1 ? word : "#{word}s" + end end end end diff --git a/lib/rubydex/cli/command/list.rb b/lib/rubydex/cli/command/list.rb new file mode 100644 index 000000000..25194c579 --- /dev/null +++ b/lib/rubydex/cli/command/list.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "pathname" +require "uri" +require "rubydex/cli/command" + +module Rubydex + module CLI + # `rdx list [docs|roots] [PATH]` — prints the files or roots used to index a workspace. + class Command + class List < Command + command "list" + arguments "[docs|roots] [PATH]" + summary "Print indexed documents or graph roots" + + #: -> void + def run + parse_options! + + kind = argv.shift || "docs" + workspace_path = File.expand_path(argv.shift || Dir.pwd) + abort_with_usage("unexpected argument: #{argv.first}") unless argv.empty? + abort_with_usage("workspace is not a directory: #{workspace_path}") unless File.directory?(workspace_path) + + config = Rubydex::Config.load(workspace_path) + graph = Rubydex::Graph.new + graph.load_config(config) + + case kind + when "docs" + graph.index_workspace + + graph.documents + .map { |document| display_path_for_uri(document.uri, workspace_path:) } + .sort + .each { |path| puts(path) } + when "roots" + (graph.workspace_paths - graph.excluded_patterns) + .map { |path| display_path_for_path(path, workspace_path:) } + .sort + .each { |path| puts(path) } + else + abort_with_usage("Unknown list target: #{kind}. Expected `docs` or `roots`.") + end + end + + private + + #: (String uri, workspace_path: String) -> String + def display_path_for_uri(uri, workspace_path:) + parsed_uri = URI(uri) + path = parsed_uri.path + return uri unless parsed_uri.scheme == "file" && path + + path.delete_prefix!("/") if Gem.win_platform? + display_path_for_path(path, workspace_path:) + end + + #: (String path, workspace_path: String) -> String + def display_path_for_path(path, workspace_path:) + relative_path = Pathname.new(path).relative_path_from(Pathname.new(workspace_path)).to_s + relative_path.start_with?("../") ? path : relative_path + end + end + end + end +end diff --git a/lib/rubydex/config.rb b/lib/rubydex/config.rb index 5b001d052..9afcd050b 100644 --- a/lib/rubydex/config.rb +++ b/lib/rubydex/config.rb @@ -20,6 +20,16 @@ def rule_enabled?(rule_class) rule = @rules[rule_class.rule_name] !rule || rule.enabled? end + + #: (singleton(Linter::Rule) rule_class) -> Array[String] + def excludes_for(rule_class) + @rules[rule_class.rule_name]&.exclude_patterns || [] + end + + #: (singleton(Linter::Rule) rule_class, default: singleton(Severity::Base)) -> singleton(Severity::Base) + def severity_for(rule_class, default:) + @rules[rule_class.rule_name]&.severity || default + end end # The settings of a single linter rule, read from a `[linter.rules.RuleName]` table. @@ -27,10 +37,18 @@ class RuleConfig #: String attr_reader :name - #: (String, bool) -> void - def initialize(name, enabled) + #: Array[String] + attr_reader :exclude_patterns + + #: singleton(Severity::Base)? + attr_reader :severity + + #: (String, bool, ?Array[String], ?singleton(Severity::Base)?) -> void + def initialize(name, enabled, exclude_patterns = [], severity = nil) @name = name @enabled = enabled + @exclude_patterns = exclude_patterns + @severity = severity end #: () -> bool diff --git a/lib/rubydex/linter.rb b/lib/rubydex/linter.rb index e53e874de..58e39f6fd 100644 --- a/lib/rubydex/linter.rb +++ b/lib/rubydex/linter.rb @@ -1,8 +1,10 @@ # frozen_string_literal: true require "rubydex" +require "rubydex/linter/helpers/path_helpers" require "rubydex/linter/result" require "rubydex/linter/rule" +require "rubydex/linter/rule_loader" require "rubydex/linter/runner" module Rubydex diff --git a/lib/rubydex/linter/helpers/path_helpers.rb b/lib/rubydex/linter/helpers/path_helpers.rb new file mode 100644 index 000000000..8e42ca8c6 --- /dev/null +++ b/lib/rubydex/linter/helpers/path_helpers.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require "pathname" +require "uri" + +module Rubydex + module Linter + module Helpers + # @requires_ancestor: Rubydex::Linter::Rule + module PathHelpers + RUBOCOP_EXCLUDE_FNMATCH_FLAGS = + File::FNM_PATHNAME | File::FNM_EXTGLOB | File::FNM_DOTMATCH #: Integer + TEST_PATHS = ["test/**", "**/test/**", "**/*_test.rb"].freeze + + class << self + #: (String, Array[String], workspace: String, ?flags: Integer) -> bool + def path_matches_patterns?(path, patterns, workspace:, flags: 0) + return false unless path == workspace || path.start_with?("#{workspace}/") + + relative_path = path.delete_prefix("#{workspace}/") + patterns.any? { |pattern| File.fnmatch?(pattern, relative_path, flags) } + end + + # Returns the path to show users for a location: relative to the workspace for files inside it, the absolute + # path for files outside it (dependencies, for example) and the URI opaque for non file URIs, so that + # `untitled:Untitled-1` displays as `Untitled-1`. + # + #: (Location, workspace: String) -> String + def display_path(location, workspace:) + path = location.to_file_path + return path unless path == workspace || path.start_with?("#{workspace}/") + + Pathname.new(path).relative_path_from(workspace).to_s + rescue Location::NotFileUriError + URI(location.uri).opaque || location.uri + end + end + + #: (Enumerable[Rubydex::Definition], Array[String]) -> Array[Rubydex::Definition] + def reject_definitions_in_paths(definitions, excluded_patterns) + workspace = graph.workspace_path + + definitions.reject do |definition| + path = path_for_definition(definition) + path.nil? || (path != workspace && !path.start_with?("#{workspace}/")) || + path_matches_patterns?(path, excluded_patterns) + end + end + + #: (Enumerable[Rubydex::Definition], Array[String]) -> Array[Rubydex::Definition] + def select_definitions_in_paths(definitions, patterns) + definitions.select do |definition| + path = path_for_definition(definition) + path && path_matches_patterns?(path, patterns) + end + end + + private + + #: (String, Array[String]) -> bool + def path_matches_patterns?(path, patterns) + PathHelpers.path_matches_patterns?(path, patterns, workspace: graph.workspace_path) + end + + #: (String) -> bool + def test_path?(path) + path_matches_patterns?(path, TEST_PATHS) + end + + #: (Rubydex::Definition) -> String? + def path_for_definition(definition) + definition.location.to_file_path + rescue Location::NotFileUriError + nil + end + end + end + end +end diff --git a/lib/rubydex/linter/rule.rb b/lib/rubydex/linter/rule.rb index 04d0c77d2..592a189e4 100644 --- a/lib/rubydex/linter/rule.rb +++ b/lib/rubydex/linter/rule.rb @@ -8,7 +8,8 @@ class Rule class << self #: () -> String def rule_name - name.split("::").last + name #: as !nil + .split("::").last #: as !nil end end @@ -26,6 +27,7 @@ def initialize(graph, config:) @graph = graph @config = config @diagnostics = [] #: Array[Diagnostic] + @verified_severity = nil #: singleton(Severity::Base)? end # @abstract @@ -34,12 +36,67 @@ def severity raise NotImplementedError, "Subclasses must implement the severity method" end + #: () -> singleton(Severity::Base) + def verified_severity + @verified_severity ||= config.severity_for(self.class, default: severity) + end + # @abstract #: () -> void def lint raise NotImplementedError, "Subclasses must implement the lint method" end + # Anchors a diagnostic on a definition's name token, falling back to its full range when no name location exists. + #: (Definition) -> Location + def diagnostic_location(definition) + definition.name_location || definition.location + end + + #: (String) -> Location + def file_location(uri) + Location.new(uri: uri, start_line: 0, end_line: 0, start_column: 0, end_column: 0) + end + + #: (String) -> String + def path_for_uri(uri) + file_location(uri).to_file_path + rescue Location::NotFileUriError + uri + end + + # Returns every class inheriting from +base_name+, excluding the base class itself. + #: (String) -> Enumerable[Rubydex::Class] + def child_classes(base_name) + required_namespace(base_name).descendants.lazy.filter_map do |child_declaration| + next unless child_declaration.is_a?(Rubydex::Class) + next if child_declaration.name == base_name + + child_declaration + end + end + + #: (String) -> Namespace + def required_namespace(name) + declaration = graph[name] + return declaration if declaration.is_a?(Namespace) + + raise MissingGraphDependencyError.new(rule_name, "`#{name}`") + end + + #: (Namespace, String) -> Rubydex::Method + def required_method(namespace, method_name) + method = namespace.find_member(method_name) + return method if method.is_a?(Rubydex::Method) + + raise MissingGraphDependencyError.new(rule_name, "`#{namespace.name}##{method_name}`") + end + + #: () -> String + def rule_name + self.class.rule_name + end + protected #: ( @@ -52,10 +109,30 @@ def add_diagnostic(message, location, related_information: []) rule: self.class.rule_name, message: message, location: location, - severity: severity, + severity: verified_severity, related_information: related_information, ) end + + private + + #: (Location) -> String? + def source_for_location(location) + File.read(location.to_file_path) + rescue Errno::ENOENT + nil + end + end + + class MissingGraphDependencyError < StandardError + #: (String, String) -> void + def initialize(rule_name, dependency) + super( + "Rubydex linter rule `#{rule_name}` requires #{dependency} to exist in the Rubydex graph. " \ + "This is a rule setup error, not a clean lint result; ensure the source that defines " \ + "the dependency is indexed and Rubydex can resolve it.", + ) + end end end end diff --git a/lib/rubydex/linter/rule_loader.rb b/lib/rubydex/linter/rule_loader.rb new file mode 100644 index 000000000..fe5883230 --- /dev/null +++ b/lib/rubydex/linter/rule_loader.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Rubydex + module Linter + # Loads project and bundled-gem rules using the Rubydex linter path convention. + class RuleLoader + RULE_GLOB = "rubydex_linter/rules/**/*.rb" #: String + BUILT_IN_RULE_GLOB = File.expand_path("../../rubydex_linter/rules/**/*.rb", __dir__) #: String + + class << self + #: (String workspace_path) -> Array[singleton(Rule)] + def load(workspace_path) + built_in_rule_files = Dir.glob(BUILT_IN_RULE_GLOB) + workspace_rule_files = Dir.glob(RULE_GLOB, base: workspace_path).map do |rule_file| + File.expand_path(rule_file, workspace_path) + end + dependency_rule_files = if ENV["BUNDLE_GEMFILE"] + Gem.find_latest_files(RULE_GLOB) + else + [] + end + + rule_files = built_in_rule_files + workspace_rule_files + dependency_rule_files + rule_files.each do |rule_file| + require rule_file + rescue LoadError, SyntaxError => error + raise RuleLoadError, "Unable to load linter rules from #{rule_file}: #{error.message}", cause: error + end + + expanded_rule_files = rule_files.map { |rule_file| File.expand_path(rule_file) } + descendants_of(Rule).select do |rule_class| + rule_name = rule_class.name #: as !nil + source_location = Object.const_source_location(rule_name) + source_location && expanded_rule_files.include?(File.expand_path(source_location.fetch(0))) + end + end + + private + + #: (singleton(Rule)) -> Array[singleton(Rule)] + def descendants_of(parent) + subclasses = parent.subclasses #: as Array[singleton(Rule)] + subclasses.flat_map do |subclass| + [subclass, *descendants_of(subclass)] + end + end + end + end + + class RuleLoadError < StandardError; end + end +end diff --git a/lib/rubydex/linter/rule_test_case.rb b/lib/rubydex/linter/rule_test_case.rb new file mode 100644 index 000000000..7e2ceab2d --- /dev/null +++ b/lib/rubydex/linter/rule_test_case.rb @@ -0,0 +1,383 @@ +# frozen_string_literal: true + +require "fileutils" +require "minitest/test" +require "pathname" +require "tmpdir" +require "uri" +require "rubydex/linter" + +module Rubydex + module Linter + # Base test case for `Rubydex::Linter::Rule` subclasses, modeled on RuboCop's + # `assert_offense` / `assert_no_offenses`. + # + # Write the source the rule should run against and inline expected + # diagnostics directly under the offending range with `^^^^^ Message` + # carets: + # + # module Rubydex + # module Linter + # module Rules + # class NoConstantsDefinedUnderObjectTest < RuleTestCase + # def test_flags_uppercase_constants_under_object + # assert_diagnostics(<<~RUBY) + # FOO = 123 + # ^^^ Failure: FOO + # RUBY + # end + # + # def test_allows_classish_names + # assert_no_diagnostics("Foo = Object\n") + # end + # end + # end + # end + # end + # + # The number of carets matches the diagnostic's location range + # (`start_column`..`end_column`); leading whitespace before the carets + # determines the start column. Use `^{}` to mark a zero-width diagnostic. + # + # For multi-line diagnostic messages, repeat the same caret line and put + # each line of the message on its own annotation row. Consecutive + # annotation lines that target the same source line and the same caret + # range are merged into one diagnostic with a newline-joined message. + # + # For rules that need fixtures across multiple files, pass a hash of + # `filename => annotated_source`: + # + # assert_diagnostics( + # "lib/base.rb" => "class ApplicationController; end\n", + # "lib/foo.rb" => <<~RUBY, + # class FooController < ApplicationController; end + # ^^^^^^^^^^^^^ Failure: FooController + # RUBY + # ) + # + # Files without annotations are still indexed but contribute no expected + # diagnostics. A diagnostic reported in a file you didn't pass in is + # treated as a failure. + # + # The rule under test is inferred from the test class name by stripping + # the trailing `Test`. Override `#rule_class` for non-conventional names. + class RuleTestCase < Minitest::Test + DEFAULT_FILE = "test.rb" #: String + + ANNOTATION_PATTERN = /\A(?\s*)(?(?.*))?\z/ #: Regexp + + #: type annotation = [Integer, Integer, Integer, String] + + #: String + attr_reader :workspace_path + + #: (String) -> void + def initialize(name) + super + @workspace_path = File.realpath(Dir.mktmpdir("rubydex-rule-test-")) #: String + end + + #: -> void + def teardown + super + ensure + FileUtils.remove_entry(workspace_path) + end + + #: -> singleton(Rule) + def rule_class + @rule_class ||= begin + name = self.class.to_s.delete_suffix("Test") + if name == self.class.to_s + raise "Could not infer rule class from #{self.class}; override `#rule_class`" + end + + Object.const_get(name) #: as singleton(Rule) + end #: singleton(Rule)? + end + + # Registers sources that are loaded into the graph for every assertion. + # Shared sources must not contain caret annotations. Test-specific + # sources win on filename collision. + #: (Hash[String, String]) -> void + def add_shared_source(sources) + shared_sources.merge!(sources) + end + + # Returns absolute file paths whose diagnostics should be ignored. + #: -> Array[String] + def ignored_diagnostic_files + @ignored_diagnostic_files ||= [] #: Array[String]? + end + + #: -> LinterConfig + def rule_config + @rule_config ||= LinterConfig.new({}) #: LinterConfig? + end + + # Runs the rule and compares every diagnostic location with the inline + # annotations in the corresponding source. + #: (*(String | Hash[String | Symbol, String])) ?{ (Graph) -> Rule } -> Array[Diagnostic] + def assert_diagnostics(*args, &rule_builder) + sources = validated_shared_source.merge(normalize_sources(args)) + clean_per_file, expected_per_file = write_sources(sources) + + diagnostics = run_rule(&rule_builder) + actual_per_file = annotations_for_diagnostics(diagnostics, clean_per_file) + + surprise_files = actual_per_file.keys - clean_per_file.keys - ignored_diagnostic_files + refute_predicate( + surprise_files, + :any?, + "Diagnostics in unexpected files: #{surprise_files.inspect}", + ) + + clean_per_file.each do |filename, clean| + expected = render_annotated(clean, expected_per_file[filename] || []) + actual = render_annotated(clean, actual_per_file[filename] || []) + assert_equal(expected, actual, "Mismatch in #{filename}") + end + + diagnostics + end + + # Runs the rule and asserts no diagnostics are reported in the provided + # sources. Sources must not contain caret annotations. + #: (*(String | Hash[String | Symbol, String])) ?{ (Graph) -> Rule } -> Array[Diagnostic] + def assert_no_diagnostics(*args, &rule_builder) + sources = validated_shared_source.merge(normalize_sources(args)) + write_sources(sources) + + diagnostics = run_rule(&rule_builder) + ignored = ignored_diagnostic_files + reported = diagnostics.reject { |diagnostic| ignored.include?(diagnostic.location.to_file_path) } + assert_empty(reported, "Expected no diagnostics, got #{reported.map(&:message).inspect}") + diagnostics + end + + #: ( + #| String, + #| *(String | Hash[String | Symbol, String]), + #| ?after_excluding: Array[String], + #| ) ?{ (Graph) -> Rule } -> void + def assert_handles_missing_required_dependency(dependency, *args, after_excluding: [], &rule_builder) + sources = validated_shared_source.dup #: Hash[String, String] + after_excluding.each { |filename| sources.delete(filename) } + sources.merge!(normalize_sources(args)) + write_sources(sources) + + error = assert_raises(MissingGraphDependencyError) do + run_rule(&rule_builder) + end + + expected_message = MissingGraphDependencyError.new(rule_class.rule_name, dependency).message + assert_equal(expected_message, error.message) + end + + private + + #: (Hash[String, String]) -> [Hash[String, String], Hash[String, Array[annotation]]] + def write_sources(sources) + clean_per_file = {} #: Hash[String, String] + expected_per_file = {} #: Hash[String, Array[annotation]] + + virtual_sources.clear + + sources.each do |filename, annotated| + clean, annotations = parse_annotations(annotated) + if Pathname(filename).absolute? + absolute_path = filename + virtual_sources[absolute_path] = clean + else + absolute_path = File.join(workspace_path, filename) + FileUtils.mkdir_p(File.dirname(absolute_path)) + File.write(absolute_path, clean) + absolute_path = File.realpath(absolute_path) + end + clean_per_file[absolute_path] = clean + expected_per_file[absolute_path] = annotations + end + + [clean_per_file, expected_per_file] + end + + #: -> Hash[String, String] + def shared_sources + @shared_sources ||= {} #: Hash[String, String]? + end + + #: -> Hash[String, String] + def virtual_sources + @virtual_sources ||= {} #: Hash[String, String]? + end + + #: -> Hash[String, String] + def validated_shared_source + shared_sources.each do |filename, source| + _, annotations = parse_annotations(source) + raise "Shared source #{filename} must not contain caret annotations" unless annotations.empty? + end + shared_sources + end + + #: (Array[String | Hash[String | Symbol, String]]) -> Hash[String, String] + def normalize_sources(args) + case args + in [] + {} + in [String => source] + { DEFAULT_FILE => source } + in [Hash => hash] + hash.transform_keys(&:to_s) + else + raise ArgumentError, "expected a String or a { filename => source } Hash, got: #{args.inspect}" + end + end + + #: (String) -> [String, Array[annotation]] + def parse_annotations(annotated_source) + clean = [] #: Array[String] + annotations = [] #: Array[annotation] + + annotated_source.each_line do |line| + if (match = ANNOTATION_PATTERN.match(line.chomp)) + indent = match[:indent]&.length #: as !nil + carets = match[:carets] #: as !nil + start_column = indent + end_column = if carets == "^{}" + indent + else + indent + carets.length + end + + target_line = clean.empty? ? 0 : clean.size - 1 + message = match[:message].to_s + last = annotations.last + if last && last[0] == target_line && last[1] == start_column && last[2] == end_column + last[3] = "#{last[3]}\n#{message}" + else + annotations << [target_line, start_column, end_column, message] + end + else + clean << line + end + end + + [clean.join, annotations] + end + + #: ?{ (Graph) -> Rule } -> Array[Diagnostic] + def run_rule(&rule_builder) + graph = Graph.configure_for_workspace(workspace_path) + file_paths = workspace_file_paths + graph.index_all(file_paths.reject { |path| File.extname(path) == ".rake" }) + index_rake_files(graph, file_paths) + + virtual_sources.each do |path, source| + graph.index_source(uri_for_path(path), source, source_id_for(path)) + end + graph.resolve + + rule = rule_builder ? rule_builder.call(graph) : rule_class.new(graph, config: rule_config) + rule.lint + rule.diagnostics + end + + #: (Graph, Array[String]) -> void + def index_rake_files(graph, file_paths) + file_paths.each do |path| + next unless File.extname(path) == ".rake" + + graph.index_source(uri_for_path(path), File.read(path), "ruby") + end + end + + #: -> Array[String] + def workspace_file_paths + Dir.glob("**/*", base: workspace_path).sort.filter_map do |relative_path| + absolute_path = File.join(workspace_path, relative_path) + next unless File.file?(absolute_path) + + case File.extname(relative_path) + when ".rb", ".rbi", ".rake", ".rbs" + File.realpath(absolute_path) + else + raise "Unsupported file type: #{relative_path}" + end + end + end + + #: (String) -> String + def source_id_for(path) + case File.extname(path) + when ".rb", ".rbi", ".rake", ".ru" + "ruby" + when ".rbs" + "rbs" + else + raise "Unsupported file type: #{path}" + end + end + + #: (Array[Diagnostic], Hash[String, String]) -> Hash[String, Array[annotation]] + def annotations_for_diagnostics(diagnostics, sources) + result = Hash.new { |hash, key| hash[key] = [] } #: Hash[String, Array[annotation]] + + diagnostics.each do |diagnostic| + located_messages = [[diagnostic.location, diagnostic.message]] #: Array[[Location, String]] + diagnostic.related_information.each do |related| + located_messages << [related.location, related.message] + end + + located_messages.each do |location, message| + path = location.to_file_path + line_index = location.start_line + start_column = location.start_column + end_column = + if location.end_line == location.start_line + location.end_column + else + clean = sources[path] + line_text = clean ? (clean.each_line.to_a[line_index] || "").chomp : "" + line_text.length + end + + annotations = result[path] #: as !nil + annotations << [line_index, start_column, end_column, message.chomp] + end + end + + result + end + + #: (String, Array[annotation]) -> String + def render_annotated(clean_source, annotations) + lines = clean_source.each_line.to_a + sorted = annotations.sort_by do |line_index, start_column, end_column, message| + [line_index, start_column, end_column, message] + end + output = lines.dup + + sorted.reverse_each do |line_index, start_column, end_column, message| + caret_count = end_column - start_column + indentation = " " * start_column + carets = caret_count.zero? ? "^{}" : "^" * caret_count + markers = if message.empty? + ["#{indentation}#{carets}\n"] + else + message.split("\n", -1).map { |line| "#{indentation}#{carets} #{line}\n" } + end + output[line_index + 1, 0] = markers + end + + output.join + end + + #: (String) -> String + def uri_for_path(path) + uri_path = Gem.win_platform? ? "/#{path}" : path + URI::File.build(path: uri_path).to_s + end + end + end +end diff --git a/lib/rubydex/linter/runner.rb b/lib/rubydex/linter/runner.rb index b232ef4c0..5e597cc9b 100644 --- a/lib/rubydex/linter/runner.rb +++ b/lib/rubydex/linter/runner.rb @@ -13,11 +13,10 @@ class Runner #: (Graph, rules: Array[singleton(Rule)], config: LinterConfig) -> void def initialize(graph, rules:, config:) - raise ArgumentError, "At least one linter rule is required" if rules.empty? - @graph = graph @config = config @rules = rules.select { |rule| config.rule_enabled?(rule) }.sort_by { |rule| rule.name.to_s } + @dependency_paths = Gem.path #: Array[String] end #: () -> Result @@ -25,7 +24,7 @@ def run rule_diagnostics = @rules.flat_map do |rule_class| rule = rule_class.new(@graph, config: @config) rule.lint - rule.diagnostics + filter_diagnostics(rule.diagnostics, @config.excludes_for(rule_class)) end diagnostics = (@graph.diagnostics + rule_diagnostics).select do |diagnostic| @@ -48,9 +47,33 @@ def run private + #: (Array[Diagnostic], Array[String]) -> Array[Diagnostic] + def filter_diagnostics(diagnostics, exclude_patterns) + diagnostics.reject do |diagnostic| + location_excluded?(diagnostic.location, exclude_patterns) + end + end + + #: (Location, Array[String]) -> bool + def location_excluded?(location, patterns) + path = location.to_file_path + return true if @dependency_paths.any? do |dependency_path| + path == dependency_path || path.start_with?("#{dependency_path}/") + end + + Helpers::PathHelpers.path_matches_patterns?( + path, + patterns, + workspace: @graph.workspace_path, + flags: Helpers::PathHelpers::RUBOCOP_EXCLUDE_FNMATCH_FLAGS, + ) + rescue Location::NotFileUriError + false + end + #: (Diagnostic) -> bool def diagnostic_in_workspace?(diagnostic) - path = URI::RFC2396_PARSER.unescape(diagnostic.location.to_file_path) + path = diagnostic.location.to_file_path workspace_path = Pathname.new(File.expand_path(@graph.workspace_path)) relative_path = Pathname.new(File.expand_path(path)).relative_path_from(workspace_path) diff --git a/lib/rubydex/location.rb b/lib/rubydex/location.rb index 95fd3f6f7..2c9891422 100644 --- a/lib/rubydex/location.rb +++ b/lib/rubydex/location.rb @@ -42,6 +42,9 @@ def to_file_path raise NotFileUriError, "URI is not a file:// URI: #{@uri}" unless uri.scheme == "file" path = uri.path + raise NotFileUriError, "URI has no file path: #{@uri}" unless path + + path = URI.decode_uri_component(path) # TODO: This has to go away once we have a proper URI abstraction path.delete_prefix!("/") if Gem.win_platform? path diff --git a/lib/rubydex_linter/rules/rule_structure.rb b/lib/rubydex_linter/rules/rule_structure.rb new file mode 100644 index 000000000..e6627e8d7 --- /dev/null +++ b/lib/rubydex_linter/rules/rule_structure.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require "rubydex/linter" + +module Rubydex + module Linter + module Rules + # Ensures discovered linter rule files contain one correctly placed and named rule class. + class RuleStructure < Rule + BASE_RULE_NAME = "Rubydex::Linter::Rule" #: String + RULE_NAMESPACE = "Rubydex::Linter::Rules" #: String + RULE_FILE_PATTERNS = [ + "rubydex_linter/rules/**/*.rb", + "lib/rubydex_linter/rules/**/*.rb", + ].freeze #: Array[String] + TEST_FILE_PATTERNS = ["test/**/*", "**/test/**/*"].freeze #: Array[String] + + # @override + #: -> singleton(Severity::Base) + def severity + Severity::Error + end + + # @override + #: -> void + def lint + rule_class_names = child_classes(BASE_RULE_NAME).to_h { |rule_class| [rule_class.name, true] } + + graph.documents.each do |document| + path = path_for_uri(document.uri) + next unless workspace_path?(path) + + rule_file = rule_file?(path) + next if !rule_file && test_file?(path) + + definitions = rule_definitions(document, rule_class_names) + validate_rule_file(document, definitions) if rule_file + + definitions.each do |name, definition| + validate_rule_definition(name, definition, path) + end + end + end + + private + + #: (Document, Hash[String, bool]) -> Hash[String, ClassDefinition] + def rule_definitions(document, rule_class_names) + definitions = {} #: Hash[String, ClassDefinition] + + document.definitions.each do |definition| + next unless definition.is_a?(ClassDefinition) + + declaration = definition.declaration + next unless declaration + next unless rule_class_names.key?(declaration.name) + + definitions[declaration.name] ||= definition + end + + definitions + end + + #: (Document, Hash[String, ClassDefinition]) -> void + def validate_rule_file(document, definitions) + return if definitions.one? + + add_diagnostic( + "Each rule file must define exactly one class that inherits from `#{BASE_RULE_NAME}`; " \ + "found #{definitions.length}.", + file_location(document.uri), + related_information: definitions.map do |name, definition| + RelatedInformation.new("`#{name}` is defined here.", diagnostic_location(definition)) + end, + ) + end + + #: (String, ClassDefinition, String) -> void + def validate_rule_definition(name, definition, path) + unless rule_file?(path) + add_diagnostic( + "`#{name}` must be defined under `rubydex_linter/rules/` or `lib/rubydex_linter/rules/`.", + diagnostic_location(definition), + ) + end + + return if name.start_with?("#{RULE_NAMESPACE}::") + + add_diagnostic( + "`#{name}` must be defined under `#{RULE_NAMESPACE}`.", + diagnostic_location(definition), + ) + end + + #: (String) -> bool + def workspace_path?(path) + workspace = graph.workspace_path + path == workspace || path.start_with?("#{workspace}/") + end + + #: (String) -> bool + def rule_file?(path) + path_matches_patterns?(path, RULE_FILE_PATTERNS) + end + + #: (String) -> bool + def test_file?(path) + path_matches_patterns?(path, TEST_FILE_PATTERNS) + end + + #: (String, Array[String]) -> bool + def path_matches_patterns?(path, patterns) + Helpers::PathHelpers.path_matches_patterns?( + path, + patterns, + workspace: graph.workspace_path, + flags: Helpers::PathHelpers::RUBOCOP_EXCLUDE_FNMATCH_FLAGS, + ) + end + end + end + end +end diff --git a/rbi/rubydex.rbi b/rbi/rubydex.rbi index 934b272fa..3dce8dd8b 100644 --- a/rbi/rubydex.rbi +++ b/rbi/rubydex.rbi @@ -357,6 +357,58 @@ class Rubydex::Diagnostic end module Rubydex::Linter; end +module Rubydex::Linter::Helpers; end +module Rubydex::Linter::Rules; end + +module Rubydex::Linter::Helpers::PathHelpers + extend T::Helpers + + requires_ancestor { Rubydex::Linter::Rule } + + RUBOCOP_EXCLUDE_FNMATCH_FLAGS = T.let(T.unsafe(nil), Integer) + TEST_PATHS = T.let(T.unsafe(nil), T::Array[String]) + + sig do + params( + path: String, + patterns: T::Array[String], + workspace: String, + flags: Integer, + ).returns(T::Boolean) + end + def self.path_matches_patterns?(path, patterns, workspace:, flags: 0); end + + sig { params(location: Rubydex::Location, workspace: String).returns(String) } + def self.display_path(location, workspace:); end + + sig do + params( + definitions: T::Enumerable[Rubydex::Definition], + excluded_patterns: T::Array[String], + ).returns(T::Array[Rubydex::Definition]) + end + def reject_definitions_in_paths(definitions, excluded_patterns); end + + sig do + params( + definitions: T::Enumerable[Rubydex::Definition], + patterns: T::Array[String], + ).returns(T::Array[Rubydex::Definition]) + end + def select_definitions_in_paths(definitions, patterns); end + + private + + sig { params(path: String, patterns: T::Array[String]).returns(T::Boolean) } + def path_matches_patterns?(path, patterns); end + + sig { params(path: String).returns(T::Boolean) } + def test_path?(path); end + + sig { params(definition: Rubydex::Definition).returns(T.nilable(String)) } + def path_for_definition(definition); end + +end class Rubydex::Linter::Rule abstract! @@ -376,9 +428,38 @@ class Rubydex::Linter::Rule sig { returns(T::Array[Rubydex::Diagnostic]) } def diagnostics; end + sig { params(definition: Rubydex::Definition).returns(Rubydex::Location) } + def diagnostic_location(definition); end + + sig { params(uri: String).returns(Rubydex::Location) } + def file_location(uri); end + + sig { params(uri: String).returns(String) } + def path_for_uri(uri); end + + sig { params(base_name: String).returns(T::Enumerable[Rubydex::Class]) } + def child_classes(base_name); end + + sig { params(name: String).returns(Rubydex::Namespace) } + def required_namespace(name); end + + sig do + params( + namespace: Rubydex::Namespace, + method_name: String, + ).returns(Rubydex::Method) + end + def required_method(namespace, method_name); end + + sig { returns(String) } + def rule_name; end + sig { abstract.returns(T.class_of(Rubydex::Severity::Base)) } def severity; end + sig { returns(T.class_of(Rubydex::Severity::Base)) } + def verified_severity; end + sig { abstract.void } def lint; end @@ -392,6 +473,91 @@ class Rubydex::Linter::Rule ).void end def add_diagnostic(message, location, related_information: []); end + + private + + sig { params(location: Rubydex::Location).returns(T.nilable(String)) } + def source_for_location(location); end +end + +class Rubydex::Linter::MissingGraphDependencyError < StandardError + sig { params(rule_name: String, dependency: String).void } + def initialize(rule_name, dependency); end +end + +class Rubydex::Linter::RuleLoadError < StandardError; end + +class Rubydex::Linter::RuleLoader + RULE_GLOB = T.let(T.unsafe(nil), String) + BUILT_IN_RULE_GLOB = T.let(T.unsafe(nil), String) + + sig { params(workspace_path: String).returns(T::Array[T.class_of(Rubydex::Linter::Rule)]) } + def self.load(workspace_path); end +end + +class Rubydex::Linter::Rules::RuleStructure < Rubydex::Linter::Rule + BASE_RULE_NAME = T.let(T.unsafe(nil), String) + RULE_NAMESPACE = T.let(T.unsafe(nil), String) + RULE_FILE_PATTERNS = T.let(T.unsafe(nil), T::Array[String]) + TEST_FILE_PATTERNS = T.let(T.unsafe(nil), T::Array[String]) + + sig { returns(T.class_of(Rubydex::Severity::Base)) } + def severity; end + + sig { void } + def lint; end +end + +class Rubydex::Linter::RuleTestCase < ::Minitest::Test + DEFAULT_FILE = T.let(T.unsafe(nil), String) + ANNOTATION_PATTERN = T.let(T.unsafe(nil), Regexp) + + sig { returns(String) } + def workspace_path; end + + sig { params(name: String).void } + def initialize(name); end + + sig { void } + def teardown; end + + sig { returns(T.class_of(Rubydex::Linter::Rule)) } + def rule_class; end + + sig { params(sources: T::Hash[String, String]).void } + def add_shared_source(sources); end + + sig { returns(T::Array[String]) } + def ignored_diagnostic_files; end + + sig { returns(Rubydex::LinterConfig) } + def rule_config; end + + sig do + params( + args: T.any(String, T::Hash[T.any(String, Symbol), String]), + rule_builder: T.nilable(T.proc.params(graph: Rubydex::Graph).returns(Rubydex::Linter::Rule)), + ).returns(T::Array[Rubydex::Diagnostic]) + end + def assert_diagnostics(*args, &rule_builder); end + + sig do + params( + args: T.any(String, T::Hash[T.any(String, Symbol), String]), + rule_builder: T.nilable(T.proc.params(graph: Rubydex::Graph).returns(Rubydex::Linter::Rule)), + ).returns(T::Array[Rubydex::Diagnostic]) + end + def assert_no_diagnostics(*args, &rule_builder); end + + sig do + params( + dependency: String, + args: T.any(String, T::Hash[T.any(String, Symbol), String]), + after_excluding: T::Array[String], + rule_builder: T.nilable(T.proc.params(graph: Rubydex::Graph).returns(Rubydex::Linter::Rule)), + ).void + end + def assert_handles_missing_required_dependency(dependency, *args, after_excluding: [], &rule_builder); end end class Rubydex::Linter::Runner @@ -496,6 +662,17 @@ class Rubydex::LinterConfig sig { params(rule_class: T.class_of(Rubydex::Linter::Rule)).returns(T::Boolean) } def rule_enabled?(rule_class); end + + sig { params(rule_class: T.class_of(Rubydex::Linter::Rule)).returns(T::Array[String]) } + def excludes_for(rule_class); end + + sig do + params( + rule_class: T.class_of(Rubydex::Linter::Rule), + default: T.class_of(Rubydex::Severity::Base), + ).returns(T.class_of(Rubydex::Severity::Base)) + end + def severity_for(rule_class, default:); end end # The settings of a single linter rule, read from a `[linter.rules.RuleName]` table. @@ -503,8 +680,21 @@ class Rubydex::RuleConfig sig { returns(String) } attr_reader :name - sig { params(name: String, enabled: T::Boolean).void } - def initialize(name, enabled); end + sig { returns(T::Array[String]) } + attr_reader :exclude_patterns + + sig { returns(T.nilable(T.class_of(Rubydex::Severity::Base))) } + attr_reader :severity + + sig do + params( + name: String, + enabled: T::Boolean, + exclude_patterns: T::Array[String], + severity: T.nilable(T.class_of(Rubydex::Severity::Base)), + ).void + end + def initialize(name, enabled, exclude_patterns = [], severity = nil); end sig { returns(T::Boolean) } def enabled?; end diff --git a/rust/rubydex-sys/src/config_api.rs b/rust/rubydex-sys/src/config_api.rs index 902f1f461..309ebf043 100644 --- a/rust/rubydex-sys/src/config_api.rs +++ b/rust/rubydex-sys/src/config_api.rs @@ -1,3 +1,4 @@ +use crate::diagnostic_api::DiagnosticSeverity; use crate::utils; use libc::{c_char, c_void}; use rubydex::config::{Config, Rule}; @@ -90,6 +91,14 @@ pub unsafe extern "C" fn rdx_config_free(config: ConfigPointer) { } } +/// Borrowed string bytes exposed while building Ruby configuration values. +#[repr(C)] +#[derive(Debug)] +pub struct CConfigString { + pub data: *const c_char, + pub length: usize, +} + /// C-compatible struct representing a single configured linter rule. #[repr(C)] #[derive(Debug)] @@ -97,14 +106,38 @@ pub struct CLinterRule { pub name: *const c_char, pub name_length: usize, pub enabled: bool, + pub exclude_patterns: *const CConfigString, + pub exclude_patterns_length: usize, + pub severity: *const DiagnosticSeverity, } impl From<&Rule> for CLinterRule { fn from(rule: &Rule) -> Self { + let exclude_patterns = rule + .exclude_patterns() + .iter() + .map(|pattern| CConfigString { + data: pattern.as_ptr().cast::(), + length: pattern.len(), + }) + .collect::>(); + let exclude_patterns_length = exclude_patterns.len(); + let exclude_patterns = if exclude_patterns.is_empty() { + ptr::null() + } else { + Box::into_raw(exclude_patterns).cast::().cast_const() + }; + let severity = rule.severity().map_or(ptr::null(), |severity| { + Box::into_raw(Box::new(DiagnosticSeverity::from(severity))).cast_const() + }); + Self { name: rule.name().as_ptr().cast::(), name_length: rule.name().len(), enabled: rule.enabled(), + exclude_patterns, + exclude_patterns_length, + severity, } } } @@ -155,6 +188,18 @@ pub unsafe extern "C" fn rdx_config_linter_rules_free(rules: CLinterRuleArray) { } unsafe { - let _ = Box::from_raw(ptr::slice_from_raw_parts_mut(rules.items, rules.len)); + let rules = Box::from_raw(ptr::slice_from_raw_parts_mut(rules.items, rules.len)); + + for rule in &*rules { + if !rule.exclude_patterns.is_null() { + let exclude_patterns = + ptr::slice_from_raw_parts_mut(rule.exclude_patterns.cast_mut(), rule.exclude_patterns_length); + let _ = Box::from_raw(exclude_patterns); + } + + if !rule.severity.is_null() { + let _ = Box::from_raw(rule.severity.cast_mut()); + } + } } } diff --git a/rust/rubydex/src/config.rs b/rust/rubydex/src/config.rs index 8a148920b..6450abfe3 100644 --- a/rust/rubydex/src/config.rs +++ b/rust/rubydex/src/config.rs @@ -1,4 +1,5 @@ use crate::assert_mem_size; +use crate::diagnostic::Severity; use crate::errors::Errors; use crate::path_helpers; use std::collections::HashSet; @@ -72,6 +73,8 @@ impl GraphSettings { pub struct Rule { name: Box, enabled: bool, + exclude_patterns: Box<[Box]>, + severity: Option, } impl Rule { @@ -85,6 +88,16 @@ impl Rule { self.enabled } + #[must_use] + pub fn exclude_patterns(&self) -> &[Box] { + &self.exclude_patterns + } + + #[must_use] + pub fn severity(&self) -> Option<&Severity> { + self.severity.as_ref() + } + /// Parses a single `[linter.rules.{name}]` table fn parse(name: &str, value: Value) -> Result { let Value::Table(mut table) = value else { @@ -98,7 +111,24 @@ impl Rule { "invalid `linter.rules.{name}.enabled` setting: expected a boolean" )); } - None => return Err(format!("missing `linter.rules.{name}.enabled` setting")), + None => true, + }; + + let exclude_patterns = match table.remove("exclude") { + Some(value) => value + .try_into::>>() + .map_err(|error| format!("invalid `linter.rules.{name}.exclude` setting: {error}"))? + .into_boxed_slice(), + None => Box::default(), + }; + + let severity = match table.remove("severity") { + Some(value) => Some( + value + .try_into::() + .map_err(|error| format!("invalid `linter.rules.{name}.severity` setting: {error}"))?, + ), + None => None, }; if let Some(key) = table.keys().next() { @@ -108,6 +138,8 @@ impl Rule { Ok(Self { name: Box::from(name), enabled, + exclude_patterns, + severity, }) } } @@ -385,17 +417,24 @@ mod tests { #[test] fn parse_parses_every_linter_rule() { - let config = parse("[linter.rules.Something]\nenabled = true\n\n[linter.rules.Other]\nenabled = false\n") - .expect("expected the config to parse"); + let config = parse( + "[linter.rules.Something]\nseverity = \"warning\"\nexclude = [\"components/legacy/**\"]\n\n\ + [linter.rules.Other]\nenabled = false\n", + ) + .expect("expected the config to parse"); let rules = config.linter().rules(); assert_eq!(rules.len(), 2); let something = rules.iter().find(|rule| rule.name() == "Something").unwrap(); assert!(something.enabled()); + assert_eq!(something.exclude_patterns(), [Box::from("components/legacy/**")]); + assert_eq!(something.severity(), Some(&Severity::Warning)); let other = rules.iter().find(|rule| rule.name() == "Other").unwrap(); assert!(!other.enabled()); + assert!(other.exclude_patterns().is_empty()); + assert_eq!(other.severity(), None); } #[test] @@ -444,14 +483,9 @@ mod tests { } #[test] - fn parse_rejects_a_rule_without_enabled() { - let error = - parse("[linter.rules.Something]\n").expect_err("a rule setting must state whether the rule is enabled"); - - assert!( - error.contains("missing `linter.rules.Something.enabled` setting"), - "unexpected error: {error}" - ); + fn parse_defaults_a_rule_to_enabled() { + let config = parse("[linter.rules.Something]\n").expect("enabled defaults to true"); + assert!(config.linter().rules()[0].enabled()); } #[test] @@ -466,15 +500,54 @@ mod tests { #[test] fn parse_rejects_an_unknown_rule_setting() { - let error = parse("[linter.rules.Something]\nenabled = true\nseverity = \"error\"\n") - .expect_err("rules only accept the enabled setting"); + let error = + parse("[linter.rules.Something]\nparallel = true\n").expect_err("unknown rule settings must be rejected"); + + assert!( + error.contains("unknown setting `linter.rules.Something.parallel`"), + "unexpected error: {error}" + ); + } + + #[test] + fn parse_rejects_an_invalid_rule_exclude_setting() { + let error = parse("[linter.rules.Something]\nexclude = \"components/legacy/**\"\n") + .expect_err("exclude must be an array of strings"); assert!( - error.contains("unknown setting `linter.rules.Something.severity`"), + error.contains("invalid `linter.rules.Something.exclude` setting"), "unexpected error: {error}" ); } + #[test] + fn parse_rejects_an_invalid_rule_severity_setting() { + for severity in ["\"critical\"", "1"] { + let error = parse(&format!("[linter.rules.Something]\nseverity = {severity}\n")) + .expect_err("severity must be one of the supported strings"); + + assert!( + error.contains("invalid `linter.rules.Something.severity` setting"), + "unexpected error: {error}" + ); + } + } + + #[test] + fn parse_accepts_every_rule_severity() { + for (value, expected) in [ + ("error", Severity::Error), + ("warning", Severity::Warning), + ("information", Severity::Information), + ("hint", Severity::Hint), + ] { + let config = + parse(&format!("[linter.rules.Something]\nseverity = \"{value}\"\n")).expect("severity should parse"); + + assert_eq!(config.linter().rules()[0].severity(), Some(&expected)); + } + } + #[test] fn load_returns_the_default_configuration_for_a_workspace_without_a_config_file() { let dir = tempfile::tempdir().expect("failed to create temp dir"); diff --git a/rust/rubydex/src/diagnostic.rs b/rust/rubydex/src/diagnostic.rs index de8ac103f..62dcfc4a9 100644 --- a/rust/rubydex/src/diagnostic.rs +++ b/rust/rubydex/src/diagnostic.rs @@ -60,7 +60,8 @@ impl Diagnostic { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, serde::Deserialize)] +#[serde(rename_all = "lowercase")] pub enum Severity { Error, Warning, diff --git a/test/cli_test.rb b/test/cli_test.rb index 454eb8f61..edfc6d830 100644 --- a/test/cli_test.rb +++ b/test/cli_test.rb @@ -25,6 +25,8 @@ def test_commands_are_discovered_from_subclasses assert_includes(commands, Rubydex::CLI::Command::Query) assert_includes(commands, Rubydex::CLI::Command::Console) + assert_includes(commands, Rubydex::CLI::Command::Explain) + assert_includes(commands, Rubydex::CLI::Command::List) assert_includes(commands, Rubydex::CLI::Command::Lint) assert_includes(commands, Rubydex::CLI::Command::Mcp) @@ -32,6 +34,8 @@ def test_commands_are_discovered_from_subclasses assert_equal("query", Rubydex::CLI::Command::Query.command_name) assert_equal("query ", Rubydex::CLI::Command::Query.usage_form) assert_equal("console", Rubydex::CLI::Command::Console.usage_form) + assert_equal("explain [PATH]", Rubydex::CLI::Command::Explain.usage_form) + assert_equal("list [docs|roots] [PATH]", Rubydex::CLI::Command::List.usage_form) assert_equal("lint [PATH]", Rubydex::CLI::Command::Lint.usage_form) end @@ -47,12 +51,14 @@ def test_commands_are_listed_alphabetically # before the offsets are compared: a missing one fails on its own assertion rather than on a # comparison against nil. We collect the beginning offset of the first match (index 0) for each # command so that we can compare their order below. - console, lint, mcp, query, help = ["console", "lint", "mcp", "query", "help"].map do |name| + console, explain, lint, list, mcp, query, help = ["console", "explain", "lint", "list", "mcp", "query", "help"].map do |name| assert_stdout_includes_pattern(result, /^ #{name}\b/).begin(0) end - assert_operator(console, :<, lint) - assert_operator(lint, :<, mcp) + assert_operator(console, :<, explain) + assert_operator(explain, :<, lint) + assert_operator(lint, :<, list) + assert_operator(list, :<, mcp) assert_operator(mcp, :<, query) # `help` is listed last rather than in alphabetical position. assert_operator(query, :<, help) @@ -96,6 +102,8 @@ def test_usage_is_generated_from_the_declared_commands [ Rubydex::CLI::Command::Query, Rubydex::CLI::Command::Console, + Rubydex::CLI::Command::Explain, + Rubydex::CLI::Command::List, Rubydex::CLI::Command::Lint, Rubydex::CLI::Command::Mcp, ].each do |command| @@ -206,7 +214,7 @@ def test_query_supports_json_output end def test_command_help_is_available_per_subcommand - ["query", "console", "lint", "mcp"].each do |command| + ["query", "console", "explain", "list", "lint", "mcp"].each do |command| result = rdx(command, "--help") assert_success_status(result) @@ -215,7 +223,7 @@ def test_command_help_is_available_per_subcommand end def test_every_command_reports_an_invalid_option_with_the_usage - ["query", "console", "lint", "mcp"].each do |command| + ["query", "console", "explain", "list", "lint", "mcp"].each do |command| result = rdx(command, "--bogus-flag") refute_success_status(result) @@ -247,6 +255,142 @@ def test_mcp_rejects_extra_arguments refute_stderr_includes(result, "Usage: rdx [options]") end + def test_explain_reports_discovered_rules_with_the_same_name_in_stable_order + with_context do |context| + context.write!("rubydex_linter/rules/shared_rule.rb", <<~RUBY) + module ExplainDuplicateFixtures + module First + # Flags raw SQL built through application query helpers. + # + # Prefer parameter binding instead. + class SharedRule < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Error + def lint; end + end + end + + module Second + class SharedRule < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Error + def lint; end + end + end + end + RUBY + + result = with_bundle_gemfile(nil) { rdx("explain", "SharedRule", context.absolute_path) } + + assert_success_status(result) + assert_stdout_equals(<<~DOCS, result) + ExplainDuplicateFixtures::First::SharedRule + + Flags raw SQL built through application query helpers. + + Prefer parameter binding instead. + + ExplainDuplicateFixtures::Second::SharedRule: no documentation available. + DOCS + end + end + + def test_explain_reports_only_the_exact_rule_name + with_context do |context| + context.write!("rubydex_linter/rules/exact_rule.rb", <<~RUBY) + module ExplainExactFixtures + # Flags raw SQL string interpolation. + class ExactRule < Rubydex::Linter::Rule + class Helper; end + + def severity = Rubydex::Severity::Error + def lint; end + end + + class ExactRuleExtension < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Error + def lint; end + end + end + RUBY + + result = with_bundle_gemfile(nil) { rdx("explain", "ExactRule", context.absolute_path) } + + assert_success_status(result) + assert_stdout_equals(<<~DOCS, result) + ExplainExactFixtures::ExactRule + + Flags raw SQL string interpolation. + DOCS + end + end + + def test_explain_rejects_an_unknown_rule + with_context do |context| + context.write!("rubydex_linter/rules/known_rule.rb", <<~RUBY) + class ExplainKnownRule < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Error + def lint; end + end + RUBY + + result = with_bundle_gemfile(nil) { rdx("explain", "MissingRule", context.absolute_path) } + + refute_success_status(result) + assert_empty_stdout(result) + assert_stderr_includes(result, "Rule does not exist: MissingRule") + end + end + + def test_explain_requires_a_rule_name + result = rdx("explain") + + refute_success_status(result) + assert_empty_stdout(result) + assert_stderr_includes(result, "`explain` requires a rule name argument") + assert_stderr_includes(result, "Usage: rdx explain [PATH]") + end + + def test_list_docs_prints_sorted_workspace_relative_paths + with_context do |context| + context.write!("z.rb", "class Z; end\n") + context.write!("lib/a.rb", "class A; end\n") + + result = rdx("list", chdir: context.absolute_path) + + assert_success_status(result) + paths = result.out.lines(chomp: true) + assert_equal(paths.sort, paths) + assert_includes(paths, "lib/a.rb") + assert_includes(paths, "z.rb") + end + end + + def test_list_roots_omits_excluded_roots + with_context do |context| + context.write!("app/main.rb", "class Main; end\n") + context.write!("ignored/skip.rb", "class Skip; end\n") + context.write!("rubydex.toml", "[graph]\nexclude = [\"ignored\"]\n") + + result = rdx("list", "roots", context.absolute_path) + + assert_success_status(result) + paths = result.out.lines(chomp: true) + assert_equal(paths.sort, paths) + assert_includes(paths, "app") + refute_includes(paths, "ignored") + end + end + + def test_list_rejects_an_unknown_target + with_context do |context| + result = rdx("list", "things", context.absolute_path) + + refute_success_status(result) + assert_empty_stdout(result) + assert_stderr_includes(result, "Unknown list target: things. Expected `docs` or `roots`.") + assert_stderr_includes(result, "Usage: rdx list [docs|roots] [PATH]") + end + end + def test_lint_reports_a_project_rule_diagnostic_with_related_information with_context do |context| write_linter_rule( @@ -260,13 +404,24 @@ def test_lint_reports_a_project_rule_diagnostic_with_related_information result = with_bundle_gemfile(nil) { rdx("lint", context.absolute_path) } refute_success_status(result) - assert_stdout_equals( + assert_stdout_includes( + result, <<~OUTPUT, - #{context.absolute_path_to("app.rb")}:1:7: error: CLITestProjectErrorRule: Foo is not allowed. - #{context.absolute_path_to("app.rb")}:2:7: Foo is also defined here. + Offenses: + + app.rb:1:7: error: CLITestProjectErrorRule: Foo is not allowed. + app.rb:2:7: Foo is also defined here. + + class Foo; end + ^^^ OUTPUT + ) + assert_stdout_includes_pattern( result, + /\d+ files inspected, 1 offense detected: 1 error, 0 warnings, 0 info, 0 hints/, ) + assert_stdout_includes(result, "For more information about a rule, run `rdx explain RuleName`.") + refute_stdout_includes(result, context.absolute_path) assert_stderr_includes(result, "Indexing workspace...") assert_stderr_includes(result, "Resolving graph...") end @@ -280,7 +435,7 @@ def test_lint_allows_a_clean_workspace result = with_bundle_gemfile(nil) { rdx("lint", context.absolute_path) } assert_success_status(result) - assert_empty_stdout(result) + assert_stdout_includes_pattern(result, /\d+ files inspected, no offenses detected/) end end @@ -288,13 +443,13 @@ def test_lint_loads_rules_from_bundled_dependencies with_context do |context| rule_path = "fake_gem/lib/rubydex_linter/rules/no_foo.rb" write_linter_rule(context, "CLITestDependencyErrorRule", path: rule_path) - context.write!("app.rb", "class Foo; end\n") + context.write!("workspace/app.rb", "class Foo; end\n") Gem.expects(:find_latest_files) .with("rubydex_linter/rules/**/*.rb") .returns([context.absolute_path_to(rule_path)]) result = with_bundle_gemfile(context.absolute_path_to("Gemfile")) do - rdx("lint", context.absolute_path) + rdx("lint", context.absolute_path_to("workspace")) end refute_success_status(result) @@ -302,16 +457,16 @@ def test_lint_loads_rules_from_bundled_dependencies end end - def test_lint_requires_a_discovered_rule_before_indexing + def test_lint_runs_built_in_rules_without_project_rules with_context do |context| - context.write!("app.rb", "class Foo; end\n") + context.write!("app.rb", "class Bar; end\n") result = with_bundle_gemfile(nil) { rdx("lint", context.absolute_path) } - refute_success_status(result) - assert_empty_stdout(result) - assert_stderr_includes(result, "No Rubydex::Linter::Rule subclasses were loaded") - refute_stderr_includes(result, "Indexing workspace...") + assert_success_status(result) + assert_stdout_includes_pattern(result, /\d+ files inspected, no offenses detected/) + assert_stderr_includes(result, "Indexing workspace...") + refute_stderr_includes(result, "No Rubydex::Linter::Rule subclasses were loaded") end end @@ -345,7 +500,7 @@ def write_linter_rule(context, class_name, path: "rubydex_linter/rules/no_foo.rb context.write!(path, <<~RUBY) # frozen_string_literal: true - class #{class_name} < Rubydex::Linter::Rule + class Rubydex::Linter::Rules::#{class_name} < Rubydex::Linter::Rule def severity = Rubydex::Severity::Error def lint @@ -358,11 +513,11 @@ def lint add_diagnostic( "Foo is not allowed.", - primary.name_location || primary.location, + diagnostic_location(primary), related_information: definitions.map do |definition| Rubydex::RelatedInformation.new( "Foo is also defined here.", - definition.name_location || definition.location, + diagnostic_location(definition), ) end, ) diff --git a/test/config_test.rb b/test/config_test.rb index 0448d5112..1a06fd4e3 100644 --- a/test/config_test.rb +++ b/test/config_test.rb @@ -68,7 +68,8 @@ def test_linter_returns_the_configured_rules with_context do |context| context.write!("rubydex.toml", <<~TOML) [linter.rules.Something] - enabled = true + severity = "warning" + exclude = ["components/legacy/**", "test/fixtures/**"] [linter.rules.Other] enabled = false @@ -80,7 +81,43 @@ def test_linter_returns_the_configured_rules assert_equal(["Other", "Something"], rules.keys.sort) assert_predicate(rules, :frozen?) assert_predicate(rules.fetch("Something"), :enabled?) + assert_equal( + ["components/legacy/**", "test/fixtures/**"], + rules.fetch("Something").exclude_patterns, + ) + assert_equal(Rubydex::Severity::Warning, rules.fetch("Something").severity) refute_predicate(rules.fetch("Other"), :enabled?) + assert_empty(rules.fetch("Other").exclude_patterns) + assert_nil(rules.fetch("Other").severity) + end + end + + def test_linter_maps_every_configured_severity + with_context do |context| + context.write!("rubydex.toml", <<~TOML) + [linter.rules.ErrorRule] + severity = "error" + + [linter.rules.WarningRule] + severity = "warning" + + [linter.rules.InformationRule] + severity = "information" + + [linter.rules.HintRule] + severity = "hint" + TOML + + rules = Rubydex::Config.load(context.absolute_path).linter.rules + + { + "ErrorRule" => Rubydex::Severity::Error, + "WarningRule" => Rubydex::Severity::Warning, + "InformationRule" => Rubydex::Severity::Information, + "HintRule" => Rubydex::Severity::Hint, + }.each do |rule_name, severity| + assert_equal(severity, rules.fetch(rule_name).severity) + end end end end diff --git a/test/linter_test.rb b/test/linter_test.rb index 6caa41ec4..c38ab129e 100644 --- a/test/linter_test.rb +++ b/test/linter_test.rb @@ -2,6 +2,7 @@ require "test_helper" require "helpers/context" +require "mocha/minitest" require "rubydex/linter" class LinterTest < Minitest::Test @@ -64,6 +65,58 @@ def lint end end + class DependencyPathRule < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Information + + def lint + path = File.join(graph.workspace_path, "vendor/bundle/gems/example.rb") + path.prepend("/") if Gem.win_platform? + uri = URI::File.build(path: path).to_s + + add_diagnostic( + "Inside a dependency path.", + Rubydex::Location.new(uri: uri, start_line: 0, end_line: 0, start_column: 0, end_column: 1), + ) + end + end + + class ExcludedPrimaryRule < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Information + + def lint + add_diagnostic("Excluded primary location.", workspace_location("components/legacy/example.rb")) + end + + private + + def workspace_location(relative_path) + path = File.join(graph.workspace_path, relative_path) + path.prepend("/") if Gem.win_platform? + Rubydex::Location.new( + uri: URI::File.build(path: path).to_s, + start_line: 0, + end_line: 0, + start_column: 0, + end_column: 1, + ) + end + end + + class ExcludedRelatedInformationRule < ExcludedPrimaryRule + def lint + add_diagnostic( + "Included primary location.", + workspace_location("components/current/example.rb"), + related_information: [ + Rubydex::RelatedInformation.new( + "Excluded related location.", + workspace_location("components/legacy/example.rb"), + ), + ], + ) + end + end + def test_runner_builds_diagnostics_with_rule_severity_and_related_information result = Rubydex::Linter::Runner.new(Rubydex::Graph.new, rules: [WarningRule], config: linter_config).run diagnostic = result.diagnostics.fetch(0) @@ -81,6 +134,13 @@ def test_rule_exposes_linter_config assert_same(config, rule.config) end + def test_configured_severity_overrides_the_rule_severity + config = configured_linter_config("WarningRule", severity: Rubydex::Severity::Error) + result = Rubydex::Linter::Runner.new(Rubydex::Graph.new, rules: [WarningRule], config:).run + + assert_equal(Rubydex::Severity::Error, result.diagnostics.fetch(0).severity) + end + def test_runner_drops_disabled_rules config = linter_config("WarningRule" => false) runner = Rubydex::Linter::Runner.new( @@ -128,12 +188,11 @@ def test_runner_includes_native_graph_diagnostics assert_predicate(result, :success?) end - def test_runner_requires_rules - error = assert_raises(ArgumentError) do - Rubydex::Linter::Runner.new(Rubydex::Graph.new, rules: [], config: linter_config) - end + def test_runner_accepts_no_rules + result = Rubydex::Linter::Runner.new(Rubydex::Graph.new, rules: [], config: linter_config).run - assert_equal("At least one linter rule is required", error.message) + assert_empty(result.diagnostics) + assert_predicate(result, :success?) end def test_runner_filters_diagnostics_outside_the_workspace @@ -148,6 +207,46 @@ def test_runner_filters_diagnostics_outside_the_workspace end end + def test_runner_filters_diagnostics_under_dependency_paths + with_context do |context| + context.write!("workspace/inside.rb") + graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + dependency_path = context.absolute_path_to("workspace/vendor/bundle") + Gem.stubs(:path).returns([dependency_path]) + + result = Rubydex::Linter::Runner.new(graph, rules: [DependencyPathRule], config: linter_config).run + + assert_empty(result.diagnostics) + end + end + + def test_runner_filters_a_diagnostic_when_its_primary_location_matches_a_rule_exclude + with_context do |context| + context.write!("workspace/inside.rb") + graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + config = configured_linter_config("ExcludedPrimaryRule", exclude_patterns: ["components/legacy/**"]) + + result = Rubydex::Linter::Runner.new(graph, rules: [ExcludedPrimaryRule], config:).run + + assert_empty(result.diagnostics) + end + end + + def test_runner_keeps_a_diagnostic_when_only_related_information_matches_a_rule_exclude + with_context do |context| + context.write!("workspace/inside.rb") + graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + config = configured_linter_config( + "ExcludedRelatedInformationRule", + exclude_patterns: ["components/legacy/**"], + ) + + result = Rubydex::Linter::Runner.new(graph, rules: [ExcludedRelatedInformationRule], config:).run + + assert_equal(["Included primary location."], result.diagnostics.map(&:message)) + end + end + def test_runner_keeps_diagnostics_indexed_through_a_symlinked_workspace_path with_context do |context| context.write!("workspace/inside.rb") @@ -173,6 +272,18 @@ def linter_config(rules = {}) ) end + #: ( + #| String, + #| ?enabled: bool, + #| ?exclude_patterns: Array[String], + #| ?severity: singleton(Rubydex::Severity::Base)?, + #| ) -> Rubydex::LinterConfig + def configured_linter_config(rule_name, enabled: true, exclude_patterns: [], severity: nil) + Rubydex::LinterConfig.new( + rule_name => Rubydex::RuleConfig.new(rule_name, enabled, exclude_patterns, severity), + ) + end + #: (singleton(Rubydex::Severity::Base) severity) -> Rubydex::Diagnostic def diagnostic(severity) Rubydex::Diagnostic.new( diff --git a/test/location_test.rb b/test/location_test.rb index 5b5f42765..cb325eab8 100644 --- a/test/location_test.rb +++ b/test/location_test.rb @@ -16,6 +16,26 @@ def test_location_from_prism ) end + def test_to_file_path_decodes_the_uri_path_once + uri = Gem.win_platform? ? "file:///D:/my%20app+%2520/file.rb" : "file:///tmp/my%20app+%2520/file.rb" + expected = Gem.win_platform? ? "D:/my app+%20/file.rb" : "/tmp/my app+%20/file.rb" + location = Rubydex::Location.new(uri: uri, start_line: 0, end_line: 0, start_column: 0, end_column: 0) + + assert_equal(expected, location.to_file_path) + end + + def test_to_file_path_rejects_a_file_uri_without_a_path + location = Rubydex::Location.new( + uri: "file:relative", + start_line: 0, + end_line: 0, + start_column: 0, + end_column: 0, + ) + + assert_raises(Rubydex::Location::NotFileUriError) { location.to_file_path } + end + def test_display_location_from_prism_raises_with_conversion_path prism_location = PrismLocation.new(start_line: 2, start_column: 12, end_line: 3, end_column: 19) diff --git a/test/rubydex_linter/rules/rule_structure_test.rb b/test/rubydex_linter/rules/rule_structure_test.rb new file mode 100644 index 000000000..fab40fb43 --- /dev/null +++ b/test/rubydex_linter/rules/rule_structure_test.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +require "test_helper" +require "rubydex/linter/rule_test_case" +require "rubydex_linter/rules/rule_structure" + +module Rubydex + module Linter + module Rules + class RuleStructureTest < RuleTestCase + BASE_RULE_SOURCE = <<~RUBY + module Rubydex + module Linter + class Rule; end + module Rules; end + end + end + RUBY + + def setup + super + add_shared_source("lib/rubydex/linter/rule.rb" => BASE_RULE_SOURCE) + end + + def test_allows_one_rule_class_in_each_supported_rule_directory + assert_no_diagnostics( + "rubydex_linter/rules/project_rule.rb" => rule_source("ProjectRule"), + "lib/rubydex_linter/rules/gem_rule.rb" => rule_source("GemRule"), + ) + end + + def test_allows_indirect_rule_subclasses_and_helper_classes + assert_no_diagnostics( + "rubydex_linter/rules/base_rule.rb" => rule_source("BaseRule"), + "rubydex_linter/rules/indirect_rule.rb" => <<~RUBY, + class Rubydex::Linter::Rules::Helper; end + class Rubydex::Linter::Rules::IndirectRule < Rubydex::Linter::Rules::BaseRule; end + RUBY + ) + end + + def test_reports_a_rule_file_without_a_rule_class + diagnostics = assert_diagnostics( + "rubydex_linter/rules/not_a_rule.rb" => <<~RUBY, + class Rubydex::Linter::Rules::NotARule; end + ^{} Each rule file must define exactly one class that inherits from `Rubydex::Linter::Rule`; found 0. + RUBY + ) + + diagnostic = diagnostics.fetch(0) + assert_same(Severity::Error, diagnostic.severity) + assert_equal("RuleStructure", diagnostic.rule) + end + + def test_reports_multiple_rule_classes_in_one_file + diagnostics = assert_diagnostics( + "rubydex_linter/rules/two_rules.rb" => <<~RUBY, + class Rubydex::Linter::Rules::FirstRule < Rubydex::Linter::Rule; end + ^{} Each rule file must define exactly one class that inherits from `Rubydex::Linter::Rule`; found 2. + ^^^^^^^^^ `Rubydex::Linter::Rules::FirstRule` is defined here. + class Rubydex::Linter::Rules::SecondRule < Rubydex::Linter::Rule; end + ^^^^^^^^^^ `Rubydex::Linter::Rules::SecondRule` is defined here. + RUBY + ) + + assert_equal(1, diagnostics.length) + assert_equal( + [ + "`Rubydex::Linter::Rules::FirstRule` is defined here.", + "`Rubydex::Linter::Rules::SecondRule` is defined here.", + ], + diagnostics.fetch(0).related_information.map(&:message), + ) + end + + def test_reports_a_rule_class_outside_the_rules_namespace + assert_diagnostics( + "rubydex_linter/rules/wrong_namespace.rb" => <<~RUBY, + module ConsumerRules + class WrongNamespace < Rubydex::Linter::Rule; end + ^^^^^^^^^^^^^^ `ConsumerRules::WrongNamespace` must be defined under `Rubydex::Linter::Rules`. + end + RUBY + ) + end + + def test_reports_a_rule_class_outside_a_rule_directory + assert_diagnostics( + "app/rules/wrong_place.rb" => <<~RUBY, + class Rubydex::Linter::Rules::WrongPlace < Rubydex::Linter::Rule; end + ^^^^^^^^^^ `Rubydex::Linter::Rules::WrongPlace` must be defined under `rubydex_linter/rules/` or `lib/rubydex_linter/rules/`. + RUBY + ) + end + + def test_ignores_rule_fixture_classes_under_test_directories + assert_no_diagnostics( + "test/fixtures/fixture_rule.rb" => <<~RUBY, + module RuleStructureTestFixtures + class FixtureRule < Rubydex::Linter::Rule; end + end + RUBY + ) + end + + def test_checks_rule_files_nested_under_a_test_directory + assert_diagnostics( + "rubydex_linter/rules/test/not_a_rule.rb" => <<~RUBY, + class NotARule; end + ^{} Each rule file must define exactly one class that inherits from `Rubydex::Linter::Rule`; found 0. + RUBY + ) + end + + private + + #: (String) -> String + def rule_source(class_name) + "class Rubydex::Linter::Rules::#{class_name} < Rubydex::Linter::Rule; end\n" + end + end + end + end +end diff --git a/test/rule_loader_test.rb b/test/rule_loader_test.rb new file mode 100644 index 000000000..c6c15cbc6 --- /dev/null +++ b/test/rule_loader_test.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require "test_helper" +require "helpers/context" +require "mocha/minitest" +require "rubydex/linter" + +module RuleLoaderTestFixtures + class IntermediateRule < Rubydex::Linter::Rule + def severity = Rubydex::Severity::Error + def lint; end + end +end + +class RuleLoaderTest < Minitest::Test + include Test::Helpers::WithContext + + def test_load_returns_project_and_bundled_rules_on_repeated_calls + with_context do |context| + project_rule = "rubydex_linter/rules/project_rule.rb" + dependency_rule = "fake_gem/lib/rubydex_linter/rules/dependency_rule.rb" + write_rule(context, project_rule, "ProjectRule") + write_rule(context, dependency_rule, "DependencyRule") + Gem.stubs(:find_latest_files).with(Rubydex::Linter::RuleLoader::RULE_GLOB).returns( + [context.absolute_path_to(dependency_rule)], + ) + + with_bundle_gemfile(context.absolute_path_to("Gemfile")) do + first_load = Rubydex::Linter::RuleLoader.load(context.absolute_path) + second_load = Rubydex::Linter::RuleLoader.load(context.absolute_path) + + rule_names = first_load.map(&:rule_name) + assert_includes(rule_names, "DependencyRule") + assert_includes(rule_names, "ProjectRule") + assert_includes(rule_names, "RuleStructure") + assert_equal(first_load, second_load) + end + end + end + + def test_load_returns_built_in_rules_without_bundler + with_context do |context| + Gem.expects(:find_latest_files).never + + rules = with_bundle_gemfile(nil) do + Rubydex::Linter::RuleLoader.load(context.absolute_path) + end + + assert_includes(rules, Rubydex::Linter::Rules::RuleStructure) + end + end + + def test_load_returns_indirect_rule_subclasses + with_context do |context| + write_rule( + context, + "rubydex_linter/rules/indirect_rule.rb", + "IndirectRule", + superclass: "RuleLoaderTestFixtures::IntermediateRule", + ) + + rules = with_bundle_gemfile(nil) do + Rubydex::Linter::RuleLoader.load(context.absolute_path) + end + + assert_includes(rules, RuleLoaderTestFixtures::IndirectRule) + refute_includes(rules, RuleLoaderTestFixtures::IntermediateRule) + end + end + + def test_load_wraps_rule_file_errors + with_context do |context| + rule_file = "rubydex_linter/rules/broken_rule.rb" + context.write!(rule_file, "class BrokenRule <\n") + + error = with_bundle_gemfile(nil) do + assert_raises(Rubydex::Linter::RuleLoadError) do + Rubydex::Linter::RuleLoader.load(context.absolute_path) + end + end + + assert_match(/Unable to load linter rules from .*broken_rule\.rb/, error.message) + assert_instance_of(SyntaxError, error.cause) + end + end + + private + + #: (Test::Helpers::Context, String, String, ?superclass: String) -> void + def write_rule(context, path, class_name, superclass: "Rubydex::Linter::Rule") + context.write!(path, <<~RUBY) + # frozen_string_literal: true + + module RuleLoaderTestFixtures + class #{class_name} < #{superclass} + def severity = Rubydex::Severity::Error + def lint; end + end + end + RUBY + end + + #: [R] (String?) { -> R } -> R + def with_bundle_gemfile(value) + previous = ENV["BUNDLE_GEMFILE"] + ENV["BUNDLE_GEMFILE"] = value + yield + ensure + ENV["BUNDLE_GEMFILE"] = previous + end +end