Inliner fails to properly reduce pure case classes in a few situations

[fhackett]

Setup:

case class A(i: Int)
case class B(a: A)

Note: results are paraphrased, removing some of the more verbose types and qualifications. Assume all the code snippets below are inside some arbitrary inline method that has A and B in scope.

The expected result for all cases is:

3

Inline match around type ascriptions

inline (A(3) : A) match {
  case A(i) => i
}

Result:

val $scrutinee1 = A(3) : A
val $elem1 = $scrutinee1.i
val i = $elem1
i

Inline match with nested patterns

inline B(A(3)) match {
  case B(A(i)) => i
}

Result:

val $scrutinee1 = B(A(3))
val $elem1 = A(3) // so clearly it's trying, it just doesn't keep going...
val $elem2 = $elem1.i
val i = $elem2
i

Notice how it keeps $scrutinee1 around despite no references to it, which might be a bug on its own.

Projecting without an inline match

A(3).i

Result:

A(3).i

It's unclear if this last one is by design, but since this kind of compile-time case class manipulation/destructuring feels like constant folding (and basically is), I've included it.

[fhackett]

I found an extra case under the same theme:

When the macro is accessed via the cake pattern

trait Cake {
  case class A(i: Int)
  inline def dummy: Int =
    inline A(3) match {
      case A(i) => i
    }
}
object Test extends Cake {
  println(code"${ dummy }")
}

Result:

val Cake_this : Test.type = this
{
  val $scrutinee1 = Cake_this.A(3)
  val i : 3 = 3 // notice the extraction does actually work...
                // problem is a lot of stuff is left after
  i
}

Note: the same macro definitions reduce properly when accessed directly.

[fhackett]

I've discovered that the cases where $scrutinee stayed in place are not what I thought.

Actually, there is not a single case where the construction of A is elided. It's actually the .show pretty printer deciding it can hide all the redundant code some times and not others. If you inspect even the simple case (inline A(3) match { case A(i) => i }) at the bytecode level all the calls are actually still there.

The reason for this is the purity check - the case class constructions are considered impure and thus not considered for removal by dropUnusedDefs.

Given my interest in this issue is precisely because I want all that redundant code to go away, I will try to change that and see what happens.

That said, I'm almost done fixing the other logic bugs causing most of the cases in this issue to not even appear to work. For ease of reviewing I will probably make 2 PRs, one with the little inliner logic fixes and one with whatever comes of my attempts at tweaking the purity checker.

I'm learning all this as I go, so comments / criticism are welcome (especially if I missed some tricky internal compiler semantics!).

[fhackett]

One more comment before I turn in for the night: what I found from some experiments with the purity checking code.

It seems safe to add one extra case to exprPurity. Specifically, extending the Apply case with an adaptation of the same case class checking code from the inliner.

I found an existing case that covers direct constructor calls to classes that satisfy isNoInitsClass (which was helping the inliner skip direct constructor calls only). I could not directly add to that case as that also checks exprPurity(fn), which turns out to be trivially false for X.apply. Because of all the information we have about this specific apply method though: synthetic, case class itself has no constructor; we can expect the case class to follow convention and, as far as I can tell, the purity check on fn can be skipped entirely.

Perhaps this does not need 2 PRs in the end - it's much less of a breaking change than I feared.

TODOs I'm leaving for tomorrow:

• the cake pattern case
• projections without an inline match (maybe that will also not be too hard / breaking and I can just put it in the PR for comment)

[fhackett]

I take back my previous update about just declaring case class applications to be pure... they're not, because accessing the apply method can cause object initialisation. 😢

So in actuality this change shouldn't be touching the purity checker - it should be special-casing these purity checks only for the inliner. As such I'll be refactoring my change into the inliner, calling it isEffectivelyPureExpr or something similar.

On the bright side, once the inliner thinks case class application is pure (and no-one else does!) all the other problems in this issue seem to have working fixes. The cake pattern case is fixed by the case class application special case, and causing case class projections to inline without a match was not a particularly problematic change.

[fhackett]

PR is up: #8348. Not sure from whom to request feedback.

Since you labeled this issue, any comments @odersky?