Skip to content

Fix #8250: Allow Expr.betaReduce to drop type ascriptions. #8251

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

Merged
merged 1 commit into from
Feb 11, 2020
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -2042,24 +2042,44 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
}}
val argVals = argVals0.reverse
val argRefs = argRefs0.reverse
def rec(fn: Tree): Tree = fn match {
def rec(fn: Tree, topAscription: Option[TypeTree]): Tree = fn match {
case Typed(expr, tpt) =>
// we need to retain any type ascriptions we see and:
// a) if we succeed, ascribe the result type of the ascription to the inlined body
// b) if we fail, re-ascribe the same type to whatever it was we couldn't inline
// note: if you see many nested ascriptions, keep the top one as that's what the enclosing expression expects
rec(expr, topAscription.orElse(Some(tpt)))
case Inlined(call, bindings, expansion) =>
// this case must go before closureDef to avoid dropping the inline node
cpy.Inlined(fn)(call, bindings, rec(expansion))
cpy.Inlined(fn)(call, bindings, rec(expansion, topAscription))
case closureDef(ddef) =>
val paramSyms = ddef.vparamss.head.map(param => param.symbol)
val paramToVals = paramSyms.zip(argRefs).toMap
new TreeTypeMap(
val result = new TreeTypeMap(
oldOwners = ddef.symbol :: Nil,
newOwners = ctx.owner :: Nil,
treeMap = tree => paramToVals.get(tree.symbol).map(_.withSpan(tree.span)).getOrElse(tree)
).transform(ddef.rhs)
topAscription match {
case Some(tpt) =>
// we assume the ascribed type has an apply that has a MethodType with a single param list (there should be no polys)
val methodType = tpt.tpe.member(nme.apply).info.asInstanceOf[MethodType]
Copy link
Contributor

Choose a reason for hiding this comment

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

@fhackett won't this crash if the ascribed type has a val apply member that's not a method? I think this could happen if the term passed as fn is not really a function, or if the apply is overloaded. Something like:

scala> object test extends (String => String) { val apply = (x:Int) => x; def apply(x: String) = x }
// defined object test

scala> test.apply("abc")
val res0: String = abc

scala> test.apply
val res1: Int => Int = test$$$Lambda$1268/578362229@6f3bd37f

Copy link
Contributor

Choose a reason for hiding this comment

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

This case is only reached if the prefix is an explicit closure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I had missed the closureDef pattern. But it seems even a Closure can have a type that contains an overloaded apply member: indeed it can be any SAM type, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, I will have to check if we can get one of those here. @LPTK did you have a specific example in mind?

Copy link
Contributor

@LPTK LPTK Feb 11, 2020

Choose a reason for hiding this comment

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

Something like betaReduce('{ (x => x) : Test }, ...) where we have:

abstract class Test extends (String => String) { val apply = 123 }

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 tried it, and we get a class cast exception.
I can figure out a fix - should I push a second commit here or open a second issue+PR?

Copy link
Contributor Author

@fhackett fhackett Feb 11, 2020

Choose a reason for hiding this comment

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

Actually, I can do one "better". See this fun edge case:

abstract class Test extends (Int => Int) {
  def impl(i: Int): Int
  def apply(i: Int): Int = ???
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is getting more involved than just a quick fix, so I'll open a fresh issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Continued on #8290

// result might contain paramrefs, so we substitute them with arg termrefs
val resultTypeWithSubst = methodType.resultType.substParams(methodType, argRefs.map(_.tpe))
Typed(result, TypeTree(resultTypeWithSubst).withSpan(fn.span)).withSpan(fn.span)
case None =>
result
}
case tpd.Block(stats, expr) =>
seq(stats, rec(expr)).withSpan(fn.span)
seq(stats, rec(expr, topAscription)).withSpan(fn.span)
case _ =>
fn.select(nme.apply).appliedToArgs(argRefs).withSpan(fn.span)
val maybeAscribed = topAscription match {
case Some(tpt) => Typed(fn, tpt).withSpan(fn.span)
case None => fn
}
maybeAscribed.select(nme.apply).appliedToArgs(argRefs).withSpan(fn.span)
}
seq(argVals, rec(fn))
seq(argVals, rec(fn, None))
}

/////////////
Expand Down
6 changes: 6 additions & 0 deletions tests/neg-macros/beta-reduce-inline-result.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@

-- [E007] Type Mismatch Error: tests/neg-macros/beta-reduce-inline-result/Test_2.scala:11:41 ---------------------------
11 | val x2: 4 = Macros.betaReduce(dummy1)(3) // error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| Found: Int
| Required: (4 : Int)
10 changes: 10 additions & 0 deletions tests/neg-macros/beta-reduce-inline-result/Macro_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import scala.quoted._

object Macros {
inline def betaReduce[Arg,Result](inline fn: Arg=>Result)(inline arg: Arg): Result =
${ betaReduceImpl('{ fn })('{ arg }) }

def betaReduceImpl[Arg,Result](fn: Expr[Arg=>Result])(arg: Expr[Arg])(using qctx: QuoteContext): Expr[Result] =
Expr.betaReduce(fn)(arg)
}

13 changes: 13 additions & 0 deletions tests/neg-macros/beta-reduce-inline-result/Test_2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

object Test {

inline def dummy1: Int => Int =
(i: Int) => i + 1

inline def dummy2: Int => Int =
(i: Int) => ???

val x1: Int = Macros.betaReduce(dummy1)(3)
val x2: 4 = Macros.betaReduce(dummy1)(3) // error
}

5 changes: 5 additions & 0 deletions tests/run-macros/beta-reduce-inline-result.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
compile-time: 4
run-time: 4
compile-time: 1
run-time: 1
run-time: 5
16 changes: 16 additions & 0 deletions tests/run-macros/beta-reduce-inline-result/Macro_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import scala.quoted._

object Macros {
inline def betaReduce[Arg,Result](inline fn : Arg=>Result)(inline arg: Arg): Result =
${ betaReduceImpl('{ fn })('{ arg }) }

def betaReduceImpl[Arg,Result](fn: Expr[Arg=>Result])(arg: Expr[Arg])(using qctx : QuoteContext): Expr[Result] =
Expr.betaReduce(fn)(arg)

inline def betaReduceAdd1[Arg](inline fn: Arg=>Int)(inline arg: Arg): Int =
${ betaReduceAdd1Impl('{ fn })('{ arg }) }

def betaReduceAdd1Impl[Arg](fn: Expr[Arg=>Int])(arg: Expr[Arg])(using qctx: QuoteContext): Expr[Int] =
'{ ${ Expr.betaReduce(fn)(arg) } + 1 }
}

33 changes: 33 additions & 0 deletions tests/run-macros/beta-reduce-inline-result/Test_2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import scala.compiletime._

object Test {

inline def dummy1: Int => Int =
(i: Int) => i + 1

inline def dummy2: (i: Int) => i.type =
(i: Int) => i

inline def dummy3: Int => Int =
(i: Int) => ???

inline def dummy4: Int => Int =
???

def main(argv : Array[String]) : Unit = {
println(code"compile-time: ${Macros.betaReduce(dummy1)(3)}")
println(s"run-time: ${Macros.betaReduce(dummy1)(3)}")
println(code"compile-time: ${Macros.betaReduce(dummy2)(1)}")
// paramrefs have to be properly substituted in this case
println(s"run-time: ${Macros.betaReduce(dummy2)(1)}")

// ensure the inlined ??? is ascribed type Int so this compiles
def throwsNotImplemented1 = Macros.betaReduceAdd1(dummy3)(4)
// ensure we handle cases where the (non-inlineable) function itself needs ascribing
def throwsNotImplemented2 = Macros.betaReduce(dummy4)(6)

// make sure paramref types work when inlining is not possible
println(s"run-time: ${Macros.betaReduce(Predef.identity)(5)}")
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have a simple neg test tests/neg-macros/beta-reduce-inline-result/Test_2.scala (using a copy of beta-reduce-inline-result/Macro_1.scala) to check that the elaboration is not changed.

import scala.compiletime._

object Test {

  inline def dummy: Int => Int =
    (i: Int) => i + 1
  
  val x1: Int = Macros.betaReduce(dummy)(3)
  val x2: 4   = Macros.betaReduce(dummy)(3) // error
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will add this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny detail: this test actually passes already because the inline expansion adds its own type ascription. I'll look for a test that properly exercises the case where betaReduce's own type ascription is needed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... actually @LPTK 's test should handle this. Thanks @LPTK 👍