Fix #5107: disallow block as Apply.fun #5190

liufengyun · 2018-10-02T08:01:56Z

Fix #5107: disallow block as Apply.fun

smarter · 2018-10-02T09:21:03Z

This should make part of https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/Splitter.scala unnecessary (it could be simplified even more if you also disallowed If trees as Apply.fun).

smarter · 2018-10-02T09:22:39Z

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

-        else readaptSimplified(tpd.Apply(tree, args))
+        else tree match {
+          case tree: Block =>
+            readaptSimplified(tpd.Block(tree.stats, tpd.Apply(tree.expr, args)))


What if tree.expr is a block itself? This is handled by https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/Splitter.scala

Just added an assert in tpd.Apply, it seems this can never happen.

smarter · 2018-10-02T09:23:06Z

compiler/src/dotty/tools/dotc/transform/TreeChecker.scala

@@ -300,6 +301,12 @@ class TreeChecker extends Phase with SymTransformer {
      res
    }

+    def checkApplyNonBlock(tree: untpd.Tree)(implicit ctx: Context) = tree match {
+      case tree: untpd.Apply => assert(!tree.fun.isInstanceOf[untpd.Block])


This is a really lightweight check, so instead of doing in TreeChecker I would just do it in the constructor of Apply.

smarter · 2018-10-02T10:20:35Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

    ta.assignType(untpd.Apply(fn, args), fn, args)
+  }

  def TypeApply(fn: Tree, args: List[Tree])(implicit ctx: Context): TypeApply =
    ta.assignType(untpd.TypeApply(fn, args), fn, args)


You could try to do the same for TypeApply.

Yes, we could do that. I'm wondering how if/else could possibly appear in fun. But it is always good to have these invariants if they don't incur much performance cost.

You will probably not be able to. You could add the assertion to make sure.

liufengyun · 2018-10-02T12:01:33Z

test performance please

dottybot · 2018-10-02T12:01:40Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2018-10-02T13:41:50Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5190/ to see the changes.

Benchmarks is based on merging with master (2b73f9d)

liufengyun · 2018-10-02T14:24:35Z

test performance please

dottybot · 2018-10-02T14:26:33Z

performance test scheduled: 4 job(s) in queue, 1 running.

dottybot · 2018-10-02T17:34:33Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5190/ to see the changes.

Benchmarks is based on merging with master (e4735d2)

nicolasstucki

Otherwise LGTM

nicolasstucki · 2018-10-18T19:59:43Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

    ta.assignType(untpd.Apply(fn, args), fn, args)
+  }

  def TypeApply(fn: Tree, args: List[Tree])(implicit ctx: Context): TypeApply =
    ta.assignType(untpd.TypeApply(fn, args), fn, args)


You will probably not be able to. You could add the assertion to make sure.

nicolasstucki · 2018-10-18T20:02:07Z

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -69,8 +69,7 @@ class Compiler {
         new ExplicitOuter,          // Add accessors to outer classes from nested ones.
         new ExplicitSelf,           // Make references to non-trivial self types explicit as casts
         new StringInterpolatorOpt,  // Optimizes raw and s string interpolators by rewriting them to string concatentations
-         new CrossCastAnd,           // Normalize selections involving intersection types.
-         new Splitter) ::            // Expand selections involving union types into conditionals


The splitter source file was nod removed

smarter · 2018-10-18T20:52:42Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

@@ -40,8 +40,11 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
  def Super(qual: Tree, mixName: TypeName, inConstrCall: Boolean, mixinClass: Symbol = NoSymbol)(implicit ctx: Context): Super =
    Super(qual, if (mixName.isEmpty) untpd.EmptyTypeIdent else untpd.Ident(mixName), inConstrCall, mixinClass)

