Skip to content

Commit

Permalink
Merge pull request #10372 from som-snytt/issue/12728-dubious-selections
Browse files Browse the repository at this point in the history
Lint select from Unit, or Int that incurs widening
  • Loading branch information
lrytz committed Apr 12, 2023
2 parents 146d763 + 905c92f commit d5247e9
Show file tree
Hide file tree
Showing 11 changed files with 268 additions and 14 deletions.
4 changes: 3 additions & 1 deletion src/compiler/scala/tools/nsc/Reporting.scala
Expand Up @@ -341,7 +341,7 @@ object Reporting {
private val insertDash = "(?=[A-Z][a-z])".r

val all: mutable.Map[String, WarningCategory] = mutable.Map.empty
private def add(c: WarningCategory): Unit = all += ((c.name, c))
private def add(c: WarningCategory): Unit = all.put(c.name, c).ensuring(_.isEmpty, s"lint '${c.name}' added twice")

object Deprecation extends WarningCategory; add(Deprecation)

Expand Down Expand Up @@ -409,6 +409,8 @@ object Reporting {
object LintMultiargInfix extends Lint; add(LintMultiargInfix)
object LintPerformance extends Lint; add(LintPerformance)
object LintIntDivToFloat extends Lint; add(LintIntDivToFloat)
object LintUniversalMethods extends Lint; add(LintUniversalMethods)
object LintNumericMethods extends Lint; add(LintNumericMethods)

sealed trait Feature extends WarningCategory { override def summaryCategory: WarningCategory = Feature }
object Feature extends Feature { override def includes(o: WarningCategory): Boolean = o.isInstanceOf[Feature] }; add(Feature)
Expand Down
4 changes: 3 additions & 1 deletion src/compiler/scala/tools/nsc/settings/Warnings.scala
Expand Up @@ -212,7 +212,8 @@ trait Warnings {
val UnitSpecialization = LintWarning("unit-special", "Warn for specialization of Unit in parameter position.")
val MultiargInfix = LintWarning("multiarg-infix", "Infix operator was defined or used with multiarg operand.")
val ImplicitRecursion = LintWarning("implicit-recursion", "Implicit resolves to an enclosing definition.")
val UniversalMethods = LintWarning("universal-methods", "Require arg to is/asInstanceOf.")
val UniversalMethods = LintWarning("universal-methods", "Require arg to is/asInstanceOf. No Unit receiver.")
val NumericMethods = LintWarning("numeric-methods", "Dubious usages, such as `42.isNaN`.")
val ArgDiscard = LintWarning("arg-discard", "-Wvalue-discard for adapted arguments.")
val IntDivToFloat = LintWarning("int-div-to-float", "Warn when an integer division is converted (widened) to floating point: `(someInt / 2): Double`.")

Expand Down Expand Up @@ -248,6 +249,7 @@ trait Warnings {
def multiargInfix = lint contains MultiargInfix
def lintImplicitRecursion = lint.contains(ImplicitRecursion) || (warnSelfImplicit.value: @nowarn("cat=deprecation"))
def lintUniversalMethods = lint.contains(UniversalMethods)
def lintNumericMethods = lint.contains(NumericMethods)
def lintArgDiscard = lint.contains(ArgDiscard)
def lintIntDivToFloat = lint.contains(IntDivToFloat)

Expand Down
13 changes: 12 additions & 1 deletion src/compiler/scala/tools/nsc/typechecker/Typers.scala
Expand Up @@ -1265,7 +1265,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
adaptAfterOverloadResolution(tree, mode, pt, original)
case NullaryMethodType(restpe) => // (2)
if (hasUndets && settings.lintUniversalMethods && (isCastSymbol(tree.symbol) || isTypeTestSymbol(tree.symbol)) && context.undetparams.exists(_.owner == tree.symbol))
context.warning(tree.pos, s"missing type argument to ${tree.symbol}", WarningCategory.LintAdaptedArgs)
context.warning(tree.pos, s"missing type argument to ${tree.symbol}", WarningCategory.LintUniversalMethods)
val resTpDeconst = // keep constant types when they are safe to fold. erasure eliminates constant types modulo some exceptions, so keep those.
if (isBeforeErasure && tree.symbol.isAccessor && tree.symbol.hasFlag(STABLE) && treeInfo.isExprSafeToInline(tree)) restpe
else restpe.deconst
Expand Down Expand Up @@ -5292,16 +5292,26 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
def asDynamicCall = mkInvoke(context, tree, qual, name) map { t =>
wrapErrors(t, _.typed1(t, mode, pt))
}
def checkDubiousUnitSelection(result: Tree): Unit =
if (!isPastTyper && isUniversalMember(result.symbol))
context.warning(tree.pos, s"dubious usage of ${result.symbol} with unit value", WarningCategory.LintUniversalMethods)

val sym = tree.symbol orElse member(qual.tpe, name) orElse inCompanionForJavaStatic(qual.tpe.typeSymbol, name)
if ((sym eq NoSymbol) && name != nme.CONSTRUCTOR && mode.inAny(EXPRmode | PATTERNmode)) {
// symbol not found? --> try to convert implicitly to a type that does have the required
// member. Added `| PATTERNmode` to allow enrichment in patterns (so we can add e.g., an
// xml member to StringContext, which in turn has an unapply[Seq] method)

def checkDubiousAdaptation(sel: Tree): Unit = if (!isPastTyper && settings.lintNumericMethods) {
val dubious = ScalaIntegralValueClasses(qualTp.typeSymbol) && (
sel.symbol.owner.eq(BoxedFloatClass) || sel.symbol.owner.eq(RichFloatClass))
if (dubious)
context.warning(tree.pos, s"dubious usage of ${sel.symbol} with integer value", WarningCategory.LintNumericMethods)
}
val qual1 = adaptToMemberWithArgs(tree, qual, name, mode)
if ((qual1 ne qual) && !qual1.isErrorTyped)
return typed(treeCopy.Select(tree, qual1, name), mode, pt)
.tap(checkDubiousAdaptation)
}

// This special-case complements the logic in `adaptMember` in erasure, it handles selections
Expand Down Expand Up @@ -5419,6 +5429,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
name
)
case _ =>
if (settings.lintUniversalMethods && qualTp.widen.eq(UnitTpe)) checkDubiousUnitSelection(result)
result
}
}
Expand Down
24 changes: 15 additions & 9 deletions src/partest/scala/tools/partest/nest/AbstractRunner.scala
Expand Up @@ -296,7 +296,8 @@ class AbstractRunner(val config: RunnerSpec.Config, protected final val testSour

val isRerun = config.optFailed
val rerunTests = if (isRerun) testKinds.failedTests else Nil
def miscTests = individualTests ++ greppedTests ++ branchedTests ++ rerunTests
val specialTests = if (realeasy) List(Path("test/files/run/t6240-universe-code-gen.scala")) else Nil
def miscTests = List(individualTests, greppedTests, branchedTests, rerunTests, specialTests).flatten

val givenKinds = standardKinds filter config.parsed.isSet
val kinds = (
Expand All @@ -312,16 +313,21 @@ class AbstractRunner(val config: RunnerSpec.Config, protected final val testSour

def testContributors = {
List(
if (rerunTests.isEmpty) "" else "previously failed tests",
if (kindsTests.isEmpty) "" else s"${kinds.size} named test categories",
if (greppedTests.isEmpty) "" else s"${greppedTests.size} tests matching '$grepExpr'",
if (branchedTests.isEmpty) "" else s"${branchedTests.size} tests modified on this branch",
if (individualTests.isEmpty) "" else "specified tests",
).filterNot(_.isEmpty).mkString(", ")
(rerunTests, "previously failed tests"),
(kindsTests, s"${kinds.size} named test categories"),
(greppedTests, s"${greppedTests.size} tests matching '$grepExpr'"),
(branchedTests, s"${branchedTests.size} tests modified on this branch"),
(individualTests, "specified tests"),
(specialTests, "other tests you might have forgotten"),
).filterNot(_._1.isEmpty).map(_._2) match {
case Nil => "the well of despair. I see you're not in a testing mood."
case one :: Nil => one
case all => all.init.mkString("", ", ", s", and ${all.last}")
}
}

val allTests: Array[Path] = distinctBy(miscTests ++ kindsTests)(_.toCanonical).sortBy(_.toString).toArray
val grouped = (allTests groupBy kindOf).toArray sortBy (x => standardKinds indexOf x._1)
val allTests: Array[Path] = distinctBy(miscTests ::: kindsTests)(_.toCanonical).sortBy(_.toString).toArray
val grouped = allTests.groupBy(kindOf).toArray.sortBy(x => standardKinds.indexOf(x._1))

onlyIndividualTests = individualTests.nonEmpty && rerunTests.isEmpty && kindsTests.isEmpty && greppedTests.isEmpty
totalTests = allTests.size
Expand Down
3 changes: 2 additions & 1 deletion src/partest/scala/tools/partest/nest/Runner.scala
Expand Up @@ -531,7 +531,8 @@ class Runner(val testInfo: TestInfo, val suiteRunner: AbstractRunner) {
else from.toInt <= currentJavaVersion && currentJavaVersion <= to.toInt
else
currentJavaVersion >= from.toInt
if (ok) None
if (ok && suiteRunner.realeasy && from.toInt > 8) Some(genSkip(s"skipped on Java $javaSpecVersion, compiling against JDK8 but must run on $v"))
else if (ok) None
else Some(genSkip(s"skipped on Java $javaSpecVersion, only running on $v"))
case v =>
Some(genFail(s"invalid javaVersion range in test comment: $v"))
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/internal/Definitions.scala
Expand Up @@ -104,6 +104,7 @@ trait Definitions extends api.StandardDefinitions {
lazy val lazyHolders = symbolsMap(ScalaValueClasses, x => getClassIfDefined("scala.runtime.Lazy" + x))
lazy val LazyRefClass = getClassIfDefined("scala.runtime.LazyRef")
lazy val LazyUnitClass = getClassIfDefined("scala.runtime.LazyUnit")
lazy val RichFloatClass = getClassIfDefined("scala.runtime.RichFloat")

lazy val allRefClasses: Set[Symbol] = {
refClass.values.toSet ++ volatileRefClass.values.toSet ++ Set(VolatileObjectRefClass, ObjectRefClass)
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
Expand Up @@ -506,6 +506,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse =>
definitions.lazyHolders
definitions.LazyRefClass
definitions.LazyUnitClass
definitions.RichFloatClass
definitions.allRefClasses
definitions.UnitClass
definitions.ByteClass
Expand Down
162 changes: 162 additions & 0 deletions test/files/neg/t12728.check
@@ -0,0 +1,162 @@
t12728.scala:10: warning: dubious usage of method != with unit value
println(u.!=(x))
^
t12728.scala:11: warning: dubious usage of method ## with unit value
println(u.##)
^
t12728.scala:12: warning: dubious usage of method == with unit value
println(u.==(x))
^
t12728.scala:13: warning: dubious usage of method asInstanceOf with unit value
println(u.asInstanceOf[Any])
^
t12728.scala:14: warning: dubious usage of method equals with unit value
println(u.equals(x))
^
t12728.scala:15: warning: dubious usage of method hashCode with unit value
println(u.hashCode)
^
t12728.scala:16: warning: dubious usage of method isInstanceOf with unit value
println(u.isInstanceOf[Any])
^
t12728.scala:17: warning: dubious usage of method toString with unit value
println(u.toString)
^
t12728.scala:20: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.isNaN)
^
t12728.scala:20: warning: dubious usage of method isNaN with integer value
println(i.isNaN)
^
t12728.scala:21: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.isInfinity)
^
t12728.scala:21: warning: dubious usage of method isInfinity with integer value
println(i.isInfinity)
^
t12728.scala:22: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.isInfinite)
^
t12728.scala:22: warning: dubious usage of method isInfinite with integer value
println(i.isInfinite)
^
t12728.scala:23: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.isFinite)
^
t12728.scala:23: warning: dubious usage of method isFinite with integer value
println(i.isFinite)
^
t12728.scala:24: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.isPosInfinity)
^
t12728.scala:24: warning: dubious usage of method isPosInfinity with integer value
println(i.isPosInfinity)
^
t12728.scala:25: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.isNegInfinity)
^
t12728.scala:25: warning: dubious usage of method isNegInfinity with integer value
println(i.isNegInfinity)
^
t12728.scala:27: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.ceil)
^
t12728.scala:27: warning: dubious usage of method ceil with integer value
println(i.ceil)
^
t12728.scala:28: warning: Widening conversion from Int to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(i.floor)
^
t12728.scala:28: warning: dubious usage of method floor with integer value
println(i.floor)
^
t12728.scala:30: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.isNaN)
^
t12728.scala:30: warning: dubious usage of method isNaN with integer value
println(l.isNaN)
^
t12728.scala:31: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.isInfinity)
^
t12728.scala:31: warning: dubious usage of method isInfinity with integer value
println(l.isInfinity)
^
t12728.scala:32: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.isInfinite)
^
t12728.scala:32: warning: dubious usage of method isInfinite with integer value
println(l.isInfinite)
^
t12728.scala:33: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.isFinite)
^
t12728.scala:33: warning: dubious usage of method isFinite with integer value
println(l.isFinite)
^
t12728.scala:34: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.isPosInfinity)
^
t12728.scala:34: warning: dubious usage of method isPosInfinity with integer value
println(l.isPosInfinity)
^
t12728.scala:35: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.isNegInfinity)
^
t12728.scala:35: warning: dubious usage of method isNegInfinity with integer value
println(l.isNegInfinity)
^
t12728.scala:37: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.ceil)
^
t12728.scala:37: warning: dubious usage of method ceil with integer value
println(l.ceil)
^
t12728.scala:38: warning: Widening conversion from Long to Float is deprecated because it loses precision. Write `.toFloat` instead.
println(l.floor)
^
t12728.scala:38: warning: dubious usage of method floor with integer value
println(l.floor)
^
t12728.scala:40: warning: dubious usage of method isNaN with integer value
println(c.isNaN)
^
t12728.scala:41: warning: dubious usage of method isInfinity with integer value
println(c.isInfinity)
^
t12728.scala:42: warning: dubious usage of method isInfinite with integer value
println(c.isInfinite)
^
t12728.scala:43: warning: dubious usage of method isFinite with integer value
println(c.isFinite)
^
t12728.scala:44: warning: dubious usage of method isPosInfinity with integer value
println(c.isPosInfinity)
^
t12728.scala:45: warning: dubious usage of method isNegInfinity with integer value
println(c.isNegInfinity)
^
t12728.scala:47: warning: dubious usage of method ceil with integer value
println(c.ceil)
^
t12728.scala:48: warning: dubious usage of method floor with integer value
println(c.floor)
^
t12728.scala:54: warning: dubious usage of method toString with unit value
def g = new java.lang.StringBuilder("hi").setCharAt(0, 'H').toString // "()"
^
t12728.scala:14: warning: comparing values of types Unit and Any using `equals` unsafely bypasses cooperative equality; use `==` instead
println(u.equals(x))
^
t12728.scala:26: warning: method round in class RichInt is deprecated (since 2.11.0): this is an integer type; there is no reason to round it. Perhaps you meant to call this on a floating-point value?
println(i.round)
^
t12728.scala:36: warning: method round in class RichLong is deprecated (since 2.11.0): this is an integer type; there is no reason to round it. Perhaps you meant to call this on a floating-point value?
println(l.round)
^
t12728.scala:46: warning: method round in class RichInt is deprecated (since 2.11.0): this is an integer type; there is no reason to round it. Perhaps you meant to call this on a floating-point value?
println(c.round)
^
error: No warnings can be incurred under -Werror.
53 warnings
1 error
55 changes: 55 additions & 0 deletions test/files/neg/t12728.scala
@@ -0,0 +1,55 @@
// scalac: -Werror -Xlint

