Skip to content
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

Fix #8530: Support inline unapply #8542

Merged
merged 7 commits into from Apr 2, 2020
Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Mar 13, 2020

Support the following kind of use case

object MyPattern {
  inline def unapply(x: T): Option[U] = Some(x)
}
x match 
  case MyPattern(y) => ....

where the call to MyPattern.unapply will get inline and possibly be a macro.

The pattern is transformed into a new anonymous class that implements the same unapply. Then the inner call to MyPattern.unnaply is inlined.

x match 
  case <new { def unapply(x: T) = MyPattern.unnaply(x) }>(y) => ....

This anonymous class is optimized away in inlinedPatterns after patternMatcher.

It supports

  • All kinds of unapplys
  • Whitebox inline

@nicolasstucki nicolasstucki self-assigned this Mar 13, 2020
@nicolasstucki nicolasstucki force-pushed the fix-#8530 branch 6 times, most recently from d857d94 to 9c62e41 Compare March 13, 2020 21:55
@nicolasstucki nicolasstucki force-pushed the fix-#8530 branch 3 times, most recently from 3f3a3da to c98e14c Compare March 14, 2020 08:53
@nicolasstucki nicolasstucki marked this pull request as ready for review March 14, 2020 09:33
@nicolasstucki nicolasstucki linked an issue Mar 14, 2020 that may be closed by this pull request
@odersky
Copy link
Contributor

odersky commented Mar 19, 2020

I did not get yet the motivation and approach for this change.

  • In what scenarios do you need an inline unapply?
  • In what scenarios do you not want to inline the unapply?
  • Could Implement inline override #8543 be used to make an unapply both inline and retained at runtime?

@odersky odersky assigned nicolasstucki and unassigned odersky Mar 19, 2020
@nicolasstucki
Copy link
Contributor Author

  • In what scenarios do you need an inline unapply?

When we want to implement it with a macro. In that case, one would recover some statically known information such as

  • StingContext
  • Precise Type of the scrutinee: Known subtypes, type refinements, ..
  • Contextual information: mirrors or other context parameters

With that information, we could either generate inlined unapplies that:

  • Type the extracted arguments precisely (with whitebox)
  • Generate fast specialized variant of the unapply

For example the unapply of xml"..." should parse the XML and type each splice precisely.

  • In what scenarios do you not want to inline the unapply?

I do not understand this question. Is that related to the old transparent unapply?

Yes, this change is orthogonal to #8543 and can be composed with retained inline unapply.

@odersky
Copy link
Contributor

odersky commented Mar 20, 2020

In what scenarios do you not want to inline the unapply?

I do not understand this question. Is that related to the old transparent unapply?

No, I mean, why not just make the unapply inline, and that's it? Why do you need the anonymous class trick? I guessed it's so that you have one version of the unapply which you can call at runtime in case you cannot inline (but I might be wrong). If my guess is correct then I believe one should investigate whether #8543 can be tweaked to do the same. (I.e. make inline unapplies automatically retained). Then we would not need the anonymous classes.

@odersky odersky assigned nicolasstucki and unassigned odersky Mar 20, 2020
@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Mar 20, 2020

It is not related with the usecase of #8543. When we inline the unapply at the call site we need to place it in the tree. It is placed in the fun of an Unapply tree. We place it in an anonymous as a placeholder until it reaches the PatternMatcher phase. Then we could eliminate it.

We cannot really place the expansion as it in the pattern because the patterns are unaware of the scrutinee. That happens knowledge is gained in PatternMatcher.In the case Some(x) is the inlined code from the example

x match 
  case <Some(x)>(y) => ....

The first thing we need is to abstract over the scrutinee. A simple way to do it is with a lambda that receives the scrutinee.

x match 
  case <(x:T) => Some(x)>(y) => ....

This works, but we have to compose it with polymorphic and context function types to support unapplys with type parameters and implicit. This does not really scale well and the implementation becomes a bit complex.

But with this previous approach, the encoding for a call to unapply and unapplySeq are identical which means that we won't know what to do in PatternMatcher.

