Feat improving ror graph july 02 26

internal/engine/cache.go

@@ -40,7 +40,17 @@ import (
 // v24: Ruby walks method default-parameter values for calls (`def f(x = self.class.foo)`) and records single-level predicate/bang calls (`viewer.rich?`, `x.save!`) which — unlike plain attribute reads — are unambiguously method invocations.
 // v25: Ruby folds `delegate :a, :b, ..., to: X` method names as calls, and records calls on a bare-method (non-local identifier) receiver when the method name is scope-like (`some_relation.pluck_job_id`).
 // v26: Ruby records scope-like (underscored) method calls on ANY identifier receiver, including local relation variables (`items = ...; items.preload_relations`).
-const cacheVersion = "v26"
+// v27: Ruby resolves underscored calls on @ivar/@@cvar/$gvar receivers (`@klass.bo_search_fields`) and indexes view templates (ERB/Slim/HAML) for embedded Ruby calls (helpers/class methods), emitting KindFileRef references.
+// v28: Ruby records method calls on a `klass`/`clazz`/`klazz` (or @klass) receiver as class-method dispatch (`klass.inline`), regardless of the method name.
+// v29: Ruby extends the interpolated-prefix dispatch heuristic to strings (`"present_#{idx}"`), not just symbols, so send()-by-computed-string-name marks same-prefix methods used.
+// v30: interpolated-string prefixes are now gated on dispatcher-proximity (committed only when the enclosing scope also calls send/public_send/…), so cache/Redis-key strings (`"fetch_#{id}"`) no longer hide genuine orphans; interpolated symbols remain unconditional.
+// v31: Ruby block parameters (`each do |user| … end`) are now treated as locals, so a bare block var whose name matches an association no longer records a spurious in-loop call (N+1 false positive); and find_in_batches/in_batches are no longer counted as element loops (their block yields a batch, so the inner .each/.map is the real per-element loop) — fixing O(n²) mislabels of single-pass batch scans.
+// v32: Ruby no longer flags `super` (climbs the inheritance chain, terminates) or a same-named call on an explicit non-self receiver (SimpleDelegator/decorator, `@delegate.render`/`new.call`) as self-recursion — both set recursive_self spuriously, the dominant recursion false positive.
+// v33: Ruby recursion is now gated to same-object self dispatch — `self.class.foo` (instance method calling its sibling class method) and `obj.try(:foo)` (dispatch to a different object) no longer set recursive_self; receiverless calls, `self.foo`, and `Const.foo` matching the method's own full name still do.
+// v34: Ruby constant-bounded iterators (`6.times`, `[…].each`, `%w[…]`, ALL-CAPS `CONST.each`) no longer add scaling loop_depth — they run a fixed number of times, so they no longer inflate a genuine O(n) into a false O(n²)/O(n³).
+// v35: constant-bounded-loop detection now unwraps trailing size-preserving chain methods (`[a,b].compact.all?`, `%w[…].map.each`), so a bounded literal/constant behind `.compact`/`.uniq`/`.map`/… is still recognized as bounded.
+// v36: Ruby module symbols now carry an `abstract` bool prop — true for mixins (modules that define instance methods) and ActiveSupport::Concerns, false for namespace/utility modules — so package-metrics abstractness (A) no longer counts Rails namespaces as abstractions. Bare-constant coupling resolution is also namespace-aware now.
+const cacheVersion = "v36"
 
 // extractorCache holds per-extractor facts keyed by a content hash of the files
 // the extractor depends on. It is loaded from disk at the start of a snapshot and

internal/extractors/rubyextractor/complexity_test.go

@@ -96,6 +96,80 @@ end
 	}
 }
 