class C {
val u = ()
val i = 42
val l = 42L
val c = 'c'
val x = null: Any

println(u.!=(x))
println(u.##)
println(u.==(x))
println(u.asInstanceOf[Any])
println(u.equals(x))
println(u.hashCode)
println(u.isInstanceOf[Any])
println(u.toString)
println(i.toString)

println(i.isNaN)
println(i.isInfinity)
println(i.isInfinite)
println(i.isFinite)
println(i.isPosInfinity)
println(i.isNegInfinity)
println(i.round)
println(i.ceil)
println(i.floor)

println(l.isNaN)
println(l.isInfinity)
println(l.isInfinite)
println(l.isFinite)
println(l.isPosInfinity)
println(l.isNegInfinity)
println(l.round)
println(l.ceil)
println(l.floor)

println(c.isNaN)
println(c.isInfinity)
println(c.isInfinite)
println(c.isFinite)
println(c.isPosInfinity)
println(c.isNegInfinity)
println(c.round)
println(c.ceil)
println(c.floor)
}

class UseCase {
def f = new scala.StringBuilder("hi").setCharAt(0, 'H').toString // "Hi"

def g = new java.lang.StringBuilder("hi").setCharAt(0, 'H').toString // "()"
}
11 changes: 11 additions & 0 deletions test/files/run/blank.scala
@@ -0,0 +1,11 @@
// javaVersion: 11+
//
// skalac: --release:8
// trivial manual test for partest --realeasy, which sets --release:8.
// under --realeasy, skip this test because of the javaVersion, irrespective of JDK in use.
// otherwise, this test passes trivially on JDK11+ and is skipped on lesser JDKs.
// note that explicit --release:8 asks to compile against JDK8 but only run on the requested version.

object Test extends App {
assert("".isBlank) // String#isBlank was added in JDK11
}
4 changes: 3 additions & 1 deletion test/junit/scala/ArrayTest.scala
Expand Up @@ -4,11 +4,13 @@ import org.junit.Assert.{ assertArrayEquals, assertFalse, assertTrue }
import org.junit.Test

import scala.runtime.BoxedUnit
import scala.util.chaining._

class ArrayTest {
@Test
def testArrayCopyOfUnit(): Unit = {
val expected = new Array[BoxedUnit](32).asInstanceOf[Array[AnyRef]]; java.util.Arrays.fill(expected, ().asInstanceOf[AnyRef])
val expected = new Array[BoxedUnit](32).asInstanceOf[Array[AnyRef]]
.tap(array => java.util.Arrays.fill(array, (): Any))
assertArrayEquals(expected, Array.copyOf(Array[Unit](), 32).asInstanceOf[Array[AnyRef]])
assertArrayEquals(expected, Array.copyAs[Unit](Array[Nothing](), 32).asInstanceOf[Array[AnyRef]])
assertArrayEquals(expected, Array.copyAs[Unit](Array[Unit](), 32).asInstanceOf[Array[AnyRef]])
Expand Down

0 comments on commit d5247e9

Please sign in to comment.