Improving RoR graph

Author: dejo1307

internal/engine/cache.go

@@ -19,7 +19,9 @@ import (
 // v3: Python route facts use method/role/bare-path Name (was http_method, verb-in-name).
 // v4: Java HTTP client detection (RestTemplate call sites + @FeignClient interfaces).
 // v5: PHP HTTP client detection + Laravel/Symfony route DSLs (attributes, YAML/XML config).
-const cacheVersion = "v5"
+// v6: Ruby bare-constant references emitted as RelCalls edges (dead-code precision).
+// v7: Ruby skips builtin-constant edges (god-class noise) + serializer attribute/include_ folding.
+const cacheVersion = "v7"
 
 // 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/ruby_ast.go

@@ -36,12 +36,13 @@ func extractFileAST(src []byte, relFile string, isRails, exportedByPackwerk bool
 
 // rubyScope tracks a class/module/eigenclass nesting level.
 type rubyScope struct {
-	name       string // simple (last) name; "" for an eigenclass (class << self)
-	kind       string // "class", "module", or "eigenclass"
-	visibility string // "public" | "private" | "protected"
-	moduleFunc bool   // module_function active: subsequent defs are class methods
-	isModel    bool   // ActiveRecord model: associations/scopes/table_name apply
-	symFactIdx int    // index into w.out of this scope's class/module symbol fact, or -1
+	name         string // simple (last) name; "" for an eigenclass (class << self)
+	kind         string // "class", "module", or "eigenclass"
+	visibility   string // "public" | "private" | "protected"
+	moduleFunc   bool   // module_function active: subsequent defs are class methods
+	isModel      bool   // ActiveRecord model: associations/scopes/table_name apply
+	isSerializer bool   // ActiveModel::Serializer: attributes/associations back methods
+	symFactIdx   int    // index into w.out of this scope's class/module symbol fact, or -1
 }
 
 type rubyWalker struct {
@@ -344,7 +345,8 @@ func (w *rubyWalker) handleClass(node *sitter.Node) {
 		})
 	}
 
-	w.push(rubyScope{name: name, kind: "class", visibility: "public", isModel: isModel, symFactIdx: clsIdx})
+	w.push(rubyScope{name: name, kind: "class", visibility: "public", isModel: isModel,
+		isSerializer: isSerializerBase(superclass), symFactIdx: clsIdx})
 	w.walkBody(node.ChildByFieldName("body"))
 	w.pop()
 }