+func TestRbComplexity_ConstantBoundLoopNoDepth(t *testing.T) {
+	// A constant-bounded iterator runs a fixed number of times, so it is a loop
+	// (cyclomatic) but adds NO scaling depth.
+	for _, tc := range []struct{ name, body string }{
+		{"integer.times", "6.times { |i| work(i) }"},
+		{"literal array", "[:a, :b, :c].each { |x| work(x) }"},
+		{"word array", "%w[a b c].each { |x| work(x) }"},
+		{"screaming constant", "STOP_CHARS.each { |c| work(c) }"},
+	} {
+		src := "class Worker\n  def run\n    " + tc.body + "\n  end\nend\n"
+		f := symbolsByName(extractFileAST([]byte(src), "app/worker.rb", false, false))["Worker#run"]
+		if _, present := f.Props["loop_depth"]; present {
+			t.Errorf("%s: loop_depth = %v, want unset (constant-bounded loop)", tc.name, f.Props["loop_depth"])
+		}
+		if got := rbIntProp(t, f, "loop_count"); got != 1 {
+			t.Errorf("%s: loop_count = %d, want 1 (still a loop for cyclomatic)", tc.name, got)
+		}
+	}
+}
+
+func TestRbComplexity_LiteralChainLoopIsBounded(t *testing.T) {
+	// A bounded literal behind a trailing chain method stays bounded: the inner
+	// `[a, b].compact.all?` is ≤2 elements, so nesting it in a scaling outer loop is
+	// O(n), not O(n²).
+	src := `class Gate
+  def valid?(items)
+    items.each do |it|
+      [it.a, it.b].compact.all? do |url|
+        allowed?(url)
+      end
+    end
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(src), "app/gate.rb", false, false))["Gate#valid?"]
+	if got := rbIntProp(t, f, "loop_depth"); got != 1 {
+		t.Errorf("loop_depth = %d, want 1 (literal-chain inner loop is bounded)", got)
+	}
+
+	// A chained bounded literal as an outer loop adds no depth at all.
+	src2 := `class Gate
+  def run
+    [1, 2].map { |x| x }.each { |y| work(y) }
+  end
+end
+`
+	g := symbolsByName(extractFileAST([]byte(src2), "app/gate.rb", false, false))["Gate#run"]
+	if _, present := g.Props["loop_depth"]; present {
+		t.Errorf("loop_depth = %v, want unset ([1,2].map.each is bounded)", g.Props["loop_depth"])
+	}
+}
+
+func TestRbComplexity_ConstantInnerLoopDoesNotMultiply(t *testing.T) {
+	// A scaling outer loop with a CONSTANT inner loop is O(n), not O(n²): only the
+	// outer contributes depth, but per-iteration I/O is still measured against n.
+	src := `class Worker
+  def run(users)
+    users.each do |u|
+      STOP_CHARS.each do |c|
+        u.save
+      end
+    end
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(src), "app/worker.rb", false, false))["Worker#run"]
+	if got := rbIntProp(t, f, "loop_depth"); got != 1 {
+		t.Errorf("loop_depth = %d, want 1 (constant inner loop must not multiply)", got)
+	}
+	if cil := rbStrSlice(f, "calls_in_loop"); !rbContains(cil, "save") {
+		t.Errorf("calls_in_loop = %v, want to contain save (per-iteration I/O still flagged)", cil)
+	}
+}
+
 func TestRbComplexity_IteratorReceiverEvaluatedOnce(t *testing.T) {
 	// User.where(...) is the iterator's receiver — evaluated once, NOT per element.
 	// Mailer.deliver(u) runs inside the block, so it is the in-loop call.
@@ -163,6 +237,94 @@ end
 	}
 }
 
