From 784cdcbbf336785ebaab996606076423e4221506 Mon Sep 17 00:00:00 2001 From: extempore Date: Mon, 5 Sep 2011 00:11:29 +0000 Subject: [PATCH] Offer warning when demonstrably non-side-effecting expressions appear in statement position, which should be unintentional by definition. Threw in removal of six places with useless discarded expressions which the warning informed me about. No review. git-svn-id: http://lampsvn.epfl.ch/svn-repos/scala/scala/trunk@25608 5e8d7ff9-d8ef-0310-90f0-a4852d11357a --- .../scala/reflect/internal/Constants.scala | 2 ++ .../scala/reflect/internal/TreeInfo.scala | 8 ++++++ .../tools/nsc/ast/parser/MarkupParsers.scala | 6 ++--- .../scala/tools/nsc/typechecker/Namers.scala | 3 +-- .../scala/tools/nsc/typechecker/Typers.scala | 14 +++++++++++ .../scala/tools/nsc/util/Statistics.scala | 1 - .../scala/collection/JavaConversions.scala | 25 ++++++++++--------- src/library/scala/xml/dtd/ContentModel.scala | 1 - src/swing/scala/swing/LayoutContainer.scala | 1 - test/files/neg/scopes.check | 4 +++ test/files/neg/stmt-expr-discard.check | 7 ++++++ test/files/neg/stmt-expr-discard.flags | 1 + test/files/neg/stmt-expr-discard.scala | 5 ++++ test/files/neg/t1181.check | 4 +++ test/files/neg/t2078.scala | 2 +- test/files/neg/t2641.check | 18 ++++++------- test/files/neg/t2641.scala | 3 +-- test/files/neg/t278.check | 4 +-- test/files/neg/t278.scala | 2 +- test/files/neg/t2910.check | 2 +- test/files/neg/t2910.scala | 1 + test/files/neg/t4166.check | 2 +- test/files/neg/t4166.scala | 4 +-- test/files/neg/t4419.check | 2 +- test/files/neg/t4419.scala | 2 +- test/files/neg/unit-returns-value.check | 5 +++- test/files/neg/variances.scala | 2 +- test/files/run/repl-bare-expr.check | 18 +++++++++++++ test/files/run/repl-parens.check | 18 +++++++++++++ 29 files changed, 123 insertions(+), 44 deletions(-) create mode 100644 test/files/neg/stmt-expr-discard.check create mode 100644 test/files/neg/stmt-expr-discard.flags create mode 100644 test/files/neg/stmt-expr-discard.scala diff --git a/src/compiler/scala/reflect/internal/Constants.scala b/src/compiler/scala/reflect/internal/Constants.scala index c498bc0bd..96188e9e9 100644 --- a/src/compiler/scala/reflect/internal/Constants.scala +++ b/src/compiler/scala/reflect/internal/Constants.scala @@ -55,6 +55,8 @@ trait Constants extends api.Constants { def isLongRange: Boolean = ByteTag <= tag && tag <= LongTag def isFloatRange: Boolean = ByteTag <= tag && tag <= FloatTag def isNumeric: Boolean = ByteTag <= tag && tag <= DoubleTag + def isNonUnitAnyVal = BooleanTag <= tag && tag <= DoubleTag + def isAnyVal = UnitTag <= tag && tag <= DoubleTag def tpe: Type = tag match { case UnitTag => UnitClass.tpe diff --git a/src/compiler/scala/reflect/internal/TreeInfo.scala b/src/compiler/scala/reflect/internal/TreeInfo.scala index 196f420e6..125b32a1d 100644 --- a/src/compiler/scala/reflect/internal/TreeInfo.scala +++ b/src/compiler/scala/reflect/internal/TreeInfo.scala @@ -68,6 +68,10 @@ abstract class TreeInfo { } /** Is tree a stable and pure expression? + * !!! Clarification on what is meant by "pure" here would be appreciated. + * This implementation allows both modules and lazy vals, which are pure in + * the sense that they always return the same result, but which are also + * side effecting. So for now, "pure" != "not side effecting". */ def isPureExpr(tree: Tree): Boolean = tree match { case EmptyTree @@ -77,6 +81,10 @@ abstract class TreeInfo { true case Ident(_) => tree.symbol.isStable + // this case is mostly to allow expressions like -5 and +7, but any + // member of an anyval should be safely pure + case Select(Literal(const), name) => + const.isAnyVal && (const.tpe.member(name) != NoSymbol) case Select(qual, _) => tree.symbol.isStable && isPureExpr(qual) case TypeApply(fn, _) => diff --git a/src/compiler/scala/tools/nsc/ast/parser/MarkupParsers.scala b/src/compiler/scala/tools/nsc/ast/parser/MarkupParsers.scala index 2fe8ad13c..fc09c9d5e 100644 --- a/src/compiler/scala/tools/nsc/ast/parser/MarkupParsers.scala +++ b/src/compiler/scala/tools/nsc/ast/parser/MarkupParsers.scala @@ -397,11 +397,9 @@ trait MarkupParsers { def xScalaPatterns: List[Tree] = escapeToScala(parser.seqPatterns(), "pattern") def reportSyntaxError(pos: Int, str: String) = parser.syntaxError(pos, str) - def reportSyntaxError(str: String) = { + def reportSyntaxError(str: String) { reportSyntaxError(curOffset, "in XML literal: " + str) - val result = ch - nextch - result + nextch() } /** '<' xPattern ::= Name [S] { xmlPattern | '{' pattern3 '}' } ETag diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala index 7537bb36d..fd50bef8c 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala @@ -928,7 +928,7 @@ trait Namers { self: Analyzer => // is more compact than: def foo[T, T2](a: T, x: T2)(implicit w: ComputeT2[T, T2]) // moreover, the latter is not an encoding of the former, which hides type inference of T2, so you can specify T while T2 is purely computed val checkDependencies: TypeTraverser = new TypeTraverser { - def traverse(tp: Type) = { + def traverse(tp: Type) { tp match { case SingleType(_, sym) => if (sym.owner == meth && sym.isValueParameter && !(okParams contains sym)) @@ -941,7 +941,6 @@ trait Namers { self: Analyzer => case _ => mapOver(tp) } - this } } for(vps <- vparamSymss) { diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 1bf2099fc..a90f3ecfc 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -2165,6 +2165,20 @@ trait Typers extends Modes with Adaptations { if (treeInfo.isSelfConstrCall(result) && result.symbol.pos.pointOrElse(0) >= exprOwner.enclMethod.pos.pointOrElse(0)) error(stat.pos, "called constructor's definition must precede calling constructor's definition") } + result match { + case EmptyTree | Literal(Constant(())) => () + case _ => + if (treeInfo isPureExpr result) { + val sym = result.symbol + if (sym != null && (sym.isModule || sym.isLazy)) { + debuglog("'Pure' but side-effecting expression in statement position: " + result) + } + else context.warning(stat.pos, + "a pure expression does nothing in statement position; " + + "you may be omitting necessary parentheses" + ) + } + } result } } diff --git a/src/compiler/scala/tools/nsc/util/Statistics.scala b/src/compiler/scala/tools/nsc/util/Statistics.scala index 13199d75a..b1d463f6f 100644 --- a/src/compiler/scala/tools/nsc/util/Statistics.scala +++ b/src/compiler/scala/tools/nsc/util/Statistics.scala @@ -71,7 +71,6 @@ abstract class StatisticsInfo { def countNodes(tree: Tree, counts: ClassCounts) { for (t <- tree) counts(t.getClass) += 1 - counts } def showRelative(base: Long)(value: Long) = diff --git a/src/library/scala/collection/JavaConversions.scala b/src/library/scala/collection/JavaConversions.scala index b406ab15c..a76b2501f 100644 --- a/src/library/scala/collection/JavaConversions.scala +++ b/src/library/scala/collection/JavaConversions.scala @@ -678,18 +678,19 @@ object JavaConversions { } } - def remove() = prev match { - case Some(k) => - underlying match { - case mm: mutable.Map[A, _] => - val v = mm remove k.asInstanceOf[A] - prev = None - v - case _ => - throw new UnsupportedOperationException("remove") - } - case _ => - throw new IllegalStateException("next must be called at least once before remove") + def remove() { + prev match { + case Some(k) => + underlying match { + case mm: mutable.Map[A, _] => + mm remove k.asInstanceOf[A] + prev = None + case _ => + throw new UnsupportedOperationException("remove") + } + case _ => + throw new IllegalStateException("next must be called at least once before remove") + } } } } diff --git a/src/library/scala/xml/dtd/ContentModel.scala b/src/library/scala/xml/dtd/ContentModel.scala index 9e411d3ec..8416a21ab 100644 --- a/src/library/scala/xml/dtd/ContentModel.scala +++ b/src/library/scala/xml/dtd/ContentModel.scala @@ -52,7 +52,6 @@ object ContentModel extends WordExp { sb append sep buildString(z, sb) } - sb } def buildString(c: ContentModel, sb: StringBuilder): StringBuilder = c match { diff --git a/src/swing/scala/swing/LayoutContainer.scala b/src/swing/scala/swing/LayoutContainer.scala index ea296d2fe..4aff2604b 100644 --- a/src/swing/scala/swing/LayoutContainer.scala +++ b/src/swing/scala/swing/LayoutContainer.scala @@ -61,7 +61,6 @@ trait LayoutContainer extends Container.Wrapper { val (v, msg) = areValid(l) if (!v) throw new IllegalArgumentException(msg) add(c, l) - this } def get(c: Component) = Option(constraintsFor(c)) override def size = peer.getComponentCount diff --git a/test/files/neg/scopes.check b/test/files/neg/scopes.check index 2f2eaa758..f8e8c3758 100644 --- a/test/files/neg/scopes.check +++ b/test/files/neg/scopes.check @@ -7,6 +7,9 @@ scopes.scala:5: error: x is already defined as value x scopes.scala:8: error: y is already defined as value y val y: Float = .0f ^ +scopes.scala:6: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + { + ^ scopes.scala:11: error: x is already defined as value x def f1(x: Int, x: Float) = x ^ @@ -19,4 +22,5 @@ scopes.scala:13: error: x is already defined as value x scopes.scala:15: error: x is already defined as value x case x::x => x ^ +one warning found 7 errors found diff --git a/test/files/neg/stmt-expr-discard.check b/test/files/neg/stmt-expr-discard.check new file mode 100644 index 000000000..2d6420a61 --- /dev/null +++ b/test/files/neg/stmt-expr-discard.check @@ -0,0 +1,7 @@ +stmt-expr-discard.scala:3: error: a pure expression does nothing in statement position; you may be omitting necessary parentheses + + 2 + ^ +stmt-expr-discard.scala:4: error: a pure expression does nothing in statement position; you may be omitting necessary parentheses + - 4 + ^ +two errors found diff --git a/test/files/neg/stmt-expr-discard.flags b/test/files/neg/stmt-expr-discard.flags new file mode 100644 index 000000000..e8fb65d50 --- /dev/null +++ b/test/files/neg/stmt-expr-discard.flags @@ -0,0 +1 @@ +-Xfatal-warnings \ No newline at end of file diff --git a/test/files/neg/stmt-expr-discard.scala b/test/files/neg/stmt-expr-discard.scala new file mode 100644 index 000000000..e60c85462 --- /dev/null +++ b/test/files/neg/stmt-expr-discard.scala @@ -0,0 +1,5 @@ +class A { + def f = 1 + + 2 + - 4 +} diff --git a/test/files/neg/t1181.check b/test/files/neg/t1181.check index 2d7205c61..3724752a8 100644 --- a/test/files/neg/t1181.check +++ b/test/files/neg/t1181.check @@ -1,4 +1,8 @@ +t1181.scala:8: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + case (Nil, Nil) => map + ^ t1181.scala:9: error: missing parameter type _ => buildMap(map.updated(keyList.head, valueList.head), keyList.tail, valueList.tail) ^ +one warning found one error found diff --git a/test/files/neg/t2078.scala b/test/files/neg/t2078.scala index 03eaa7ed0..342ba088c 100644 --- a/test/files/neg/t2078.scala +++ b/test/files/neg/t2078.scala @@ -5,5 +5,5 @@ class A[-S](y : S) { object Test extends App { val a = new A(1) val b = a : A[Nothing] - b.f.x + println(b.f.x) } diff --git a/test/files/neg/t2641.check b/test/files/neg/t2641.check index 771624e8d..2056a1b9a 100644 --- a/test/files/neg/t2641.check +++ b/test/files/neg/t2641.check @@ -1,35 +1,35 @@ -t2641.scala:19: error: illegal cyclic reference involving trait ManagedSeq +t2641.scala:18: error: illegal cyclic reference involving trait ManagedSeq with TraversableViewLike[A, ManagedSeqStrict[A], ManagedSeq[A]] ^ -t2641.scala:17: error: illegal inheritance; +t2641.scala:16: error: illegal inheritance; self-type ManagedSeq does not conform to ManagedSeqStrict[A]'s selftype ManagedSeqStrict[A] extends ManagedSeqStrict[A] ^ -t2641.scala:18: error: illegal inheritance; +t2641.scala:17: error: illegal inheritance; self-type ManagedSeq does not conform to scala.collection.TraversableView[A,ManagedSeqStrict[A]]'s selftype scala.collection.TraversableView[A,ManagedSeqStrict[A]] with TraversableView[A, ManagedSeqStrict[A]] ^ -t2641.scala:17: error: illegal inheritance; +t2641.scala:16: error: illegal inheritance; self-type ManagedSeq does not conform to ScalaObject's selftype ScalaObject extends ManagedSeqStrict[A] ^ -t2641.scala:25: error: something is wrong (wrong class file?): trait ManagedSeq with type parameters [A,Coll] gets applied to arguments [], phase = typer +t2641.scala:24: error: something is wrong (wrong class file?): trait ManagedSeq with type parameters [A,Coll] gets applied to arguments [], phase = typer trait Transformed[+B] extends ManagedSeq[B, Coll] with super.Transformed[B] ^ -t2641.scala:27: error: something is wrong (wrong class file?): trait ManagedSeq with type parameters [A,Coll] gets applied to arguments [], phase = namer +t2641.scala:26: error: something is wrong (wrong class file?): trait ManagedSeq with type parameters [A,Coll] gets applied to arguments [], phase = namer trait Sliced extends Transformed[A] with super.Sliced { ^ -t2641.scala:27: error: illegal inheritance; superclass Any +t2641.scala:26: error: illegal inheritance; superclass Any is not a subclass of the superclass ManagedSeqStrict of the mixin trait Transformed trait Sliced extends Transformed[A] with super.Sliced { ^ -t2641.scala:27: error: illegal inheritance; superclass Any +t2641.scala:26: error: illegal inheritance; superclass Any is not a subclass of the superclass Object of the mixin trait Sliced trait Sliced extends Transformed[A] with super.Sliced { ^ -t2641.scala:28: error: value managedIterator is not a member of ManagedSeq +t2641.scala:27: error: value managedIterator is not a member of ManagedSeq override def managedIterator = self.managedIterator slice (from, until) ^ 9 errors found diff --git a/test/files/neg/t2641.scala b/test/files/neg/t2641.scala index 68a4ca35b..626d5d785 100644 --- a/test/files/neg/t2641.scala +++ b/test/files/neg/t2641.scala @@ -9,8 +9,7 @@ abstract class ManagedSeqStrict[+A] { override def companion: GenericCompanion[ManagedSeqStrict] = null - override def foreach[U](f: A => U): Unit = - null + override def foreach[U](f: A => U): Unit = () } trait ManagedSeq[+A, +Coll] diff --git a/test/files/neg/t278.check b/test/files/neg/t278.check index ad1078f89..675ef910e 100644 --- a/test/files/neg/t278.check +++ b/test/files/neg/t278.check @@ -2,8 +2,8 @@ t278.scala:5: error: overloaded method value a with alternatives: => C.this.A => Unit => () => Unit does not take type parameters - a[A] - ^ + println(a[A]) + ^ t278.scala:4: error: method a is defined twice def a = (p:A) => () ^ diff --git a/test/files/neg/t278.scala b/test/files/neg/t278.scala index 16ffe1059..39a711bb0 100644 --- a/test/files/neg/t278.scala +++ b/test/files/neg/t278.scala @@ -2,5 +2,5 @@ class C { class A def a = () => () def a = (p:A) => () - a[A] + println(a[A]) } diff --git a/test/files/neg/t2910.check b/test/files/neg/t2910.check index ff190122d..44bf1993d 100644 --- a/test/files/neg/t2910.check +++ b/test/files/neg/t2910.check @@ -10,7 +10,7 @@ t2910.scala:16: error: forward reference extends over definition of value z t2910.scala:30: error: forward reference extends over definition of value x lazy val f: Int = x ^ -t2910.scala:34: error: forward reference extends over definition of variable x +t2910.scala:35: error: forward reference extends over definition of variable x lazy val f: Int = g ^ 5 errors found diff --git a/test/files/neg/t2910.scala b/test/files/neg/t2910.scala index aafeb59f3..fa51038dc 100644 --- a/test/files/neg/t2910.scala +++ b/test/files/neg/t2910.scala @@ -29,6 +29,7 @@ object Test { { lazy val f: Int = x val x: Int = f + println(x) } { lazy val f: Int = g diff --git a/test/files/neg/t4166.check b/test/files/neg/t4166.check index 24129c54a..10b77d841 100644 --- a/test/files/neg/t4166.check +++ b/test/files/neg/t4166.check @@ -1,4 +1,4 @@ t4166.scala:3: error: super constructor arguments cannot reference unconstructed `this` -class Demo extends Base(new { Demo.this }) { +class Demo extends Base(new { Demo.this.toString }) { ^ one error found diff --git a/test/files/neg/t4166.scala b/test/files/neg/t4166.scala index c20796c43..a2ee0671a 100644 --- a/test/files/neg/t4166.scala +++ b/test/files/neg/t4166.scala @@ -1,11 +1,11 @@ class Base(a: Any) -class Demo extends Base(new { Demo.this }) { +class Demo extends Base(new { Demo.this.toString }) { val x: Any = () } -class Demo2 extends Base(new { this }) { +class Demo2 extends Base(new { this.toString }) { val x: Any = () } diff --git a/test/files/neg/t4419.check b/test/files/neg/t4419.check index 8a5d95ca4..a53e0c95d 100644 --- a/test/files/neg/t4419.check +++ b/test/files/neg/t4419.check @@ -1,4 +1,4 @@ t4419.scala:2: error: forward reference extends over definition of value b - { val b = a; val a = 1 } + { val b = a; val a = 1 ; println(a) } ^ one error found diff --git a/test/files/neg/t4419.scala b/test/files/neg/t4419.scala index 38a34be48..5dc86d354 100644 --- a/test/files/neg/t4419.scala +++ b/test/files/neg/t4419.scala @@ -1,3 +1,3 @@ class A { - { val b = a; val a = 1 } + { val b = a; val a = 1 ; println(a) } } \ No newline at end of file diff --git a/test/files/neg/unit-returns-value.check b/test/files/neg/unit-returns-value.check index 18368f45a..ab458a350 100644 --- a/test/files/neg/unit-returns-value.check +++ b/test/files/neg/unit-returns-value.check @@ -1,4 +1,7 @@ +unit-returns-value.scala:4: error: a pure expression does nothing in statement position; you may be omitting necessary parentheses + if (b) return 5 + ^ unit-returns-value.scala:4: error: enclosing method f has result type Unit: return value discarded if (b) return 5 ^ -one error found +two errors found diff --git a/test/files/neg/variances.scala b/test/files/neg/variances.scala index 57abba130..726bc3527 100644 --- a/test/files/neg/variances.scala +++ b/test/files/neg/variances.scala @@ -29,7 +29,7 @@ object Covariant { def b2a(b : B) : A def doit(b : B) = setA(b2a(b)) } - () + println("") } } class Foo3[+A] { diff --git a/test/files/run/repl-bare-expr.check b/test/files/run/repl-bare-expr.check index 04daa4823..8b6434e98 100644 --- a/test/files/run/repl-bare-expr.check +++ b/test/files/run/repl-bare-expr.check @@ -4,15 +4,33 @@ Type :help for more information. scala> scala> 2 ; 3 +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 2 ;; + ^ res0: Int = 3 scala> { 2 ; 3 } +:8: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + { 2 ; 3 } + ^ res1: Int = 3 scala> 5 ; 10 ; case object Cow ; 20 ; class Moo { override def toString = "Moooooo" } ; 30 ; def bippy = { 1 + 2 + 3 } ; bippy+88+11 +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; 10 ; case object Cow ; 20 ; class Moo { override def toString = "Moooooo" } ; 30 ; def bippy = { + ^ +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; 10 ; case object Cow ; 20 ; class Moo { override def toString = "Moooooo" } ; 30 ; def bippy = { + ^ +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; 10 ; case object Cow ; 20 ; class Moo { override def toString = "Moooooo" } ; 30 ; def bippy = { + ^ +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; 10 ; case object Cow ; 20 ; class Moo { override def toString = "Moooooo" } ; 30 ; def bippy = { + ^ defined module Cow defined class Moo bippy: Int diff --git a/test/files/run/repl-parens.check b/test/files/run/repl-parens.check index 2f56e5ddd..a3b0fd193 100644 --- a/test/files/run/repl-parens.check +++ b/test/files/run/repl-parens.check @@ -20,6 +20,12 @@ scala> ( (2 + 2 ) ) res5: Int = 4 scala> 5 ; ( (2 + 2 ) ) ; ((5)) +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; ( (2 + 2 ) ) ;; + ^ +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; ( (2 + 2 ) ) ;; + ^ res6: Int = 5 scala> (((2 + 2)), ((2 + 2))) @@ -34,9 +40,18 @@ res9: String = 4423 scala> scala> 55 ; ((2 + 2)) ; (1, 2, 3) +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ; ((2 + 2)) ;; + ^ +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ; ((2 + 2)) ;; + ^ res10: (Int, Int, Int) = (1,2,3) scala> 55 ; (x: Int) => x + 1 ; () => ((5)) +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ; (x: Int) => x + 1 ;; + ^ res11: () => Int = scala> @@ -45,6 +60,9 @@ scala> () => 5 res12: () => Int = scala> 55 ; () => 5 +:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ;; + ^ res13: () => Int = scala> () => { class X ; new X }