Permalink
Browse files

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.
  • Loading branch information...
1 parent dc8854e commit 9b2ce26887d5322131f10d85530c733b76f3de33 @som-snytt som-snytt committed Dec 16, 2013
@@ -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)
@@ -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
Oops, something went wrong.

0 comments on commit 9b2ce26

Please sign in to comment.