From 22769a67cb7c622a5d0fd7af5e0bc0179b04e661 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Tue, 4 Aug 2026 18:14:40 +0800 Subject: [PATCH] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound ### What changes were proposed in this pull request? Follow-up to #56430, addressing review comments from cloud-fan. 1. The JDK backend rewrote a reference to a class Java cannot name - an anonymous or local class, or a class nested inside one - into its nearest nameable supertype unconditionally. That is only sound when the supertype is itself referenceable and offers every member the generated code could access, and nothing enforced either condition. Such a unit is now routed to Janino instead, joining the two existing deterministic-routing cases (REPL contexts and Scala package-object classes). `canNarrowSafely` decides this. A member matches by its exact erased signature, with an allowance for bridges: an override of a generic method erases narrower than the supertype declaration it implements (`compare(String, String)` against `Comparator.compare(Object, Object)`) and carries a bridge with the supertype's signature, so narrowing keeps dispatching to the override. An overload has no bridge and is rejected - it must be, because `Invoke` codegen wraps every call in an explicit cast, which would hide the resulting type mismatch from javac and silently bind the call to the supertype's method. The replacement type and all its enclosing classes must also be public: same-package is not sufficient, because the generated class is loaded by `InMemoryClassLoader` and its runtime package differs from the same-named package on the application loader. `nameableSupertype` now climbs while `getCanonicalName` is null rather than while the class is anonymous or local, which additionally covers a named class nested inside one of those - neither anonymous nor local itself, yet equally unnameable. 2. Reflection in this path can raise a `LinkageError` when a class loads but a type in its signature does not (a partial or shaded jar). `NonFatal` does not cover that and an escaping `Error` would bypass the codegen fallbacks, so `canNarrowSafely` and `sourceNameOf` catch it and degrade to "cannot narrow" / the binary name. 3. Comment and documentation fixes: two wordings in the `CodeCompiler.active` scaladoc, and the `spark.sql.codegen.compiler` config doc, which enumerated only two always-Janino cases. ### Why are the changes needed? Fixes the unchecked assumption. Without it, an anonymous type whose accessed member is absent from the supertype either fails to compile or - for a same-arity overload - compiles clean and returns the supertype's answer. ### Does this PR introduce _any_ user-facing change? No. The default backend is Janino, and for `spark.sql.codegen.compiler=jdk` this only moves units that javac would have mishandled onto the working path. ### How was this patch tested? Eight cases in `CodeCompilerSuite` covering the `$`-digit gate, narrowable and unnarrowable shapes, an unnameable supertype, a class nested in a local class, the routing decision under both backend settings, an end-to-end compile asserting the JDK backend rejects the unit that narrowing breaks, and its counterpart asserting a bridge-preserved call stays on the JDK backend. Each production check was mutated in turn and the corresponding test fails without it. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5 --- .../expressions/codegen/CodeCompiler.scala | 291 ++++++++++++++---- .../apache/spark/sql/internal/SQLConf.scala | 8 +- .../codegen/CodeCompilerSuite.scala | 199 ++++++++++++ 3 files changed, 442 insertions(+), 56 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompiler.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompiler.scala index 3c7f605b806ba..b69cc6fee0ca1 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompiler.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompiler.scala @@ -18,6 +18,7 @@ package org.apache.spark.sql.catalyst.expressions.codegen import java.io.{ByteArrayInputStream, ByteArrayOutputStream, InputStream, IOException, StringWriter} +import java.lang.reflect.{Method, Modifier} import java.net.{JarURLConnection, URI, URL} import java.util.Locale import java.util.concurrent.{Callable, ExecutionException, ExecutorService} @@ -143,10 +144,11 @@ object CodeCompiler extends Logging { * * The configured backend ([[SQLConf.CODEGEN_COMPILER]]) governs ordinary codegen. The * exception is codegen the JDK compiler is fundamentally *incapable* of compiling - not - * merely slower at - which is always routed to Janino regardless of the configured - * backend. This is deterministic routing decided up front from the execution context and - * the generated source; it is never a fallback after a failed compile. Two such cases, - * both classes the JDK compiler cannot name that Janino's lenient loader/lexer accepts: + * merely slower at compiling - which is always routed to Janino regardless of the + * configured backend. This is deterministic routing decided up front from the execution + * context and the generated source; it is never a fallback after a failed compile. Three + * such cases, all involving classes the JDK compiler cannot name that Janino's lenient + * loader/lexer accepts: * * - REPL / interactive sessions (spark-shell `$line*` wrappers, Spark Connect / * Ammonite session artifacts): reachable only through a runtime class loader and @@ -159,17 +161,31 @@ object CodeCompiler extends Logging { * an identifier in any form - Java has no backtick/escape, unlike Scala - so javac * can neither parse `a.b.package.Inner` nor resolve the flat `a.b.package$Inner`. * See [[requiresJaninoSource]]. + * - A reference to a class the Java language forbids naming - an anonymous or local + * class (`a.b.Outer$1`, `a.b.Outer$$anon$1`) or a class nested inside one + * (`a.b.Outer$1$Inner`) - that cannot be narrowed soundly. The JDK backend rewrites + * such a reference to the nearest nameable supertype, which works only while that + * supertype is itself referenceable and offers every member the generated code could + * access. See [[JdkCodeCompiler.referencesUnnarrowableClass]]. */ def active(code: CodeAndComment): CodeCompiler = { - val requested = SQLConf.get.codegenCompiler - if (requested != JANINO && isReplContext) { + // Resolve the configured backend first: when it is already Janino - by configuration or + // because javac is absent - none of the overrides below can change the outcome, and + // their source scans would be pure overhead. + val configured = forBackend(SQLConf.get.codegenCompiler) + if (configured eq JaninoCodeCompiler) { + configured + } else if (isReplContext) { logReplRoutingOnce() JaninoCodeCompiler - } else if (requested != JANINO && requiresJaninoSource(code)) { + } else if (requiresJaninoSource(code)) { logPackageObjectRoutingOnce() JaninoCodeCompiler + } else if (code != null && JdkCodeCompiler.referencesUnnarrowableClass(code.body)) { + logUnnarrowableClassRoutingOnce() + JaninoCodeCompiler } else { - forBackend(requested) + configured } } @@ -193,6 +209,16 @@ object CodeCompiler extends Logging { log"cannot name). This notice is logged once per JVM.") } } + private val unnarrowableClassRoutingLogged = new java.util.concurrent.atomic.AtomicBoolean(false) + private def logUnnarrowableClassRoutingOnce(): Unit = { + if (unnarrowableClassRoutingLogged.compareAndSet(false, true)) { + logInfo(log"Generated code references a class Java cannot name (anonymous, local, or " + + log"nested in one) that cannot be narrowed to a nameable supertype; that unit is " + + log"routed to Janino although " + + log"${MDC(LogKeys.CONFIG, SQLConf.CODEGEN_COMPILER.key)} requests another backend. " + + log"This notice is logged once per JVM.") + } + } // A `package` segment in a qualified/binary class name - a Scala `package object`'s nested // class such as `a.b.package$Inner`. `package` is a Java reserved word the JDK compiler can @@ -731,58 +757,51 @@ object JdkCodeCompiler extends CodeCompiler with Logging { * prefix resolves (e.g. inner classes of the not-yet-compiled generated unit). */ private def rewriteQualifiedName(token: String, classLoader: ClassLoader): String = { + loadLongestPrefix(token, classLoader) match { + case Some((cls, rest)) => + val sourceName = sourceNameOf(nameableSupertype(cls)) + val resolved = if (rest.isEmpty) sourceName else s"$sourceName.$rest" + // `split('.')` drops a trailing empty segment, so a token that ends in `.` + // (member access wrapped onto the next line) must get its dot restored. + if (token.endsWith(".")) resolved + "." else resolved + case None => + InnerClassRefPattern.replaceAllIn(token, ".") + } + } + + /** + * Find the longest dot-delimited prefix of `token` that loads as a class, and return it + * with the remaining member access. Only a prefix containing `$` is tried, since only + * such a prefix can be a binary inner-class name. Returns None when nothing loads (e.g. + * an inner class of the not-yet-compiled generated unit). + */ + private def loadLongestPrefix( + token: String, + classLoader: ClassLoader): Option[(Class[_], String)] = { val parts = token.split('.') var k = parts.length while (k >= 1) { val prefix = parts.iterator.take(k).mkString(".") - // Only a prefix that itself contains `$` can be a binary inner-class name. if (prefix.indexOf('$') >= 0) { - resolveSourceName(prefix, classLoader) match { - case Some(sourceName) => - val rest = parts.iterator.drop(k).mkString(".") - val resolved = if (rest.isEmpty) sourceName else s"$sourceName.$rest" - // `split('.')` drops a trailing empty segment, so a token that ends in `.` - // (member access wrapped onto the next line) must get its dot restored. - return if (token.endsWith(".")) resolved + "." else resolved + loadWithoutInit(prefix, classLoader) match { + case Some(cls) => return Some((cls, parts.iterator.drop(k).mkString("."))) case None => // try a shorter prefix } } k -= 1 } - InnerClassRefPattern.replaceAllIn(token, ".") + None } - /** - * Load `binaryName` without initializing it and return the source name the JDK - * compiler accepts: the canonical name when it is a plain dotted identifier - * name (regular nesting, e.g. `java.util.Map.Entry`), otherwise the binary - * name itself. The binary name is required for classes nested in Scala objects - * and for Scala companion-object classes, whose canonical form carries a - * module `$` that javac cannot resolve, and for Scala operator-named classes - * (e.g. `scala.collection.immutable.::`) whose canonical form is not a valid - * Java identifier - in all those cases the binary name is itself a legal Java - * type reference. Returns None when the name is not a loadable class. - * - * The canonical name is also rejected when it is not a faithful rename of the - * binary name - it must keep the same package. Scala REPL classes (e.g. - * `$line21.$read$$iw$TestCaseClass`) report a misleading `getCanonicalName` that - * drops the package and returns just the simple name (`TestCaseClass`); using it - * would corrupt the reference into an unqualified one javac cannot resolve. - */ - private def resolveSourceName(binaryName: String, classLoader: ClassLoader): Option[String] = { + /** Load `binaryName` without initializing it; None when it is not a loadable class. */ + private def loadWithoutInit(binaryName: String, classLoader: ClassLoader): Option[Class[_]] = { try { // scalastyle:off classforname // Load with the exact loader passed in (the task's context loader), not the Spark // class loader, so the JDK compiler sees what the runtime would; Utils.classForName // cannot target an arbitrary loader. - val loaded = Class.forName(binaryName, false, classLoader) + Some(Class.forName(binaryName, false, classLoader)) // scalastyle:on classforname - val cls = nameableSupertype(loaded) - val canonical = cls.getCanonicalName - val pkg = cls.getPackageName - val usableCanonical = canonical != null && isPlainDottedName(canonical) && - (pkg.isEmpty || canonical.startsWith(pkg + ".")) - Some(if (usableCanonical) canonical else cls.getName) } catch { case _: ClassNotFoundException | _: LinkageError => None case NonFatal(_) => None @@ -790,20 +809,58 @@ object JdkCodeCompiler extends CodeCompiler with Logging { } /** - * Climb to the nearest class that can be named in Java source. Anonymous and local - * classes (e.g. a Scala `new HashMap[..]() {...}` compiled to `Outer$$anon$1`) have no - * source-referenceable name: the JDK compiler rejects a qualified reference to them - * even when the `.class` file is on the classpath, because the Java language forbids - * naming them. Janino does not - it resolves any class by its runtime binary name - - * so this only matters for the JDK backend. The generated code casts an object to - * this type and then invokes methods declared on it; every such method is inherited - * from the supertype, so the nearest nameable supertype is a sound cast target. For an - * anonymous class implementing an interface (`new Comparator() {...}`, whose superclass - * is `Object`), the implemented interface is preferred over `Object`. + * The source name the JDK compiler accepts for `cls`: the canonical name when it is a + * plain dotted identifier name (regular nesting, e.g. `java.util.Map.Entry`), otherwise + * the binary name. The binary name is required for classes nested in Scala objects and + * for Scala companion-object classes, whose canonical form carries a module `$` that + * javac cannot resolve, and for Scala operator-named classes (e.g. + * `scala.collection.immutable.::`) whose canonical form is not a valid Java identifier - + * in all those cases the binary name is itself a legal Java type reference. + * + * The canonical name is also rejected when it is not a faithful rename of the binary + * name - it must keep the same package. Scala REPL classes (e.g. + * `$line21.$read$$iw$TestCaseClass`) report a misleading `getCanonicalName` that drops + * the package and returns just the simple name (`TestCaseClass`); using it would corrupt + * the reference into an unqualified one javac cannot resolve. + * + * Reflection here can raise a `LinkageError` when the class loaded but its enclosing + * class did not (a partial or shaded jar); `NonFatal` does not cover that, so it is + * caught explicitly and the binary name used, matching what an unresolvable prefix + * yields in [[loadLongestPrefix]]. + */ + private def sourceNameOf(cls: Class[_]): String = { + val canonical = + try cls.getCanonicalName + catch { + case _: LinkageError => null + case NonFatal(_) => null + } + val pkg = cls.getPackageName + val usableCanonical = canonical != null && isPlainDottedName(canonical) && + (pkg.isEmpty || canonical.startsWith(pkg + ".")) + if (usableCanonical) canonical else cls.getName + } + + /** + * Climb to the nearest class that can be named in Java source. A class whose + * `getCanonicalName` is null has no source-referenceable name: the JDK compiler rejects + * a qualified reference to it even when the `.class` file is on the classpath, because + * the Java language forbids naming it. That covers anonymous classes (a Scala + * `new HashMap[..]() {...}` compiles to `Outer$$anon$1`), local classes, and classes + * nested inside either of those (`Outer$1$Inner`), which are themselves neither + * anonymous nor local. Janino does not care - it resolves any class by its runtime + * binary name - so this only matters for the JDK backend. For an anonymous class + * implementing an interface (`new Comparator() {...}`, whose superclass is `Object`), + * the implemented interface is preferred over `Object`. + * + * Narrowing a reference this way is only sound while every member the generated code + * could access remains reachable through the replacement type; a unit referencing a + * class for which that does not hold is routed to Janino instead of being rewritten + * (see [[referencesUnnarrowableClass]] and [[CodeCompiler.active]]). */ private def nameableSupertype(start: Class[_]): Class[_] = { var c: Class[_] = start - while (c != null && (c.isAnonymousClass || c.isLocalClass)) { + while (c != null && c.getCanonicalName == null) { val sup: Class[_] = c.getSuperclass c = if (sup != null && (sup ne classOf[Object])) sup @@ -812,6 +869,134 @@ object JdkCodeCompiler extends CodeCompiler with Logging { if (c == null) classOf[Object] else c } + /** + * True when `body` references a class that [[nameableSupertype]] cannot narrow soundly, + * i.e. the class carries a public member that the replacement type does not offer, or + * the replacement type is itself one javac cannot reference. Such a unit must go to + * Janino: rewriting the reference would either drop the member or emit a type name javac + * rejects. + * + * Called from [[CodeCompiler.active]] on every compile, so it is gated behind a scan for + * `$` followed by a digit. Every class the Java language forbids naming carries that in + * its binary name (`Outer$1`, `Outer$1Local`, Scala's `Outer$$anon$1`, and their nested + * members `Outer$1$Inner`), while the other `$` forms the rewrite handles - regular + * nesting (`Map$Entry`), Scala modules (`Foo$`, `Model$Load$Leaf`), package objects + * (`pkg$Inner`), specialized (`Function1$mcII$sp`) and operator-named (`$colon$colon`) + * classes - do not. Lambdas (`Outer$$Lambda$14/0x...`) do carry it and are neither + * anonymous nor local, but they are inert here: the tokenizer stops at `/`, leaving a + * name no loader can resolve. Janino cannot name them either, so a lambda reference + * never reaches generated source in the first place. + * + * The scan adds one linear pass over the body ahead of the compile-cache lookup, behind + * the intrinsified `$`-digit gate that ordinary generated code fails immediately. + * + * The scan reads the raw body, so a `$`-digit sequence inside a string literal or a + * comment can trigger the resolution attempt. That is harmless: an unloadable token is + * ignored, and a loadable one only ever picks Janino, which accepts a superset of what + * javac does. + */ + private[codegen] def referencesUnnarrowableClass(body: String): Boolean = { + if (!containsDollarDigit(body)) return false + val classLoader = Utils.getContextOrSparkClassLoader + val checked = mutable.HashSet.empty[String] + var i = 0 + val n = body.length + while (i < n) { + if (isNameStart(body.charAt(i))) { + val start = i + i += 1 + while (i < n && isNamePart(body.charAt(i))) i += 1 + val token = body.substring(start, i) + if (containsDollarDigit(token) && checked.add(token) && + loadLongestPrefix(token, classLoader).exists { + case (cls, _) => !canNarrowSafely(cls) + }) { + return true + } + } else { + i += 1 + } + } + false + } + + /** True iff `s` holds a `$` immediately followed by an ASCII digit. */ + private[codegen] def containsDollarDigit(s: String): Boolean = { + var i = s.indexOf('$') + while (i >= 0 && i < s.length - 1) { + val next = s.charAt(i + 1) + if (next >= '0' && next <= '9') return true + i = s.indexOf('$', i + 1) + } + false + } + + /** + * True when a reference to `cls` can be replaced by [[nameableSupertype]] without losing + * access to any member. A class that is already nameable needs no narrowing and always + * qualifies. + * + * Otherwise two things must hold. First, the replacement type must be one the generated + * unit can reference: it and every enclosing class must be public. Same-package is NOT + * sufficient even though javac would accept it - the generated class is defined into + * `org.apache.spark.sql.catalyst.expressions` but loaded by [[InMemoryClassLoader]], so + * its runtime package differs from the same-named package on the app loader and a + * package-private access would fail with `IllegalAccessError` at execution time instead + * of at compile time. Second, every public member of the concrete class - including + * inherited ones, since the generated code may access any of them - must be reachable on + * the replacement type. + * + * A member is matched by its exact erased signature, with one allowance for bridges: an + * override of a generic method has a narrower erasure than the supertype declaration it + * implements (`compare(String, String)` against `Comparator.compare(Object, Object)`), + * and the compiler emits a bridge carrying the supertype's signature. Such a method is + * safe to narrow because `invokevirtual` on the supertype signature still dispatches to + * the override. An overload has no bridge, so it is rejected - and it must be, because + * narrowing binds the call to the supertype's method instead: `Invoke` codegen always + * wraps the call in an explicit cast, which would hide the type mismatch from javac and + * silently produce the wrong result rather than fail to compile. + * + * Reflection over the concrete class can raise a `LinkageError` when a member signature + * names a class the loader cannot find (a partial or shaded jar). `NonFatal` does not + * cover that, and an escaping `Error` would bypass the codegen fallbacks, so it is + * caught here and reported as "cannot narrow" - Janino compiles what javac cannot. + */ + private def canNarrowSafely(cls: Class[_]): Boolean = { + val target = nameableSupertype(cls) + if (cls eq target) return true + if (!isPubliclyNameable(target)) return false + try { + val reachable: Seq[Class[_]] = Seq(target, classOf[Object]) + val targetSignatures = reachable.flatMap(_.getMethods).map(erasedSignature).toSet + val targetFields = reachable.flatMap(_.getFields).map(_.getName).toSet + val methods = cls.getMethods + val bridgedTo = methods.iterator + .filter(m => m.isBridge && targetSignatures.contains(erasedSignature(m))) + .map(m => (m.getName, m.getParameterCount)) + .toSet + methods.forall { m => + targetSignatures.contains(erasedSignature(m)) || + bridgedTo.contains((m.getName, m.getParameterCount)) + } && cls.getFields.forall(f => targetFields.contains(f.getName)) + } catch { + case _: LinkageError => false + case NonFatal(_) => false + } + } + + private def erasedSignature(m: Method): (String, Seq[Class[_]]) = + (m.getName, m.getParameterTypes.toSeq) + + /** True iff `cls` and every class enclosing it are public, i.e. javac can name it. */ + private def isPubliclyNameable(cls: Class[_]): Boolean = { + var c = cls + while (c != null) { + if (!Modifier.isPublic(c.getModifiers)) return false + c = c.getEnclosingClass + } + true + } + /** True iff `s` contains only `[A-Za-z0-9_.]` - a dotted Java identifier path. */ private def isPlainDottedName(s: String): Boolean = { var i = 0 diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala index 8a0d88346b63c..d966b667f27d4 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala @@ -2960,9 +2960,11 @@ object SQLConf { "When 'jdk' is requested but javax.tools.JavaCompiler is unavailable " + "(e.g. JRE-only image) Spark falls back to 'janino' with a warning. " + "Regardless of this setting, codegen in REPL / interactive sessions (spark-shell, " + - "Spark Connect session artifacts) and generated code referencing a class nested in " + - "a Scala package object always compile with 'janino', because the JDK compiler " + - "cannot resolve such classes; a one-time INFO log records each such routing.") + "Spark Connect session artifacts), generated code referencing a class nested in " + + "a Scala package object, and generated code referencing an anonymous or local class " + + "that cannot be rewritten to a nameable supertype always compile with 'janino', " + + "because the JDK compiler cannot resolve such classes; a one-time INFO log records " + + "each such routing.") .version("4.3.0") .withBindingPolicy(ConfigBindingPolicy.SESSION) .stringConf diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompilerSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompilerSuite.scala index 8adb537f1110a..616c8a715cff6 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompilerSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompilerSuite.scala @@ -726,6 +726,146 @@ class CodeCompilerSuite extends SparkFunSuite with SQLHelper { assert(generated.generate(Array[Any](anon)) === "v") } + // ---------------- unnarrowable anonymous/local classes ---------------- + + test("containsDollarDigit gates the scan on names Java cannot spell") { + // The `$`-digit gate must admit every unnameable shape and reject the nameable `$` + // forms the rewrite handles, or the scan either misses a class or runs needlessly. + for (name <- Seq("Outer$1", "Outer$1Local", "Outer$$anon$1", "Outer$1$Inner", + "a.b.Outer$$anon$12")) { + assert(JdkCodeCompiler.containsDollarDigit(name), s"expected $name to be gated in") + } + for (name <- Seq("", "$", "a$", "java.util.Map$Entry", "Foo$", "Model$SaveLoad$Leaf", + "scala.Function1$mcII$sp", "scala.collection.immutable.$colon$colon", + "pkg.package$Inner", "$line21.$read$$iw$T")) { + assert(!JdkCodeCompiler.containsDollarDigit(name), s"expected $name to be gated out") + } + } + + test("referencesUnnarrowableClass: false when the supertype offers every member") { + // The shapes codegen actually produces: the anonymous or local class only overrides + // methods the supertype already declares, so narrowing the reference loses nothing. + val anonSubclass = new java.util.HashMap[String, String]() { put("k", "v") } + val anonInterface = new java.util.Comparator[String] { + override def compare(a: String, b: String): Int = a.compareTo(b) + } + val narrowable = Seq[Any](anonSubclass, anonInterface, CodeCompilerSuite.plainLocal) + for (o <- narrowable) { + val cls = o.getClass + // Guard the fixture's own precondition: a nameable class would pass the assertion + // below for the wrong reason. + assert(cls.getCanonicalName == null, s"expected an unnameable class, got: ${cls.getName}") + val name = cls.getName + assert(!JdkCodeCompiler.referencesUnnarrowableClass(s"$name v = ($name) references[0];"), + s"expected $name to be narrowable") + } + } + + test("referencesUnnarrowableClass: true when narrowing would lose access") { + // An extra public method, public fields inherited from a second interface, a member on + // a second interface, an overload that shadows nothing, and a local class with an extra + // method: each puts something out of reach of the nearest nameable supertype. + val unnarrowable = Seq[Any]( + CodeCompilerSuite.anonWithExtraMethod, + CodeCompilerSuite.anonWithPublicFields, + CodeCompilerSuite.anonWithSecondInterface, + CodeCompilerSuite.anonWithOverload, + CodeCompilerSuite.localWithExtraMethod) + for (o <- unnarrowable) { + val cls = o.getClass + assert(cls.getCanonicalName == null, s"expected an unnameable class, got: ${cls.getName}") + val name = cls.getName + assert(JdkCodeCompiler.referencesUnnarrowableClass(s"$name v = ($name) references[0];"), + s"expected $name to be rejected") + } + } + + test("referencesUnnarrowableClass: true when the supertype itself cannot be named") { + // Members line up here, but the nearest nameable supertype is a private nested class + // (scala.collection.mutable.HashSet$HashSetIterator), so javac could not write the + // narrowed cast at all. + val cls = scala.collection.mutable.HashSet("a").iterator.getClass + assert(cls.getCanonicalName == null, s"expected an unnameable class, got: ${cls.getName}") + val name = cls.getName + assert(JdkCodeCompiler.referencesUnnarrowableClass(s"$name v = ($name) references[0];"), + s"expected $name to be rejected for an unnameable supertype") + } + + test("rewriteInnerClassRefs: narrows a class nested inside a local class") { + assume(JdkCodeCompiler.isAvailable, "javax.tools.JavaCompiler not available") + // A named class declared inside a local class is neither anonymous nor local, but Java + // cannot name it either: its canonical name is null and the binary name is not a legal + // type reference. It must be narrowed to its nameable supertype, not emitted verbatim. + val cls = CodeCompilerSuite.memberOfLocalClass.getClass + assert(!cls.isAnonymousClass && !cls.isLocalClass && cls.isMemberClass, + s"expected a member class of a local class, got: ${cls.getName}") + assert(cls.getCanonicalName == null, s"expected no canonical name for ${cls.getName}") + val name = cls.getName + assert(rewrite(s"$name v = ($name) references[0];") === + "java.util.ArrayList v = (java.util.ArrayList) references[0];") + } + + test("active(code) routes an unnarrowable class reference to Janino") { + assume(JdkCodeCompiler.isAvailable, "javax.tools.JavaCompiler not available") + val narrowableName = (new java.util.HashMap[String, String]() { put("k", "v") }) + .getClass.getName + val unnarrowableName = CodeCompilerSuite.anonWithExtraMethod.getClass.getName + withSQLConf(SQLConf.CODEGEN_COMPILER.key -> "jdk") { + assert(CodeCompiler.active(newCodeAndComment(s"$narrowableName v;")) eq JdkCodeCompiler) + assert(CodeCompiler.active(newCodeAndComment(s"$unnarrowableName v;")) eq JaninoCodeCompiler) + } + withSQLConf(SQLConf.CODEGEN_COMPILER.key -> "janino") { + assert(CodeCompiler.active(newCodeAndComment(s"$unnarrowableName v;")) eq JaninoCodeCompiler) + } + } + + test("JDK backend cannot compile a member access that narrowing drops") { + assume(JdkCodeCompiler.isAvailable, "javax.tools.JavaCompiler not available") + // Both halves of the routing decision: javac rejects the rewritten unit (the reference + // narrows to java.util.HashMap, which has no `extra()`), and `active` therefore hands + // the unit to Janino, which compiles it and produces the right answer. + val anon = CodeCompilerSuite.anonWithExtraMethod + val anonName = anon.getClass.getName + val code = newCodeAndComment( + s""" + |public java.lang.Object generate(Object[] references) { + | $anonName m = ($anonName) references[0]; + | return m.extra(); + |} + """.stripMargin) + intercept[CompileException] { + JdkCodeCompiler.compile(code) + } + withSQLConf(SQLConf.CODEGEN_COMPILER.key -> "jdk") { + assert(CodeCompiler.active(code) eq JaninoCodeCompiler) + val (generated, _) = CodeGenerator.compile(code) + assert(generated.generate(Array[Any](anon)) === "extra") + } + } + + test("JDK backend compiles a call that narrowing preserves through a bridge") { + assume(JdkCodeCompiler.isAvailable, "javax.tools.JavaCompiler not available") + // The counterpart to the test above: an override of a generic method erases narrower + // than the interface declares, but the compiler-emitted bridge keeps dispatch correct, + // so this must stay on the JDK backend rather than being routed away. + val anon = new java.util.Comparator[String] { + override def compare(a: String, b: String): Int = a.compareTo(b) + } + val anonName = anon.getClass.getName + val code = newCodeAndComment( + s""" + |public java.lang.Object generate(Object[] references) { + | $anonName c = ($anonName) references[0]; + | return Integer.valueOf(c.compare("a", "b")); + |} + """.stripMargin) + withSQLConf(SQLConf.CODEGEN_COMPILER.key -> "jdk") { + assert(CodeCompiler.active(code) eq JdkCodeCompiler) + } + val (generated, _) = JdkCodeCompiler.compile(code) + assert(generated.generate(Array[Any](anon)) === -1) + } + // ---------------- Function1 apply(Object) bridge ---------------- test("stripFunction1ApplyBridges removes the bridge but keeps apply(InternalRow)") { @@ -834,4 +974,63 @@ object CodeCompilerSuite { object SaveLoadV1 { case class Leaf(x: Int) } + + private[codegen] trait Greeter { + def hello(): String + } + + private[codegen] abstract class Converter { + def convert(o: Any): String = "base" + } + + // Anonymous and local classes for the narrowing-soundness tests. They are held in vals + // rather than built inline in the tests because scalac keeps an anonymous class's extra + // members `public` only when the binding's type is inferred as the refined type; giving + // the val an explicit type, or passing the expression as `Any`, makes them private. + val anonWithExtraMethod = new java.util.HashMap[String, String]() { + def extra(): String = "extra" + } + + // Mixing in a Java constants interface is what actually yields public FIELDS: a Scala + // `val` compiles to a private field plus an accessor, which only exercises the method + // check. ObjectStreamConstants contributes 30 public static final fields and no method + // beyond Comparator's, so this isolates the field clause of `canNarrowSafely`. + val anonWithPublicFields = new java.util.Comparator[String] with java.io.ObjectStreamConstants { + override def compare(a: String, b: String): Int = a.compareTo(b) + } + + val anonWithSecondInterface = new java.util.Comparator[String] with Greeter { + override def compare(a: String, b: String): Int = a.compareTo(b) + override def hello(): String = "hi" + } + + // An OVERLOAD, not an override: `convert(String)` does not implement `convert(Any)`, so + // no bridge is emitted. Narrowing would silently bind the call to the supertype's method. + val anonWithOverload = new Converter { + def convert(s: String): String = "anon" + } + + val plainLocal: java.util.ArrayList[String] = { + class PlainLocal extends java.util.ArrayList[String] + new PlainLocal + } + + val localWithExtraMethod = { + class LocalWithExtra extends java.util.ArrayList[String] { + def extra(): String = "extra" + } + new LocalWithExtra + } + + // A named class declared inside a local class: neither anonymous nor local itself + // (`isMemberClass` is true), yet Java cannot name it either. Its supertype offers every + // member, so only the canonical-name test keeps it from being narrowed to a binary name + // javac would reject. + val memberOfLocalClass: Any = { + class Holder { + class Inner extends java.util.ArrayList[String] + def make(): Any = new Inner + } + new Holder().make() + } }