If we wrap the expansion in a method instead (in a class), then the signature of this method will match the original. This allows PatternMatcher to handle the tree as is and simplifies considerably the logic to generate the placeholder. In this case we would generate:

x match 
  case <(new { def unapply(x: T) = Some(x) }).unapply>(y) => ....

This implies that the Unapply tree has a fun that is actually a call to unapply.

Then PatternMatcher will transform the code into something like

val s = x
val x$1 = new { def unapply(x: T) = Some(x) }.unapply(s)
if x$1.isDefined then
  val y = x$1.get
  ...

where new { def unapply(x: T) = Some(x) }.unapply(s) can be optimized to Some(y). The phase InlinedPatterns will do this optimization.

val s = x
val x$1 = Some(s)
if x$1.isDefined then
  val y = x$1.get
  ...

@odersky
Copy link
Contributor

odersky commented Mar 21, 2020

I still don't understand. The generated code looks much worse than either the non-inlined code, or an imagined inlined code.

What I am missing: How does the inliner come into play here? I understand that the rhs of the unapply in the anonymous class is inlined. But what does that buy us? I think I need to see an end-to-end example where the inline unapply does something useful. Assume a reader who knows nothing about the matter and go from first principles. Otherwise we will end up with code in the compiler that nobody will understand.

@odersky odersky removed their assignment Mar 21, 2020
@nicolasstucki
Copy link
Contributor Author

Now I added a phase inlinePatterns that removes the anonymous classes. I also updated the comments in the code with the details.

result of tests/run/i8530-b.scala after typer:
...
        val myRegexp1: MyRegex[("foo" : String)] = new MyRegex["foo".type]()
        "foo" match {
            case
              {
                final class $anon() extends Object() {
                  def unapplySeq(s: CharSequence): Option[List[String]] = {
                      (if s.==("foo") then Some.apply[scala.collection.immutable.Nil.type](Nil)
                       else None):Option[List[String]]
                  }
                }
                new Object {...}()
              }()
              =>
              ()
        }
...

result of tests/run/i8530-b.scala after MegaPhase{elimOpaque, tryCatchPatterns, patternMatcher, explicitOuter, explicitSelf, stringInterpolatorOpt, crossCast}:
...
        val myRegexp1: MyRegex[("foo" : String)] = new MyRegex[("foo" : String)]()
        matchResult1[Unit]:
          {
            case val x1: ("foo" : String) = "foo"
            {
              case val x2: Option[List[String]] =
                {
                  final class $anon() extends Object() {
                    def unapplySeq(s: CharSequence): Option[List[String]] =
                      {
                        (if s.==("foo") then Some.apply[scala.collection.immutable.Nil.type](Nil)
                         else None):Option[List[String]]
                      }
                  }
                  new Object {...}()
                }.unapplySeq(x1)
              if x2.isEmpty.unary_! then
                ...
              else ()
            }
            throw new MatchError(x1)
          }
...

result of tests/run/i8530-b.scala after MegaPhase{pruneErasedDefs, inlinePatterns, vcInlineMethods, seqLiterals, intercepted, getters, elimByName, liftTry, collectNullableFields, elimOuterSelect, augmentScala2Traits, resolveSuper, functionXXLForwarders, paramForwarding, genericTuples, arrayConstructors}:
...
        val myRegexp1: MyRegex[("foo" : String)] = new MyRegex[("foo" : String)]()
        matchResult1[Unit]:
          {
            case val x1: ("foo" : String) = "foo"
            {
              case val x2: Option[List[String]] =
                {
                  (if x1.==("foo") then Some.apply[scala.collection.immutable.Nil.type](Nil)
                   else None):Option[List[String]]
                }
              if x2.isEmpty.unary_! then
                ...
              else ()
            }
            throw new MatchError(x1)
          }
      }
...

@nicolasstucki
Copy link
Contributor Author

@odersky, apart from taking care of the review comments I also made BetaReduction reduce more trees. This change deserves an extra review.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Changes to beta reduction LGTM

@odersky odersky merged commit 7ae44d4 into scala:master Apr 2, 2020
@odersky odersky deleted the fix-#8530 branch April 2, 2020 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline unapply patterns
2 participants