@@ -430,14 +432,17 @@ func (w *rubyWalker) handleMethod(node *sitter.Node, isClassMethod bool) {
 // RelCalls edges to the owner fact. It does not descend into nested
 // method/class/module definitions — those receive their own owner.
 //
-// Three reference shapes are captured: (1) qualified calls via callTarget
+// Four reference shapes are captured: (1) qualified calls via callTarget
 // ("Const.method", "var.method"); (2) bare calls with a method name but no
 // receiver ("render :x", "helper(arg)") → the bare method name; (3) lone
 // identifiers in expression position ("current_user") that are not known locals
-// → the bare name. (2) and (3) are why Ruby methods invoked without a receiver
-// (the common Rails case) are now recorded as referenced. Bare targets carry no
-// "." and never resolve to a constant, so the package-metrics coupling graph
-// (which keys off constant receivers) is unaffected.
+// → the bare name; (4) bare constant references ("MyJob", "Chat::Message") used
+// as values → the constant name. (2) and (3) are why Ruby methods invoked without
+// a receiver (the common Rails case) are recorded as referenced; (4) is why a
+// class/module used only as a value (registered, passed as an argument, matched in
+// case/when) is. Bare targets — from (2), (3) and (4) — carry no ".", so
+// constFromCall ignores them and the package-metrics coupling graph (which keys
+// off "Recv.method" constant receivers) is unaffected.
 func (w *rubyWalker) walkForCalls(node *sitter.Node, ownerIdx int, seen, locals map[string]bool) {
 	if node == nil {
 		return
@@ -538,6 +543,22 @@ func (w *rubyWalker) walkForCalls(node *sitter.Node, ownerIdx int, seen, locals
 			w.recordCallMetrics(name)
 		}
 		return
+	case "constant", "scope_resolution":
+		// A bare constant reference in expression position — an argument
+		// (register(MyJob)), array/hash element, case/when or rescue clause,
+		// assignment RHS, or a lone `Foo` value. It is NOT a `Const.method` call
+		// (that is captured as the receiver via callTarget above) and NOT a
+		// definition name (handleClass/handleModule consume those), so without this
+		// a class/module used only as a value looks unreferenced and is mis-reported
+		// as dead. Record it as a use of that constant. The target carries no ".",
+		// so constFromCall ignores it and the package-metrics coupling graph is
+		// unaffected; it is not a method invocation, so perf metrics are untouched.
+		// scope_resolution is recorded whole (e.g. "Chat::Message") and not
+		// descended into, so the qualified path is matched rather than its segments.
+		if name := stripLeadingColons(rubyText(node, w.src)); name != "" && !rubyBuiltinConsts[name] {
+			w.addCall(ownerIdx, seen, name)
+		}
+		return
 	}
 	for i := uint(0); i < node.ChildCount(); i++ {
 		w.walkForCalls(node.Child(i), ownerIdx, seen, locals)
@@ -587,6 +608,43 @@ var rubyCallbackDSL = map[string]bool{
 	"validate": true,
 }
 
+// rubySerializerDSL are ActiveModel::Serializer class-body methods whose symbol
+// arguments name attributes/associations. Each declared name is backed by an
+// optional same-named method (and an `include_<name>?` predicate) that the
+// serializer framework invokes — never an explicit Ruby call — so they are folded
+// in as references (see handleBodyCall). Applied only inside a serializer class.
+var rubySerializerDSL = map[string]bool{
+	"attributes": true, "attribute": true,
+	"has_one": true, "has_many": true, "belongs_to": true, "has_and_belongs_to_many": true,
+}
+
+// rubyBuiltinConsts are Ruby core and common stdlib constants. A bare reference to
+// one is real, but emitting a call edge to it inflates fan-in on monkey-patch
+// reopenings (Discourse's freedom_patches `class Array`/`String`/`Time`), turning
+// uninteresting core classes into spurious god-class / hotspot findings while
+// never being a useful dead-code lead. They are skipped when recording bare
+// constant references. Namespaced constants (Foo::Array) are unaffected.
+var rubyBuiltinConsts = map[string]bool{
+	"Object": true, "BasicObject": true, "Module": true, "Class": true, "Method": true,
+	"UnboundMethod": true, "Proc": true, "Binding": true, "Data": true,
+	"Array": true, "Hash": true, "String": true, "Symbol": true, "Set": true,
+	"Integer": true, "Float": true, "Numeric": true, "Rational": true, "Complex": true,
+	"Range": true, "Regexp": true, "MatchData": true, "Struct": true, "Enumerator": true,
+	"TrueClass": true, "FalseClass": true, "NilClass": true,
+	"Time": true, "Date": true, "DateTime": true,
+	"Comparable": true, "Enumerable": true, "Kernel": true, "Math": true,
+	"IO": true, "File": true, "Dir": true, "FileUtils": true, "Pathname": true,
+	"StringIO": true, "Tempfile": true,
+	"Thread": true, "Mutex": true, "ConditionVariable": true, "Queue": true,
+	"SizedQueue": true, "Fiber": true, "ThreadGroup": true,
+	"Exception": true, "StandardError": true, "RuntimeError": true, "ArgumentError": true,
+	"TypeError": true, "NameError": true, "NoMethodError": true, "IndexError": true,
+	"KeyError": true, "RangeError": true, "IOError": true, "NotImplementedError": true,
+	"StopIteration": true, "ZeroDivisionError": true, "FrozenError": true,
+	"Marshal": true, "ObjectSpace": true, "GC": true, "Process": true, "Signal": true,
+	"Encoding": true, "Random": true, "SecureRandom": true, "Mutex_m": true,
+}
+
 // rubyNonCalls are bare identifiers that must not be treated as method-call
 // references: keywords/builtins that commonly appear in expression position.
 // (Most Ruby keywords — self, nil, super, yield, return — are their own AST node
@@ -768,6 +826,22 @@ func (w *rubyWalker) handleBodyCall(node *sitter.Node) {
 		return
 	}
 
+	// ActiveModel::Serializer attribute/association DSL: `attributes :a, :b`,
+	// `attribute :c`, `has_one :user`, `has_many :posts`. Each declared name is
+	// backed by a same-named method the serializer framework calls (when defined),
+	// plus an optional `include_<name>?` predicate it calls to decide inclusion —
+	// neither is an explicit Ruby call, so the backing methods look dead. Fold both
+	// forms in as references on the enclosing serializer class. Gated on isSerializer
+	// so the shared has_one/has_many/belongs_to names still reach the ActiveRecord
+	// association handling below for models.
+	if cur := w.cur(); cur != nil && cur.isSerializer && cur.symFactIdx >= 0 && rubySerializerDSL[method] {
+		for _, name := range symbolArgs(args, w.src) {
+			w.addCallToFact(cur.symFactIdx, name)
+			w.addCallToFact(cur.symFactIdx, "include_"+name+"?")
+		}
+		return
+	}
+
 	switch method {
 	case "require", "require_relative":
 		path := firstStringArg(args, w.src)

internal/extractors/rubyextractor/ruby_test.go

@@ -252,6 +252,135 @@ end
 	}
 }
 
+// TestExtractFile_BareConstantReferences checks that a constant used as a value
+// (registered, passed as an argument, matched in case/when, in an array) is
+// recorded as a RelCalls edge so the referenced class/module is not mis-reported
+// as dead code. scope_resolution paths are recorded whole.
+func TestExtractFile_BareConstantReferences(t *testing.T) {
+	src := `class Registry
+  def wire
+    register(MyJob)
+    handlers = [FooHandler, BarHandler]
+    klass = Chat::Message
+    case obj
+    when SomeError
+      retry
+    end
+  end
+end
+`
+	result := extractFileAST([]byte(src), "app/services/registry.rb", false, true)
+	meth, ok := symbolsByName(result)["Registry#wire"]
+	if !ok {
+		t.Fatal("missing method Registry#wire")
+	}
+	for _, want := range []string{"MyJob", "FooHandler", "BarHandler", "Chat::Message", "SomeError"} {
+		if !hasCall(meth, want) {
+			t.Errorf("missing bare-constant RelCalls -> %s; relations = %v", want, meth.Relations)
+		}
+	}
+	// The bare constant target carries no ".", so it is not a coupling-graph
+	// "Recv.method" form — guard that we did not accidentally emit one.
+	if hasCall(meth, "MyJob.register") {
+		t.Errorf("bare constant must not become a Recv.method target; relations = %v", meth.Relations)
+	}
+}
+
+// TestExtractFile_ConstantReceiverCallStillQualified checks that the bare-constant
+// capture does not regress the qualified-call form: a Const.method call must still
+// produce the "Const.method" edge (for coupling), now alongside a bare "Const" one.
+func TestExtractFile_ConstantReceiverCallStillQualified(t *testing.T) {
+	src := `class Builder
+  def run(ids)
+    Items::Facade.fetch(ids)
+  end
+end
+`
+	result := extractFileAST([]byte(src), "app/services/builder.rb", false, true)
+	meth := symbolsByName(result)["Builder#run"]
+	if !hasCall(meth, "Items::Facade.fetch") {
+		t.Errorf("qualified call edge lost; relations = %v", meth.Relations)
+	}
+	if !hasCall(meth, "Items::Facade") {
+		t.Errorf("missing bare-receiver constant edge; relations = %v", meth.Relations)
+	}
+}
+
+// TestExtractFile_BuiltinConstantsSkipped checks that bare references to Ruby
+// core/stdlib constants are not emitted as call edges (they inflate fan-in on
+// monkey-patch reopenings), while application constants still are.
+func TestExtractFile_BuiltinConstantsSkipped(t *testing.T) {
+	src := `class Worker
+  def run
+    Array.new
+    x = [String, Time]
+    enqueue(MyJob)
+  end
+end
+`
+	result := extractFileAST([]byte(src), "app/services/worker.rb", false, true)
+	meth := symbolsByName(result)["Worker#run"]
+	for _, skip := range []string{"Array", "String", "Time"} {
+		if hasCall(meth, skip) {
+			t.Errorf("builtin constant %s must not be emitted as a call edge; relations = %v", skip, meth.Relations)
+		}
+	}
+	if !hasCall(meth, "MyJob") {
+		t.Errorf("application constant MyJob should still be recorded; relations = %v", meth.Relations)
+	}
+}
+
+// TestExtractFile_SerializerAttributeFold checks that a serializer's attribute and
+// association DSL folds the backing methods (and the include_<name>? predicate) in
+// as references on the serializer class, so they are not mis-reported as dead.
+func TestExtractFile_SerializerAttributeFold(t *testing.T) {
+	src := `class PostSerializer < ApplicationSerializer
+  attributes :cooked, :score
+  has_one :user
+  def cooked
+    object.cooked
+  end
+  def include_score?
+    scope.admin?
+  end
+end
+`
+	result := extractFileAST([]byte(src), "app/serializers/post_serializer.rb", true, true)
+	cls, ok := symbolsByName(result)["PostSerializer"]
+	if !ok {
+		t.Fatal("missing class PostSerializer")
+	}
+	for _, want := range []string{"cooked", "include_cooked?", "score", "include_score?", "user", "include_user?"} {
+		if !hasCall(cls, want) {
+			t.Errorf("missing serializer DSL fold RelCalls -> %s; relations = %v", want, cls.Relations)
+		}
+	}
+}
+
+// TestExtractFile_NonSerializerHasManyUnaffected guards that has_many on a model
+// (not a serializer) still produces an association dependency fact and is NOT
+// short-circuited by the serializer fold.
+func TestExtractFile_NonSerializerHasManyUnaffected(t *testing.T) {
+	src := `class User < ApplicationRecord
+  has_many :posts
+end
+`
+	result := extractFileAST([]byte(src), "app/models/user.rb", true, true)
+	var sawAssoc bool
+	for _, f := range result {
+		if f.Kind == facts.KindDependency {
+			for _, r := range f.Relations {
+				if r.Kind == facts.RelDependsOn && r.Target == "Post" {
+					sawAssoc = true
+				}
+			}
+		}
+	}
+	if !sawAssoc {
+		t.Error("model has_many :posts should still emit a depends_on association fact (serializer fold must not swallow it)")
+	}
+}
+
 func TestExtractFile_BareCallSkipsLocalsAndKeywords(t *testing.T) {
 	src := `class A
   def b

internal/extractors/rubyextractor/storage.go

@@ -32,6 +32,14 @@ func isARBaseClass(superclass string) bool {
 	return false
 }
 
+// isSerializerBase reports whether a superclass marks an ActiveModel::Serializer
+// subclass. By convention every serializer base ends in "Serializer"
+// (ApplicationSerializer, BasicPostSerializer, ActiveModel::Serializer, …), so the
+// suffix is a reliable, dependency-free signal.
+func isSerializerBase(superclass string) bool {
+	return strings.HasSuffix(superclass, "Serializer")
+}
+
 // inferTableName derives the conventional Rails table name from a class name.
 // e.g. "Item" -> "items", "UserAddress" -> "user_addresses", "Api::V2::Item" -> "items"
 func inferTableName(className string) string {