Implement initial complexity analysis pass and CLI command - #961
Implement initial complexity analysis pass and CLI command#961rafaelfranca wants to merge 1 commit into
Conversation
e0a4058 to
11bedd2
Compare
The complexity pass is a flog-style ABC report that scores assignments, branches, and calls in Ruby source files. The CLI command `rdx complexity` computes the complexity report for the given paths and can output in text or JSON format. It also supports diffing against a baseline JSON report to show changes in complexity.
11bedd2 to
8de6c75
Compare
st0012
left a comment
There was a problem hiding this comment.
There is a lot of complexity in complexity.rs especially on receiver/namespace handling, that we already went through in https://github.com/Shopify/rubydex/blob/main/rust/rubydex/src/indexing/ruby_indexer.rs. Is it possible to build part of the complexity detection using Definition instead?
It may look like:
- For class & modules....etc. that we create definition for, we already handled a lot of the context building, and we can use definition attributes to calculate the penalty
- For things that we don't index, we keep the existing visiting logic. Yes we double index, but it's also fast
And then we merge the results from 1 and 2. As time goes on and Rubydex index more and more stuff, we'll gradually expand 1 and shrink 2, until hopefully 1 day the indexing phase's result & CFG would already be enough for complexity analysis?
| #[must_use] | ||
| pub fn workspace_path_for(paths: &[String]) -> Option<PathBuf> { | ||
| let first_path = paths.first()?; | ||
| fs::canonicalize(first_path).ok().filter(|path| path.is_dir()) |
There was a problem hiding this comment.
I'm not sure if we can do paths.first and treat it as workspace path, especially if we want to support complexity check on specific paths only.
Let's say I use rdx complexity lib/, this will use lib/ as the workspace, but then miss the ./rubydex.toml file.
| } else { | ||
| '#' | ||
| }; | ||
| let key = format!("{namespace}{separator}{method_name}"); |
There was a problem hiding this comment.
We're losing the receiver information here. Consider this:
class Foo
def Bar.bar; end
endthe key becomes Foo.bar.
| let baseline_map: HashMap<(String, String), &MethodEntry> = baseline | ||
| .methods | ||
| .iter() | ||
| .map(|entry| ((entry.name.clone(), entry.file.clone()), entry)) |
There was a problem hiding this comment.
I think entry.file is an absolute path? Which means we can't compare reports from different machines as the absolute path may differ. Should we use the relative path to the workspace instead?
| def analyze(paths, format: :text, top: 25, methods_only: false, details: false, group: false) | ||
| raise TypeError, "no implicit conversion of #{paths.class} into Array" unless paths.is_a?(Array) | ||
|
|
||
| native_analyze(paths.map(&:to_s), format, top, methods_only, details, group) |
There was a problem hiding this comment.
Why the to_s, isn't paths an Array[String] from the signature?
| puts parser | ||
| exit | ||
| end | ||
| end.parse! |
There was a problem hiding this comment.
Should we rescue OptionParser::ParseError?
| if diff_file | ||
| baseline = begin | ||
| File.read(diff_file) | ||
| rescue Errno::ENOENT |
There was a problem hiding this comment.
You could broaden to SystemCallError and say can't read baseline report so you also cover EACCESS, EISDIR and friends.
| rdxi_check_array_of_strings(paths); | ||
|
|
||
| long length = RARRAY_LEN(paths); | ||
| char **paths_array = rdxi_str_array_to_char(paths, (size_t)length); |
There was a problem hiding this comment.
This is going to leak if rdxi_symbol_or_string_cstr or NUM2SIZET raise
| return CComplexityResult::error(&format!( | ||
| "unknown complexity format `{other}` (expected `text` or `json`)" | ||
| )); |
There was a problem hiding this comment.
Let's validate the output format before doing the expensive computation in analyze
| } | ||
|
|
||
| fn in_singleton(&self) -> bool { | ||
| self.scope_stack.iter().any(|frame| frame.is_singleton) |
There was a problem hiding this comment.
Should we limit this to the next scope?
Consider this:
class Foo
class << self
class Bar
def run
work
end
end
end
end| total / f64::from(u32::try_from(methods_count).unwrap_or(u32::MAX)) | ||
| }; | ||
| Report { | ||
| schema_version: 1, |
| let (key, start_line, end_line) = if let Some((k, s, e)) = self.method_stack.last() { | ||
| (k.clone(), *s, *e) | ||
| } else { | ||
| let k = format!("{}#none", self.namespace_string()); |
There was a problem hiding this comment.
Won't this collide with an actual #none method if --methods-only=false? 🤔
f46711c to
8de6c75
Compare
|
@st0012 I tried that approach right now and it instead of deleting code required me to add way more code, not to speak on the duplicated indexing for each file. I'm not sure it is worth it |
Summary
Adds an ABC complexity analysis pass to rubydex: a standalone Prism-based scorer that
computes assignment, branch, and call complexity for Ruby source, exposed through both
the Rust CLI (
rubydex_cli --complexity) and the Ruby gem (rdx complexity/Rubydex::Complexity). It runs as its own analysis pass — no graph or resolutionrequired — so it is fast enough to run repeatedly over a large codebase.
The scoring rules are modeled on flog; thanks to
Ryan Davis for the original.
A score is reported per method as
sqrt(a² + b² + c²), wherea,b, andcaccumulate assignment, branch, and call weight respectively (each scaled by the current
nesting multiplier).
.rbsfiles are excluded; everything else the indexer treats asRuby (
.rb,.rake,.ru, …) is scored.CLI
Text output
JSON output
The JSON report (
schema_version: 1) carriestotal,average,methods_count, anda
methodsarray of entries with per-bucket breakdowns andstart_line/end_linelocations (
--topis ignored for JSON, so all methods are emitted):{ "schema_version": 1, "total": 17.2, "average": 5.7, "methods_count": 3, "methods": [ { "name": "Rubydex::Complexity.analyze", "file": "app/models/rubydex/complexity.rb", "start_line": 8, "end_line": 12, "assignments": 1.0, "branches": 1.5, "calls": 10.1, "score": 10.3 } ] }Diffing against a baseline
Diff a fresh report against a baseline JSON report to track drift over time. Both
reports must be generated with the same
--methods-onlysetting.bundle exec rdx complexity app/models --diff baseline.jsonPer-construct breakdown, methods-only, and grouping
--detailsadds a per-method breakdown:--groupis text-only (rejected with--format json); neither--detailsnor--groupapplies to--diff(both are rejected with it).Ruby API
Configured exclusions
Exclusions are configured in
rubydex.tomland are decoupled from indexing: thetop-level
excludekey affects indexing only, while a separate[complexity]tablecontrols what
rdx complexityskips. This lets you keep a file in the graph but out ofthe complexity report. Both share the default skipped directories (
.git,node_modules,tmp, …).Malformed globs in either set are reported as a config error at load time rather than
silently dropped.
Implementation
complexitymodule in the Rust crate: a PrismVisitimplementation that scoreseach method into
a/b/cbuckets with compounding nesting penalties, plusReport/DiffReportwithrender_text/to_json/from_json/diff.rdx_complexity_analyze/rdx_complexity_diff) and a C extension(
Rubydex::Complexity.native_analyze/native_diff).analyzemirrors the indexer's job queue; per-file non-fatal errors aresurfaced as warnings.
behavior is pinned by inline regression tests.
Test plan
cargo clippy --all-targets --all-features -- -D warningsclean.bundle exec rake lintclean.