Various small improvements to prepare for transparent methods #4620

odersky · 2018-06-05T11:53:15Z

Supersedes #4589.

This PR contains some fixes accumulated while working on inlining untyped trees. It does not contain any code to inline, nor pickle untyped trees. We are currently considering an alternative design by @gsps and @OlivierBlanvillain which would not need to do this.

Main changes:

Regularization of modifier structure: Mods no longer add any new information, are kept
only as carriers of positions for syntactic analysis in IDE and elsewhere.
Improvements in debug and printing options.
Drop inline as a modifier for function parameters, use by-name instead.
Fixes to inliner to make the previous change work, and in general to do a better job
reducing the amount of code that's generated.

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! ☀️

allanrenucci · 2018-06-05T11:58:39Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+  private def paramBindingDef(name: Name, paramtp: Type, arg: Tree,
+                              bindingsBuf: mutable.ListBuffer[ValOrDefDef]): ValOrDefDef = {
+    val argtpe = arg.tpe.dealias
+    def isByName = paramtp.dealias.isInstanceOf[ExprType]


Should be a val. It is always evaluated

smarter · 2018-06-05T18:41:43Z

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

+      (flags, annots.reverse, privateWithin)
+    }
+
+    private val readAnnot: Context => Annotation = {


Why a val and not a def?

We use it as a closure everytime we read a symbol. Making it a val avoids a lot of allocations.

smarter · 2018-06-05T18:47:21Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

-          super.typedApply(tree, pt)
+    override def typedApply(tree: untpd.Apply, pt: Type)(implicit ctx: Context) = {
+
+      def betaReduce(tree: Tree) = tree match {


Would be nice to document this method since it's non-trivial and the name is a bit vague.

I thought beta-reduce is as specific as it gets? 😄

Just joking. I agree the details of this should be documented.

smarter · 2018-06-05T18:53:11Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+    override def typedApply(tree: untpd.Apply, pt: Type)(implicit ctx: Context) = {
+
+      def betaReduce(tree: Tree) = tree match {
+        case Apply(Select(cl @ closureDef(ddef), nme.apply), args) =>


If the closure is a SAM type then there is no guarantee that the implement method is called apply, in fact it's possible that apply is a completely different method that has nothing to do with ddef so this optimization is incorrect in general, e.g.:

trait Foo { def apply(x: Int): Int = 42 def realApply(x: Int): Int } object Test { def main(args: Array[String]): Unit = { println(((x => x): Foo).apply(10)) } }

Right. We need to restrict this to function closures.

Use combination of Enum and Case instead.

It's useful to know something was an enum when it started, and it's necessary to pickle this when serializing untyped trees.

- Mark splices in code under -print-debug - Print owners of all definitions under -print-debug-owners

- some fixes to format docs - some refactorings for better legibility

Everything should be reflected in flags and privateWithin already.

Avoid creation of `op1` methods if tracing is not enabled.

…fter typer

Rely on call-by-name parameters instead. Fix "unused defs" logic so that referred-to cbn parameters are not eliminated.

Since all Mod values are now reflected in flags and privateWithin, there is no need anymore to test whether a Mod exists.

liufengyun

A small comment about doc, otherwise, LGTM

liufengyun · 2018-06-06T08:27:50Z

compiler/src/dotty/tools/dotc/core/Flags.scala

@@ -563,6 +563,9 @@ object Flags {
  /** An inline parameter */
  final val InlineParam = allOf(Inline, Param)

+  /** An enum case */
+  final val EnumCase = allOf(Enum, Case)


The documentation for Enum needs to be updated:

/** Symbol is a Java enum */ final val Enum = commonFlag(40, "<enum>")

That's done in the latest commit.

Also, polishing and new test

smarter · 2018-06-06T12:14:51Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+       *  refs among the ei's directly without creating an intermediate binding.
+       */
+      def betaReduce(tree: Tree) = tree match {
+        case Apply(Select(cl @ closureDef(ddef), nme.apply), args) if defn.isFunctionType(cl.tpe) =>


defn.isFunctionType(cl.tpe)

A more efficient way to check this is cl.tpt.isEmpty

smarter · 2018-06-06T12:16:13Z

Too late for this PR now, but I think we really need more tests to make sure inline behave as we think it does (to not repeat issues such as #4614). We currently only have one test in https://github.com/lampepfl/dotty/blob/master/compiler/test/dotty/tools/backend/jvm/InlineBytecodeTests.scala but it's fairly easy to add more.

dottybot reviewed Jun 5, 2018

View reviewed changes

allanrenucci reviewed Jun 5, 2018

View reviewed changes

This was referenced Jun 5, 2018

Towards Inlining Untyped Code #4589

Closed

Add transparent methods #4622

Merged

smarter requested changes Jun 5, 2018

View reviewed changes

odersky requested a review from allanrenucci June 5, 2018 19:41

odersky assigned allanrenucci and liufengyun and unassigned allanrenucci Jun 5, 2018

odersky requested a review from liufengyun June 5, 2018 19:42

odersky added 15 commits June 6, 2018 10:28

Piggyback unrelated test case

2a6d77d

Drop EnumCase modifier

69c7712

Use combination of Enum and Case instead.

Liberate Enum flag so that it can be used for Scala enums as well.

ee5bfed

Re-use Enum flag for Scala enums

aa2c63c

Pickle Enum flag

9733479

It's useful to know something was an enum when it started, and it's necessary to pickle this when serializing untyped trees.

More debug options for printing

c73a21b

- Mark splices in code under -print-debug - Print owners of all definitions under -print-debug-owners

Tweaks to Tasty pickling and unpickling

401e1d9

- some fixes to format docs - some refactorings for better legibility

Fix pickler printing of TERMREFin, TYPEREFin

0e82b01

Enforce that mods in Modifiers don't add new information

2796d11

Everything should be reflected in flags and privateWithin already.

Don't forget Mode.Printing in RefinedPrinter

559306e

Streamline trace implementation

087cf9f

Avoid creation of `op1` methods if tracing is not enabled.

Relax assertion to allow printing trees with overloaded denotations a…

5e4a340

…fter typer

Don't treat inline closures specially.

66727e3

Rely on call-by-name parameters instead. Fix "unused defs" logic so that referred-to cbn parameters are not eliminated.

Drop Modifiers.hasMod

5c74218

Since all Mod values are now reflected in flags and privateWithin, there is no need anymore to test whether a Mod exists.

Address reviewer comment on previous PR

8800d74

odersky force-pushed the prepare-transparent branch from c169f27 to cd3e7e3 Compare June 6, 2018 08:30

liufengyun approved these changes Jun 6, 2018

View reviewed changes

Bump Tasty version

d2c19ce

Also, polishing and new test

odersky force-pushed the prepare-transparent branch from cd3e7e3 to d2c19ce Compare June 6, 2018 09:13

Address review comments

0c7ef13

odersky merged commit efb8115 into scala:master Jun 6, 2018

allanrenucci deleted the prepare-transparent branch June 6, 2018 12:12

smarter reviewed Jun 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Various small improvements to prepare for transparent methods #4620

Various small improvements to prepare for transparent methods #4620

Uh oh!

odersky commented Jun 5, 2018 •

edited

Loading

Uh oh!

dottybot left a comment

Uh oh!

allanrenucci Jun 5, 2018

Uh oh!

smarter Jun 5, 2018

Uh oh!

odersky Jun 6, 2018

Uh oh!

smarter Jun 5, 2018

Uh oh!

odersky Jun 6, 2018

Uh oh!

odersky Jun 6, 2018

Uh oh!

smarter Jun 5, 2018

Uh oh!

odersky Jun 6, 2018

Uh oh!

liufengyun left a comment

Uh oh!

liufengyun Jun 6, 2018

Uh oh!

odersky Jun 6, 2018

Uh oh!

smarter Jun 6, 2018

Uh oh!

smarter commented Jun 6, 2018

Uh oh!

Uh oh!

Various small improvements to prepare for transparent methods #4620

Various small improvements to prepare for transparent methods #4620

Uh oh!

Conversation

odersky commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

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!

liufengyun left a comment

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!

smarter commented Jun 6, 2018

Uh oh!

Uh oh!

odersky commented Jun 5, 2018 •

edited

Loading