Some fixes for Scala 3 mock-method metadata (#641)

Author: jozic

README.md

@@ -40,8 +40,30 @@ The library has independent developers, release cycle and versioning from core m
 ## Partial unification
 If you're in Scala 2.12 you'll probably want to add the compiler flag `-Ypartial-unification`, if you don't you risk some compile errors when trying to stub complex types using the idiomatic syntax
 
+## Notes for 2.1.0
+
+### Scala 3: `mock[T]` wrapper methods must be `inline`
+
+In Scala 3, `mock[T]` uses a compile-time macro to register by-name/vararg parameter metadata
+and value-class return types for `T`. This macro only works when `T` is concrete at the call site,
+which requires every method in the call chain to be `inline`.
+
+If you wrap `mock[T]` in a helper, declare it `inline`:
+
+```scala
+// CORRECT — T is concrete at every call site
+inline def lenientMock[T <: AnyRef: ClassTag]: T =
+  mock[T](Mockito.withSettings().strictness(Strictness.LENIENT))
+
+// WRONG — T is erased; by-name/vararg metadata will NOT be registered
+def lenientMock[T <: AnyRef: ClassTag]: T =
+  mock[T](Mockito.withSettings().strictness(Strictness.LENIENT))
+```
+
+Without `inline`, stubs on by-name or vararg methods will silently fail to match at runtime.
+
 ## Notes for 2.0.0
-We dropped support for Scala 2.11 and Java 8, as Mockito 5 dropped support for Java 8. 
+We dropped support for Scala 2.11 and Java 8, as Mockito 5 dropped support for Java 8.
 Java 11 is now the minimum supported version.
 
 ## Notes for 1.13.6

common/src/main/scala-3/org/mockito/MockCreator.scala

@@ -18,6 +18,23 @@ import scala.reflect.ClassTag
  *   - `inline` keeps the concrete `T` at call sites, allowing compile-time metadata extraction.
  *   - `createMock` registers per-method metadata via [[org.mockito.internal.MockMethodMetadata.registerByNameAndVarArgInfo]] before delegating to runtime creation; this feeds
  *     `ReflectionUtils`/`ScalaMockHandler` with by-name, vararg and return metadata.
+ *
+ * ==Wrapper methods must be `inline` (Scala 3)==
+ *
+ * All `mock[T]` overloads are `inline` so that `T` is concrete at the macro expansion point. If you wrap `mock[T]` in your own helper, that helper must also be `inline`:
+ *
+ * {{{
+ * // CORRECT – T is known at every call site
+ * inline def lenientMock[T <: AnyRef: ClassTag]: T =
+ *   mock[T](Mockito.withSettings().strictness(Strictness.LENIENT))
+ *
+ * // WRONG – T is erased; by-name / vararg metadata will NOT be registered
+ * def lenientMock[T <: AnyRef: ClassTag]: T =
+ *   mock[T](Mockito.withSettings().strictness(Strictness.LENIENT))
+ * }}}
+ *
+ * Without `inline`, the compile-time macro [[org.mockito.internal.MockMethodMetadata.registerByNameAndVarArgInfo]] sees `T` as an abstract type variable and produces no output. At
+ * runtime the `ScalaMockHandler` will not find the method in its cache, so vararg arrays won't be expanded and stubs on vararg methods will silently fail to match.
  */
 private[mockito] trait MockCreator extends MockCreatorRuntime {
 

common/src/main/scala-3/org/mockito/ReflectionUtils.scala

@@ -5,7 +5,7 @@ import org.mockito.internal.MockMethodMetadata
 import org.mockito.internal.MockMetadataCache
 import org.mockito.invocation.InvocationOnMock
 
-import java.lang.reflect.{ Method, TypeVariable }
+import java.lang.reflect.Method
 import scala.reflect.ClassTag
 
 object ReflectionUtils {
@@ -39,24 +39,15 @@ object ReflectionUtils {
    * Scala 3 strategy (in order):
    *   1. primitive fast-path (`returnType.isPrimitive`)
    *   2. compile-time metadata from [[MockMetadataCache.getReturnsValueClass]]
-   *   3. conservative JVM fallback:
-   *      - generic `TypeVariable` return => `false` (erased/ambiguous)
-   *      - otherwise `classOf[AnyVal].isAssignableFrom(returnType)`
+   *   3. `false` — if the method is not in the cache it means the macro did not classify it as a value-class return, so it is a plain reference type.
    *
-   * This mirrors Scala 2 intent ("identify returns that require value-like handling") while replacing runtime Scala reflection with compile-time metadata plus conservative runtime
-   * checks.
+   * NOTE: `classOf[AnyVal].isAssignableFrom(returnType)` cannot be used as a fallback in Scala 3 because `classOf[AnyVal]` compiles to `java.lang.Object` on the JVM, making the
+   * check return `true` for every reference type and causing null stubs to be replaced by smart-null proxies.
    */
   private[mockito] def returnsValueClass(invocation: InvocationOnMock): Boolean =
     val method     = invocation.method
     val returnType = method.getReturnType
-    returnType.isPrimitive || MockMetadataCache
-      .getReturnsValueClass(method)
-      .getOrElse {
-        method.getGenericReturnType match {
-          case _: TypeVariable[?] => false
-          case _                  => classOf[AnyVal].isAssignableFrom(returnType)
-        }
-      }
+    returnType.isPrimitive || MockMetadataCache.getReturnsValueClass(method).getOrElse(false)
 
   /**
    * Extract extra interfaces from an intersection/refined type at compile time.

common/src/test/scala-3/org/mockito/ReflectionUtilsTest.scala

@@ -1,6 +1,7 @@
 package org.mockito
 
 import org.mockito.internal.MockMetadataCache
+import org.mockito.invocation.InvocationOnMock
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.wordspec.AnyWordSpec
 
@@ -30,6 +31,14 @@ private[mockito] trait RUT_WithByNameAndFunction0        {
  * NOTE: plain ScalaTest assertions — NOT property-based tests.
  */
 class ReflectionUtilsTest extends AnyWordSpec with Matchers {
+
+  /** Create a minimal InvocationOnMock whose getMethod() returns the given method. */
+  private def makeInvocation(m: java.lang.reflect.Method): InvocationOnMock = {
+    val inv = Mockito.mock(classOf[InvocationOnMock])
+    Mockito.when(inv.getMethod()).thenReturn(m)
+    inv
+  }
+
   private val primitiveMethod          = classOf[RUT_WithPrimitiveBacked].getMethod("id")
   private val referenceMethod          = classOf[RUT_WithReferenceBacked].getMethod("id")
   private val typeMemberMethod         = classOf[RUT_WithTypeMemberBound].getMethod("id")
@@ -79,6 +88,20 @@ class ReflectionUtilsTest extends AnyWordSpec with Matchers {
     "be false for generic AnyVal bound method returning type variable" in {
       MockMetadataCache.getReturnsValueClass(genericBoundMethod) shouldBe Some(false)
     }
+
+    "make ReflectionUtils.returnsValueClass return true via cache" in {
+      // JVM return type of RUT_UserId is String (not primitive) — exercises the cache path.
+      ReflectionUtils.returnsValueClass(makeInvocation(referenceMethod)) shouldBe true
+    }
+
+    "make ReflectionUtils.returnsValueClass return true via primitive fast-path" in {
+      // JVM return type of RUT_IntId is int (primitive) — isPrimitive fires before cache lookup.
+      ReflectionUtils.returnsValueClass(makeInvocation(primitiveMethod)) shouldBe true
+    }
+
+    "make ReflectionUtils.returnsValueClass return false for a method absent from the cache" in {
+      ReflectionUtils.returnsValueClass(makeInvocation(stringReturnMethod)) shouldBe false
+    }
   }
 
   "MockMetadataCache returnType registration" should {

macro-common/src/main/scala-3/org/mockito/internal/MockMetadataCache.scala

@@ -8,9 +8,9 @@ import java.util.concurrent.ConcurrentHashMap
  *
  * Keys and contract:
  *   - `byName`: keyed by declaring `Class[?]`, stores `Seq[(Method, Set[Int])]`:
- *       - outer `Seq`: one entry per method on that class with metadata
- *       - inner `Set[Int]`: zero-based parameter indices that are by-name or vararg for that method
- *         (example: for `def foo(x: => Int, ys: String*, z: () => String)`, indices are `Set(0, 1)`; `z` is plain `Function0`, so excluded)
+ *     - outer `Seq`: one entry per method on that class with metadata
+ *     - inner `Set[Int]`: zero-based parameter indices that are by-name or vararg for that method (example: for `def foo(x: => Int, ys: String*, z: () => String)`, indices are
+ *       `Set(0, 1)`; `z` is plain `Function0`, so excluded)
  *   - `returnsValueClass`: keyed by `Method`, stores whether the declared Scala return type extends `AnyVal`.
  *   - `returnType`: keyed by `Method`, stores the concrete declared return class when JVM erasure gives `Object` but the Scala source type is more specific (e.g. abstract type
  *     aliases, path-dependent types).

macro-common/src/main/scala-3/org/mockito/internal/MockMethodMetadata.scala

@@ -16,7 +16,13 @@ import scala.reflect.ClassTag
  */
 object MockMethodMetadata {
 
-  /** Entry point called from Scala 3 `MockCreator` during mock creation. */
+  /**
+   * Entry point called from Scala 3 `MockCreator` during mock creation.
+   *
+   * `T` '''must be a concrete type''' at the call site — the macro inspects `T`'s methods at compile time. This is guaranteed when every method in the call chain from user code
+   * down to this call is `inline`. If any intermediate method is not `inline`, `T` will be an abstract type variable and the macro will produce no output, leaving the runtime
+   * cache empty for that mock type.
+   */
   inline def registerByNameAndVarArgInfo[T](using classTag: ClassTag[T]): Unit = ${ registerImpl[T]('classTag) }
 
   /** Macro implementation that inspects `T` and emits cache-registration runtime code. */
@@ -68,9 +74,23 @@ object MockMethodMetadata {
         case _          => resultTypeOf(methodType)
       }
 
+    def isPrimitiveType(normalized: TypeRepr): Boolean =
+      normalized =:= TypeRepr.of[Boolean] ||
+        normalized =:= TypeRepr.of[Byte] ||
+        normalized =:= TypeRepr.of[Short] ||
+        normalized =:= TypeRepr.of[Int] ||
+        normalized =:= TypeRepr.of[Long] ||
+        normalized =:= TypeRepr.of[Float] ||
+        normalized =:= TypeRepr.of[Double] ||
+        normalized =:= TypeRepr.of[Char] ||
+        normalized =:= TypeRepr.of[Unit]
+
     def returnsValueClassFor(normalized: TypeRepr): Boolean = {
       val returnSym = normalized.typeSymbol
-      returnSym.isClassDef && returnSym != defn.AnyValClass && (normalized <:< TypeRepr.of[AnyVal])
+      returnSym.isClassDef &&
+      returnSym != defn.AnyValClass &&
+      !isPrimitiveType(normalized) &&
+      (normalized <:< TypeRepr.of[AnyVal])
     }
 
     def returnTypeClassFor(normalized: TypeRepr): Option[Expr[Class[?]]] = {
@@ -109,27 +129,38 @@ object MockMethodMetadata {
     // Phase 1: compile-time analysis of T and its inherited declarations.
     def collectMethodInfos(tpe: TypeRepr, typeSymbol: Symbol): List[MethodInfo] = {
       val allMethods = {
-        val seen = mutable.Set.empty[String] // track by fullName to avoid duplicates
+        // Deduplicate by symbol identity: the same symbol may appear in both `declarations`
+        // and `baseClasses.flatMap(_.declarations)`, so we use the symbol itself as the key.
+        // Using `fullName` would wrongly deduplicate overloaded methods (e.g. `request()`,
+        // `request(String...)` and `request(MediaType...)` all share the same fullName).
+        val seen = mutable.Set.empty[Symbol]
         (typeSymbol.declarations ++ tpe.baseClasses.flatMap(_.declarations))
-          .filter(symbol => symbol.isDefDef && !symbol.isClassConstructor && seen.add(symbol.fullName))
+          .filter(symbol => symbol.isDefDef && !symbol.isClassConstructor && seen.add(symbol))
       }
 
       allMethods.flatMap { methodSym =>
         val methodType           = tpe.memberType(methodSym)
         val isJavaMethod         = methodSym.flags.is(Flags.JavaDefined)
         val params               = collectParams(methodType, 0)
         val byNameOrVarArgFields = params.collect { case param if param.isByName || param.isVarArg => param.index }.toSet
+        val inferredReturnType   = resultTypeOf(methodType).dealias.simplified
         val normalizedReturnType = declaredOrResolvedReturnType(methodSym, methodType).dealias.simplified
-        val jvmParamTypes        = params.map(param => jvmParamTypeExpr(param, isJavaMethod))
-        Some(
-          MethodInfo(
-            methodSym.name,
-            jvmParamTypes,
-            byNameOrVarArgFields,
-            returnsValueClassFor(normalizedReturnType),
-            returnTypeClassFor(normalizedReturnType)
+        val returnsValueClass    = returnsValueClassFor(normalizedReturnType)
+        val returnTypeClassOpt   =
+          if (!(normalizedReturnType =:= inferredReturnType)) returnTypeClassFor(normalizedReturnType)
+          else None
+        val jvmParamTypes = params.map(param => jvmParamTypeExpr(param, isJavaMethod))
+        if (byNameOrVarArgFields.nonEmpty || returnsValueClass || returnTypeClassOpt.nonEmpty)
+          Some(
+            MethodInfo(
+              methodSym.name,
+              jvmParamTypes,
+              byNameOrVarArgFields,
+              returnsValueClass,
+              returnTypeClassOpt
+            )
           )
-        )
+        else None
       }
     }
 
@@ -172,10 +203,8 @@ object MockMethodMetadata {
             resolvedMethodInfos.collect { case (method, indices, _, _) if indices.nonEmpty => (method, indices) }
           if (byNameInfos.nonEmpty) MockMetadataCache.registerByName(clazz, byNameInfos)
 
-          if (resolvedMethodInfos.nonEmpty)
-            MockMetadataCache.registerReturnsValueClass(
-              resolvedMethodInfos.map { case (method, _, returnsValueClass, _) => (method, returnsValueClass) }
-            )
+          val returnsValueClassInfos = resolvedMethodInfos.collect { case (method, _, true, _) => (method, true) }
+          if (returnsValueClassInfos.nonEmpty) MockMetadataCache.registerReturnsValueClass(returnsValueClassInfos)
 
           val returnTypeInfos = resolvedMethodInfos.collect { case (method, _, _, Some(returnTypeClass)) =>
             (method, returnTypeClass)

macro-common/src/test/scala-3/org/mockito/internal/MockMethodMetadataTest.scala

@@ -15,6 +15,15 @@ private trait MMT_MetadataTarget {
   def valueId: MMT_ValueId
 }
 
+/** Trait with overloaded methods that share the same `fullName` — tests dedup-by-symbol fix. */
+private trait MMT_Overloaded {
+  def request(): String
+  def request(path: String): String
+  def request(paths: String*): String  // vararg — must be registered even though earlier overloads exist
+  def request(path: => String): String // by-name — same fullName, different symbol; must also be registered
+  def primitive(): Int
+}
+
 private trait MMT_ExtraA
 private trait MMT_ExtraB
 private trait MMT_ExtraC
@@ -38,6 +47,33 @@ class MockMethodMetadataTest extends AnyWordSpec with Matchers {
     }
   }
 
+  "registerByNameAndVarArgInfo for overloaded methods" should {
+    "register vararg and by-name overloads independently when all share the same fullName" in {
+      MockMethodMetadata.registerByNameAndVarArgInfo[MMT_Overloaded]
+
+      val varargMethod = classOf[MMT_Overloaded].getMethod("request", classOf[Seq[?]])
+      val byNameMethod = classOf[MMT_Overloaded].getMethod("request", classOf[scala.Function0[?]])
+      val byNameInfos  = MockMetadataCache.getByName(classOf[MMT_Overloaded])
+      byNameInfos should not be empty
+      byNameInfos.get.toMap.get(varargMethod) shouldBe Some(Set(0))
+      byNameInfos.get.toMap.get(byNameMethod) shouldBe Some(Set(0))
+    }
+
+    "not classify primitive return types as value-class returns" in {
+      MockMethodMetadata.registerByNameAndVarArgInfo[MMT_Overloaded]
+
+      val primitiveMethod = classOf[MMT_Overloaded].getMethod("primitive")
+      MockMetadataCache.getReturnsValueClass(primitiveMethod) shouldBe None
+    }
+
+    "not store false returnsValueClass entries — only true entries go into the cache" in {
+      MockMethodMetadata.registerByNameAndVarArgInfo[MMT_MetadataTarget]
+
+      val plainMethod = classOf[MMT_MetadataTarget].getMethods.find(_.getName == "plainFunction0").get
+      MockMetadataCache.getReturnsValueClass(plainMethod) shouldBe None
+    }
+  }
+
   "extraInterfacesImpl" should {
     "exclude the primary runtime class and return only extras for intersections" in {
       type Intersection = MMT_ExtraA & MMT_ExtraB & MMT_ExtraC