From 83ae74ce9d3ff56c47b39e44332daa3da2981133 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 16 Jun 2013 14:15:30 -0400 Subject: SI-7584 Fix typer regression with by-name parameter types It regressed in fada1ef6b#L4L614. Partially reverting just this change restores the correct behaviour: ``` - if (sym.isStable && pre.isStable && !isByNameParamType(tree.tpe) && + if (treeInfo.admitsTypeSelection(tree) && ``` This patch embeds the check for by-name parameter types into `TreeInfo.isStableIdentifier`. That code already checks for `Symbol#isStable`, which exludes direct references to by-name parameters. But the additional check is required to deal with by-name parameters in function types, e.g `(=> Int) => Any`. Open question: should we go further and embed this check in `isStable`? Currently: final def isStable = isTerm && !isMutable && !(hasFlag(BYNAMEPARAM)) && (!isMethod || hasStableFlag) Such function types are an underspecified corner of the language, albeit one that is pretty useful writing, for example, in the signature of a lazy foldRight that can operate over infinite structures: def foldRight[A, B](fa: F[A], z: => B)(f: (A, => B) => B): B The next commit subjects them to a little testing. --- src/reflect/scala/reflect/internal/TreeInfo.scala | 9 ++++++++- test/files/pos/t7584.scala | 11 +++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 test/files/pos/t7584.scala diff --git a/src/reflect/scala/reflect/internal/TreeInfo.scala b/src/reflect/scala/reflect/internal/TreeInfo.scala index 3a8d3fd460..a5cf46071f 100644 --- a/src/reflect/scala/reflect/internal/TreeInfo.scala +++ b/src/reflect/scala/reflect/internal/TreeInfo.scala @@ -98,7 +98,7 @@ abstract class TreeInfo { */ def isStableIdentifier(tree: Tree, allowVolatile: Boolean): Boolean = tree match { - case Ident(_) => symOk(tree.symbol) && tree.symbol.isStable && !tree.symbol.hasVolatileType // TODO SPEC: not required by spec + case i @ Ident(_) => isStableIdent(i) case Select(qual, _) => isStableMemberOf(tree.symbol, qual, allowVolatile) && isPath(qual, allowVolatile) case Apply(Select(free @ Ident(_), nme.apply), _) if free.symbol.name endsWith nme.REIFY_FREE_VALUE_SUFFIX => // see a detailed explanation of this trick in `GenSymbols.reifyFreeTerm` @@ -119,6 +119,13 @@ abstract class TreeInfo { typeOk(tree.tpe) && (allowVolatile || !hasVolatileType(tree)) && !definitions.isByNameParamType(tree.tpe) ) + private def isStableIdent(tree: Ident): Boolean = ( + symOk(tree.symbol) + && tree.symbol.isStable + && !definitions.isByNameParamType(tree.tpe) + && !tree.symbol.hasVolatileType // TODO SPEC: not required by spec + ) + /** Is `tree`'s type volatile? (Ignored if its symbol has the @uncheckedStable annotation.) */ def hasVolatileType(tree: Tree): Boolean = diff --git a/test/files/pos/t7584.scala b/test/files/pos/t7584.scala new file mode 100644 index 0000000000..52d127ecb9 --- /dev/null +++ b/test/files/pos/t7584.scala @@ -0,0 +1,11 @@ +object Test { + def fold[A, B](f: (A, => B) => B) = ??? + def f[A, B](x: A, y: B): B = ??? + def bip[A, B] = fold[A, B]((x, y) => f(x, y)) + def bop[A, B] = fold[A, B](f) + + // these work: + fold[Int, Int]((x, y) => f(x, y)) + fold[Int, Int](f) +} + -- cgit v1.2.3 From 9f2b2894ba2b45135943e943d13898aefc333cc2 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 16 Jun 2013 14:47:26 -0400 Subject: SI-7584 Test and spurious warning fix for by-name closures The enclosed test case exercises by-name closures, which were the subject of the previous commit. In the process, a spurious warning was eliminated. --- src/reflect/scala/reflect/internal/TreeInfo.scala | 2 +- test/files/run/t7584.check | 6 ++++++ test/files/run/t7584.flags | 1 + test/files/run/t7584.scala | 14 ++++++++++++++ 4 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t7584.check create mode 100644 test/files/run/t7584.flags create mode 100644 test/files/run/t7584.scala diff --git a/src/reflect/scala/reflect/internal/TreeInfo.scala b/src/reflect/scala/reflect/internal/TreeInfo.scala index a5cf46071f..cd1f5f2e47 100644 --- a/src/reflect/scala/reflect/internal/TreeInfo.scala +++ b/src/reflect/scala/reflect/internal/TreeInfo.scala @@ -208,7 +208,7 @@ abstract class TreeInfo { } def isWarnableSymbol = { val sym = tree.symbol - (sym == null) || !(sym.isModule || sym.isLazy) || { + (sym == null) || !(sym.isModule || sym.isLazy || definitions.isByNameParamType(sym.tpe_*)) || { debuglog("'Pure' but side-effecting expression in statement position: " + tree) false } diff --git a/test/files/run/t7584.check b/test/files/run/t7584.check new file mode 100644 index 0000000000..9f53e5dde5 --- /dev/null +++ b/test/files/run/t7584.check @@ -0,0 +1,6 @@ +no calls +call A +a +call B twice +b +b diff --git a/test/files/run/t7584.flags b/test/files/run/t7584.flags new file mode 100644 index 0000000000..e8fb65d50c --- /dev/null +++ b/test/files/run/t7584.flags @@ -0,0 +1 @@ +-Xfatal-warnings \ No newline at end of file diff --git a/test/files/run/t7584.scala b/test/files/run/t7584.scala new file mode 100644 index 0000000000..6d7f4f7ebb --- /dev/null +++ b/test/files/run/t7584.scala @@ -0,0 +1,14 @@ +// Test case added to show the behaviour of functions with +// by-name parameters. The evaluation behaviour was already correct. +// +// We did flush out a spurious "pure expression does nothing in statement position" +// warning, hence -Xfatal-warnings in the flags file. +object Test extends App { + def foo(f: (=> Int, => Int) => Unit) = f({println("a"); 0}, {println("b"); 1}) + println("no calls") + foo((a, b) => ()) + println("call A") + foo((a, b) => a) + println("call B twice") + foo((a, b) => {b; b}) +} -- cgit v1.2.3