Compiler audit: performance & correctness review

Scope · Scala 3 (Dotty) compiler — compiler/src/dotty/tools/dotc (~217K LOC, 586 files)
Branch reviewed · change-unscoped-fresh

Method. 40 finder agents deep-read the compiler's highest-value subsystems in parallel; every candidate finding was then handed to an adversarial verifier that read the real code, checked callers/invariants, and tried to refute it.

Result. 75 raw findings → 29 confirmed, 44 refuted, 2 uncertain. Two independent finders converged on the same transClassifiers typo, so there are 28 distinct issues. Every confirmed item was rated fixSafe (semantics-preserving) by its verifier. Findings #1, #2, #3, #6 in Tier 1 were additionally spot-checked by hand.

No code has been changed. Each item lists its file:line and exact fix so any subset can be cherry-picked.

Tier 1 — Real bugs & highest-value fixes

1. Capture-set classifier cache is permanently disabled — == typo for =

compiler/src/dotty/tools/dotc/cc/Capability.scala:671

perf · severity medium · found independently by 2 agents; verified by hand

Inside transClassifiers, the cache-validation line is classifiersValid == currentId (a discarded boolean compare) instead of classifiersValid = currentId. classifiersValid is therefore never marked valid, so the whole classifier computation — including recursive descent through ReadOnly/Maybe/Reach wrappers and captureSetOfInfo.transClassifiers — re-runs on every call. Hit per-element on Var.addThisElem, subsumes/maxSubsumes, isKnownEmpty, and separation checking. The myClassifiers memo field is written but never honored.

        if myClassifiers != UnknownClassifier then
          classifiersValid == currentId   // BUG: should be `=`
      myClassifiers

Fix. classifiersValid = currentId. Mirrors captureSetValid = currentId in the structurally identical captureSetOfInfo. currentId already encodes runId+iterationId, so the cache self-invalidates per run/iteration.

2. SubstBindingsMap.mapCapability silently skips non-MethodType result-cap binders

compiler/src/dotty/tools/dotc/core/Substituters.scala:196

bug · severity medium · verified by hand

The fused multi-binder map matches case c @ ResultCap(binder: MethodType) and casts the target to MethodType, but the single SubstBindingMap (line 169) was widened in commit 97ce2b9c9c to ResultCap(binder) / MethodicType. A ResultCap whose binder is a parameterless PolyType (e.g. [X] => C^{fresh}, produced by Setup.toResult for existential-scope-marking polys) no longer matches when two binder substitutions fuse → the binder is left stale and un-substituted (silent, not a crash). Directly relevant to the capture-checking work on this branch.

    override def mapCapability(c: Capability, deep: Boolean = false) = c match
      case c @ ResultCap(binder: MethodType) =>          // too narrow
        ...
        if i < from.length then c.derivedResult(to(i).asInstanceOf[MethodType]) else c

Fix. case c @ ResultCap(binder) and cast to MethodicType, mirroring the sibling single map. (ResultCap.binder is already declared MethodicType; derivedResult accepts MethodicType.)

3. MegaPhase Splice prepare-hook is dead — dispatch table built with a non-existent method name

compiler/src/dotty/tools/dotc/transform/MegaPhase.scala:578

bug (latent) · severity medium · verified by hand

init("prepareForPrep") — but the hook is prepareForSplice (every other entry follows prepareForX/transformX). defines(_, "prepareForPrep") is always false, so nxSplicePrepPhase stays the all-null array and no mini-phase's prepareForSplice ever runs on Splice subtrees. Latent today (no current override) but silently breaks the prepare/transform context contract for any future one. Introduced in 687d06ea78.

Fix. init("prepareForSplice").

4. TreeTypeMap.mapType allocates a fresh TypeMap + a no-op substSym(Nil,Nil) traversal per call

compiler/src/dotty/tools/dotc/ast/TreeTypeMap.scala:71-77

perf · severity medium

On the inlining/transform hot path (once per tree node in transform, once per symbol info in mapSymbols). When substFrom is empty (common — changeOwner, new TreeTypeMap(), etc.) it still allocates an anonymous TypeMap and runs a full identity SubstSymMap traversal of compound types.

Fix. hoist substMap to a private val and short-circuit: val tp1 = typeMap(tp); val tp2 = if substFrom.isEmpty then tp1 else substMap(tp1); mapOwnerThis(tp2). Hoisting is safe because withSubstitution creates a fresh TreeTypeMap via copy.

The verifier empirically rebuilt the compiler and ran the i19436/i19493 regression tests plus 12 inline tests with this fix — all pass.

5. Inliner strips retains over the whole call tree even when capture checking is off

compiler/src/dotty/tools/dotc/inlines/Inlines.scala:105-117

perf · severity medium

inlineCall (central inline hot path) unconditionally builds a CleanupRetains TreeTypeMap (stripper) and traverses the entire call tree. With cc off there are no retains annotations anywhere, so it's a full tree+type walk that accomplishes nothing.

