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

fhackett · 2020-02-08T01:07:17Z

When a blackbox inline def is inlined, a type ascription is added to preserve the apparent return type of the inline def. Prior to this commit, the quoted.Expr.betaReduce function was unable to properly inline a known lambda expression coming from an inline def as it would see the type ascription and give up.

This change instead makes betaReduce drop type ascriptions, allowing these two features to interact properly.

Testing is done by verifying that betaReduce can correctly reduce the application of the result of an inline def to a constant value, with the reduction result being printed using the compiletime.code interpolator.

Consideration: dropping type ascriptions might change observable typing behaviour in some edge cases - it certainly changes one of the existing test results (see diff below). While you will certainly get some type ascription back as this code can only be run from the body of an inline method (which will put back an ascription at the end), I'm not sure if that's good enough.

Since the proposed change is so small I figured I would start a discussion by submitting this PR.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

sjrd · 2020-02-08T04:03:21Z

Consideration: dropping type ascriptions might change observable typing behaviour in some edge cases - it certainly changes one of the existing test results (see diff below). While you will certainly get some type ascription back as this code can only be run from the body of an inline method (which will put back an ascription at the end), I'm not sure if that's good enough.

The crucial question is: can it change the elaboration of code calling that inline def? If yes (even in one edge case), then it is not acceptable.

nicolasstucki · 2020-02-08T08:01:52Z

There is one type ascription that should critically not be dropped. But that one is around the code inserted by the macro expansion. The fire this should not affect it. I will double check it.

nicolasstucki · 2020-02-08T08:19:42Z

tests/run-macros/beta-reduce-inline-result/Test_2.scala

+    println(code"${Macros.betaReduce(dummy)(3)}")
+  }
+}
+


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 }

Agreed, I will add this.

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...

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

nicolasstucki · 2020-02-08T08:26:15Z

compiler/src/dotty/tools/dotc/tastyreflect/ReflectionCompilerInterface.scala

@@ -2043,6 +2043,8 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
    val argVals = argVals0.reverse
    val argRefs = argRefs0.reverse
    def rec(fn: Tree): Tree = fn match {
+      case Typed(expr, tpt) =>
+        rec(expr)


We should probably get the return type out from tpt and add it as an ascription to the result. Even it the other tests pass, this would preserve the original typing of the inner expressions.

Yes, that sounds good. See my comment below for more thoughts (which I'll try when I get back to coding this).

It might be somthing like

Typed(rec(expr), TypeTree(tpt.tpe.finalResult))

Thanks for the hint - I did not know about finalResult.

LPTK · 2020-02-08T22:35:01Z

In Scala, in general you can't remove type ascriptions and expect the code to still type check.

Here is a small variation on your test:

import scala.quoted._

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

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

import scala.compiletime._

object Test {

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

  def main(argv : Array[String]) : Unit = {
    println(code"${Macros.betaReduce(dummy)(3)}")
  }
}

It results in:

-- Error: tests/run-macros/beta-reduce-inline-result/Test_2.scala:9:43 -------------------------------------------------
9 |    println(code"${Macros.betaReduce(dummy)(3)}")
  |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |            undefined: {
  |              {
  |                {
  |                  {
  |                    ???
  |                  }
  |                }
  |              }
  |            }.+ # 10336: TermRef(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Nothing),+) at typer
  | This location contains code that was inlined from Macro_1.scala:9

fhackett · 2020-02-08T23:34:39Z

Thanks for the counter-examples. Clearly we need to replace the type ascription rather than remove it entirely - I'm currently trying to figure out how to compute the correct type to put back. Since we can't expect the ascribed type tree to be exactly AppliedType(Functionx, List(arg1, ..., result)), I've been looking up compiler internals I can use to reliably get the right type even when type abstractions are involved.

At the moment the strategy I'll be testing when I get back to coding this is to project out the tpt's apply method's MethodType, then catch the return type from there.

I'll also look for some more test cases with unusual ascribed type trees that refer to a function that can be beta-reduced.

nicolasstucki · 2020-02-09T06:52:09Z

Don't mind to much the member selection issue. Though adding the type ascription back will probably fix it. That is a more general issue (#7110).

LPTK · 2020-02-09T11:07:28Z

@nicolasstucki is it really a special case of #7110? Solving #7110 would not necessarily solve this. Note that field selections are not the only things that can break when dropping type ascriptions. Another example is ascription to regain a transitive subtyping relationship that's otherwise not known to Dotty.

@fhackett you should also watch out for/test with dependent function types.

fhackett · 2020-02-09T12:53:24Z

@LPTK I'll make sure to include tests for that.

fhackett · 2020-02-09T12:56:40Z

@nicolasstucki I actually had no idea about the member selection issue, I just didn't know about Type.finalResult, which I will try rather than the construction with apply that I mentioned before. Interesting to learn about the other issue though.

When a blackbox inline def is inlined, a type ascription is added to preserve the apparent return type of the inline def. Prior to this commit, the quoted.Expr.betaReduce function was unable to properly inline a known lambda expression coming from an inline def as it would see the type ascription and give up. This change makes betaReduce rewrite type ascriptions, allowing it to inline type-ascribed functions such as those coming from inlined defs. - When type ascriptions are not present, behaviour does not change. - When type ascriptions are encountered, the outermost ascription is kept. - If the inline is successful, then the result type of the ascribed function type is ascribed to the inlined body. - If the inline is not successful, the outermost ascribed type is re-ascribed to the non-inlineable expression before inserting a call to that expression. There are several test cases, covering: - verifying that betaReduce can correctly reduce the application of the result of an inline def to a constant value, with the reduction result being printed using the `compiletime.code` interpolator. - verifying that the result type ascription handles dependent function types - inserting ??? as either the inlined body or a function expression that cannot be inlined compiles without error (by re-ascribing the apparent type) - a neg-test that a function whose type is `Int=>Int` and whose body inlines to `4` does not typecheck against the singleton type `4`.

fhackett · 2020-02-10T23:55:31Z

Here is a new version taking all we've discussed so far (and some other fun details/edge cases) into account.

nicolasstucki · 2020-02-11T10:22:48Z

Thank you @fhackett

LPTK · 2020-02-11T10:54:03Z

compiler/src/dotty/tools/dotc/tastyreflect/ReflectionCompilerInterface.scala

+        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]


@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

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

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?

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

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

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

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?

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 = ??? }

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

