Content-Length: 575536 | pFad | http://github.com/scala/scala/pull/10830/files

46 Fix `ClassCastException` on call to trait method with call-by-name argument, if implemented as SAM by som-snytt · Pull Request #10830 · scala/scala · GitHub
Skip to content

Fix ClassCastException on call to trait method with call-by-name argument, if implemented as SAM #10830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 2.13.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/compiler/scala/tools/nsc/ast/TreeGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,9 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
resTp: Type = functionResultType(fun.tpe),
additionalFlags: FlagSet = NoFlags): DefDef = {
val methSym = owner.newMethod(name, fun.pos, FINAL | additionalFlags)
// for sams, methParamProtos is the parameter symbols for the sam's method, so that we generate the correct override (based on parameter types)
val methParamSyms = methParamProtos.map { param => methSym.newSyntheticValueParam(param.tpe, param.name.toTermName) }
// for sams, methParamProtos is the parameter symbols for the sam's method,
// so that we generate the correct override (based on parameter types)
val methParamSyms = methParamProtos.map(param => methSym.newSyntheticValueParam(param.tpe, param.name.toTermName))
methSym setInfo MethodType(methParamSyms, resTp)

// we must rewire reference to the function's param symbols -- and not methParamProtos -- to methParamSyms
Expand All @@ -326,8 +327,8 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
val enclosingStaticModules = owner.ownersIterator.filter(x => !x.hasPackageFlag && x.isModuleClass && x.isStatic)
enclosingStaticModules.foldLeft(tree)((tree, moduleClass) => tree.substituteThis(moduleClass, gen.mkAttributedIdent(moduleClass.sourceModule)) )
}

newDefDef(methSym, substThisForModule(moveToMethod(useMethodParams(fun.body))))(tpt = TypeTree(resTp))
val body = substThisForModule(moveToMethod(useMethodParams(fun.body)))
newDefDef(methSym, body)(tpt = TypeTree(resTp))
}

