diff options
author | Som Snytt <som.snytt@gmail.com> | 2013-12-15 20:10:25 -0800 |
---|---|---|
committer | Som Snytt <som.snytt@gmail.com> | 2013-12-16 09:11:19 -0800 |
commit | 9b2ce26887d5322131f10d85530c733b76f3de33 (patch) | |
tree | b8dae755096ce3006c2786019cf73dbaa73c85cc | |
parent | dc8854e059f3a75b2d95a1826ff61ed6c5b5eba8 (diff) | |
download | scala-9b2ce26887d5322131f10d85530c733b76f3de33.tar.gz scala-9b2ce26887d5322131f10d85530c733b76f3de33.tar.bz2 scala-9b2ce26887d5322131f10d85530c733b76f3de33.zip |
SI-6120 Suppress extra warnings
This is a mere polish for the fix to allow multiple warnings.
Sensibility checks in refchecks were shown to be redundant.
This commit includes a mild refactor to reduce tabbage, and
uses a local var to flag that a warning has already been emitted.
It would be better to have the checks return true if warned,
to facilitate `nonSensically || unrelatedly`, etc., but that's
a lot of `else false`.
The check files that were updated with the redundant warnings
are reverted.
-rw-r--r-- | src/compiler/scala/tools/nsc/typechecker/RefChecks.scala | 299 | ||||
-rw-r--r-- | test/files/neg/checksensible.check | 17 | ||||
-rw-r--r-- | test/files/neg/t5426.check | 8 | ||||
-rw-r--r-- | test/files/neg/t5663-badwarneq.check | 5 | ||||
-rw-r--r-- | test/files/neg/t7756b.check | 5 |
5 files changed, 156 insertions, 178 deletions
diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index ec916719dc..95f2620061 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -954,162 +954,166 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans case Object_eq | Object_ne | Object_== | Object_!= | Any_== | Any_!= => true case _ => false } - def checkSensible(pos: Position, fn: Tree, args: List[Tree]) = fn match { - case Select(qual, name @ (nme.EQ | nme.NE | nme.eq | nme.ne)) if args.length == 1 && isObjectOrAnyComparisonMethod(fn.symbol) => - // Make sure the 'eq' or 'ne' method is the one in AnyRef. - def isReferenceOp = fn.symbol == Object_eq || fn.symbol == Object_ne - def isNew(tree: Tree) = tree match { - case Function(_, _) - | Apply(Select(New(_), nme.CONSTRUCTOR), _) => true - case _ => false - } - def underlyingClass(tp: Type): Symbol = { - val sym = tp.widen.typeSymbol - if (sym.isAbstractType) underlyingClass(sym.info.bounds.hi) - else sym - } - val actual = underlyingClass(args.head.tpe) - val receiver = underlyingClass(qual.tpe) - def onTrees[T](f: List[Tree] => T) = f(List(qual, args.head)) - def onSyms[T](f: List[Symbol] => T) = f(List(receiver, actual)) - - // @MAT normalize for consistency in error message, otherwise only part is normalized due to use of `typeSymbol` - def typesString = normalizeAll(qual.tpe.widen)+" and "+normalizeAll(args.head.tpe.widen) - - /* Symbols which limit the warnings we can issue since they may be value types */ - val isMaybeValue = Set[Symbol](AnyClass, AnyRefClass, AnyValClass, ObjectClass, ComparableClass, JavaSerializableClass) - - // Whether def equals(other: Any) has known behavior: it is the default - // inherited from java.lang.Object, or it is a synthetically generated - // case equals. TODO - more cases are warnable if the target is a synthetic - // equals. - def isUsingWarnableEquals = { - val m = receiver.info.member(nme.equals_) - ((m == Object_equals) || (m == Any_equals) || isMethodCaseEquals(m)) - } - def isMethodCaseEquals(m: Symbol) = m.isSynthetic && m.owner.isCase - def isCaseEquals = isMethodCaseEquals(receiver.info.member(nme.equals_)) - // Whether this == or != is one of those defined in Any/AnyRef or an overload from elsewhere. - def isUsingDefaultScalaOp = { - val s = fn.symbol - (s == Object_==) || (s == Object_!=) || (s == Any_==) || (s == Any_!=) - } - def haveSubclassRelationship = (actual isSubClass receiver) || (receiver isSubClass actual) - - // Whether the operands+operator represent a warnable combo (assuming anyrefs) - // Looking for comparisons performed with ==/!= in combination with either an - // equals method inherited from Object or a case class synthetic equals (for - // which we know the logic.) - def isWarnable = isReferenceOp || (isUsingDefaultScalaOp && isUsingWarnableEquals) - def isEitherNullable = (NullTpe <:< receiver.info) || (NullTpe <:< actual.info) - def isEitherValueClass = actual.isDerivedValueClass || receiver.isDerivedValueClass - def isBoolean(s: Symbol) = unboxedValueClass(s) == BooleanClass - def isUnit(s: Symbol) = unboxedValueClass(s) == UnitClass - def isNumeric(s: Symbol) = isNumericValueClass(unboxedValueClass(s)) || isAnyNumber(s) - def isScalaNumber(s: Symbol) = s isSubClass ScalaNumberClass - def isJavaNumber(s: Symbol) = s isSubClass JavaNumberClass - // includes java.lang.Number if appropriate [SI-5779] - def isAnyNumber(s: Symbol) = isScalaNumber(s) || isJavaNumber(s) - def isMaybeAnyValue(s: Symbol) = isPrimitiveValueClass(unboxedValueClass(s)) || isMaybeValue(s) - // used to short-circuit unrelatedTypes check if both sides are special - def isSpecial(s: Symbol) = isMaybeAnyValue(s) || isAnyNumber(s) - val nullCount = onSyms(_ filter (_ == NullClass) size) - def isNonsenseValueClassCompare = ( - !haveSubclassRelationship - && isUsingDefaultScalaOp - && isEitherValueClass - && !isCaseEquals - ) + /** Check the sensibility of using the given `equals` to compare `qual` and `other`. */ + private def checkSensibleEquals(pos: Position, qual: Tree, name: Name, sym: Symbol, other: Tree) = { + def isReferenceOp = sym == Object_eq || sym == Object_ne + def isNew(tree: Tree) = tree match { + case Function(_, _) | Apply(Select(New(_), nme.CONSTRUCTOR), _) => true + case _ => false + } + def underlyingClass(tp: Type): Symbol = { + val sym = tp.widen.typeSymbol + if (sym.isAbstractType) underlyingClass(sym.info.bounds.hi) + else sym + } + val actual = underlyingClass(other.tpe) + val receiver = underlyingClass(qual.tpe) + def onTrees[T](f: List[Tree] => T) = f(List(qual, other)) + def onSyms[T](f: List[Symbol] => T) = f(List(receiver, actual)) + + // @MAT normalize for consistency in error message, otherwise only part is normalized due to use of `typeSymbol` + def typesString = normalizeAll(qual.tpe.widen)+" and "+normalizeAll(other.tpe.widen) + + /* Symbols which limit the warnings we can issue since they may be value types */ + val isMaybeValue = Set[Symbol](AnyClass, AnyRefClass, AnyValClass, ObjectClass, ComparableClass, JavaSerializableClass) + + // Whether def equals(other: Any) has known behavior: it is the default + // inherited from java.lang.Object, or it is a synthetically generated + // case equals. TODO - more cases are warnable if the target is a synthetic + // equals. + def isUsingWarnableEquals = { + val m = receiver.info.member(nme.equals_) + ((m == Object_equals) || (m == Any_equals) || isMethodCaseEquals(m)) + } + def isMethodCaseEquals(m: Symbol) = m.isSynthetic && m.owner.isCase + def isCaseEquals = isMethodCaseEquals(receiver.info.member(nme.equals_)) + // Whether this == or != is one of those defined in Any/AnyRef or an overload from elsewhere. + def isUsingDefaultScalaOp = sym == Object_== || sym == Object_!= || sym == Any_== || sym == Any_!= + def haveSubclassRelationship = (actual isSubClass receiver) || (receiver isSubClass actual) + + // Whether the operands+operator represent a warnable combo (assuming anyrefs) + // Looking for comparisons performed with ==/!= in combination with either an + // equals method inherited from Object or a case class synthetic equals (for + // which we know the logic.) + def isWarnable = isReferenceOp || (isUsingDefaultScalaOp && isUsingWarnableEquals) + def isEitherNullable = (NullTpe <:< receiver.info) || (NullTpe <:< actual.info) + def isEitherValueClass = actual.isDerivedValueClass || receiver.isDerivedValueClass + def isBoolean(s: Symbol) = unboxedValueClass(s) == BooleanClass + def isUnit(s: Symbol) = unboxedValueClass(s) == UnitClass + def isNumeric(s: Symbol) = isNumericValueClass(unboxedValueClass(s)) || isAnyNumber(s) + def isScalaNumber(s: Symbol) = s isSubClass ScalaNumberClass + def isJavaNumber(s: Symbol) = s isSubClass JavaNumberClass + // includes java.lang.Number if appropriate [SI-5779] + def isAnyNumber(s: Symbol) = isScalaNumber(s) || isJavaNumber(s) + def isMaybeAnyValue(s: Symbol) = isPrimitiveValueClass(unboxedValueClass(s)) || isMaybeValue(s) + // used to short-circuit unrelatedTypes check if both sides are special + def isSpecial(s: Symbol) = isMaybeAnyValue(s) || isAnyNumber(s) + val nullCount = onSyms(_ filter (_ == NullClass) size) + def isNonsenseValueClassCompare = ( + !haveSubclassRelationship + && isUsingDefaultScalaOp + && isEitherValueClass + && !isCaseEquals + ) - def nonSensibleWarning(what: String, alwaysEqual: Boolean) = { - val msg = alwaysEqual == (name == nme.EQ || name == nme.eq) - unit.warning(pos, "comparing "+what+" using `"+name.decode+"' will always yield " + msg) - } - def nonSensible(pre: String, alwaysEqual: Boolean) = - nonSensibleWarning(pre+"values of types "+typesString, alwaysEqual) - def nonSensiblyEq() = nonSensible("", true) - def nonSensiblyNeq() = nonSensible("", false) - def nonSensiblyNew() = nonSensibleWarning("a fresh object", false) - - def unrelatedMsg = name match { - case nme.EQ | nme.eq => "never compare equal" - case _ => "always compare unequal" - } - def unrelatedTypes() = { - val weaselWord = if (isEitherValueClass) "" else " most likely" - unit.warning(pos, s"$typesString are unrelated: they will$weaselWord $unrelatedMsg") - } + // Have we already determined that the comparison is non-sensible? I mean, non-sensical? + var isNonSensible = false + + def nonSensibleWarning(what: String, alwaysEqual: Boolean) = { + val msg = alwaysEqual == (name == nme.EQ || name == nme.eq) + unit.warning(pos, s"comparing $what using `${name.decode}' will always yield $msg") + isNonSensible = true + } + def nonSensible(pre: String, alwaysEqual: Boolean) = + nonSensibleWarning(s"${pre}values of types $typesString", alwaysEqual) + def nonSensiblyEq() = nonSensible("", alwaysEqual = true) + def nonSensiblyNeq() = nonSensible("", alwaysEqual = false) + def nonSensiblyNew() = nonSensibleWarning("a fresh object", alwaysEqual = false) + + def unrelatedMsg = name match { + case nme.EQ | nme.eq => "never compare equal" + case _ => "always compare unequal" + } + def unrelatedTypes() = if (!isNonSensible) { + val weaselWord = if (isEitherValueClass) "" else " most likely" + unit.warning(pos, s"$typesString are unrelated: they will$weaselWord $unrelatedMsg") + } - if (nullCount == 2) // null == null + if (nullCount == 2) // null == null + nonSensiblyEq() + else if (nullCount == 1) { + if (onSyms(_ exists isPrimitiveValueClass)) // null == 5 + nonSensiblyNeq() + else if (onTrees( _ exists isNew)) // null == new AnyRef + nonSensiblyNew() + } + else if (isBoolean(receiver)) { + if (!isBoolean(actual) && !isMaybeValue(actual)) // true == 5 + nonSensiblyNeq() + } + else if (isUnit(receiver)) { + if (isUnit(actual)) // () == () nonSensiblyEq() - else if (nullCount == 1) { - if (onSyms(_ exists isPrimitiveValueClass)) // null == 5 - nonSensiblyNeq() - else if (onTrees( _ exists isNew)) // null == new AnyRef - nonSensiblyNew() - } - else if (isBoolean(receiver)) { - if (!isBoolean(actual) && !isMaybeValue(actual)) // true == 5 + else if (!isUnit(actual) && !isMaybeValue(actual)) // () == "abc" + nonSensiblyNeq() + } + else if (isNumeric(receiver)) { + if (!isNumeric(actual)) + if (isUnit(actual) || isBoolean(actual) || !isMaybeValue(actual)) // 5 == "abc" nonSensiblyNeq() - } - else if (isUnit(receiver)) { - if (isUnit(actual)) // () == () - nonSensiblyEq() - else if (!isUnit(actual) && !isMaybeValue(actual)) // () == "abc" + } + else if (isWarnable && !isCaseEquals) { + if (isNew(qual)) // new X == y + nonSensiblyNew() + else if (isNew(other) && (receiver.isEffectivelyFinal || isReferenceOp)) // object X ; X == new Y + nonSensiblyNew() + else if (receiver.isEffectivelyFinal && !(receiver isSubClass actual) && !actual.isRefinementClass) { // object X, Y; X == Y + if (isEitherNullable) + nonSensible("non-null ", false) + else nonSensiblyNeq() } - else if (isNumeric(receiver)) { - if (!isNumeric(actual)) - if (isUnit(actual) || isBoolean(actual) || !isMaybeValue(actual)) // 5 == "abc" - nonSensiblyNeq() + } + + // warn if one but not the other is a derived value class + // this is especially important to enable transitioning from + // regular to value classes without silent failures. + if (isNonsenseValueClassCompare) + unrelatedTypes() + // possibleNumericCount is insufficient or this will warn on e.g. Boolean == j.l.Boolean + else if (isWarnable && nullCount == 0 && !(isSpecial(receiver) && isSpecial(actual))) { + // better to have lubbed and lost + def warnIfLubless(): Unit = { + val common = global.lub(List(actual.tpe, receiver.tpe)) + if (ObjectTpe <:< common) + unrelatedTypes() } - else if (isWarnable && !isCaseEquals) { - if (isNew(qual)) // new X == y - nonSensiblyNew() - else if (isNew(args.head) && (receiver.isEffectivelyFinal || isReferenceOp)) // object X ; X == new Y - nonSensiblyNew() - else if (receiver.isEffectivelyFinal && !(receiver isSubClass actual) && !actual.isRefinementClass) { // object X, Y; X == Y - if (isEitherNullable) - nonSensible("non-null ", false) - else - nonSensiblyNeq() + // warn if actual has a case parent that is not same as receiver's; + // if actual is not a case, then warn if no common supertype, as below + if (isCaseEquals) { + def thisCase = receiver.info.member(nme.equals_).owner + actual.info.baseClasses.find(_.isCase) match { + case Some(p) if p != thisCase => nonSensible("case class ", false) + case None => + // stronger message on (Some(1) == None) + //if (receiver.isCase && receiver.isEffectivelyFinal && !(receiver isSubClass actual)) nonSensiblyNeq() + //else + // if a class, it must be super to thisCase (and receiver) since not <: thisCase + if (!actual.isTrait && !(receiver isSubClass actual)) nonSensiblyNeq() + else if (!haveSubclassRelationship) warnIfLubless() + case _ => } } - - // warn if one but not the other is a derived value class - // this is especially important to enable transitioning from - // regular to value classes without silent failures. - if (isNonsenseValueClassCompare) - unrelatedTypes() - // possibleNumericCount is insufficient or this will warn on e.g. Boolean == j.l.Boolean - else if (isWarnable && nullCount == 0 && !(isSpecial(receiver) && isSpecial(actual))) { - // better to have lubbed and lost - def warnIfLubless(): Unit = { - val common = global.lub(List(actual.tpe, receiver.tpe)) - if (ObjectTpe <:< common) - unrelatedTypes() - } - // warn if actual has a case parent that is not same as receiver's; - // if actual is not a case, then warn if no common supertype, as below - if (isCaseEquals) { - def thisCase = receiver.info.member(nme.equals_).owner - actual.info.baseClasses.find(_.isCase) match { - case Some(p) if p != thisCase => nonSensible("case class ", false) - case None => - // stronger message on (Some(1) == None) - //if (receiver.isCase && receiver.isEffectivelyFinal && !(receiver isSubClass actual)) nonSensiblyNeq() - //else - // if a class, it must be super to thisCase (and receiver) since not <: thisCase - if (!actual.isTrait && !(receiver isSubClass actual)) nonSensiblyNeq() - else if (!haveSubclassRelationship) warnIfLubless() - case _ => - } - } - // warn only if they have no common supertype below Object - else if (!haveSubclassRelationship) { - warnIfLubless() - } + // warn only if they have no common supertype below Object + else if (!haveSubclassRelationship) { + warnIfLubless() } + } + } + /** Sensibility check examines flavors of equals. */ + def checkSensible(pos: Position, fn: Tree, args: List[Tree]) = fn match { + case Select(qual, name @ (nme.EQ | nme.NE | nme.eq | nme.ne)) if args.length == 1 && isObjectOrAnyComparisonMethod(fn.symbol) => + checkSensibleEquals(pos, qual, name, fn.symbol, args.head) case _ => } @@ -1526,7 +1530,8 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans transform(qual) case Apply(fn, args) => - // sensicality should be subsumed by the unreachability/exhaustivity/irrefutability analyses in the pattern matcher + // sensicality should be subsumed by the unreachability/exhaustivity/irrefutability + // analyses in the pattern matcher if (!inPattern) { checkImplicitViewOptionApply(tree.pos, fn, args) checkSensible(tree.pos, fn, args) diff --git a/test/files/neg/checksensible.check b/test/files/neg/checksensible.check index ef3aee5ee4..e5f1a38d96 100644 --- a/test/files/neg/checksensible.check +++ b/test/files/neg/checksensible.check @@ -28,9 +28,6 @@ checksensible.scala:27: warning: comparing values of types Int and Unit using `= checksensible.scala:29: warning: comparing values of types Int and String using `==' will always yield false 1 == "abc" ^ -checksensible.scala:29: warning: Int and String are unrelated: they will most likely never compare equal - 1 == "abc" - ^ checksensible.scala:33: warning: comparing values of types Some[Int] and Int using `==' will always yield false Some(1) == 1 // as above ^ @@ -64,18 +61,12 @@ checksensible.scala:51: warning: comparing values of types Int and Unit using `! checksensible.scala:52: warning: comparing values of types Int and Symbol using `!=' will always yield true (1 != 'sym) ^ -checksensible.scala:52: warning: Int and Symbol are unrelated: they will most likely always compare unequal - (1 != 'sym) - ^ checksensible.scala:58: warning: comparing a fresh object using `==' will always yield false ((x: Int) => x + 1) == null ^ checksensible.scala:59: warning: comparing a fresh object using `==' will always yield false Bep == ((_: Int) + 1) ^ -checksensible.scala:59: warning: Bep.type and Int => Int are unrelated: they will most likely never compare equal - Bep == ((_: Int) + 1) - ^ checksensible.scala:61: warning: comparing a fresh object using `==' will always yield false new Object == new Object ^ @@ -91,9 +82,6 @@ checksensible.scala:66: warning: comparing values of types Int and Null using `= checksensible.scala:71: warning: comparing values of types Bip and Bop using `==' will always yield false (x1 == x2) ^ -checksensible.scala:71: warning: Bip and Bop are unrelated: they will most likely never compare equal - (x1 == x2) - ^ checksensible.scala:81: warning: comparing values of types EqEqRefTest.this.C3 and EqEqRefTest.this.Z1 using `==' will always yield false c3 == z1 ^ @@ -106,12 +94,9 @@ checksensible.scala:83: warning: comparing values of types EqEqRefTest.this.Z1 a checksensible.scala:84: warning: comparing values of types EqEqRefTest.this.C3 and String using `!=' will always yield true c3 != "abc" ^ -checksensible.scala:84: warning: EqEqRefTest.this.C3 and String are unrelated: they will most likely always compare unequal - c3 != "abc" - ^ checksensible.scala:95: warning: comparing values of types Unit and Int using `!=' will always yield true while ((c = in.read) != -1) ^ error: No warnings can be incurred under -Xfatal-warnings. -38 warnings found +33 warnings found one error found diff --git a/test/files/neg/t5426.check b/test/files/neg/t5426.check index c042cdcec3..98f3ddaaae 100644 --- a/test/files/neg/t5426.check +++ b/test/files/neg/t5426.check @@ -4,18 +4,12 @@ t5426.scala:2: warning: comparing values of types Some[Int] and Int using `==' w t5426.scala:3: warning: comparing values of types Int and Some[Int] using `==' will always yield false def f2 = 5 == Some(5) ^ -t5426.scala:3: warning: Int and Some[Int] are unrelated: they will most likely never compare equal - def f2 = 5 == Some(5) - ^ t5426.scala:8: warning: comparing values of types Int and Some[Int] using `==' will always yield false (x1 == x2) ^ -t5426.scala:8: warning: Int and Some[Int] are unrelated: they will most likely never compare equal - (x1 == x2) - ^ t5426.scala:9: warning: comparing values of types Some[Int] and Int using `==' will always yield false (x2 == x1) ^ error: No warnings can be incurred under -Xfatal-warnings. -6 warnings found +four warnings found one error found diff --git a/test/files/neg/t5663-badwarneq.check b/test/files/neg/t5663-badwarneq.check index 4b7795585b..732e4f44d0 100644 --- a/test/files/neg/t5663-badwarneq.check +++ b/test/files/neg/t5663-badwarneq.check @@ -25,9 +25,6 @@ t5663-badwarneq.scala:72: warning: ValueClass1 and Int are unrelated: they will t5663-badwarneq.scala:74: warning: comparing values of types Int and ValueClass1 using `==' will always yield false println(5 == new ValueClass1(5)) // bad ^ -t5663-badwarneq.scala:74: warning: Int and ValueClass1 are unrelated: they will never compare equal - println(5 == new ValueClass1(5)) // bad - ^ t5663-badwarneq.scala:78: warning: ValueClass2[String] and String are unrelated: they will never compare equal println(new ValueClass2("abc") == "abc") // bad ^ @@ -41,5 +38,5 @@ t5663-badwarneq.scala:82: warning: comparing values of types ValueClass3 and Int println(ValueClass3(5) == 5) // bad ^ error: No warnings can be incurred under -Xfatal-warnings. -14 warnings found +13 warnings found one error found diff --git a/test/files/neg/t7756b.check b/test/files/neg/t7756b.check index e764783241..2817a7e230 100644 --- a/test/files/neg/t7756b.check +++ b/test/files/neg/t7756b.check @@ -1,9 +1,6 @@ t7756b.scala:3: warning: comparing values of types Int and String using `==' will always yield false case _ => 0 == "" ^ -t7756b.scala:3: warning: Int and String are unrelated: they will most likely never compare equal - case _ => 0 == "" - ^ error: No warnings can be incurred under -Xfatal-warnings. -two warnings found +one warning found one error found |