diff options
author | Paul Phillips <paulp@improving.org> | 2011-09-05 00:11:29 +0000 |
---|---|---|
committer | Paul Phillips <paulp@improving.org> | 2011-09-05 00:11:29 +0000 |
commit | fa2deeb4304d149c4870cfb013e7790d6fe00d86 (patch) | |
tree | bf0bbd999592d412811f4647f1e94ad3c1165191 | |
parent | 6817244d64fca81810e6e45f8a4ea53e9e6d76c2 (diff) | |
download | scala-fa2deeb4304d149c4870cfb013e7790d6fe00d86.tar.gz scala-fa2deeb4304d149c4870cfb013e7790d6fe00d86.tar.bz2 scala-fa2deeb4304d149c4870cfb013e7790d6fe00d86.zip |
Offer warning when demonstrably non-side-effect...
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.
29 files changed, 123 insertions, 44 deletions
diff --git a/src/compiler/scala/reflect/internal/Constants.scala b/src/compiler/scala/reflect/internal/Constants.scala index 55bb7c2ceb..2edb0d1fe6 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 a8ec7e34f5..de46069ee2 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 6c1245dd46..9eda2e4097 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 7d68abccf0..9adb7dce20 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 725bf90af3..3b6b44316b 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 95cdc77a09..27239b9b9f 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 f67fd47708..fad990e140 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 cae2236346..1e9a3a4b58 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 da1c766437..0570e7bb57 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 2f2eaa758f..f8e8c3758a 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 0000000000..2d6420a61d --- /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 0000000000..e8fb65d50c --- /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 0000000000..e60c854625 --- /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 2d7205c61f..3724752a85 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 03eaa7ed0b..342ba088c7 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 771624e8d9..2056a1b9ab 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 5529035f79..bc048e039e 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 ad1078f897..675ef910ee 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 <and> => () => 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 16ffe10595..39a711bb09 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 ff190122d6..44bf1993db 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 b772ee4d43..d9a781032c 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 24129c54ad..10b77d841a 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 c20796c43c..a2ee0671ab 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 8a5d95ca4e..a53e0c95da 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 38a34be489..5dc86d354e 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 18368f45ab..ab458a350b 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 181783f48a..f693480d1d 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 04daa48232..8b6434e986 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 +<console>:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 2 ;; + ^ res0: Int = 3 scala> { 2 ; 3 } +<console>: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 +<console>: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 = { + ^ +<console>: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 = { + ^ +<console>: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 = { + ^ +<console>: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 2f56e5ddd4..a3b0fd1939 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)) +<console>:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 5 ; ( (2 + 2 ) ) ;; + ^ +<console>: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) +<console>:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ; ((2 + 2)) ;; + ^ +<console>: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)) +<console>:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ; (x: Int) => x + 1 ;; + ^ res11: () => Int = <function0> scala> @@ -45,6 +60,9 @@ scala> () => 5 res12: () => Int = <function0> scala> 55 ; () => 5 +<console>:7: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses + 55 ;; + ^ res13: () => Int = <function0> scala> () => { class X ; new X } |