Fix. gate on Feature.ccEnabledSomewhere (run-wide — inlining can pull trees from other units), constructing stripRetains/stripper only inside that branch; otherwise use tree unchanged.

6. Mode.toString drops bit 31 (NoInline)

compiler/src/dotty/tools/dotc/core/Mode.scala:25

bug · severity low · verified by hand

(0 until 31) excludes bit 31; NoInline = newMode(31, ...) is actively set (Dynamic, PostTyper). Mode(NoInline) renders as Mode() in debug/assert output. Diagnostics only, no compilation effect.

Fix. (0 until 32) — the modeName array is already sized 32 and the sign-bit compare bits & (1 << 31) against 0 is fine.

7. EqHashSet.hash left-shifts by 1, wasting the low index bit

compiler/src/dotty/tools/dotc/util/EqHashSet.scala:28

perf · severity low

identityHashCode(key) << 1 forces every home slot even (the mask is & (length - 1) for single-cell entries) → only half the buckets are ever initial homes, roughly doubling probe-chain length. The <<1 is correct only for EqHashMap (2-cell entries, mask & (length - 2)); it was copy-pasted here. Sibling HashSet.hash correctly does not shift. Used for hot visited-sets (ExtractDependencies.scratchSeen, cc seen sets).

Fix. drop the shift — System.identityHashCode(key). (Small dense sets ≤8 elements use isDense/firstIndex == 0 and are unaffected.)

8. WeakHashSet.clear(resetToInitial) ignores its parameter

compiler/src/dotty/tools/dotc/util/WeakHashSet.scala:207-215

bug · severity low

The resetToInitial parameter is unused; the method always reallocates at the current (grown) size and never shrinks to initial — diverging from both the MutableSet.clear contract and the sibling GenericHashSet.clear. Contexts.reset() calls uniques.clear() per run with the default resetToInitial = true; uniques starts at 0x8000 and grows to potentially millions of buckets, so a reused ContextBase (resident/REPL/IDE) never reclaims the oversized array. Bounded retention (entries are weak), not an unbounded leak.

Fix. honor the flag like GenericHashSet.clear — reallocate at computeCapacity when resetToInitial, else null-fill; set table before threshold (it reads table.size).

9. Implicit-scope cache guard mis-parenthesized — caches NotCached types

compiler/src/dotty/tools/dotc/typer/Implicits.scala:777-781

perf · severity low

&& binds tighter than ||, so the guard parses as (Config.cacheImplicitScopes && tp.hash != NotCached && (tp eq rootTp)) || !incomplete.contains(tp) instead of the documented … && ((tp eq rootTp) || !incomplete.contains(tp)). The parens were lost in an inlining refactor (original 3c6f20dc20 was correct). The tp.hash != NotCached guard is bypassed, so transient types bloat the per-run implicitScopeCache (identity-keyed, so they rarely ever hit). No incorrect output — the cache is cleared on reset().

Fix. restore the inner parens: if Config.cacheImplicitScopes && tp.hash != NotCached && ((tp eq rootTp) || !incomplete.contains(tp)).

Tier 2 — Confirmed micro-fixes

All fixSafe, mostly one-liners.

Uncertain

Real mechanism, impact likely negligible.

• util/concurrent.scala:41-48 — the Executor work queue dequeues from the front of an ArrayBuffer (pending.dropInPlace(1) → arraycopy), O(n) per item / O(n²) drain. Only caller is Pickler.runOn under parallel pickling. Verifier benchmarked the pathological ceiling at ~26ms for 10k classes and near-zero in practice (consumer drains concurrently). A mutable.Queue/ArrayDeque is a clean drop-in if desired.
• ast/Desugar.scala:406-417 — addDefaultGetters recomputes paramssNoRHS (rewrites all param lists) once per default getter → O(k·params); only matters for methods with several defaults, and zero cost for methods without defaults. Change the def to a val.

Coverage

Reviewed. core type system (Types, TypeComparer, SymDenotations, Denotations, Definitions, Contexts/TyperState, TypeOps/TypeApplications, TypeErasure, constraint solving, names/scopes/flags, substituters), capture checking (CheckCaptures, CaptureSet, Capability/CaptureOps, Setup/SepCheck/Mutability), typer (Typer, Applications, Implicits, Namer, Inferencing/ProtoTypes, RefChecks, Checking/Synthesizer), util data structures (hash maps/sets, identity maps, weak/LRU caches, spans/source), AST, transforms, parsing, inlining, tasty.

Not covered this pass (lower ROI for a perf/bug sweep; available as a follow-up): JVM & Scala.js backends, semanticdb, quotes runtime impl, interactive/IDE, config plumbing.

The audit is saved as a reusable workflow — /compiler-perf-bug-audit — and can be re-run or extended to the uncovered subsystems.

28 distinct issues · 75 raw findings → 29 confirmed · adversarially verified · no code changed