From 4e488a60594664046c3449e1aa2239adca7a012e Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Fri, 29 Jul 2011 15:38:50 +0000 Subject: Added two new compiler options: -Ywarn-adapted-args // also included in -Xlint -Yno-adapted-args The former warns when a () is inserted or an argument list is implicitly tupled. The latter errors under the same conditions. Using these options I found several bugs in the distribution which would otherwise be nearly impossible to spot. These bugs were innocuous (I think) but similar bugs could easily be (and have been) otherwise. Certain particularly threatening scenarios are at minimum warned about regardless of options given. Closes SI-4851, no review. --- src/actors/scala/actors/Future.scala | 4 +- .../scala/reflect/internal/Definitions.scala | 5 ++ src/compiler/scala/reflect/internal/Symbols.scala | 5 +- src/compiler/scala/tools/nsc/CompileServer.scala | 3 +- .../scala/tools/nsc/ast/parser/Parsers.scala | 5 +- .../scala/tools/nsc/interpreter/ByteCode.scala | 2 +- .../scala/tools/nsc/settings/ScalaSettings.scala | 3 +- .../scala/tools/nsc/settings/Warnings.scala | 4 +- .../scala/tools/nsc/typechecker/Adaptations.scala | 82 ++++++++++++++++++++++ .../scala/tools/nsc/typechecker/Modes.scala | 3 + .../scala/tools/nsc/typechecker/Typers.scala | 17 +++-- src/compiler/scala/tools/reflect/Shield.scala | 2 +- src/library/scala/Enumeration.scala | 2 +- test/files/neg/bug4851.check | 43 ++++++++++++ test/files/neg/bug4851.flags | 1 + test/files/neg/bug4851/J.java | 15 ++++ test/files/neg/bug4851/J2.java | 11 +++ test/files/neg/bug4851/S.scala | 23 ++++++ 18 files changed, 212 insertions(+), 18 deletions(-) create mode 100644 src/compiler/scala/tools/nsc/typechecker/Adaptations.scala create mode 100644 test/files/neg/bug4851.check create mode 100644 test/files/neg/bug4851.flags create mode 100644 test/files/neg/bug4851/J.java create mode 100644 test/files/neg/bug4851/J2.java create mode 100644 test/files/neg/bug4851/S.scala diff --git a/src/actors/scala/actors/Future.scala b/src/actors/scala/actors/Future.scala index c6b575d0ee..aaaf5691f1 100644 --- a/src/actors/scala/actors/Future.scala +++ b/src/actors/scala/actors/Future.scala @@ -102,7 +102,9 @@ private class FutureActor[T](fun: SyncVar[T] => Unit, channel: Channel[T]) exten loop { react { - case Eval => reply() + // This is calling ReplyReactor#reply(msg: Any). + // Was: reply(). Now: reply(()). + case Eval => reply(()) } } } diff --git a/src/compiler/scala/reflect/internal/Definitions.scala b/src/compiler/scala/reflect/internal/Definitions.scala index 00d2c232a0..857d7470d4 100644 --- a/src/compiler/scala/reflect/internal/Definitions.scala +++ b/src/compiler/scala/reflect/internal/Definitions.scala @@ -250,6 +250,8 @@ trait Definitions extends reflect.api.StandardDefinitions { def scalaRuntimeSameElements = getMember(ScalaRunTimeModule, nme.sameElements) // classes with special meanings + lazy val StringAddClass = getClass("scala.runtime.StringAdd") + lazy val StringAdd_+ = getMember(StringAddClass, nme.PLUS) lazy val NotNullClass = getClass("scala.NotNull") lazy val DelayedInitClass = getClass("scala.DelayedInit") def delayedInitMethod = getMember(DelayedInitClass, nme.delayedInit) @@ -363,6 +365,9 @@ trait Definitions extends reflect.api.StandardDefinitions { lazy val NoneModule: Symbol = getModule("scala.None") lazy val SomeModule: Symbol = getModule("scala.Some") + // The given symbol represents either String.+ or StringAdd.+ + def isStringAddition(sym: Symbol) = sym == String_+ || sym == StringAdd_+ + def isOptionType(tp: Type) = cond(tp.normalize) { case TypeRef(_, OptionClass, List(_)) => true } def isSomeType(tp: Type) = cond(tp.normalize) { case TypeRef(_, SomeClass, List(_)) => true } def isNoneType(tp: Type) = cond(tp.normalize) { case TypeRef(_, NoneModule, List(_)) => true } diff --git a/src/compiler/scala/reflect/internal/Symbols.scala b/src/compiler/scala/reflect/internal/Symbols.scala index af6b561699..5f2e060d6d 100644 --- a/src/compiler/scala/reflect/internal/Symbols.scala +++ b/src/compiler/scala/reflect/internal/Symbols.scala @@ -1804,6 +1804,7 @@ trait Symbols extends api.Symbols { self: SymbolTable => case s => " in " + s } def fullLocationString = toString + locationString + def signatureString = if (hasRawInfo) infoString(rawInfo) else "<_>" /** String representation of symbol's definition following its name */ final def infoString(tp: Type): String = { @@ -1866,9 +1867,7 @@ trait Symbols extends api.Symbols { self: SymbolTable => def defString = compose( defaultFlagString, keyString, - varianceString + nameString + ( - if (hasRawInfo) infoString(rawInfo) else "<_>" - ) + varianceString + nameString + signatureString ) /** Concatenate strings separated by spaces */ diff --git a/src/compiler/scala/tools/nsc/CompileServer.scala b/src/compiler/scala/tools/nsc/CompileServer.scala index e72bc705c1..c23a671ef8 100644 --- a/src/compiler/scala/tools/nsc/CompileServer.scala +++ b/src/compiler/scala/tools/nsc/CompileServer.scala @@ -87,8 +87,7 @@ class StandardCompileServer extends SocketServer { val input = in.readLine() def fscError(msg: String): Unit = out println ( - FakePos("fsc"), - msg + "\n fsc -help gives more information" + FakePos("fsc") + msg + "\n fsc -help gives more information" ) if (input == null || password != guessedPassword) return diff --git a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala index 3ea091c394..cf2814165a 100644 --- a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala +++ b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala @@ -2589,9 +2589,12 @@ self => */ def templateParents(isTrait: Boolean): (List[Tree], List[List[Tree]]) = { val parents = new ListBuffer[Tree] += startAnnotType() - val argss = + val argss = ( + // TODO: the insertion of List(Nil) here is where "new Foo" becomes + // indistinguishable from "new Foo()". if (in.token == LPAREN && !isTrait) multipleArgumentExprs() else List(Nil) + ) while (in.token == WITH) { in.nextToken() diff --git a/src/compiler/scala/tools/nsc/interpreter/ByteCode.scala b/src/compiler/scala/tools/nsc/interpreter/ByteCode.scala index ece80af272..90d8fbb356 100644 --- a/src/compiler/scala/tools/nsc/interpreter/ByteCode.scala +++ b/src/compiler/scala/tools/nsc/interpreter/ByteCode.scala @@ -18,7 +18,7 @@ object ByteCode { */ private lazy val DECODER: Option[AnyRef] = for (clazz <- getSystemLoader.tryToLoadClass[AnyRef]("scala.tools.scalap.Decode$")) yield - clazz.getField(MODULE_INSTANCE_NAME).get() + clazz.getField(MODULE_INSTANCE_NAME).get(null) private def decoderMethod(name: String, args: JClass*): Option[reflect.Method] = { for (decoder <- DECODER ; m <- Option(decoder.getClass.getMethod(name, args: _*))) yield m diff --git a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala index 838dd7404f..6325fd6eb4 100644 --- a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala +++ b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala @@ -59,7 +59,7 @@ trait ScalaSettings extends AbsScalaSettings val future = BooleanSetting ("-Xfuture", "Turn on future language features.") val genPhaseGraph = StringSetting ("-Xgenerate-phase-graph", "file", "Generate the phase graphs (outputs .dot files) to fileX.dot.", "") val XlogImplicits = BooleanSetting ("-Xlog-implicits", "Show more detail on why some implicits are not applicable.") - val maxClassfileName = IntSetting ("-Xmax-classfile-name", "Maximum filename length for generated classes", 255, Some(72, 255), _ => None) + val maxClassfileName = IntSetting ("-Xmax-classfile-name", "Maximum filename length for generated classes", 255, Some(72, 255), _ => None) val Xmigration28 = BooleanSetting ("-Xmigration", "Warn about constructs whose behavior may have changed between 2.7 and 2.8.") val nouescape = BooleanSetting ("-Xno-uescape", "Disable handling of \\u unicode escapes.") val Xnojline = BooleanSetting ("-Xnojline", "Do not use JLine for editing.") @@ -117,6 +117,7 @@ trait ScalaSettings extends AbsScalaSettings val Ynogenericsig = BooleanSetting ("-Yno-generic-signatures", "Suppress generation of generic signatures for Java.") val noimports = BooleanSetting ("-Yno-imports", "Compile without importing scala.*, java.lang.*, or Predef.") val nopredef = BooleanSetting ("-Yno-predef", "Compile without importing Predef.") + val noAdaptedArgs = BooleanSetting ("-Yno-adapted-args", "Do not adapt an argument list (either by inserting () or creating a tuple) to match the receiver.") val Yprofile = PhasesSetting ("-Yprofile", "(Requires jvm -agentpath to contain yjgpagent) Profile CPU usage of given phases.") val YprofileMem = BooleanSetting ("-Yprofile-memory", "Profile memory, get heap snapshot after each compiler run (requires yjpagent, see above).") val YprofileClass = StringSetting ("-Yprofile-class", "class", "Name of profiler class.", "scala.tools.util.YourkitProfiling") diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala index 0d42fa503d..b2e3d51ad4 100644 --- a/src/compiler/scala/tools/nsc/settings/Warnings.scala +++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala @@ -28,7 +28,8 @@ trait Warnings { warnDeadCode, warnInaccessible, warnNullaryOverride, - warnNullaryUnit + warnNullaryUnit, + warnAdaptedArgs ) // Warning groups. @@ -43,6 +44,7 @@ trait Warnings { // Individual warnings. val warnSelectNullable = BooleanSetting ("-Xcheck-null", "Warn upon selection of nullable reference.") + val warnAdaptedArgs = BooleanSetting ("-Ywarn-adapted-args", "Warn if an argument list is modified to match the receiver.") val warnDeadCode = BooleanSetting ("-Ywarn-dead-code", "Warn when dead code is identified.") val warnValueDiscard = BooleanSetting ("-Ywarn-value-discard", "Warn when non-Unit expression results are unused.") val warnNumericWiden = BooleanSetting ("-Ywarn-numeric-widen", "Warn when numerics are widened.") diff --git a/src/compiler/scala/tools/nsc/typechecker/Adaptations.scala b/src/compiler/scala/tools/nsc/typechecker/Adaptations.scala new file mode 100644 index 0000000000..3dee4650de --- /dev/null +++ b/src/compiler/scala/tools/nsc/typechecker/Adaptations.scala @@ -0,0 +1,82 @@ +/* NSC -- new Scala compiler + * Copyright 2005-2011 LAMP/EPFL + * @author Paul Phillips + */ + +package scala.tools.nsc +package typechecker + +/** This trait provides logic for assessing the validity of argument + * adaptations, such as tupling, unit-insertion, widening, etc. Such + * logic is spread around the compiler, without much ability on the + * part of the user to tighten the potentially dangerous bits. + * + * TODO: unifying/consolidating said logic under consistent management. + * + * @author Paul Phillips + */ +trait Adaptations { + self: Analyzer => + + import global._ + import definitions._ + + trait Adaptation { + self: Typer => + + def checkValidAdaptation(t: Tree, args: List[Tree]): Boolean = { + def applyArg = t match { + case Apply(_, arg :: Nil) => arg + case _ => EmptyTree + } + def callString = ( + ( if (t.symbol.isConstructor) "new " else "" ) + + ( t.symbol.owner.decodedName ) + + ( if (t.symbol.isConstructor || t.symbol.name == nme.apply) "" else "." + t.symbol.decodedName ) + ) + def sigString = t.symbol.owner.decodedName + ( + if (t.symbol.isConstructor) t.symbol.signatureString + else "." + t.symbol.decodedName + t.symbol.signatureString + ) + def givenString = if (args.isEmpty) "" else args.mkString(", ") + def adaptedArgs = if (args.isEmpty) "(): Unit" else args.mkString("(", ", ", "): " + applyArg.tpe) + + def adaptWarning(msg: String) = context.warning(t.pos, msg + + "\n signature: " + sigString + + "\n given arguments: " + givenString + + "\n after adaptation: " + callString + "(" + adaptedArgs + ")" + ) + // A one-argument method accepting Object (which may look like "Any" + // at this point if the class is java defined) is a "leaky target" for + // which we should be especially reluctant to insert () or auto-tuple. + def isLeakyTarget = { + val oneArgObject = t.symbol.paramss match { + case (param :: Nil) :: Nil => ObjectClass isSubClass param.tpe.typeSymbol + case _ => false + } + // Unfortunately various "universal" methods and the manner in which + // they are used limits our ability to enforce anything sensible until + // an opt-in compiler option is given. + oneArgObject && !( + isStringAddition(t.symbol) + || t.symbol.name == nme.equals_ + || t.symbol.name == nme.EQ + || t.symbol.name == nme.NE + ) + } + + if (settings.noAdaptedArgs.value) + adaptWarning("No automatic adaptation here: use explicit parentheses.") + else if (settings.warnAdaptedArgs.value) + adaptWarning( + if (args.isEmpty) "Adapting argument list by inserting (): " + ( + if (isLeakyTarget) "leaky (Object-receiving) target makes this especially dangerous." + else "this is unlikely to be what you want." + ) + else "Adapting argument list by creating a " + args.size + "-tuple: this may not be what you want." + ) + + !settings.noAdaptedArgs.value + } + } +} diff --git a/src/compiler/scala/tools/nsc/typechecker/Modes.scala b/src/compiler/scala/tools/nsc/typechecker/Modes.scala index ad4be4662c..48068b58d4 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Modes.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Modes.scala @@ -106,6 +106,9 @@ trait Modes { final def inPolyMode(mode: Int) = (mode & POLYmode) != 0 final def inPatternMode(mode: Int) = (mode & PATTERNmode) != 0 + final def inExprModeButNot(mode: Int, prohibited: Int) = + (mode & (EXPRmode | prohibited)) == EXPRmode + /** Translates a mask of mode flags into something readable. */ private val modeNameMap = Map[Int, String]( diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 13e0f13d4c..4125ab0285 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -27,7 +27,7 @@ import scala.tools.util.StringOps.{ countAsString, countElementsAsString } * @author Martin Odersky * @version 1.0 */ -trait Typers extends Modes { +trait Typers extends Modes with Adaptations { self: Analyzer => import global._ @@ -77,7 +77,7 @@ trait Typers extends Modes { // that are turned private by typedBlock private final val SYNTHETIC_PRIVATE = TRANS_FLAG - abstract class Typer(context0: Context) extends TyperDiagnostics { + abstract class Typer(context0: Context) extends TyperDiagnostics with Adaptation { import context0.unit import typeDebug.{ ptTree, ptBlock, ptLine } @@ -882,7 +882,7 @@ trait Typers extends Modes { typed(atPos(tree.pos)(Select(qual, nme.apply)), mode, pt) } else if (!context.undetparams.isEmpty && !inPolyMode(mode)) { // (9) assert(!inHKMode(mode)) //@M - if ((mode & (EXPRmode | FUNmode)) == EXPRmode && (pt.typeSymbol == UnitClass)) + if (inExprModeButNot(mode, FUNmode) && pt.typeSymbol == UnitClass) instantiateExpectingUnit(tree, mode) else instantiate(tree, mode, pt) @@ -898,7 +898,7 @@ trait Typers extends Modes { val tree1 = constfold(tree, pt) // (10) (11) if (tree1.tpe <:< pt) adapt(tree1, mode, pt, original) else { - if ((mode & (EXPRmode | FUNmode)) == EXPRmode) { + if (inExprModeButNot(mode, FUNmode)) { pt.normalize match { case TypeRef(_, sym, _) => // note: was if (pt.typeSymbol == UnitClass) but this leads to a potentially @@ -2346,8 +2346,13 @@ trait Typers extends Modes { val savedUndetparams = context.undetparams silent(_.doTypedApply(tree, fun, tupleArgs, mode, pt)) match { case t: Tree => -// println("tuple conversion to "+t+" for "+mt)//DEBUG - Some(t) + // Depending on user options, may warn or error here if + // a Unit or tuple was inserted. + Some(t) filter (tupledTree => + !inExprModeButNot(mode, FUNmode) + || tupledTree.symbol == null + || checkValidAdaptation(tupledTree, args) + ) case ex => context.undetparams = savedUndetparams None diff --git a/src/compiler/scala/tools/reflect/Shield.scala b/src/compiler/scala/tools/reflect/Shield.scala index 19730791b9..f9c7e54454 100644 --- a/src/compiler/scala/tools/reflect/Shield.scala +++ b/src/compiler/scala/tools/reflect/Shield.scala @@ -36,7 +36,7 @@ trait Shield { def method(name: String, arity: Int) = uniqueMethod(name, arity) def field(name: String) = clazz getField name - def matchingMethods(name: String, arity: Int) = methods filter (m => nameAndArity(m) == (name, arity)) + def matchingMethods(name: String, arity: Int) = methods filter (m => nameAndArity(m) == ((name, arity))) def uniqueMethod(name: String, arity: Int) = matchingMethods(name, arity) match { case List(x) => x case _ => onError("No unique match for " + name) diff --git a/src/library/scala/Enumeration.scala b/src/library/scala/Enumeration.scala index 22bb98ba46..0feac3143f 100644 --- a/src/library/scala/Enumeration.scala +++ b/src/library/scala/Enumeration.scala @@ -61,7 +61,7 @@ abstract class Enumeration(initial: Int, names: String*) extends Serializable { /* Note that `readResolve` cannot be private, since otherwise the JVM does not invoke it when deserializing subclasses. */ - protected def readResolve(): AnyRef = thisenum.getClass.getField(MODULE_INSTANCE_NAME).get() + protected def readResolve(): AnyRef = thisenum.getClass.getField(MODULE_INSTANCE_NAME).get(null) /** The name of this enumeration. */ diff --git a/test/files/neg/bug4851.check b/test/files/neg/bug4851.check new file mode 100644 index 0000000000..8011350f23 --- /dev/null +++ b/test/files/neg/bug4851.check @@ -0,0 +1,43 @@ +S.scala:2: error: Adapting argument list by inserting (): leaky (Object-receiving) target makes this especially dangerous. + signature: J(x: Any): J + given arguments: + after adaptation: new J((): Unit) + val x1 = new J + ^ +S.scala:3: error: Adapting argument list by inserting (): leaky (Object-receiving) target makes this especially dangerous. + signature: J(x: Any): J + given arguments: + after adaptation: new J((): Unit) + val x2 = new J() + ^ +S.scala:4: error: Adapting argument list by creating a 5-tuple: this may not be what you want. + signature: J(x: Any): J + given arguments: 1, 2, 3, 4, 5 + after adaptation: new J((1, 2, 3, 4, 5): (Int, Int, Int, Int, Int)) + val x3 = new J(1, 2, 3, 4, 5) + ^ +S.scala:6: error: Adapting argument list by creating a 3-tuple: this may not be what you want. + signature: Some.apply[A](x: A): Some[A] + given arguments: 1, 2, 3 + after adaptation: Some((1, 2, 3): (Int, Int, Int)) + val y1 = Some(1, 2, 3) + ^ +S.scala:7: error: Adapting argument list by creating a 3-tuple: this may not be what you want. + signature: Some(x: A): Some[A] + given arguments: 1, 2, 3 + after adaptation: new Some((1, 2, 3): (Int, Int, Int)) + val y2 = new Some(1, 2, 3) + ^ +S.scala:9: error: Adapting argument list by inserting (): this is unlikely to be what you want. + signature: J2[T](x: T): J2[T] + given arguments: + after adaptation: new J2((): Unit) + val z1 = new J2 + ^ +S.scala:10: error: Adapting argument list by inserting (): this is unlikely to be what you want. + signature: J2[T](x: T): J2[T] + given arguments: + after adaptation: new J2((): Unit) + val z2 = new J2() + ^ +7 errors found diff --git a/test/files/neg/bug4851.flags b/test/files/neg/bug4851.flags new file mode 100644 index 0000000000..0545cb8b84 --- /dev/null +++ b/test/files/neg/bug4851.flags @@ -0,0 +1 @@ +-Ywarn-adapted-args -Xfatal-warnings diff --git a/test/files/neg/bug4851/J.java b/test/files/neg/bug4851/J.java new file mode 100644 index 0000000000..9c35b8a16e --- /dev/null +++ b/test/files/neg/bug4851/J.java @@ -0,0 +1,15 @@ +public class J { + Object x; + + public J(Object x) { + this.x = x; + } + + public J(int x1, int x2, int x3, int x4, int x5, int x6) { + this.x = null; + } + + public String toString() { + return "J:" + x.getClass(); + } +} \ No newline at end of file diff --git a/test/files/neg/bug4851/J2.java b/test/files/neg/bug4851/J2.java new file mode 100644 index 0000000000..82954d9489 --- /dev/null +++ b/test/files/neg/bug4851/J2.java @@ -0,0 +1,11 @@ +public class J2 { + T x; + + public J(T x) { + this.x = x; + } + + public String toString() { + return "J2:" + x.getClass(); + } +} \ No newline at end of file diff --git a/test/files/neg/bug4851/S.scala b/test/files/neg/bug4851/S.scala new file mode 100644 index 0000000000..deb42980ab --- /dev/null +++ b/test/files/neg/bug4851/S.scala @@ -0,0 +1,23 @@ +object Test { + val x1 = new J + val x2 = new J() + val x3 = new J(1, 2, 3, 4, 5) + + val y1 = Some(1, 2, 3) + val y2 = new Some(1, 2, 3) + + val z1 = new J2 + val z2 = new J2() + val z3 = new J2(()) + + def main(args: Array[String]): Unit = { + println(x1) + println(x2) + println(x3) + println(y1) + + println(z1) + println(z2) + println(z3) + } +} -- cgit v1.2.3