Continued on #8290

dottybot reviewed Feb 8, 2020

View reviewed changes

fhackett mentioned this pull request Feb 8, 2020

Expr.betaReduce can't reduce past the type ascription from inline def expansion #8250

Closed

nicolasstucki reviewed Feb 8, 2020

View reviewed changes

nicolasstucki linked an issue Feb 8, 2020 that may be closed by this pull request

Expr.betaReduce can't reduce past the type ascription from inline def expansion #8250

Closed

nicolasstucki assigned nicolasstucki and fhackett Feb 8, 2020

nicolasstucki reviewed Feb 8, 2020

View reviewed changes

fhackett force-pushed the fhackett-fix-8250 branch from d95f8f9 to 0c34994 Compare February 10, 2020 23:54

fhackett requested a review from nicolasstucki February 11, 2020 00:03

nicolasstucki approved these changes Feb 11, 2020

View reviewed changes

nicolasstucki merged commit a29178c into scala:master Feb 11, 2020

LPTK reviewed Feb 11, 2020

View reviewed changes

fhackett deleted the fhackett-fix-8250 branch February 11, 2020 17:05

anatoliykmetyuk added this to the 0.23.0-RC1 milestone Mar 12, 2020

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

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

Uh oh!

Conversation

fhackett commented Feb 8, 2020

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

sjrd commented Feb 8, 2020

Uh oh!

nicolasstucki commented Feb 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LPTK commented Feb 8, 2020

Uh oh!

fhackett commented Feb 8, 2020

Uh oh!

nicolasstucki commented Feb 9, 2020

Uh oh!

LPTK commented Feb 9, 2020

Uh oh!

fhackett commented Feb 9, 2020

Uh oh!

fhackett commented Feb 9, 2020

Uh oh!

fhackett commented Feb 10, 2020

Uh oh!

nicolasstucki commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LPTK Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhackett Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LPTK Feb 11, 2020 •

edited

Loading

fhackett Feb 11, 2020 •

edited

Loading