Implement inline

[odersky]

• Basic inliner
• Keep original call alongside inlined code
• Print inlining history in error messages
• Support for separate compilation
• Support for outer this references in inlined code
• Create accessors for non-public members accessed from inlined code
• Inline closures that come from an inline method
• Use new inline keyword instead of annotation
• Clarify relation to SIP

Review by @xeno-by

[DarkDimius]

why is inliner a typer? Cant it be made it into a miniphase\part of post-typer?

[odersky]

1. inlining needs to be done during typing, since macro expansion depends on it, and macro expansion needs to e.g. create implicit values.

2. The inliner could possibly be formulated as another TreeMap instead of a full typer. I made it a type primarily for two reasons.

   2.1 That way we get constant folding for free (would have to be implemented explicitly in a treemap).
   2.2 That way we make sure the inlined code is type correct instead of failing during translation.

[odersky]

/rebuild

[odersky]

I think we are done for now. Over to you, @xeno-by!

[xeno-by]

@odersky Congrats with finishing the initial version of this fun project! I'm on vacation together with Mano and Denys until the end of the week, but will do my best to review after I return.

[odersky]

/rebuild

[odersky]

Last two days commits

• add inline function parameters
• contain fixes for inline methods in inline arguments, which are needed for compiling nested fors
• removes InlineInfo in favor of a simpler annotation-based scheme

[smarter]

} Indented two spaces too many

[smarter]

I would be more explicit in the error message proposed solution and say something like write $name instead of $name to keep using it as an identifier

[smarter]

Comment needs to be updated to include inline

[smarter]

write tp @ ExprType(_) since tp1 is not used

[smarter]

Why not?

[smarter]

You don't actually need a temporary variable here, x_=(y); y_=(x) works fine.

[smarter]

Typo: it's -> its

[smarter]

This is only correct for closures created by desugar.makeClosure and only for certain phases, we have similar pattern matches in other places that makes similar assumption and I don't like it either, I think ExpandSAMs#transformBlock is the only one which tries to be careful, maybe we could use that here:

case Block(stats @ (fn: DefDef) :: Nil, Closure(_, fnRef, tpt)) if fnRef.symbol == fn.symbol =>

[odersky]

Not sure about this one. The question is, how should closures in other blocks be categorized? Is that still a closure or something else? For the moment I'd leave as is.

[smarter]

Typo: suported

[smarter]

Typo: dyanmic -> dynamic

[smarter]

Couldn't this be extended to:

if (refPart.symbol.isStatic || refPart.symbol.isContainedIn(inlineMethod.owner) {
  ...
}

This way when we have:

    private val x: Int
    inline def get1 = x

we'll get:

def $inlineAccessor$get1$1: T = C.this.x

instead of:

def $inlineAccessor$get1$1(x$0: Test.C[T]): T = x$0.x

[odersky]

No, because the reference to the symbol might not be via this. So the right check is

    val qual = qualifier(refPart)
    def refIsLocal = qual match {
      case qual: This => qual.symbol == refPart.symbol.owner
      case _ => false
    }
    val (accessorDef, accessorRef) =
      if (refPart.symbol.isStatic || refIsLocal) { ...

[smarter]

Can't we just disable access checks for type members in inlined blocks? We need accessors for term members because of the JVM but type members get erased anyway.

[odersky]

Not true for classes. They don't get erased.

[smarter]

Indeed, but does the JVM restrict access to classes in any way? As far as I know it only restricts access to the constructor of the class, but we take care of that by adding a term accessor for the constructor, not a type accessor.

[odersky]

I thought access to private classes is caught by the JVM

[smarter]

Doesn't seem so, here's some Java code:
Foo.java:

public class Foo {
  public class Inner {
    int get() { return 42; }
  }
  Inner inner = new Inner();
}

Bar.java:

public class Bar {
  public static void main(String[] args) {
    Foo.Inner inner = (new Foo()).inner;
    System.out.println(inner.get());
  }
}

$ javac Foo.java
$ javac Bar.java
$ java Bar
42
$ edit Foo.java, make Inner private
$ javac Foo.java
$ java Bar
42

[odersky]

Inner classes indeed get the private modifier removed. But package inner classes are maintained in the JVM I think

[smarter]

Ah you're right indeed, the following breaks:

package a;

public class Foo {
  public class Inner {
    public int get() { return 42; }
  }
  public Inner inner = new Inner();
}

package b;

public class Bar {
  public static void main(String[] args) {
    a.Foo.Inner inner = (new a.Foo()).inner;
    System.out.println(inner.get());
  }
}

$ javac a/Foo.java
$ javac b/Bar.java
$ java b.Bar
$ Remove public from "public class Inner"
$ javac a/Foo.java
$ java b.Bar
Exception in thread "main" java.lang.IllegalAccessError: tried to access class a.Foo$Inner from class b.Bar
        at b.Bar.main(Bar.java:6)

I don't think adding a type alias is going to help us here since it's just going to get erased, I think we need to mark public in the bytecode all the package inner classes referenced from inline methods

[smarter]

Interestingly, scalac 2.11 and 2.12 always emit private inner classes with ACC_PUBLIC:

package a;

class Foo {
  private class Inner {
    def get = 42
  }
}

But dotty does emit them without ACC_PUBLIC unless we remove private from private class Inner.

Both scalac and dotty will prevent you from leaking a reference to a private class:

package a;

class Foo {
  private class Inner {
    def get = 42
  }
  val inner = new Inner // error: non-private value inner refers to private class Inner
  type Duck = Inner // error: non-private type Duck refers to private class Inner
}

The easiest solution would be to always emit inner classes with ACC_PUBLIC as scalac does, @DarkDimius: do you think that this is reasonable ?

[smarter]

This is the only call to inlineCall and it's in a branch with tree.tpe <:< pt this means that when adaptToSubType is necessary we never inline, for example get isn't inlined in foo:

object Test {
  inline def get: Int = 1

  def foo: Unit = {
    get
  }
}

[odersky]

It turns out we cannot move this test earlier or we get inline errors. I fixed the problem by recursively calling adapt when converting to Unit. I think that's the only place where an inline was missing.

[smarter]

This means that the following doesn't work with -language:Scala2 because inline is treated as an identifier:

inline def x = ...

you have to use @inline instead. We can fix this by only treating INLINE specially if the previous token is AT.

[odersky]

Rebased to master

[smarter]

This is not enough, the Block itself is not inlinable, so adapting the block doesn't help:

object X {
  inline def foo: Int = 1

  def test: Unit = {
    X.foo
  }
}

-Xprint:frontend:

package <empty> {
  final lazy module val X: X$ = new X$()
  final module class X$() extends Object() { this: X.type => 
    @1 @inline() inline def foo: Int = 1
    def test: Unit = 
      {
        {
          X.foo
          ()
        }
      }
  }
}

[odersky]

I have a tendency to merge this now. We already got fine-grained comments, and questions about more fundamental issues might just as well be expressed as separate issues. Since the PR is large it tends to get into conflicts with master quickly. It took me considerable time to rebase everything against current master today.

What do people think?

[smarter]

I think it would be good to get this code in soon, yeah

[odersky]

LGTM, as far as @smarter's commits are concerned.

[smarter]

LGTM too, with the following needing to be addressed as follow-ups:

• inline with -language:Scala: Implement inline #1492 (comment)
• access to non-public types: Implement inline #1492 (comment)