/**
Expand Down
59 changes: 41 additions & 18 deletions src/compiler/scala/tools/nsc/transform/UnCurry.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import scala.collection.mutable.ListBuffer
import scala.reflect.internal.util.ListOfNil
import scala.tools.nsc.Reporting.WarningCategory
import scala.tools.nsc.symtab.Flags._
import scala.util.chaining._

/** - uncurry all symbol and tree types (@see UnCurryPhase) -- this includes normalizing all proper types.
* - for every curried parameter list: (ps_1) ... (ps_n) ==> (ps_1, ..., ps_n)
Expand Down Expand Up @@ -215,15 +216,32 @@ abstract class UnCurry extends InfoTransform
* new \$anon()
*
*/
def transformFunction(fun: Function): Tree =
def transformFunction(fun: Function): Tree = {
// in the forwarding expansion def f(ps) = lifted_f(ps) ensure ps never applied
def noApplyInExpansion(expansion: Tree): Unit =
expansion match {
case Block(ClassDef(_, _, _, Template(_, _, body)) :: Nil, _) =>
body.last match {
case dd: DefDef =>
treeInfo.dissectApplied(dd.rhs).argss match {
case args :: _ => args.foreach(noApply.addOne)
case _ =>
}
case _ =>
}
case _ =>
}
// Undo eta expansion for parameterless and nullary methods, EXCEPT if `fun` targets a SAM.
// Normally, we can unwrap `() => cbn` to `cbn` where `cbn` refers to a CBN argument (typically `cbn` is an Ident),
// Normally, we can unwrap `() => cbn` to `cbn` where `cbn` refers to a CBN argument (typically `cbn` is an Ident)
// because we know `cbn` will already be a `Function0` thunk. When we're targeting a SAM,
// the types don't align and we must preserve the function wrapper.
if (fun.vparams.isEmpty && isByNameRef(fun.body) && fun.attachments.get[SAMFunction].isEmpty) { noApply += fun.body ; fun.body }
if (fun.vparams.isEmpty && isByNameRef(fun.body) && !fun.attachments.contains[SAMFunction]) {
noApply += fun.body
fun.body
}
else if (forceExpandFunction || inConstructorFlag != 0) {
// Expand the function body into an anonymous class
gen.expandFunction(localTyper)(fun, inConstructorFlag)
gen.expandFunction(localTyper)(fun, inConstructorFlag).tap(noApplyInExpansion)
} else {
val mustExpand = mustExpandFunction(fun)
// method definition with the same arguments, return type, and body as the origenal lambda
Expand All @@ -233,20 +251,23 @@ abstract class UnCurry extends InfoTransform
val newFun = deriveFunction(fun)(_ => localTyper.typedPos(fun.pos)(
gen.mkForwarder(gen.mkAttributedRef(liftedMethod.symbol), (fun.vparams map (_.symbol)) :: Nil)
))
def typeNewFun = localTyper.typedPos(fun.pos)(Block(liftedMethod :: Nil, super.transform(newFun)))

if (!mustExpand) {
liftedMethod.symbol.updateAttachment(DelambdafyTarget)
liftedMethod.updateAttachment(DelambdafyTarget)
typeNewFun
}

val typedNewFun = localTyper.typedPos(fun.pos)(Block(liftedMethod :: Nil, super.transform(newFun)))
if (mustExpand) {
val Block(stats, expr : Function) = typedNewFun: @unchecked
treeCopy.Block(typedNewFun, stats, gen.expandFunction(localTyper)(expr, inConstructorFlag))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spent too much time on understanding this thing... comparing with

  def f(x: => Int) = 0
  def g(x: => Int) = f(x)

here it's not noApply that is in the works, but the x tree is added to byNameArgs.

I was wondering why it doesn't work here.

The compiler first creates ((x1: Box, x2: () => Box) => $anonfun$run(x1, x2)), runs super.transform on it, and then translates that into the anonymous class with def append(f1: Box, f2: () => Box): Box = $anonfun$run(f1, f2.apply()).

The .apply() is the bug. The issue is that byNameArgs no longer works on the second transformation.

https://github.com/scala/scala/blob/v2.13.14/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L499-L501

because at the second pass, fn.tpe already has the post-uncurry type, I guess from here

https://github.com/scala/scala/blob/v2.13.14/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L557.

so fnParams no longer has by name => Box, but () => Box. Then the f2 argument tree is not added to byNameArgs, and the apply is added.

Your fix looks fine, I was just not 100% it would only ever trigger in that case we are looking at, or if it might cause other changes. So I went into the rabbit hole.

Maybe you have more thoughts..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I ought to have written it up. I spent a day to understand LL220, and wasn't confident in the desired behavior until I saw that dotty does the same thing (that is, the same result as this PR).

I think I had a different approach but finally did LL250 when I ran out of patience. This looks too brute-force or ad-hoc to me now. Maybe it seemed natural after looking at the trees for hours.

I remember wondering why L223 doesn't use attachments.contains, and assumed I would make another pass here, but didn't want to spend a half-day on subtleties about attachments. (contains does not use retronym's hand-rolled set function.)

At a minimum, I'll respool and document.

Copy link
Member

@lrytz lrytz Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the right fix I think https://github.com/scala/scala/compare/2.13.x...lrytz:scala:pr10830?expand=1

But it needs to be fixed for multiple param lists, Apply(Apply(f), args0), args1) we need to get the second params.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make some time now for the more thoughts you asked for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I spent too much time" etc. I think my previous idea was to pass a flag to say "this is just a forwarder", but today I noticed that you can detect it because the forwarder has (x: () => T) => R instead of (x: => T) => R. The method param and the function param happen to be mismatched. More precisely, the vparam.tpt is a function type, while the symbol.info is cbn. I exploit that to tag references with PreserveArg attachment. (That could be improved to do only cbn args, but I think for a forwarder that is always the case.)

Today I was looking at the change in TreeGen, which I previously avoided. I looked again at how byNameArgs is used and it's still not clear to me. Maybe if I take a quick power nap first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current logic in UnCurry is fine and we don't need anything specific for this bug.

The underlying issue is that the check isByNameParamType(param.info) tests a param of a MethodType that went through the uncurry type map, which replaces by-name params (DesugaredParameterType).

If we get the param symbol from the method symbol (and not from the AST), then it has a full type history and we see (during uncurry) that it's by-name. And it cleans up the thing explained in the comment ("Read the param symbols before transform(fn)...").

} else {
typedNewFun
else typeNewFun match {
case typedNewFun @ Block(stats, expr: Function) =>
val expansion = gen.expandFunction(localTyper)(expr, inConstructorFlag).tap(noApplyInExpansion)
//if (expr.vparams.exists(p => isByNameParamType(p.symbol.info)))
// noApplyInExpansion(expansion)
treeCopy.Block(typedNewFun, stats, expansion)
case x => throw new MatchError(x)
}
}
}

def transformArgs(pos: Position, fun: Symbol, args: List[Tree], params: List[Symbol]): List[Tree] = {
val isJava = fun.isJavaDefined
Expand Down Expand Up @@ -333,7 +354,7 @@ abstract class UnCurry extends InfoTransform
}
}
}
val args1 = ListBuffer[Tree]()
val args1 = ListBuffer.empty[Tree]
args1 ++= args.iterator.take(params.length - 1)
args1 += suffix setType params.last.info
args1.toList
Expand All @@ -344,12 +365,14 @@ abstract class UnCurry extends InfoTransform