+func TestRbComplexity_SuperIsNotRecursion(t *testing.T) {
+	// `super` climbs the inheritance chain and terminates — it is not self-recursion.
+	// It was the dominant recursion false positive (every override calling super).
+	src := `class ShadowUser < Base
+  def hood_id
+    deprecate(__method__)
+    super
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(src), "app/shadow_user.rb", false, false))["ShadowUser#hood_id"]
+	if v, _ := f.Props["recursive_self"].(bool); v {
+		t.Errorf("recursive_self = true for a method whose only self-name call is `super`; want unset")
+	}
+	// The `super` call edge must still be recorded (dead-code marks the ancestor used).
+	if !hasCall(f, "hood_id") {
+		t.Errorf("super should still record a call edge to the same-named ancestor; relations = %v", f.Relations)
+	}
+}
+
+func TestRbComplexity_DelegationIsNotRecursion(t *testing.T) {
+	// A same-named call on an explicit, non-self receiver (SimpleDelegator/decorator,
+	// `@delegate.render`) is a call on a DIFFERENT object, not self-recursion. A
+	// receiverless self-call still is.
+	delegating := `class Presenter
+  def render(x)
+    @delegate_object.render(x)
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(delegating), "app/presenter.rb", false, false))["Presenter#render"]
+	if v, _ := f.Props["recursive_self"].(bool); v {
+		t.Errorf("recursive_self = true for a delegated same-named call; want unset")
+	}
+
+	genuine := `class Walker
+  def render(node)
+    render(node.child)
+  end
+end
+`
+	g := symbolsByName(extractFileAST([]byte(genuine), "app/walker.rb", false, false))["Walker#render"]
+	if v, ok := g.Props["recursive_self"].(bool); !ok || !v {
+		t.Errorf("recursive_self = %v (ok=%v) for a receiverless self-call; want true", g.Props["recursive_self"], ok)
+	}
+}
+
+func TestRbComplexity_SelfClassSiblingIsNotRecursion(t *testing.T) {
+	// An instance method delegating to the same-named CLASS method
+	// (`self.class.photo_url`) calls a DIFFERENT method — not recursion. But plain
+	// `self.foo` (same object, same method) still is.
+	sibling := `class Presenter
+  def photo_url(user)
+    self.class.photo_url(user)
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(sibling), "app/presenter.rb", false, false))["Presenter#photo_url"]
+	if v, _ := f.Props["recursive_self"].(bool); v {
+		t.Errorf("recursive_self = true for a self.class sibling delegation; want unset")
+	}
+
+	selfDispatch := `class Worker
+  def run
+    self.run
+  end
+end
+`
+	g := symbolsByName(extractFileAST([]byte(selfDispatch), "app/worker.rb", false, false))["Worker#run"]
+	if v, ok := g.Props["recursive_self"].(bool); !ok || !v {
+		t.Errorf("recursive_self = %v (ok=%v) for `self.run`; want true", g.Props["recursive_self"], ok)
+	}
+}
+
+func TestRbComplexity_TryDispatchOnOtherReceiverIsNotRecursion(t *testing.T) {
+	// `object.try(:ben?)` dispatches `ben?` to a DIFFERENT object — not recursion.
+	src := `class AuthorType
+  def ben?
+    !!object.try(:ben?)
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(src), "app/author_type.rb", false, false))["AuthorType#ben?"]
+	if v, _ := f.Props["recursive_self"].(bool); v {
+		t.Errorf("recursive_self = true for `object.try(:ben?)` on another receiver; want unset")
+	}
+}
+
 func TestRbComplexity_InLoopAssociationRead(t *testing.T) {
 	// The classic N+1: a no-arg association read inside an iterator block. It must
 	// land in calls_in_loop (for the perf metric) but NOT become a graph edge, and
@@ -252,3 +414,52 @@ end
 		t.Errorf("persist should still be a call edge; relations=%v", f.Relations)
 	}
 }
