Skip to content

Commit 7550215

Browse files
authored
Fix kotlin extractor graph pass july 3rd 26 (#64)
* Fix Kotlin dead-code false positives: emit method-kind for members, receiver/property/callable-reference/outside-body call edges, and Groovy-namespace cross-module imports in the extractor, and exclude framework/DI/override/androidTest entry points in the orphan detector * Kotlin/Java: make analyze_performance perf-facts precise — arity- and super-aware recursion detection, RxJava/Flow chains no longer counted as loops, and Retrofit/Room methods tagged performs_io (cacheVersion v57). * fix(package-metrics): eliminate false positives on multi-module Kotlin/Java by resolving cross-module imports via a declared-package index, excluding test-support/DI/enum symbols from the population, and reclassifying data-holder packages instead of flagging them rigid * Adding missing files * Fixing lint
1 parent 3435cb7 commit 7550215

16 files changed

Lines changed: 1200 additions & 48 deletions

File tree

internal/engine/cache.go

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,42 @@ import (
6565
// v49: Swift models XcodeGen test-bundle targets (bundle.unit-test/bundle.ui-testing) as one module each, so a test bundle's files (e.g. Tests/Core/**) collapse into a single module instead of exploding into per-leaf-directory modules; and every module fact now carries a normalized `module_role` prop (production/test/tooling/unknown) — derived from the XcodeGen target type, the SPM target vs testTarget call, or a path heuristic for leaf-directory fallback — so package-metrics and other analyses can measure the production population without re-parsing manifests.
6666
// v50: the `module_role` prop is now emitted by the Ruby extractor too (packwerk packages → production; leaf-directory modules → path heuristic), and the path heuristic was hoisted to facts.ModuleRoleForPath and broadened to common cross-language conventions (spec/test/tests + scripts/bin/fastlane/ci_scripts), so Ruby build-tooling modules (fastlane/, Scripts/) are classified as tooling rather than defaulting to the production population.
6767
// v51: Swift no longer emits type-reference-derived module→module dependency edges for files that belong to a resolved SPM/XcodeGen target — those files' cross-module deps are captured completely by their `import X` statements plus the declared target graph, whereas the type-reference pass resolved bare short names through a collision-prone global index (Swift namespaces nested types, so names like Event/State/Coordinator recur across targets) and fabricated impossible back-edges (a Foundation-level target "importing" a feature target) that produced a false module cycle. For loose Swift projects (leaf-directory fallback, no target graph) the pass still runs but now skips any type name defined in more than one module. Fixes the false Swift dependency cycle.
68-
const cacheVersion = "v51"
68+
// v52: Kotlin emits SymbolMethod (not SymbolFunc) for functions declared inside a class/object (parity with Go/Java), keeping member functions out of the high-confidence orphan bucket; records a short-name RelCalls edge for every navigation expression (receiver method call `repo.getUser()` and property/field access `slot.uniqueId`), which the short-name-matching dead-code detector needs to see live members as used; and tags `override` methods and Dagger/Hilt `@Provides`/`@Binds` methods with `override`/`di_provider` props so framework/DI entry points are excluded from orphan reporting. Fixes the mass Kotlin/Android dead-code false positives (thousands of live Retrofit/lifecycle/interface members reported as high-confidence orphans).
69+
// v53: Kotlin base-package detection now matches Groovy build scripts (`namespace 'x'`, single quotes) in addition to Kotlin-DSL (`namespace = "x"`). A double-quote-only regex left the base package empty for `.gradle` (Groovy) projects, so every in-repo import resolved as external and bare calls to imported top-level/extension functions (`formatPrettyDate(...)`, `setMargin(...)`) emitted no call edge — reporting live utilities as high-confidence orphans.
70+
// v54: Kotlin now walks calls that live OUTSIDE a function body — function default-parameter values (`fun f(x = helper())`) and supertype constructor-delegation arguments (`class NpeId(id) : Enrichment(npeEntity(id))`, and the object equivalent). These were previously skipped, so a helper/factory referenced only from a default value or a base-class initializer was mis-reported as a high-confidence orphan (e.g. Snowplow entity builders, Compose default-arg providers).
71+
// v55: Kotlin captures callable references (`::foo`, `Type::foo`, e.g. `onClick = ::doNothing`, `.map(::helper)`) as short-name RelCalls edges. A function referenced only as a method reference (never called directly) was previously mis-reported as a high-confidence orphan.
72+
// v56: Kotlin/Java perf-fact precision — (1) recursion (`recursive_self`) is now argument-count aware: a call sharing the enclosing function's name is only flagged recursion when its arg count matches the parameter count, so a call to a same-named overload (`updateItem(x)` → `updateItem(i, x)`, `onChangeStarted(2)` override → `onChangeStarted(3)`) is no longer read as self-recursion — the dominant Kotlin/Android recursion false positive (Conductor `onChange*` lifecycle). (2) Kotlin RxJava/coroutine-Flow chains no longer inflate loop_depth: in a reactive function (reactive return type Single/Observable/Maybe/Flowable/Completable/Flow, or body reactive operators subscribeOn/observeOn/applySchedulers/andThen/.subscribe/flowOn/.collect/…), the ambiguous operators (map/flatMap/filter/fold/reduce/onEach) are stream transforms, not per-element collection loops, so a `Single.flatMap { … .map { } }` is no longer a false O(n²)/O(n³). Fixes the analyze_performance false positives on this RxJava-heavy codebase.
73+
// v57: Kotlin/Java recursion also clears the arity-matched case where an `override` delegates to a same-name, same-arity overload declared in a parent (invisible here): a body that calls `super.<self>()` marks the sibling `<self>(…)` call as delegation, not recursion (fixes the residual Conductor `onChangeEnded` false positives). And Kotlin methods now carry `io_direct`/`performs_io` when annotated as a Retrofit endpoint (@GET/@POST/@PUT/@DELETE/@PATCH/@HEAD/@OPTIONS/@HTTP) or a Room DAO op (@Query/@Insert/@Update/@Upsert/@Delete/@RawQuery) — a precise per-method I/O identity that lets analyze_performance flag a per-iteration call to a real network/DB method as a genuine N+1 (ranked high) without relying on the cross-language keyword guess.
74+
// v58: Kotlin/Java multi-module import resolution — imports are now resolved via a
75+
// cross-language declared-package→directory index (built from every non-test .kt AND
76+
// .java file) instead of assuming a single global source root. In a multi-module
77+
// Gradle project where several modules root packages at the same prefix (app/, api/,
78+
// business/ all under de.foo.*), the old Kotlin resolver mapped every internal import
79+
// under the single most common source root (the app module), collapsing all
80+
// cross-module afferent coupling onto the app package (bogus Ca god-package) and
81+
// starving library modules of Ca (falsely "useless" in package-metrics). Now an
82+
// import resolves to the module that actually declares the package. The Java extractor
83+
// seeds its FQN resolver with the same cross-language index so Java→Kotlin imports no
84+
// longer drop. Kotlin & Java module facts now carry `module_role` (test for
85+
// src/test & src/androidTest, else production) so package-metrics excludes test source
86+
// sets; and Kotlin `sealed` classes are marked `abstract` so abstractness (A) counts
87+
// them (they are non-instantiable), fixing inflated Distance / false "rigid" findings.
88+
// v59: the package index now prefers a main source set (…/src/main/…) over a Gradle
89+
// build-variant source set (src/debug, src/release, src/staging) when both declare the
90+
// same package. An Android app's src/main and src/debug both declare the root
91+
// application package; the v58 lexicographic tie-break wrongly mapped it to src/debug
92+
// ('d' < 'm'), misrouting the whole app's afferent coupling onto the debug variant
93+
// (a bogus Ca god-package). Imports of the root package now resolve to the main module.
94+
// v60: package-metrics precision — (1) module_role now sub-token-matches compound test
95+
// module names (split each path segment on -/_ and match an exact `test`/`tests` token),
96+
// so Gradle test-automation modules that compile as src/main (release-tests, ui-test-utils,
97+
// test-lab) are classified test rather than leaking into the production population, without
98+
// misfiring on latest/contest/abtest. (2) Dagger/Hilt DI infrastructure is now tagged:
99+
// @Component/@Subcomponent interfaces get `di_component`, @Module classes get `di_module`
100+
// (Java & Kotlin); a Dagger @Component interface is no longer mislabeled a Spring component
101+
// (disambiguated by interface-vs-class). Lets package-metrics exclude DI wiring from
102+
// abstractness/type counts (a Dagger component package was falsely "useless").
103+
const cacheVersion = "v60"
69104

70105
// extractorCache holds per-extractor facts keyed by a content hash of the files
71106
// the extractor depends on. It is loaded from disk at the start of a snapshot and
@@ -169,15 +204,27 @@ func computeExtractorKeys(all []extractors.Extractor, files []string, hashes map
169204
}
170205

171206
// Partition files: per-owner owned lists + the shared (un-owned) remainder.
207+
// keyFiles is owned ∪ AffectsKey — the full set whose contents feed the key,
208+
// so a cross-language file that a KeyDependent extractor reads (but does not
209+
// own) still invalidates its cache. Ownership (and thus the shared remainder)
210+
// is decided purely by OwnsFile; AffectsKey only widens the key, never the
211+
// ownership partition.
172212
owned := map[string][]string{}
213+
keyFiles := map[string][]string{}
173214
var shared []string
174215
for _, f := range files {
175216
ownedByAny := false
176217
for name, fo := range owners {
177-
if fo.OwnsFile(f) {
218+
owns := fo.OwnsFile(f)
219+
if owns {
178220
owned[name] = append(owned[name], f)
179221
ownedByAny = true
180222
}
223+
if owns {
224+
keyFiles[name] = append(keyFiles[name], f)
225+
} else if kd, ok := fo.(plugin.KeyDependent); ok && kd.AffectsKey(f) {
226+
keyFiles[name] = append(keyFiles[name], f)
227+
}
181228
}
182229
if !ownedByAny {
183230
shared = append(shared, f)
@@ -189,7 +236,7 @@ func computeExtractorKeys(all []extractors.Extractor, files []string, hashes map
189236
for name := range owners {
190237
h := sha256.New()
191238
h.Write([]byte(cacheVersion + "\x00" + name + "\x00" + sharedHash + "\x00"))
192-
h.Write([]byte(hashFileSet(owned[name], hashes)))
239+
h.Write([]byte(hashFileSet(keyFiles[name], hashes)))
193240
keys[name] = hex.EncodeToString(h.Sum(nil))
194241
}
195242
return keys

internal/extractors/javaextractor/java.go

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"path/filepath"
88
"strings"
99

10+
"github.com/enola-labs/enola/internal/extractors/jvmsrc"
1011
"github.com/enola-labs/enola/internal/facts"
1112
"github.com/enola-labs/enola/internal/parallel"
1213
)
@@ -72,7 +73,12 @@ func (e *JavaExtractor) Extract(ctx context.Context, repoPath string, files []st
7273
modules[filepath.Dir(javaFiles[i])] = true
7374
}
7475

75-
canonicalizeTargets(allFacts)
76+
// Cross-language package index (.kt AND .java) so a Java import of a Kotlin
77+
// type resolves to the module that declares it, instead of being dropped as
78+
// external. Java→Java imports still resolve via the in-facts FQN index below;
79+
// this only fills the cross-language gap.
80+
packageIndex := jvmsrc.BuildPackageIndex(repoPath, files)
81+
canonicalizeTargets(allFacts, packageIndex)
7682
resolveTableConstants(allFacts)
7783

7884
for dir := range modules {
@@ -81,7 +87,8 @@ func (e *JavaExtractor) Extract(ctx context.Context, repoPath string, files []st
8187
Name: dir,
8288
File: dir,
8389
Props: map[string]any{
84-
"language": "java",
90+
"language": "java",
91+
facts.PropModuleRole: jvmsrc.ModuleRole(dir),
8592
},
8693
})
8794
}
@@ -97,10 +104,16 @@ func (e *JavaExtractor) Extract(ctx context.Context, repoPath string, files []st
97104
// - import dependency facts whose target FQN resolves to a declared type — or whose
98105
// value names a known source package — are marked source="internal" and pointed at
99106
// the owning module dir.
100-
func canonicalizeTargets(allFacts []facts.Fact) {
107+
func canonicalizeTargets(allFacts []facts.Fact, crossLangIndex map[string]string) {
101108
typeIndex := make(map[string]string) // FQN -> "<dir>.<Type>" canonical name
102109
typeDir := make(map[string]string) // FQN -> dir
103110
packageDir := make(map[string]string)
111+
// Seed with cross-language packages (e.g. Kotlin modules) so a Java import of
112+
// a package we didn't index from .java files still resolves. Java-declared
113+
// packages below take precedence (they overwrite these entries).
114+
for pkg, dir := range crossLangIndex {
115+
packageDir[pkg] = dir
116+
}
104117
for _, f := range allFacts {
105118
if f.Kind != facts.KindSymbol {
106119
continue
@@ -240,6 +253,13 @@ func isJavaFile(path string) bool {
240253
// OwnsFile implements plugin.FileOwner for incremental caching.
241254
func (e *JavaExtractor) OwnsFile(relFile string) bool { return isJavaFile(relFile) }
242255

256+
// AffectsKey implements plugin.KeyDependent: a .kt file's package declaration
257+
// feeds the cross-language package index used to resolve Java imports of Kotlin
258+
// types, so a change to any Kotlin source must invalidate the Java extractor's cache.
259+
func (e *JavaExtractor) AffectsKey(relFile string) bool {
260+
return strings.HasSuffix(strings.ToLower(relFile), ".kt")
261+
}
262+
243263
// containsJavaSource reports whether any .java file exists under root within
244264
// maxDepth directory levels. It returns on the first match and skips hidden and
245265
// common build/dependency directories so it stays cheap on large repos.

internal/extractors/javaextractor/java_ast.go

Lines changed: 59 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ type astWalker struct {
8080
loopDepth int
8181
selfName string
8282
selfShort string
83+
// selfParams is the enclosing method's declared parameter count. A resolved
84+
// self-call is only genuine recursion when its argument count matches — otherwise
85+
// it is a call to a same-named overload, not recursion.
86+
selfParams int
8387
}
8488

8589
// javaBodyMetrics accumulates per-method complexity signals during the single
@@ -88,9 +92,10 @@ type javaBodyMetrics struct {
8892
loopDepth int // max loop nesting depth
8993
loopCount int // number of loop constructs (syntactic + stream lambdas)
9094
decisions int // decision points (cyclomatic = 1 + decisions)
91-
callsInLoop []string // distinct call targets invoked at loop depth >= 1
92-
inLoopSeen map[string]bool // dedup set for callsInLoop
93-
recursive bool // body directly calls the enclosing method
95+
callsInLoop []string // distinct call targets invoked at loop depth >= 1
96+
inLoopSeen map[string]bool // dedup set for callsInLoop
97+
recursive bool // body directly calls the enclosing method
98+
sawSuperSelf bool // body calls super.<enclosingName>() (override delegation)
9499
}
95100

96101
// javaIterators are Stream/Collection methods whose lambda argument runs once per
@@ -119,16 +124,53 @@ var javaCheapMethods = map[string]bool{
119124

120125
// recordCallMetrics notes a resolved call target against the current method's
121126
// complexity metrics: flags direct recursion and records calls made inside loops.
122-
func (w *astWalker) recordCallMetrics(target string) {
127+
// argCount is the invocation's argument count; recursion is flagged only when it
128+
// matches the enclosing method's parameter count, so a call to a same-named overload
129+
// is not mistaken for self-recursion.
130+
func (w *astWalker) recordCallMetrics(target string, argCount int) {
123131
if w.metrics == nil || target == "" {
124132
return
125133
}
126-
if target == w.selfName || target == w.selfShort {
134+
if (target == w.selfName || target == w.selfShort) && argCount == w.selfParams {
127135
w.metrics.recursive = true
128136
}
129137
w.recordInLoop(target)
130138
}
131139

140+
// javaArgCount returns the number of arguments of a method_invocation node.
141+
func javaArgCount(node *sitter.Node) int {
142+
args := node.ChildByFieldName("arguments")
143+
if args == nil {
144+
return 0
145+
}
146+
n := 0
147+
for i := uint(0); i < uint(args.ChildCount()); i++ {
148+
if args.Child(i).IsNamed() {
149+
n++
150+
}
151+
}
152+
return n
153+
}
154+
155+
// javaParamCount returns the declared parameter count of a method declaration node.
156+
func javaParamCount(node *sitter.Node) int {
157+
params := node.ChildByFieldName("parameters")
158+
if params == nil {
159+
params = findChildByKind(node, "formal_parameters")
160+
}
161+
if params == nil {
162+
return 0
163+
}
164+
n := 0
165+
for i := uint(0); i < uint(params.ChildCount()); i++ {
166+
switch params.Child(i).Kind() {
167+
case "formal_parameter", "spread_parameter":
168+
n++
169+
}
170+
}
171+
return n
172+
}
173+
132174
// recordInLoop adds a target to calls_in_loop (deduped) when inside a loop, without
133175
// the recursion check — used for raw instance-method names.
134176
func (w *astWalker) recordInLoop(target string) {
@@ -525,10 +567,12 @@ func (w *astWalker) handleMethod(node *sitter.Node) {
525567
// may be invalidated if the body walk grows w.out).
526568
savedMetrics, savedDepth := w.metrics, w.loopDepth
527569
savedName, savedShort := w.selfName, w.selfShort
570+
savedParams := w.selfParams
528571
w.metrics = &javaBodyMetrics{}
529572
w.loopDepth = 0
530573
w.selfName = f.Name
531574
w.selfShort = name
575+
w.selfParams = javaParamCount(node)
532576
if body := node.ChildByFieldName("body"); body != nil {
533577
w.walkForCalls(body)
534578
}
@@ -544,11 +588,14 @@ func (w *astWalker) handleMethod(node *sitter.Node) {
544588
if len(m.callsInLoop) > 0 {
545589
props["calls_in_loop"] = m.callsInLoop
546590
}
547-
if m.recursive {
591+
// A body that calls super.<self>() is an override delegating to a same-named
592+
// overload, not genuine recursion — clear the arity-matched self-call flag.
593+
if m.recursive && !m.sawSuperSelf {
548594
props["recursive_self"] = true
549595
}
550596
w.metrics, w.loopDepth = savedMetrics, savedDepth
551597
w.selfName, w.selfShort = savedName, savedShort
598+
w.selfParams = savedParams
552599
w.popOwner()
553600
}
554601

@@ -807,14 +854,19 @@ func (w *astWalker) handleInvocation(node *sitter.Node) {
807854
// own methods. Calls on other receivers are left unresolved (the receiver's
808855
// type is not tracked), matching the Kotlin extractor's conservative model.
809856
isThis := obj != nil && nodeText(obj, w.src) == "this"
857+
if obj != nil && w.metrics != nil && name == w.selfShort && nodeText(obj, w.src) == "super" {
858+
// super.<self>() — an override delegating to its supertype. Note it so an
859+
// arity-matched bare <self>(…) call is read as overload delegation, not recursion.
860+
w.metrics.sawSuperSelf = true
861+
}
810862
if obj == nil || isThis {
811863
if methods := w.currentMethods(); methods[name] {
812864
target := w.dir + "." + w.enclosingType() + "." + name
813865
owner.Relations = append(owner.Relations, facts.Relation{
814866
Kind: facts.RelCalls,
815867
Target: target,
816868
})
817-
w.recordCallMetrics(target)
869+
w.recordCallMetrics(target, javaArgCount(node))
818870
}
819871
} else if w.metrics != nil && w.loopDepth > 0 && !javaCheapMethods[name] {
820872
// Method call on a non-this receiver inside a loop (repo.findById(), …). No

internal/extractors/javaextractor/spring.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,21 @@ func findAnnotation(anns []javaAnnotation, name string) *javaAnnotation {
115115
// classifyComponent tags a type symbol fact with framework/component props for
116116
// Spring stereotypes and Dubbo SPI. It mutates f.Props in place.
117117
func classifyComponent(f *facts.Fact, name string, annotations []javaAnnotation, supertypes []string) {
118+
// Dagger/Hilt DI infrastructure. Dagger @Component/@Subcomponent are declared
119+
// on INTERFACES, whereas Spring stereotypes are always concrete classes — so a
120+
// @Component on an interface is Dagger, not Spring. This disambiguates the
121+
// simple-name collision between dagger.Component and springframework…Component
122+
// (annotations are matched by simple name) and keeps DI wiring out of the
123+
// domain-architecture metrics. @Module classes are DI wiring regardless of kind.
124+
if hasAnnotation(annotations, "Module") {
125+
f.Props["di_module"] = true
126+
}
127+
if f.Props["symbol_kind"] == facts.SymbolInterface &&
128+
(hasAnnotation(annotations, "Component") || hasAnnotation(annotations, "Subcomponent")) {
129+
f.Props["di_component"] = true
130+
return // do NOT fall through to the Spring stereotype switch (avoids mislabel)
131+
}
132+
118133
switch {
119134
case hasAnnotation(annotations, "RestController"):
120135
f.Props["framework"] = "spring"

0 commit comments

Comments
 (0)