-  def Apply(fn: Tree, args: List[Tree])(implicit ctx: Context): Apply =
+  def Apply(fn: Tree, args: List[Tree])(implicit ctx: Context): Apply = {
+    assert(!fn.isInstanceOf[Block])


Just a suggestion, but you could be even more radical and assert what fn can be instead of what it cannot be, is assert(fn.isInstanceOf[RefTree] || fn.isInstanceOf[GenericApply]) enough ?

Same for TypeApply.

liufengyun · 2018-10-19T11:21:16Z

Thanks @nicolasstucki @smarter . I just updated the PR, could you have a look again?

allanrenucci · 2018-10-19T11:25:05Z

compiler/src/dotty/tools/dotc/ast/tpd.scala


-  def TypeApply(fn: Tree, args: List[Tree])(implicit ctx: Context): TypeApply =
+  def TypeApply(fn: Tree, args: List[Tree])(implicit ctx: Context): TypeApply = {
+    assert(fn.isInstanceOf[RefTree] || fn.isInstanceOf[GenericApply[_]])


Can we have nested TypeApply? Should we check fn.isInstanceOf[Apply[_]] instead of fn.isInstanceOf[GenericApply[_]]?

With named type arguments yes I think so foo[A=Int][String]

... also with curried type lambdas.

smarter · 2018-10-19T11:24:47Z

compiler/test/dotty/tools/dotc/transform/PatmatExhaustivityTest.scala

@@ -17,7 +17,7 @@ import vulpix.TestConfiguration
 class PatmatExhaustivityTest {
  val testsDir = "tests/patmat"
  // stop-after: patmatexhaust-huge.scala crash compiler


Does it still crash the compiler with the new labelled blocks ?

Just tried locally, it works now.

Edit: CI not happy, reverted.

liufengyun requested a review from allanrenucci October 2, 2018 09:18

liufengyun assigned allanrenucci Oct 2, 2018

smarter reviewed Oct 2, 2018

View reviewed changes

liufengyun force-pushed the fix-5107 branch 2 times, most recently from 8b60d82 to 34711c2 Compare October 2, 2018 11:38

liufengyun force-pushed the fix-5107 branch from f6ff366 to 0a2fe87 Compare October 7, 2018 18:47

liufengyun assigned odersky and unassigned allanrenucci Oct 8, 2018

liufengyun requested review from odersky and removed request for allanrenucci October 8, 2018 19:39

nicolasstucki approved these changes Oct 18, 2018

View reviewed changes

smarter reviewed Oct 18, 2018

View reviewed changes

liufengyun force-pushed the fix-5107 branch 2 times, most recently from 5994cca to 28c2cba Compare October 19, 2018 06:53

allanrenucci reviewed Oct 19, 2018

View reviewed changes

smarter approved these changes Oct 19, 2018

View reviewed changes

liufengyun force-pushed the fix-5107 branch 2 times, most recently from 863c058 to 756c7ea Compare October 19, 2018 11:38

Fix scala#5107: disallow block as Apply.fun

9616be1

liufengyun force-pushed the fix-5107 branch from 756c7ea to 9616be1 Compare October 19, 2018 12:18

nicolasstucki approved these changes Oct 19, 2018

View reviewed changes

odersky approved these changes Oct 20, 2018

View reviewed changes

odersky merged commit 661ed61 into scala:master Oct 20, 2018

liufengyun deleted the fix-5107 branch October 20, 2018 08:00

Fix #5107: disallow block as Apply.fun #5190

Fix #5107: disallow block as Apply.fun #5190

Uh oh!

Conversation

liufengyun commented Oct 2, 2018

Uh oh!

smarter commented Oct 2, 2018

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 commented Oct 2, 2018

Uh oh!

dottybot commented Oct 2, 2018

Uh oh!

dottybot commented Oct 2, 2018

Uh oh!

liufengyun commented Oct 2, 2018

Uh oh!

dottybot commented Oct 2, 2018

Uh oh!

dottybot commented Oct 2, 2018

Uh oh!

nicolasstucki 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!

Choose a reason for hiding this comment

Uh oh!

liufengyun commented Oct 19, 2018

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 Oct 19, 2018 • 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!

Uh oh!

liufengyun Oct 19, 2018 •

edited

Loading