+
+func TestRbComplexity_BlockParamNotCall(t *testing.T) {
+	// A block parameter is a local, not a method call — even when referenced bare
+	// (as an argument value) and even when its name matches an association. This is
+	// the dominant N+1 false positive: `each do |user| … user … end`. A genuine
+	// association read on the block var (user.posts) must still be captured, proving
+	// the fix is surgical.
+	src := `class Worker
+  def run(users)
+    users.each do |user|
+      notify(user)
+      user.posts
+    end
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(src), "app/worker.rb", false, false))["Worker#run"]
+	cil := rbStrSlice(f, "calls_in_loop")
+	if rbContains(cil, "user") {
+		t.Errorf("calls_in_loop = %v, must NOT contain user (block-local variable)", cil)
+	}
+	if !rbContains(cil, "notify") {
+		t.Errorf("calls_in_loop = %v, want to contain notify", cil)
+	}
+	if !rbContains(cil, "posts") {
+		t.Errorf("calls_in_loop = %v, want to contain posts (real association read on block var)", cil)
+	}
+}
+
+func TestRbComplexity_FindInBatchesNotElementLoop(t *testing.T) {
+	// find_in_batches yields a batch (array); the inner .map over that batch is the
+	// real per-element loop. The pair must score loop_depth 1 (a single O(n) pass),
+	// not 2 — otherwise a batched reindex is mislabeled O(n²).
+	src := `class Reindex
+  def run(model)
+    model.find_in_batches do |batch|
+      batch.map { |obj| present(obj) }
+    end
+  end
+end
+`
+	f := symbolsByName(extractFileAST([]byte(src), "app/reindex.rb", false, false))["Reindex#run"]
+	if got := rbIntProp(t, f, "loop_depth"); got != 1 {
+		t.Errorf("loop_depth = %d, want 1 (find_in_batches yields batches, not elements)", got)
+	}
+	if cil := rbStrSlice(f, "calls_in_loop"); !rbContains(cil, "present") {
+		t.Errorf("calls_in_loop = %v, want to contain present", cil)
+	}
+}

internal/extractors/rubyextractor/resolve.go

@@ -42,10 +42,10 @@ func resolveImports(allFacts []facts.Fact, isRails bool) []facts.Fact {
 			for _, rel := range f.Relations {
 				switch rel.Kind {
 				case facts.RelImplements: // inheritance
-					add(src, ix.resolve(rel.Target))
+					add(src, ix.resolve(rel.Target, src))
 				case facts.RelCalls:
 					if c := constFromCall(rel.Target); c != "" {
-						add(src, ix.resolve(c))
+						add(src, ix.resolve(c, src))
 					}
 				}
 			}
@@ -55,14 +55,14 @@ func resolveImports(allFacts []facts.Fact, isRails bool) []facts.Fact {
 				rel := &f.Relations[j]
 				switch rel.Kind {
 				case facts.RelImplements: // include/extend/prepend mixins
-					if dst := ix.resolve(rel.Target); dst != "" {
+					if dst := ix.resolve(rel.Target, src); dst != "" {
 						add(src, dst)
 						setSource(f, "internal")
 					} else {
 						setSource(f, "external")
 					}
 				case facts.RelDependsOn: // ActiveRecord associations
-					if dst := ix.resolve(rel.Target); dst != "" {
+					if dst := ix.resolve(rel.Target, src); dst != "" {
 						add(src, dst)
 						setSource(f, "internal")
 					} else {
@@ -127,19 +127,55 @@ func buildConstIndex(allFacts []facts.Fact) *constIndex {
 	return ix
 }
 
-// resolve returns the declaring module dir of a constant reference, or "".
-func (ix *constIndex) resolve(ref string) string {
+// resolve returns the declaring module dir of a constant reference, or "". src is
+// the referring symbol's dir, used to disambiguate bare-name collisions: Ruby
+// constant lookup is lexical, so a declarer in the referrer's own namespace wins.
+func (ix *constIndex) resolve(ref, src string) string {
 	ref = stripLeadingColons(ref)
 	if ref == "" {
 		return ""
 	}
 	if dir, ok := ix.qualified[ref]; ok {
 		return dir
 	}
-	if dirs := ix.bare[lastSegment(ref)]; len(dirs) > 0 {
-		return dirs[0] // pre-sorted: shortest dir, then lexicographic
+	dirs := ix.bare[lastSegment(ref)]
+	switch len(dirs) {
+	case 0:
+		return ""
+	case 1:
+		return dirs[0]
 	}
-	return ""
+	// Ambiguous bare name (several declarers of the same simple name). Prefer the
+	// candidate sharing the longest leading path with the referrer's dir. If the
+	// best match is not strictly better than the runner-up, or no candidate shares
+	// any namespace with the referrer, the reference is genuinely ambiguous — drop
+	// it rather than fabricate an edge to an arbitrary (shortest-dir) declarer.
+	best, bestScore := "", 0
+	tie := false
+	for _, d := range dirs {
+		s := commonPrefixSegments(src, d)
+		switch {
+		case s > bestScore:
+			best, bestScore, tie = d, s, false
+		case s == bestScore:
+			tie = true
+		}
+	}
+	if bestScore == 0 || tie {
+		return ""
+	}
+	return best
+}
+
+// commonPrefixSegments counts the equal leading '/'-separated path segments of a
+// and b (e.g. "app/models/x" vs "app/models/y" → 2, "app/x" vs "lib/y" → 0).
+func commonPrefixSegments(a, b string) int {
+	as, bs := strings.Split(a, "/"), strings.Split(b, "/")
+	n := 0
+	for n < len(as) && n < len(bs) && as[n] == bs[n] {
+		n++
+	}
+	return n
 }
 
 // classifyRequire resolves a require/require_relative dependency fact: relative