Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion ext/rubydex/diagnostic.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"));

Expand All @@ -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);
}
2 changes: 1 addition & 1 deletion lib/rubydex/cli/command/lint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we ever reach this point from the CLI if known_rule_classes is empty?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add a test where the CLI without a rule should at least show the built-in diagnostics?

unknown_rule_names = config.rules.keys.reject { |name| known_rule_names.include?(name) }.sort
return if unknown_rule_names.empty?

Expand Down
35 changes: 24 additions & 11 deletions lib/rubydex/linter/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
[
Expand All @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions rbi/rubydex.rbi
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
35 changes: 34 additions & 1 deletion rust/rubydex-sys/src/diagnostic_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like it belongs to Rule and not Diagnostic. Conceptually, a Rule is a type of mistake someone can make and a Diagnostic is an occurrence of that mistake.

If we want the names of the rules, so that they can be filtered, then shouldn't this method be in Rule? Like, Rule.all or Rule.built_in?

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
Expand Down
9 changes: 9 additions & 0 deletions rust/rubydex/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
33 changes: 33 additions & 0 deletions test/cli_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
18 changes: 18 additions & 0 deletions test/diagnostic_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
57 changes: 57 additions & 0 deletions test/linter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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")
Expand Down
Loading