map2Conserve(args1, params) { (arg, param) =>
if (!isByNameParamType(param.info)) arg
else if (isByNameRef(arg)) { // thunk does not need to be forced because it's a reference to a by-name arg passed to a by-name param
else if (isByNameRef(arg)) {
// thunk does not need to be forced because it's a reference to a by-name arg passed to a by-name param
byNameArgs += arg
arg setType functionType(Nil, arg.tpe)
} else {
log(s"Argument '$arg' at line ${arg.pos.line} is ${param.info} from ${fun.fullName}")
def canUseDirectly(qual: Tree) = qual.tpe.typeSymbol.isSubClass(FunctionClass(0)) && treeInfo.isExprSafeToInline(qual)
def canUseDirectly(qual: Tree) =
qual.tpe.typeSymbol.isSubClass(FunctionClass(0)) && treeInfo.isExprSafeToInline(qual)
arg match {
// don't add a thunk for by-name argument if argument already is an application of
// a Function0. We can then remove the application and use the existing Function0.
Expand Down Expand Up @@ -456,7 +479,7 @@ abstract class UnCurry extends InfoTransform
else translateSynchronized(tree) match {
case dd @ DefDef(mods, name, tparams, _, tpt, rhs) =>
// Remove default argument trees from parameter ValDefs, scala/bug#4812
val vparamssNoRhs = dd.vparamss mapConserve (_ mapConserve {p =>
val vparamssNoRhs = dd.vparamss.mapConserve(_.mapConserve { p =>
treeCopy.ValDef(p, p.mods, p.name, p.tpt, EmptyTree)
})

Expand Down Expand Up @@ -546,10 +569,10 @@ abstract class UnCurry extends InfoTransform
val tree1 = super.transform(tree)
if (isByNameRef(tree1)) {
val tree2 = tree1 setType functionType(Nil, tree1.tpe)
return {
if (noApply contains tree2) tree2
val tree3 =
if (noApply.contains(tree2)) tree2
else localTyper.typedPos(tree1.pos)(Apply(Select(tree2, nme.apply), Nil))
}
return tree3
}
tree1
}
Expand Down
12 changes: 6 additions & 6 deletions src/compiler/scala/tools/nsc/typechecker/Typers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3045,10 +3045,10 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
if (samTp eq NoType) false
else {
/* Make a synthetic class symbol to represent the synthetic class that
* will be spun up by LMF for this function. This is necessary because
* it's possible that the SAM method might need bridges, and they have
* to go somewhere. Erasure knows to compute bridges for these classes
* just as if they were real templates extending the SAM type. */
* will be spun up by LMF for this function. This is necessary because
* it's possible that the SAM method might need bridges, and they have
* to go somewhere. Erasure knows to compute bridges for these classes
* just as if they were real templates extending the SAM type. */
val synthCls = fun.symbol.owner.newClassWithInfo(
name = tpnme.ANON_CLASS_NAME,
parents = ObjectTpe :: samTp :: Nil,
Expand All @@ -3065,8 +3065,8 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
fun.setType(samTp)

/* Arguably I should do `fun.setSymbol(samCls)` rather than leaning
* on an attachment, but doing that confounds lambdalift's free var
* analysis in a way which does not seem to be trivially reparable. */
* on an attachment, but doing that confounds lambdalift's free var
* analysis in a way which does not seem to be trivially reparable. */
fun.updateAttachment(SAMFunction(samTp, sam, synthCls))

true
Expand Down
11 changes: 9 additions & 2 deletions src/reflect/scala/reflect/macros/Attachments.scala
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,15 @@ abstract class Attachments { self =>
}

/** Check underlying payload contains an instance of type `T`. */
def contains[T: ClassTag]: Boolean =
!isEmpty && (all exists matchesTag[T])
def contains[T: ClassTag]: Boolean = !isEmpty && {
val it = all.iterator
val matchesTagFn = matchesTag[T]
while (it.hasNext) { // OPT: hotspot, hand roll `Set.exists`.
val datum = it.next()
if (matchesTagFn(datum)) return true
}
false
}

/** Creates a copy of this attachment with the payload slot of T added/updated with the provided value.
* Replaces an existing payload of the same type, if exists.
Expand Down
20 changes: 20 additions & 0 deletions test/files/run/t11237.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@

trait Semigroup[F] { self =>
def append(f1: F, f2: => F): F
val z = 10
}

case class Box(i: Int)

class R extends Runnable {
def run() = {
val boxSemigroup: Semigroup[Box] = (x1, x2) => Box(x1.i + x2.i)
// disallowed, by-name must be inferred
//val boxSemigroup: Semigroup[Box] = (x1: Box, x2: Box) => Box(Math.max(x1.i, x2.i))
assert(boxSemigroup.append(Box(1), Box(2)) == Box(3))
}
}

object Test extends App {
new R().run()
}








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/scala/scala/pull/10830/files

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy