From 2b164df56a2d18198cc17440a0d8d4100580a537 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Tue, 11 Aug 2026 21:11:33 +0100 Subject: [PATCH 1/2] Allow linter config to filter graph diagnostics --- ext/rubydex/diagnostic.c | 26 +++++++++++- lib/rubydex/cli/command/lint.rb | 2 +- lib/rubydex/linter/runner.rb | 35 +++++++++++----- rbi/rubydex.rbi | 3 ++ rust/rubydex-sys/src/diagnostic_api.rs | 35 +++++++++++++++- rust/rubydex/src/diagnostic.rs | 9 ++++ test/cli_test.rb | 33 +++++++++++++++ test/diagnostic_test.rb | 18 ++++++++ test/linter_test.rb | 57 ++++++++++++++++++++++++++ 9 files changed, 204 insertions(+), 14 deletions(-) diff --git a/ext/rubydex/diagnostic.c b/ext/rubydex/diagnostic.c index 2f6df4b36..fa972a15b 100644 --- a/ext/rubydex/diagnostic.c +++ b/ext/rubydex/diagnostic.c @@ -8,6 +8,27 @@ VALUE cDiagnostic; +/* + * call-seq: + * Rubydex::Diagnostic.graph_rule_names -> Array[String] + * + * Returns the names of all diagnostics that the graph can emit. + */ +static VALUE rdxr_graph_diagnostic_names(VALUE klass) { + (void)klass; + + const char *const *names = NULL; + size_t count = rdx_graph_diagnostic_names(&names); + VALUE rule_names = rb_ary_new_capa((long)count); + + for (size_t i = 0; i < count; i++) { + rb_ary_push(rule_names, rb_str_freeze(rb_utf8_str_new_cstr(names[i]))); + } + + free_c_string_array(names, count); + return rb_obj_freeze(rule_names); +} + VALUE rdxi_build_diagnostic_severity_value(VALUE mRubydex, DiagnosticSeverity severity) { VALUE mSeverity = rb_const_get(mRubydex, rb_intern("Severity")); @@ -27,4 +48,7 @@ VALUE rdxi_build_diagnostic_severity_value(VALUE mRubydex, DiagnosticSeverity se return Qnil; } -void rdxi_initialize_diagnostic(VALUE mRubydex) { cDiagnostic = rb_define_class_under(mRubydex, "Diagnostic", rb_cObject); } +void rdxi_initialize_diagnostic(VALUE mRubydex) { + cDiagnostic = rb_define_class_under(mRubydex, "Diagnostic", rb_cObject); + rb_define_singleton_method(cDiagnostic, "graph_rule_names", rdxr_graph_diagnostic_names, 0); +} diff --git a/lib/rubydex/cli/command/lint.rb b/lib/rubydex/cli/command/lint.rb index 80f86c570..badc9cbd3 100644 --- a/lib/rubydex/cli/command/lint.rb +++ b/lib/rubydex/cli/command/lint.rb @@ -43,7 +43,7 @@ def run #: (Rubydex::LinterConfig config, Array[singleton(Rubydex::Linter::Rule)] known_rule_classes) -> void def warn_unknown_rules(config, known_rule_classes) - known_rule_names = known_rule_classes.map(&:rule_name).uniq.sort + known_rule_names = (known_rule_classes.map(&:rule_name) + Rubydex::Diagnostic.graph_rule_names).uniq.sort unknown_rule_names = config.rules.keys.reject { |name| known_rule_names.include?(name) }.sort return if unknown_rule_names.empty? diff --git a/lib/rubydex/linter/runner.rb b/lib/rubydex/linter/runner.rb index 5e597cc9b..0d38d81b1 100644 --- a/lib/rubydex/linter/runner.rb +++ b/lib/rubydex/linter/runner.rb @@ -24,11 +24,15 @@ def run rule_diagnostics = @rules.flat_map do |rule_class| rule = rule_class.new(@graph, config: @config) rule.lint - filter_diagnostics(rule.diagnostics, @config.excludes_for(rule_class)) + rule.diagnostics end + # Graph diagnostics are surfaced by the linter, but not owned by it. Linter configuration controls + # surfacing here, not registration in the graph. diagnostics = (@graph.diagnostics + rule_diagnostics).select do |diagnostic| - diagnostic_in_workspace?(diagnostic) + !location_in_dependency_path?(diagnostic.location) && + diagnostic_included_by_config?(diagnostic) && + diagnostic_in_workspace?(diagnostic) end.sort_by do |diagnostic| location = diagnostic.location [ @@ -47,22 +51,31 @@ 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 + #: (Diagnostic) -> bool + def diagnostic_included_by_config?(diagnostic) + rule_config = @config.rules[diagnostic.rule] + return true unless rule_config + + rule_config.enabled? && !location_matches_patterns?(diagnostic.location, rule_config.exclude_patterns) end - #: (Location, Array[String]) -> bool - def location_excluded?(location, patterns) + #: (Location) -> bool + def location_in_dependency_path?(location) path = location.to_file_path - return true if @dependency_paths.any? do |dependency_path| + + @dependency_paths.any? do |dependency_path| path == dependency_path || path.start_with?("#{dependency_path}/") end + rescue Location::NotFileUriError + false + end + + #: (Location, Array[String]) -> bool + def location_matches_patterns?(location, patterns) + return false if patterns.empty? Helpers::PathHelpers.path_matches_patterns?( - path, + location.to_file_path, patterns, workspace: @graph.workspace_path, flags: Helpers::PathHelpers::RUBOCOP_EXCLUDE_FNMATCH_FLAGS, diff --git a/rbi/rubydex.rbi b/rbi/rubydex.rbi index d812272c4..095e6dba8 100644 --- a/rbi/rubydex.rbi +++ b/rbi/rubydex.rbi @@ -329,6 +329,9 @@ class Rubydex::RelatedInformation end class Rubydex::Diagnostic + sig { returns(T::Array[String]) } + def self.graph_rule_names; end + sig do params( rule: String, diff --git a/rust/rubydex-sys/src/diagnostic_api.rs b/rust/rubydex-sys/src/diagnostic_api.rs index 8188d1a06..43861b1fa 100644 --- a/rust/rubydex-sys/src/diagnostic_api.rs +++ b/rust/rubydex-sys/src/diagnostic_api.rs @@ -3,7 +3,7 @@ use crate::graph_api::{GraphPointer, with_graph}; use crate::location_api::{Location, create_location_for_uri_and_offset}; use libc::c_char; -use rubydex::diagnostic::Severity; +use rubydex::diagnostic::{Rule, Severity}; use std::{ffi::CString, mem, ptr}; /// C-compatible enum representing diagnostic severity levels. @@ -52,6 +52,39 @@ impl DiagnosticArray { } } +/// Writes a Rust-allocated array containing every graph diagnostic rule name to `out_names` and returns its length. +/// The caller must free the array with `free_c_string_array`. Writes null and returns zero when there are no names. +/// +/// # Panics +/// +/// Panics if a generated rule name contains `\0`, which C strings cannot represent. +/// +/// # Safety +/// +/// - `out_names` must be a valid, writable pointer. +#[unsafe(no_mangle)] +pub unsafe extern "C" fn rdx_graph_diagnostic_names(out_names: *mut *const *const c_char) -> usize { + let c_strings: Vec<*const c_char> = Rule::ALL + .iter() + .map(ToString::to_string) + .map(|name| { + CString::new(name) + .expect("generated graph diagnostic rule names cannot contain null bytes") + .into_raw() + .cast_const() + }) + .collect(); + let count = c_strings.len(); + + if count == 0 { + unsafe { *out_names = ptr::null() }; + return 0; + } + + unsafe { *out_names = Box::into_raw(c_strings.into_boxed_slice()).cast::<*const c_char>() }; + count +} + /// Returns all diagnostics currently recorded in the global graph. /// /// # Safety diff --git a/rust/rubydex/src/diagnostic.rs b/rust/rubydex/src/diagnostic.rs index a39139cad..aad23ea1d 100644 --- a/rust/rubydex/src/diagnostic.rs +++ b/rust/rubydex/src/diagnostic.rs @@ -98,6 +98,15 @@ macro_rules! rules { )* } + impl Rule { + /// All diagnostic rules emitted by the graph. + pub const ALL: &[Self] = &[ + $( + Self::$variant, + )* + ]; + } + impl std::fmt::Display for Rule { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", match self { diff --git a/test/cli_test.rb b/test/cli_test.rb index 60cfb2f53..4d92b5a0b 100644 --- a/test/cli_test.rb +++ b/test/cli_test.rb @@ -296,6 +296,39 @@ def test_lint_allows_a_clean_workspace end end + def test_lint_accepts_and_disables_a_graph_diagnostic_rule + with_context do |context| + write_linter_rule(context, "CLITestGraphDiagnosticConfigRule") + context.write!("app.rb", "unused = true") + context.write!("rubydex.toml", <<~TOML) + [linter.rules.parse-warning] + enabled = false + TOML + + result = with_bundle_gemfile(nil) { rdx("lint", context.absolute_path) } + + assert_success_status(result) + assert_stdout_includes_pattern(result, /\d+ files inspected, no offenses detected/) + refute_stderr_includes(result, "linter config references rules that were not loaded") + end + end + + def test_lint_warns_about_an_unknown_configured_rule + with_context do |context| + write_linter_rule(context, "CLITestUnknownConfiguredRule") + context.write!("app.rb", "class Bar; end") + context.write!("rubydex.toml", <<~TOML) + [linter.rules.not-a-rule] + enabled = false + TOML + + result = with_bundle_gemfile(nil) { rdx("lint", context.absolute_path) } + + assert_success_status(result) + assert_stderr_includes(result, "linter config references rules that were not loaded: `not-a-rule`") + end + end + def test_lint_loads_rules_from_bundled_dependencies with_context do |context| rule_path = "fake_gem/lib/rubydex_linter/rules/no_foo.rb" diff --git a/test/diagnostic_test.rb b/test/diagnostic_test.rb index 8caf9bd10..d0440b738 100644 --- a/test/diagnostic_test.rb +++ b/test/diagnostic_test.rb @@ -3,6 +3,24 @@ require "test_helper" class DiagnosticTest < Minitest::Test + def test_graph_rule_names + assert_equal( + [ + "parse-error", + "parse-warning", + "dynamic-constant-reference", + "dynamic-singleton-definition", + "dynamic-ancestor", + "top-level-mixin-self", + "invalid-constant-visibility", + "invalid-method-visibility", + "undefined-method-visibility-target", + "undefined-constant-visibility-target", + ], + Rubydex::Diagnostic.graph_rule_names, + ) + end + def test_severity_from_value { error: Rubydex::Severity::Error, diff --git a/test/linter_test.rb b/test/linter_test.rb index 6913e5634..d7bb81f44 100644 --- a/test/linter_test.rb +++ b/test/linter_test.rb @@ -188,6 +188,47 @@ def test_runner_includes_native_graph_diagnostics refute_predicate(result, :success?) end + def test_runner_drops_disabled_graph_diagnostics_by_rule_name + with_context do |context| + context.write!("workspace/warning.rb", "unused = true") + context.write!("workspace/error.rb", "class Broken") + graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + graph.index_all(context.glob("workspace/**/*.rb")) + config = configured_linter_config("parse-warning", enabled: false) + + assert_equal(["parse-error", "parse-error", "parse-warning"], graph.diagnostics.map(&:rule).sort) + + result = Rubydex::Linter::Runner.new(graph, rules: [], config:).run + + assert_equal(["parse-error", "parse-error"], result.diagnostics.map(&:rule)) + end + end + + def test_runner_does_not_report_graph_diagnostics_from_excluded_paths + with_context do |context| + context.write!("workspace/components/legacy/example.rb", "unused = true") + context.write!("workspace/components/current/example.rb", "unused = true") + graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + graph.index_all(context.glob("workspace/**/*.rb")) + config = configured_linter_config("parse-warning", exclude_patterns: ["components/legacy/**"]) + + expected_uris = [ + context.uri_to("workspace/components/current/example.rb"), + context.uri_to("workspace/components/legacy/example.rb"), + ] + # Graph diagnostics are not disabled via linter configs and should still be created. + assert_equal(expected_uris.sort, graph.diagnostics.map { |diagnostic| diagnostic.location.uri }.sort) + + result = Rubydex::Linter::Runner.new(graph, rules: [], config:).run + + # But the excluded graph diagnostics will not appear in the linter results. + assert_equal( + [expected_uris.first], + result.diagnostics.map { |diagnostic| diagnostic.location.uri }, + ) + end + end + def test_runner_accepts_no_rules result = Rubydex::Linter::Runner.new(Rubydex::Graph.new, rules: [], config: linter_config).run @@ -220,6 +261,22 @@ def test_runner_filters_diagnostics_under_dependency_paths end end + def test_runner_filters_graph_diagnostics_under_dependency_paths + with_context do |context| + context.write!("workspace/vendor/bundle/gems/broken.rb", "class Broken") + graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + graph.index_all(context.glob("workspace/**/*.rb")) + dependency_path = context.absolute_path_to("workspace/vendor/bundle") + Gem.stubs(:path).returns([dependency_path]) + + assert_equal(["parse-error", "parse-error"], graph.diagnostics.map(&:rule)) + + result = Rubydex::Linter::Runner.new(graph, rules: [], 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") From aed370506c601ed8046c578e25508fc359791faa Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Wed, 12 Aug 2026 22:01:54 +0100 Subject: [PATCH 2/2] Apply linter configuration to built-in rules Assisted-By: devx/8c506d68-42fe-4150-abdf-cf627a09bef8 --- README.md | 1 + ext/rubydex/diagnostic.c | 22 -- ext/rubydex/linter_rule.c | 56 +++ ext/rubydex/linter_rule.h | 8 + ext/rubydex/rubydex.c | 2 + lib/rubydex/cli/command/lint.rb | 3 +- lib/rubydex/linter/rule.rb | 19 + lib/rubydex/linter/runner.rb | 25 -- rbi/rubydex.rbi | 6 +- rust/rubydex-sys/src/graph_api.rs | 4 +- rust/rubydex/src/config.rs | 329 +++++++++++++++++- rust/rubydex/src/diagnostic.rs | 58 ++- rust/rubydex/src/indexing.rs | 138 +++++++- rust/rubydex/src/indexing/rbs_indexer.rs | 9 +- rust/rubydex/src/indexing/ruby_indexer.rs | 10 +- rust/rubydex/src/model/document.rs | 23 +- rust/rubydex/src/model/graph.rs | 20 +- rust/rubydex/src/operation/ruby_builder.rs | 9 +- rust/rubydex/src/test_utils/graph_test.rs | 8 +- .../src/test_utils/local_graph_test.rs | 11 +- test/cli_test.rb | 12 +- test/diagnostic_test.rb | 18 - test/linter_test.rb | 88 +++-- 23 files changed, 723 insertions(+), 156 deletions(-) create mode 100644 ext/rubydex/linter_rule.c create mode 100644 ext/rubydex/linter_rule.h diff --git a/README.md b/README.md index dbc2051b5..a37661605 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,7 @@ Configure a rule in `rubydex.toml`: [linter.rules.] enabled = true exclude = ["path_to_skip/**"] +severity = "warning" ``` ### `rdx mcp` diff --git a/ext/rubydex/diagnostic.c b/ext/rubydex/diagnostic.c index fa972a15b..12ccc300f 100644 --- a/ext/rubydex/diagnostic.c +++ b/ext/rubydex/diagnostic.c @@ -8,27 +8,6 @@ VALUE cDiagnostic; -/* - * call-seq: - * Rubydex::Diagnostic.graph_rule_names -> Array[String] - * - * Returns the names of all diagnostics that the graph can emit. - */ -static VALUE rdxr_graph_diagnostic_names(VALUE klass) { - (void)klass; - - const char *const *names = NULL; - size_t count = rdx_graph_diagnostic_names(&names); - VALUE rule_names = rb_ary_new_capa((long)count); - - for (size_t i = 0; i < count; i++) { - rb_ary_push(rule_names, rb_str_freeze(rb_utf8_str_new_cstr(names[i]))); - } - - free_c_string_array(names, count); - return rb_obj_freeze(rule_names); -} - VALUE rdxi_build_diagnostic_severity_value(VALUE mRubydex, DiagnosticSeverity severity) { VALUE mSeverity = rb_const_get(mRubydex, rb_intern("Severity")); @@ -50,5 +29,4 @@ VALUE rdxi_build_diagnostic_severity_value(VALUE mRubydex, DiagnosticSeverity se void rdxi_initialize_diagnostic(VALUE mRubydex) { cDiagnostic = rb_define_class_under(mRubydex, "Diagnostic", rb_cObject); - rb_define_singleton_method(cDiagnostic, "graph_rule_names", rdxr_graph_diagnostic_names, 0); } diff --git a/ext/rubydex/linter_rule.c b/ext/rubydex/linter_rule.c new file mode 100644 index 000000000..c9d235b32 --- /dev/null +++ b/ext/rubydex/linter_rule.c @@ -0,0 +1,56 @@ +#include "linter_rule.h" +#include "rustbindings.h" + +/* + * RDoc parser workaround for https://github.com/ruby/rdoc/issues/1744: + * mRubydex = rb_define_module("Rubydex") + */ + +struct BuiltInRuleNames { + const char *const *names; + size_t count; +}; + +static VALUE linter_rule_build_built_in_rules_names(VALUE opaque_names) { + struct BuiltInRuleNames *names = (struct BuiltInRuleNames *)(uintptr_t)opaque_names; + VALUE rule_names = rb_ary_new_capa((long)names->count); + + for (size_t i = 0; i < names->count; i++) { + rb_ary_push(rule_names, rb_str_freeze(rb_utf8_str_new_cstr(names->names[i]))); + } + + return rb_obj_freeze(rule_names); +} + +static VALUE linter_rule_free_built_in_rules_names(VALUE opaque_names) { + struct BuiltInRuleNames *names = (struct BuiltInRuleNames *)(uintptr_t)opaque_names; + free_c_string_array(names->names, names->count); + return Qnil; +} + +/* + * call-seq: + * Rubydex::Linter::Rule.built_in_rules_names -> Array[String] + * + * Returns the names of all built-in linter rules. + */ +static VALUE rdxr_linter_rule_built_in_rules_names(VALUE klass) { + (void)klass; + + struct BuiltInRuleNames names = {.names = NULL, .count = 0}; + names.count = rdx_graph_diagnostic_names(&names.names); + VALUE opaque_names = (VALUE)(uintptr_t)&names; + + return rb_ensure( + linter_rule_build_built_in_rules_names, + opaque_names, + linter_rule_free_built_in_rules_names, + opaque_names + ); +} + +void rdxi_initialize_linter_rule(VALUE mRubydex) { + VALUE mLinter = rb_define_module_under(mRubydex, "Linter"); + VALUE cRule = rb_define_class_under(mLinter, "Rule", rb_cObject); + rb_define_singleton_method(cRule, "built_in_rules_names", rdxr_linter_rule_built_in_rules_names, 0); +} diff --git a/ext/rubydex/linter_rule.h b/ext/rubydex/linter_rule.h new file mode 100644 index 000000000..ffce3627b --- /dev/null +++ b/ext/rubydex/linter_rule.h @@ -0,0 +1,8 @@ +#ifndef RUBYDEX_LINTER_RULE_H +#define RUBYDEX_LINTER_RULE_H + +#include "ruby.h" + +void rdxi_initialize_linter_rule(VALUE mRubydex); + +#endif // RUBYDEX_LINTER_RULE_H diff --git a/ext/rubydex/rubydex.c b/ext/rubydex/rubydex.c index 8cc2ed763..690f7ff84 100644 --- a/ext/rubydex/rubydex.c +++ b/ext/rubydex/rubydex.c @@ -5,6 +5,7 @@ #include "document.h" #include "graph.h" #include "location.h" +#include "linter_rule.h" #include "query.h" #include "reference.h" #include "signature.h" @@ -28,6 +29,7 @@ void Init_rubydex(void) { rdxi_initialize_definition(mRubydex); rdxi_initialize_location(mRubydex); rdxi_initialize_diagnostic(mRubydex); + rdxi_initialize_linter_rule(mRubydex); rdxi_initialize_reference(mRubydex); rdxi_initialize_signature(mRubydex); } diff --git a/lib/rubydex/cli/command/lint.rb b/lib/rubydex/cli/command/lint.rb index badc9cbd3..56f475f91 100644 --- a/lib/rubydex/cli/command/lint.rb +++ b/lib/rubydex/cli/command/lint.rb @@ -24,7 +24,6 @@ def run require "rubydex/linter" rules = load_linter_rules(workspace_path) - abort("No Rubydex::Linter::Rule subclasses were loaded") if rules.empty? config = Rubydex::Config.load(workspace_path) warn_unknown_rules(config.linter, rules) @@ -43,7 +42,7 @@ def run #: (Rubydex::LinterConfig config, Array[singleton(Rubydex::Linter::Rule)] known_rule_classes) -> void def warn_unknown_rules(config, known_rule_classes) - known_rule_names = (known_rule_classes.map(&:rule_name) + Rubydex::Diagnostic.graph_rule_names).uniq.sort + known_rule_names = (known_rule_classes.map(&:rule_name) + Rubydex::Linter::Rule.built_in_rules_names).uniq.sort unknown_rule_names = config.rules.keys.reject { |name| known_rule_names.include?(name) }.sort return if unknown_rule_names.empty? diff --git a/lib/rubydex/linter/rule.rb b/lib/rubydex/linter/rule.rb index 4d00cdd4b..15a96cd13 100644 --- a/lib/rubydex/linter/rule.rb +++ b/lib/rubydex/linter/rule.rb @@ -93,6 +93,9 @@ def rule_name #| ?related_information: Array[RelatedInformation], #| ) -> void def add_diagnostic(message, location, related_information: []) + exclude_patterns = config.excludes_for(self.class) + return if location_matches_patterns?(location, exclude_patterns) + @diagnostics << Diagnostic.new( rule: self.class.rule_name, message: message, @@ -101,6 +104,22 @@ def add_diagnostic(message, location, related_information: []) related_information: related_information, ) end + + private + + #: (Location, Array[String]) -> bool + def location_matches_patterns?(location, patterns) + return false if patterns.empty? + + Helpers::PathHelpers.path_matches_patterns?( + location.to_file_path, + patterns, + workspace: graph.workspace_path, + flags: Helpers::PathHelpers::RUBOCOP_EXCLUDE_FNMATCH_FLAGS, + ) + rescue Location::NotFileUriError + false + end end class MissingGraphDependencyError < StandardError diff --git a/lib/rubydex/linter/runner.rb b/lib/rubydex/linter/runner.rb index 0d38d81b1..bfb0d882a 100644 --- a/lib/rubydex/linter/runner.rb +++ b/lib/rubydex/linter/runner.rb @@ -27,11 +27,8 @@ def run rule.diagnostics end - # Graph diagnostics are surfaced by the linter, but not owned by it. Linter configuration controls - # surfacing here, not registration in the graph. diagnostics = (@graph.diagnostics + rule_diagnostics).select do |diagnostic| !location_in_dependency_path?(diagnostic.location) && - diagnostic_included_by_config?(diagnostic) && diagnostic_in_workspace?(diagnostic) end.sort_by do |diagnostic| location = diagnostic.location @@ -51,14 +48,6 @@ def run private - #: (Diagnostic) -> bool - def diagnostic_included_by_config?(diagnostic) - rule_config = @config.rules[diagnostic.rule] - return true unless rule_config - - rule_config.enabled? && !location_matches_patterns?(diagnostic.location, rule_config.exclude_patterns) - end - #: (Location) -> bool def location_in_dependency_path?(location) path = location.to_file_path @@ -70,20 +59,6 @@ def location_in_dependency_path?(location) false end - #: (Location, Array[String]) -> bool - def location_matches_patterns?(location, patterns) - return false if patterns.empty? - - Helpers::PathHelpers.path_matches_patterns?( - location.to_file_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 = diagnostic.location.to_file_path diff --git a/rbi/rubydex.rbi b/rbi/rubydex.rbi index 095e6dba8..346cc6362 100644 --- a/rbi/rubydex.rbi +++ b/rbi/rubydex.rbi @@ -329,9 +329,6 @@ class Rubydex::RelatedInformation end class Rubydex::Diagnostic - sig { returns(T::Array[String]) } - def self.graph_rule_names; end - sig do params( rule: String, @@ -428,6 +425,9 @@ end class Rubydex::Linter::Rule abstract! + sig { returns(T::Array[String]) } + def self.built_in_rules_names; end + sig { returns(String) } def self.rule_name; end diff --git a/rust/rubydex-sys/src/graph_api.rs b/rust/rubydex-sys/src/graph_api.rs index 05d0586d5..1101e5ec7 100644 --- a/rust/rubydex-sys/src/graph_api.rs +++ b/rust/rubydex-sys/src/graph_api.rs @@ -246,8 +246,8 @@ pub unsafe extern "C" fn rdx_graph_workspace_path(pointer: GraphPointer) -> *con } /// Applies a parsed configuration file to the graph, which adopts the workspace it was loaded for along with the -/// settings of its `[graph]` section. This is the only way to point the graph at a workspace other than the current -/// directory, and it replaces any previously applied configuration. Tool-specific sections are ignored. +/// settings of its `[graph]` and `[linter]` sections. This is the only way to point the graph at a workspace other than +/// the current directory, and it replaces any previously applied configuration. /// /// # Safety /// diff --git a/rust/rubydex/src/config.rs b/rust/rubydex/src/config.rs index 6450abfe3..5902b6bd4 100644 --- a/rust/rubydex/src/config.rs +++ b/rust/rubydex/src/config.rs @@ -1,5 +1,5 @@ use crate::assert_mem_size; -use crate::diagnostic::Severity; +use crate::diagnostic::{Diagnostic, Rule as DiagnosticRule, Severity}; use crate::errors::Errors; use crate::path_helpers; use std::collections::HashSet; @@ -68,12 +68,274 @@ impl GraphSettings { } } +/// Precompiled equivalent of the Ruby `File.fnmatch` flags used by custom linter rule excludes. +#[derive(Debug, Clone)] +struct FnmatchPattern { + tokens: Box<[FnmatchToken]>, +} + +#[derive(Debug, Clone)] +enum FnmatchToken { + Literal(char), + AnyChar, + AnySequence, + AnyRecursiveSequence, + CharacterClass { + negated: bool, + specifiers: Box<[CharacterSpecifier]>, + }, +} + +#[derive(Debug, Clone, Copy)] +enum CharacterSpecifier { + Character(char), + Range(char, char), +} + +#[derive(Debug, Clone, Copy)] +struct CharacterClassAtom { + character: char, + escaped: bool, +} + +impl FnmatchPattern { + fn compile_all(pattern: &str) -> Vec { + expand_braces(pattern) + .unwrap_or_default() + .into_iter() + .filter_map(|pattern| Self::compile(&pattern)) + .collect() + } + + fn compile(pattern: &str) -> Option { + let characters: Vec = pattern.chars().collect(); + let mut tokens = Vec::new(); + let mut index = 0; + + while index < characters.len() { + match characters[index] { + '\\' => { + index += 1; + tokens.push(FnmatchToken::Literal(*characters.get(index)?)); + index += 1; + } + '?' => { + tokens.push(FnmatchToken::AnyChar); + index += 1; + } + '*' => { + let start = index; + while characters.get(index) == Some(&'*') { + index += 1; + } + + let is_recursive = index - start == 2 + && (start == 0 || characters[start - 1] == '/') + && characters.get(index) == Some(&'/'); + if is_recursive { + tokens.push(FnmatchToken::AnyRecursiveSequence); + index += 1; + } else { + tokens.push(FnmatchToken::AnySequence); + } + } + '[' => { + let (token, next_index) = parse_character_class(&characters, index)?; + tokens.push(token); + index = next_index; + } + '{' => return None, + character => { + tokens.push(FnmatchToken::Literal(character)); + index += 1; + } + } + } + + Some(Self { + tokens: tokens.into_boxed_slice(), + }) + } + + fn matches(&self, path: &str) -> bool { + let path: Vec = path.chars().collect(); + let mut memo = vec![vec![None; path.len() + 1]; self.tokens.len() + 1]; + self.matches_from(0, 0, &path, &mut memo) + } + + fn matches_from( + &self, + token_index: usize, + path_index: usize, + path: &[char], + memo: &mut [Vec>], + ) -> bool { + if let Some(result) = memo[token_index][path_index] { + return result; + } + + let result = match self.tokens.get(token_index) { + None => path_index == path.len(), + Some(FnmatchToken::Literal(expected)) => { + path.get(path_index) == Some(expected) && self.matches_from(token_index + 1, path_index + 1, path, memo) + } + Some(FnmatchToken::AnyChar) => { + path.get(path_index).is_some_and(|character| *character != '/') + && self.matches_from(token_index + 1, path_index + 1, path, memo) + } + Some(FnmatchToken::AnySequence) => { + self.matches_from(token_index + 1, path_index, path, memo) + || (path.get(path_index).is_some_and(|character| *character != '/') + && self.matches_from(token_index, path_index + 1, path, memo)) + } + Some(FnmatchToken::AnyRecursiveSequence) => { + self.matches_from(token_index + 1, path_index, path, memo) + || path[path_index..].iter().enumerate().any(|(offset, character)| { + *character == '/' && self.matches_from(token_index + 1, path_index + offset + 1, path, memo) + }) + } + Some(FnmatchToken::CharacterClass { negated, specifiers }) => path + .get(path_index) + .filter(|character| **character != '/') + .is_some_and(|character| { + let included = specifiers.iter().any(|specifier| match specifier { + CharacterSpecifier::Character(expected) => character == expected, + CharacterSpecifier::Range(start, end) if start <= end => start <= character && character <= end, + CharacterSpecifier::Range(start, end) => character == start || character == end, + }); + included != *negated && self.matches_from(token_index + 1, path_index + 1, path, memo) + }), + }; + + memo[token_index][path_index] = Some(result); + result + } +} + +fn parse_character_class(characters: &[char], open: usize) -> Option<(FnmatchToken, usize)> { + let mut index = open + 1; + let negated = matches!(characters.get(index), Some('!' | '^')); + if negated { + index += 1; + } + + let mut atoms = Vec::new(); + loop { + match *characters.get(index)? { + ']' => break, + '{' | '}' => return None, + '\\' => { + index += 1; + atoms.push(CharacterClassAtom { + character: *characters.get(index)?, + escaped: true, + }); + index += 1; + } + character => { + atoms.push(CharacterClassAtom { + character, + escaped: false, + }); + index += 1; + } + } + } + + if atoms.is_empty() { + return None; + } + + let mut specifiers = Vec::new(); + let mut atom_index = 0; + while atom_index < atoms.len() { + if let (Some(start), Some(hyphen), Some(end)) = ( + atoms.get(atom_index), + atoms.get(atom_index + 1), + atoms.get(atom_index + 2), + ) && hyphen.character == '-' + && !hyphen.escaped + { + specifiers.push(CharacterSpecifier::Range(start.character, end.character)); + atom_index += 3; + } else { + specifiers.push(CharacterSpecifier::Character(atoms[atom_index].character)); + atom_index += 1; + } + } + + Some(( + FnmatchToken::CharacterClass { + negated, + specifiers: specifiers.into_boxed_slice(), + }, + index + 1, + )) +} + +fn expand_braces(pattern: &str) -> Result, ()> { + let Some((open, close, commas)) = brace_group(pattern)? else { + return Ok(vec![pattern.to_string()]); + }; + + let mut boundaries = Vec::with_capacity(commas.len() + 2); + boundaries.push(open + 1); + boundaries.extend(commas.iter().map(|comma| comma + 1)); + + let mut expanded_patterns = Vec::new(); + for (index, start) in boundaries.iter().enumerate() { + let end = commas.get(index).copied().unwrap_or(close); + let mut expanded = String::with_capacity(pattern.len()); + expanded.push_str(&pattern[..open]); + expanded.push_str(&pattern[*start..end]); + expanded.push_str(&pattern[close + 1..]); + expanded_patterns.extend(expand_braces(&expanded)?); + } + + Ok(expanded_patterns) +} + +fn brace_group(pattern: &str) -> Result)>, ()> { + let mut groups: Vec<(usize, Vec)> = Vec::new(); + let mut escaped = false; + let mut in_character_class = false; + + for (index, character) in pattern.char_indices() { + if escaped { + escaped = false; + continue; + } + + match character { + '\\' => escaped = true, + '[' => in_character_class = true, + ']' if in_character_class => in_character_class = false, + _ if in_character_class => {} + '{' => groups.push((index, Vec::new())), + ',' => { + if let Some((_, commas)) = groups.last_mut() { + commas.push(index); + } + } + '}' => { + if let Some((open, commas)) = groups.pop() { + return Ok(Some((open, index, commas))); + } + } + _ => {} + } + } + + if groups.is_empty() { Ok(None) } else { Err(()) } +} + /// The setting of a single linter rule, read from a `[linter.rules.RuleName]` table #[derive(Debug, Clone)] pub struct Rule { name: Box, enabled: bool, exclude_patterns: Box<[Box]>, + exclude_matchers: Box<[FnmatchPattern]>, severity: Option, } @@ -98,6 +360,10 @@ impl Rule { self.severity.as_ref() } + fn excludes(&self, path: &str) -> bool { + self.exclude_matchers.iter().any(|pattern| pattern.matches(path)) + } + /// Parses a single `[linter.rules.{name}]` table fn parse(name: &str, value: Value) -> Result { let Value::Table(mut table) = value else { @@ -135,10 +401,16 @@ impl Rule { return Err(format!("unknown setting `linter.rules.{name}.{key}`")); } + let exclude_matchers = exclude_patterns + .iter() + .flat_map(|pattern| FnmatchPattern::compile_all(pattern)) + .collect(); + Ok(Self { name: Box::from(name), enabled, exclude_patterns, + exclude_matchers, severity, }) } @@ -156,6 +428,12 @@ impl LinterSettings { &self.rules } + #[must_use] + pub(crate) fn diagnostic_rule(&self, rule: DiagnosticRule) -> Option<&Rule> { + let name = rule.name(); + self.rules.iter().find(|configured_rule| configured_rule.name() == name) + } + /// Parses the `[linter]` section fn parse(mut table: Table) -> Result { let rules = match table.remove("rules") { @@ -288,6 +566,27 @@ impl Config { &self.linter } + pub(crate) fn configure_diagnostic(&self, document_path: Option<&Path>, diagnostic: &mut Diagnostic) -> bool { + let Some(configured_rule) = self.linter.diagnostic_rule(*diagnostic.rule()) else { + return true; + }; + if !configured_rule.enabled() { + return false; + } + + if let Some(document_path) = document_path + && let Ok(relative_path) = document_path.strip_prefix(&self.workspace_path) + && configured_rule.excludes(&relative_path.to_string_lossy().replace(MAIN_SEPARATOR, "/")) + { + return false; + } + + if let Some(severity) = configured_rule.severity() { + diagnostic.set_severity(*severity); + } + true + } + /// Parses the content of the configuration file of the workspace rooted at `workspace_path` into the typed /// settings of each section fn parse(workspace_path: PathBuf, content: &str) -> Result { @@ -354,6 +653,34 @@ mod tests { Config::parse(PathBuf::from("/workspace"), content) } + #[test] + fn linter_excludes_match_ruby_fnmatch_semantics() { + for (pattern, path, expected) in [ + ("a/**", "a/x", true), + ("a/**", "a/x/y", false), + ("a/**/b", "a/x/y/b", true), + ("a/**b", "a/xxb", true), + ("a***b", "axxxb", true), + (r"foo/\*.rb", "foo/*.rb", true), + (r"foo/\*.rb", "foo/x.rb", false), + ("[^a]", "b", true), + ("[z-a]", "z", true), + ("[z-a]", "m", false), + ("a{b}c", "abc", true), + ("a{b,{c,d}}e", "ade", true), + ("{", "{", false), + ("*", ".hidden", true), + ] { + let matches = FnmatchPattern::compile_all(pattern) + .iter() + .any(|pattern| pattern.matches(path)); + assert_eq!( + expected, matches, + "unexpected match result for {pattern:?} and {path:?}" + ); + } + } + #[test] fn excluded_patterns_resolves_patterns_against_the_workspace_path() { let mut config = parse("").expect("an empty config is valid"); diff --git a/rust/rubydex/src/diagnostic.rs b/rust/rubydex/src/diagnostic.rs index aad23ea1d..28b5654d9 100644 --- a/rust/rubydex/src/diagnostic.rs +++ b/rust/rubydex/src/diagnostic.rs @@ -34,6 +34,10 @@ impl Diagnostic { &self.severity } + pub(crate) fn set_severity(&mut self, severity: Severity) { + self.severity = severity; + } + #[must_use] pub fn uri_id(&self) -> &UriId { &self.uri_id @@ -70,26 +74,9 @@ pub enum Severity { Hint, } -fn camel_to_snake(s: &str) -> String { - let mut snake = String::new(); - for (i, ch) in s.chars().enumerate() { - if ch.is_uppercase() { - if i != 0 { - snake.push('-'); - } - for lc in ch.to_lowercase() { - snake.push(lc); - } - } else { - snake.push(ch); - } - } - snake -} - macro_rules! rules { ( - $( $variant:ident );* $(;)? + $( $variant:ident => $name:literal );* $(;)? ) => { #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum Rule { @@ -105,15 +92,20 @@ macro_rules! rules { Self::$variant, )* ]; + + #[must_use] + pub const fn name(self) -> &'static str { + match self { + $( + Self::$variant => $name, + )* + } + } } impl std::fmt::Display for Rule { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", match self { - $( - Rule::$variant => camel_to_snake(stringify!($variant)), - )* - }) + f.write_str(self.name()) } } } @@ -121,18 +113,18 @@ macro_rules! rules { rules! { // Parsing - ParseError; - ParseWarning; + ParseError => "parse-error"; + ParseWarning => "parse-warning"; // Indexing - DynamicConstantReference; - DynamicSingletonDefinition; - DynamicAncestor; - TopLevelMixinSelf; - InvalidConstantVisibility; - InvalidMethodVisibility; + DynamicConstantReference => "dynamic-constant-reference"; + DynamicSingletonDefinition => "dynamic-singleton-definition"; + DynamicAncestor => "dynamic-ancestor"; + TopLevelMixinSelf => "top-level-mixin-self"; + InvalidConstantVisibility => "invalid-constant-visibility"; + InvalidMethodVisibility => "invalid-method-visibility"; // Resolution - UndefinedMethodVisibilityTarget; - UndefinedConstantVisibilityTarget; + UndefinedMethodVisibilityTarget => "undefined-method-visibility-target"; + UndefinedConstantVisibilityTarget => "undefined-constant-visibility-target"; } diff --git a/rust/rubydex/src/indexing.rs b/rust/rubydex/src/indexing.rs index cfac3e817..d975e5238 100644 --- a/rust/rubydex/src/indexing.rs +++ b/rust/rubydex/src/indexing.rs @@ -1,4 +1,5 @@ use crate::{ + config::Config, errors::Errors, indexing::{local_graph::LocalGraph, rbs_indexer::RBSIndexer, ruby_indexer::RubyIndexer}, job_queue::{Job, JobQueue}, @@ -53,6 +54,7 @@ impl LanguageId { pub struct IndexingJob { path: PathBuf, backend: IndexerBackend, + config: Arc, local_graph_tx: Sender, errors_tx: Sender, } @@ -62,12 +64,14 @@ impl IndexingJob { pub fn new( path: PathBuf, backend: IndexerBackend, + config: Arc, local_graph_tx: Sender, errors_tx: Sender, ) -> Self { Self { path, backend, + config, local_graph_tx, errors_tx, } @@ -101,7 +105,13 @@ impl Job for IndexingJob { }; let language = self.path.extension().map_or(LanguageId::Ruby, LanguageId::from); - let local_graph = build_local_graph(url.to_string(), &source, &language, self.backend); + let local_graph = build_local_graph( + url.to_string(), + &source, + &language, + self.backend, + Arc::clone(&self.config), + ); self.local_graph_tx .send(local_graph) @@ -111,7 +121,13 @@ impl Job for IndexingJob { /// Indexes a single source string in memory, dispatching to the appropriate indexer based on `language_id`. pub fn index_source(graph: &mut Graph, uri: &str, source: &str, language_id: &LanguageId) { - let local_graph = build_local_graph(uri.to_string(), source, language_id, IndexerBackend::RubyIndexer); + let local_graph = build_local_graph( + uri.to_string(), + source, + language_id, + IndexerBackend::RubyIndexer, + graph.config(), + ); graph.consume_document_changes(local_graph); } @@ -124,11 +140,13 @@ pub fn index_files(graph: &mut Graph, paths: Vec, backend: IndexerBacke let queue = Arc::new(JobQueue::new()); let (local_graphs_tx, local_graphs_rx) = unbounded(); let (errors_tx, errors_rx) = unbounded(); + let config = graph.config(); for path in paths { queue.push(Box::new(IndexingJob::new( path, backend, + Arc::clone(&config), local_graphs_tx.clone(), errors_tx.clone(), ))); @@ -153,22 +171,28 @@ pub fn index_files(graph: &mut Graph, paths: Vec, backend: IndexerBacke /// Indexes a source string using the appropriate indexer for the given language. #[must_use] -pub fn build_local_graph(uri: String, source: &str, language: &LanguageId, backend: IndexerBackend) -> LocalGraph { +pub fn build_local_graph( + uri: String, + source: &str, + language: &LanguageId, + backend: IndexerBackend, + config: Arc, +) -> LocalGraph { match language { LanguageId::Ruby => match backend { IndexerBackend::RubyIndexer => { - let mut indexer = RubyIndexer::new(uri, source); + let mut indexer = RubyIndexer::new_with_config(uri, source, config); indexer.index(); indexer.local_graph() } IndexerBackend::OperationBuilder => { - let builder = RubyOperationBuilder::new(uri, source); + let builder = RubyOperationBuilder::new_with_config(uri, source, config); let result = builder.build(); crate::operation::applier::apply_operations(result) } }, LanguageId::Rbs => { - let mut indexer = RBSIndexer::new(uri, source); + let mut indexer = RBSIndexer::new_with_config(uri, source, config); indexer.index(); indexer.local_graph() } @@ -177,11 +201,23 @@ pub fn build_local_graph(uri: String, source: &str, language: &LanguageId, backe #[cfg(test)] mod tests { - use std::path::PathBuf; + use std::fs; + use std::path::{Path, PathBuf}; use super::*; + use crate::config::Config; + use crate::diagnostic::{Rule, Severity}; + use crate::model::ids::UriId; + use crate::resolution::Resolver; use crate::test_utils::Context; - use std::path::Path; + + fn graph_with_config(workspace: &Path, content: &str) -> Graph { + fs::write(workspace.join("rubydex.toml"), content).unwrap(); + let config = Config::load(workspace).unwrap(); + let mut graph = Graph::new(); + graph.load_config(&config); + graph + } #[test] fn index_relative_paths() { @@ -233,4 +269,90 @@ mod tests { assert_eq!(5, graph.definitions().len()); assert_eq!(2, graph.documents().len()); } + + #[test] + fn single_source_indexing_does_not_store_disabled_graph_diagnostics() { + let workspace = tempfile::tempdir().unwrap(); + let mut graph = graph_with_config(workspace.path(), "[linter.rules.parse-warning]\nenabled = false\n"); + let path = workspace.path().join("warning.rb"); + let uri = Url::from_file_path(path).unwrap().to_string(); + + index_source(&mut graph, &uri, "foo = 42", &LanguageId::Ruby); + + assert!( + graph + .all_diagnostics() + .iter() + .all(|diagnostic| diagnostic.rule() != &Rule::ParseWarning) + ); + } + + #[test] + fn parallel_indexing_applies_workspace_excludes_and_severity_to_graph_diagnostics() { + let workspace = tempfile::tempdir().unwrap(); + let workspace_path = crate::path_helpers::resolved(workspace.path()).unwrap(); + let excluded_path = workspace_path.join("components/legacy/warning.rb"); + let included_path = workspace_path.join("components/current/warning.rb"); + fs::create_dir_all(excluded_path.parent().unwrap()).unwrap(); + fs::create_dir_all(included_path.parent().unwrap()).unwrap(); + fs::write(&excluded_path, "foo = 42").unwrap(); + fs::write(&included_path, "foo = 42").unwrap(); + let included_uri = Url::from_file_path(&included_path).unwrap().to_string(); + + for backend in [IndexerBackend::RubyIndexer, IndexerBackend::OperationBuilder] { + let mut graph = graph_with_config( + workspace.path(), + "[linter.rules.parse-warning]\nexclude = [\"components/{legacy,generated}/**\"]\nseverity = \"hint\"\n", + ); + let errors = index_files(&mut graph, vec![excluded_path.clone(), included_path.clone()], backend); + assert!(errors.is_empty(), "unexpected indexing errors: {errors:?}"); + + let diagnostics: Vec<_> = graph + .all_diagnostics() + .into_iter() + .filter(|diagnostic| diagnostic.rule() == &Rule::ParseWarning) + .collect(); + assert_eq!(1, diagnostics.len(), "unexpected diagnostics for {backend:?}"); + assert_eq!(&UriId::from(included_uri.as_str()), diagnostics[0].uri_id()); + assert_eq!(&Severity::Hint, diagnostics[0].severity()); + } + } + + #[test] + fn resolution_diagnostics_use_the_same_graph_configuration() { + let source = "class Foo\n private :nonexistent\nend"; + + for (settings, expected_severity) in [ + ("enabled = false", None), + ("severity = \"information\"", Some(Severity::Information)), + ] { + let workspace = tempfile::tempdir().unwrap(); + let path = workspace.path().join("foo.rb"); + let uri = Url::from_file_path(path).unwrap().to_string(); + let config = format!("[linter.rules.undefined-method-visibility-target]\n{settings}\n"); + fs::write(workspace.path().join("rubydex.toml"), config).unwrap(); + let config = Config::load(workspace.path()).unwrap(); + + for backend in [IndexerBackend::RubyIndexer, IndexerBackend::OperationBuilder] { + let mut graph = Graph::new(); + let local_graph = build_local_graph(uri.clone(), source, &LanguageId::Ruby, backend, graph.config()); + graph.consume_document_changes(local_graph); + graph.load_config(&config); + Resolver::new(&mut graph).resolve(); + + let diagnostics: Vec<_> = graph + .all_diagnostics() + .into_iter() + .filter(|diagnostic| diagnostic.rule() == &Rule::UndefinedMethodVisibilityTarget) + .collect(); + match expected_severity { + Some(severity) => { + assert_eq!(1, diagnostics.len(), "unexpected diagnostics for {backend:?}"); + assert_eq!(&severity, diagnostics[0].severity()); + } + None => assert!(diagnostics.is_empty(), "unexpected diagnostics for {backend:?}"), + } + } + } + } } diff --git a/rust/rubydex/src/indexing/rbs_indexer.rs b/rust/rubydex/src/indexing/rbs_indexer.rs index 76fbb970c..7b0c6802c 100644 --- a/rust/rubydex/src/indexing/rbs_indexer.rs +++ b/rust/rubydex/src/indexing/rbs_indexer.rs @@ -1,12 +1,14 @@ //! Visit the RBS AST and create type definitions. use core::panic; +use std::sync::Arc; use ruby_rbs::node::{ self, AliasKind, ClassNode, CommentNode, ConstantNode, ExtendNode, FunctionTypeNode, GlobalNode, IncludeNode, ModuleNode, Node, NodeList, PrependNode, TypeNameNode, Visit, }; +use crate::config::Config; use crate::diagnostic::{Rule, Severity}; use crate::indexing::local_graph::LocalGraph; use crate::model::comment::Comment; @@ -33,8 +35,13 @@ pub struct RBSIndexer<'a> { impl<'a> RBSIndexer<'a> { #[must_use] pub fn new(uri: String, source: &'a str) -> Self { + Self::new_with_config(uri, source, Arc::new(Config::default())) + } + + #[must_use] + pub(crate) fn new_with_config(uri: String, source: &'a str, config: Arc) -> Self { let uri_id = UriId::from(&uri); - let local_graph = LocalGraph::new(uri_id, Document::new(uri, source)); + let local_graph = LocalGraph::new(uri_id, Document::new_with_config(uri, source, config)); Self { uri_id, diff --git a/rust/rubydex/src/indexing/ruby_indexer.rs b/rust/rubydex/src/indexing/ruby_indexer.rs index 9ff4cce02..6a5371b45 100644 --- a/rust/rubydex/src/indexing/ruby_indexer.rs +++ b/rust/rubydex/src/indexing/ruby_indexer.rs @@ -1,5 +1,8 @@ //! Visit the Ruby AST and create the definitions. +use std::sync::Arc; + +use crate::config::Config; use crate::diagnostic::{Rule, Severity}; use crate::indexing::local_graph::LocalGraph; use crate::model::comment::Comment; @@ -94,8 +97,13 @@ pub struct RubyIndexer<'a> { impl<'a> RubyIndexer<'a> { #[must_use] pub fn new(uri: String, source: &'a str) -> Self { + Self::new_with_config(uri, source, Arc::new(Config::default())) + } + + #[must_use] + pub(crate) fn new_with_config(uri: String, source: &'a str, config: Arc) -> Self { let uri_id = UriId::from(&uri); - let local_graph = LocalGraph::new(uri_id, Document::new(uri, source)); + let local_graph = LocalGraph::new(uri_id, Document::new_with_config(uri, source, config)); Self { uri_id, diff --git a/rust/rubydex/src/model/document.rs b/rust/rubydex/src/model/document.rs index 3917a4e1d..aedcee18f 100644 --- a/rust/rubydex/src/model/document.rs +++ b/rust/rubydex/src/model/document.rs @@ -1,10 +1,12 @@ use std::path::PathBuf; +use std::sync::Arc; use line_index::LineIndex; use url::Url; use xxhash_rust::xxh3::xxh3_64; use crate::assert_mem_size; +use crate::config::Config; use crate::diagnostic::Diagnostic; use crate::model::ids::{ConstantReferenceId, DefinitionId, MethodReferenceId}; @@ -19,12 +21,18 @@ pub struct Document { constant_reference_ids: Vec, diagnostics: Vec, content_hash: u64, + config: Arc, } -assert_mem_size!(Document, 184); +assert_mem_size!(Document, 192); impl Document { #[must_use] pub fn new(uri: String, source: &str) -> Self { + Self::new_with_config(uri, source, Arc::new(Config::default())) + } + + #[must_use] + pub(crate) fn new_with_config(uri: String, source: &str, config: Arc) -> Self { Self { uri, line_index: LineIndex::new(source), @@ -33,9 +41,14 @@ impl Document { constant_reference_ids: Vec::new(), diagnostics: Vec::new(), content_hash: xxh3_64(source.as_bytes()), + config, } } + pub(crate) fn set_config(&mut self, config: Arc) { + self.config = config; + } + #[must_use] pub fn uri(&self) -> &str { &self.uri @@ -88,7 +101,13 @@ impl Document { &self.diagnostics } - pub fn add_diagnostic(&mut self, diagnostic: Diagnostic) { + pub fn add_diagnostic(&mut self, mut diagnostic: Diagnostic) { + if !self + .config + .configure_diagnostic(self.file_path().as_deref(), &mut diagnostic) + { + return; + } self.diagnostics.push(diagnostic); } diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index 105e5065e..5cf603ddf 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -1,6 +1,7 @@ use std::collections::HashSet; use std::collections::hash_map::Entry; use std::path::Path; +use std::sync::Arc; use crate::config::Config; use crate::diagnostic::Diagnostic; @@ -90,9 +91,9 @@ pub struct Graph { pending_work: Vec, /// Project configuration - config: Config, + config: Arc, } -assert_mem_size!(Graph, 368); +assert_mem_size!(Graph, 296); assert_send_sync!(Graph); impl Graph { @@ -109,7 +110,7 @@ impl Graph { position_encoding: Encoding::default(), name_dependents: IdentityHashMap::default(), pending_work: Vec::default(), - config: Config::default(), + config: Arc::new(Config::default()), }; add_built_in_data(&mut graph); @@ -131,7 +132,7 @@ impl Graph { /// Adds glob patterns to exclude from file discovery during indexing. Excluded directories will be skipped entirely /// during directory traversal. pub fn exclude_patterns(&mut self, patterns: Vec>) { - self.config.exclude_patterns(patterns); + Arc::make_mut(&mut self.config).exclude_patterns(patterns); } /// Returns the set of exclusion patterns. @@ -148,7 +149,16 @@ impl Graph { /// Loads a config for the graph pub fn load_config(&mut self, config: &Config) { - self.config = config.clone(); + let config = Arc::new(config.clone()); + for document in self.documents.values_mut() { + document.set_config(Arc::clone(&config)); + } + self.config = config; + } + + #[must_use] + pub(crate) fn config(&self) -> Arc { + Arc::clone(&self.config) } /// # Panics diff --git a/rust/rubydex/src/operation/ruby_builder.rs b/rust/rubydex/src/operation/ruby_builder.rs index e594ccb38..66ceaa9fa 100644 --- a/rust/rubydex/src/operation/ruby_builder.rs +++ b/rust/rubydex/src/operation/ruby_builder.rs @@ -4,7 +4,9 @@ //! by the applier to create definitions and declarations in a `LocalGraph`. use std::collections::hash_map::Entry; +use std::sync::Arc; +use crate::config::Config; use crate::diagnostic::{Diagnostic, Rule, Severity}; use crate::model::comment::Comment; use crate::model::definitions::{DefinitionFlags, Parameter, ParameterStruct, Signatures}; @@ -88,6 +90,11 @@ pub struct RubyOperationBuilder<'a> { impl<'a> RubyOperationBuilder<'a> { #[must_use] pub fn new(uri: String, source: &'a str) -> Self { + Self::new_with_config(uri, source, Arc::new(Config::default())) + } + + #[must_use] + pub(crate) fn new_with_config(uri: String, source: &'a str, config: Arc) -> Self { let uri_id = UriId::from(&uri); Self { @@ -95,7 +102,7 @@ impl<'a> RubyOperationBuilder<'a> { source, strings: IdentityHashMap::default(), names: IdentityHashMap::default(), - document: Document::new(uri, source), + document: Document::new_with_config(uri, source, config), comments: Vec::new(), nesting_stack: Vec::new(), visibility_stack: vec![VisibilityModifier::new(Visibility::Private, false, Offset::new(0, 0))], diff --git a/rust/rubydex/src/test_utils/graph_test.rs b/rust/rubydex/src/test_utils/graph_test.rs index 4ddb8cbdc..6b2dd6bc6 100644 --- a/rust/rubydex/src/test_utils/graph_test.rs +++ b/rust/rubydex/src/test_utils/graph_test.rs @@ -44,7 +44,13 @@ impl GraphTest { /// Indexes a Ruby source pub fn index_uri(&mut self, uri: &str, source: &str) { let source = normalize_indentation(source); - let local_graph = indexing::build_local_graph(uri.to_string(), &source, &LanguageId::Ruby, self.backend); + let local_graph = indexing::build_local_graph( + uri.to_string(), + &source, + &LanguageId::Ruby, + self.backend, + self.graph.config(), + ); self.graph.consume_document_changes(local_graph); } diff --git a/rust/rubydex/src/test_utils/local_graph_test.rs b/rust/rubydex/src/test_utils/local_graph_test.rs index 43c8dda4b..c32b2d748 100644 --- a/rust/rubydex/src/test_utils/local_graph_test.rs +++ b/rust/rubydex/src/test_utils/local_graph_test.rs @@ -1,4 +1,7 @@ +use std::sync::Arc; + use super::normalize_indentation; +use crate::config::Config; use crate::indexing::local_graph::LocalGraph; use crate::indexing::rbs_indexer::RBSIndexer; use crate::indexing::{IndexerBackend, LanguageId, build_local_graph}; @@ -26,7 +29,13 @@ impl LocalGraphTest { pub fn new_with_backend(uri: &str, source: &str, backend: IndexerBackend) -> Self { let uri = uri.to_string(); let source = normalize_indentation(source); - let graph = build_local_graph(uri.clone(), &source, &LanguageId::Ruby, backend); + let graph = build_local_graph( + uri.clone(), + &source, + &LanguageId::Ruby, + backend, + Arc::new(Config::default()), + ); Self { uri, source, graph } } diff --git a/test/cli_test.rb b/test/cli_test.rb index 4d92b5a0b..6a8743312 100644 --- a/test/cli_test.rb +++ b/test/cli_test.rb @@ -296,9 +296,8 @@ def test_lint_allows_a_clean_workspace end end - def test_lint_accepts_and_disables_a_graph_diagnostic_rule + def test_lint_accepts_and_disables_a_built_in_rule_without_custom_rules with_context do |context| - write_linter_rule(context, "CLITestGraphDiagnosticConfigRule") context.write!("app.rb", "unused = true") context.write!("rubydex.toml", <<~TOML) [linter.rules.parse-warning] @@ -347,16 +346,15 @@ 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_custom_rules with_context do |context| - context.write!("app.rb", "class Foo; end\n") + context.write!("app.rb", "class Broken") 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_stdout_includes(result, "error: parse-error:") + assert_stderr_includes(result, "Indexing workspace...") end end diff --git a/test/diagnostic_test.rb b/test/diagnostic_test.rb index d0440b738..8caf9bd10 100644 --- a/test/diagnostic_test.rb +++ b/test/diagnostic_test.rb @@ -3,24 +3,6 @@ require "test_helper" class DiagnosticTest < Minitest::Test - def test_graph_rule_names - assert_equal( - [ - "parse-error", - "parse-warning", - "dynamic-constant-reference", - "dynamic-singleton-definition", - "dynamic-ancestor", - "top-level-mixin-self", - "invalid-constant-visibility", - "invalid-method-visibility", - "undefined-method-visibility-target", - "undefined-constant-visibility-target", - ], - Rubydex::Diagnostic.graph_rule_names, - ) - end - def test_severity_from_value { error: Rubydex::Severity::Error, diff --git a/test/linter_test.rb b/test/linter_test.rb index d7bb81f44..961ffee29 100644 --- a/test/linter_test.rb +++ b/test/linter_test.rb @@ -117,6 +117,24 @@ def lint end end + def test_rule_lists_built_in_rule_names + assert_equal( + [ + "parse-error", + "parse-warning", + "dynamic-constant-reference", + "dynamic-singleton-definition", + "dynamic-ancestor", + "top-level-mixin-self", + "invalid-constant-visibility", + "invalid-method-visibility", + "undefined-method-visibility-target", + "undefined-constant-visibility-target", + ], + Rubydex::Linter::Rule.built_in_rules_names, + ) + 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) @@ -188,44 +206,67 @@ def test_runner_includes_native_graph_diagnostics refute_predicate(result, :success?) end - def test_runner_drops_disabled_graph_diagnostics_by_rule_name + def test_graph_does_not_add_disabled_diagnostics with_context do |context| context.write!("workspace/warning.rb", "unused = true") context.write!("workspace/error.rb", "class Broken") - graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + context.write!("workspace/rubydex.toml", <<~TOML) + [linter.rules.parse-warning] + enabled = false + TOML + config = Rubydex::Config.load(context.absolute_path_to("workspace")) + graph = Rubydex::Graph.new + graph.load_config(config) graph.index_all(context.glob("workspace/**/*.rb")) - config = configured_linter_config("parse-warning", enabled: false) - assert_equal(["parse-error", "parse-error", "parse-warning"], graph.diagnostics.map(&:rule).sort) + assert_equal(["parse-error", "parse-error"], graph.diagnostics.map(&:rule)) - result = Rubydex::Linter::Runner.new(graph, rules: [], config:).run + result = Rubydex::Linter::Runner.new(graph, rules: [], config: config.linter).run assert_equal(["parse-error", "parse-error"], result.diagnostics.map(&:rule)) end end - def test_runner_does_not_report_graph_diagnostics_from_excluded_paths + def test_graph_does_not_add_diagnostics_from_excluded_paths with_context do |context| context.write!("workspace/components/legacy/example.rb", "unused = true") context.write!("workspace/components/current/example.rb", "unused = true") - graph = Rubydex::Graph.configure_for_workspace(context.absolute_path_to("workspace")) + context.write!("workspace/rubydex.toml", <<~TOML) + [linter.rules.parse-warning] + exclude = ["components/legacy/**"] + TOML + config = Rubydex::Config.load(context.absolute_path_to("workspace")) + graph = Rubydex::Graph.new + graph.load_config(config) graph.index_all(context.glob("workspace/**/*.rb")) - config = configured_linter_config("parse-warning", exclude_patterns: ["components/legacy/**"]) - expected_uris = [ - context.uri_to("workspace/components/current/example.rb"), - context.uri_to("workspace/components/legacy/example.rb"), - ] - # Graph diagnostics are not disabled via linter configs and should still be created. - assert_equal(expected_uris.sort, graph.diagnostics.map { |diagnostic| diagnostic.location.uri }.sort) + expected_uri = context.uri_to("workspace/components/current/example.rb") + assert_equal([expected_uri], graph.diagnostics.map { |diagnostic| diagnostic.location.uri }) - result = Rubydex::Linter::Runner.new(graph, rules: [], config:).run + result = Rubydex::Linter::Runner.new(graph, rules: [], config: config.linter).run - # But the excluded graph diagnostics will not appear in the linter results. - assert_equal( - [expected_uris.first], - result.diagnostics.map { |diagnostic| diagnostic.location.uri }, - ) + assert_equal([expected_uri], result.diagnostics.map { |diagnostic| diagnostic.location.uri }) + end + end + + def test_graph_applies_configured_diagnostic_severity + with_context do |context| + context.write!("workspace/example.rb", "unused = true") + context.write!("workspace/rubydex.toml", <<~TOML) + [linter.rules.parse-warning] + severity = "error" + TOML + config = Rubydex::Config.load(context.absolute_path_to("workspace")) + graph = Rubydex::Graph.new + graph.load_config(config) + graph.index_all(context.glob("workspace/**/*.rb")) + + diagnostic = graph.diagnostics.fetch(0) + assert_equal("parse-warning", diagnostic.rule) + assert_equal(Rubydex::Severity::Error, diagnostic.severity) + + result = Rubydex::Linter::Runner.new(graph, rules: [], config: config.linter).run + refute_predicate(result, :success?) end end @@ -277,15 +318,16 @@ def test_runner_filters_graph_diagnostics_under_dependency_paths end end - def test_runner_filters_a_diagnostic_when_its_primary_location_matches_a_rule_exclude + def test_rule_does_not_add_a_diagnostic_from_an_excluded_path 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/**"]) + rule = ExcludedPrimaryRule.new(graph, config:) - result = Rubydex::Linter::Runner.new(graph, rules: [ExcludedPrimaryRule], config:).run + rule.lint - assert_empty(result.diagnostics) + assert_empty(rule.diagnostics) end end