From 399bd6240f775583ee9709311bd0b02e8359c15c Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Tue, 20 Mar 2012 10:35:10 -0700 Subject: Never write final fields outside of constructors. Closes SI-3569, SI-3770. Also threw in experimental -Yoverride-vars. It's not robust. --- src/compiler/scala/reflect/internal/Flags.scala | 2 +- .../scala/tools/nsc/backend/jvm/GenJVM.scala | 34 ++++++++++++++------ .../scala/tools/nsc/settings/ScalaSettings.scala | 3 +- .../scala/tools/nsc/typechecker/Namers.scala | 5 +-- .../scala/tools/nsc/typechecker/RefChecks.scala | 15 ++++++--- .../tools/nsc/typechecker/SuperAccessors.scala | 3 +- test/files/run/finalvar.check | 6 ++++ test/files/run/finalvar.flags | 1 + test/files/run/finalvar.scala | 37 ++++++++++++++++++++++ 9 files changed, 86 insertions(+), 20 deletions(-) create mode 100644 test/files/run/finalvar.check create mode 100644 test/files/run/finalvar.flags create mode 100644 test/files/run/finalvar.scala diff --git a/src/compiler/scala/reflect/internal/Flags.scala b/src/compiler/scala/reflect/internal/Flags.scala index 3110d73461..ce1c8d0908 100644 --- a/src/compiler/scala/reflect/internal/Flags.scala +++ b/src/compiler/scala/reflect/internal/Flags.scala @@ -84,7 +84,7 @@ import scala.collection.{ mutable, immutable } */ class ModifierFlags { final val IMPLICIT = 0x00000200 - final val FINAL = 0x00000020 + final val FINAL = 0x00000020 // May not be overridden. Note that java final implies much more than scala final. final val PRIVATE = 0x00000004 final val PROTECTED = 0x00000001 diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala index 8e326c202a..b7b4212b93 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala @@ -871,7 +871,7 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with debuglog("Adding field: " + f.symbol.fullName) val jfield = jclass.addNewField( - javaFlags(f.symbol) | javaFieldFlags(f.symbol), + javaFieldFlags(f.symbol), javaName(f.symbol), javaType(f.symbol.tpe) ) @@ -1915,16 +1915,30 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with val privateFlag = sym.isPrivate || (sym.isPrimaryConstructor && isTopLevelModule(sym.owner)) - // This does not check .isFinal (which checks flags for the FINAL flag), - // instead checking rawflags for that flag so as to exclude symbols which - // received lateFINAL. These symbols are eligible for inlining, but to - // avoid breaking proxy software which depends on subclassing, we avoid - // insisting on their finality in the bytecode. + // Final: the only fields which can receive ACC_FINAL are eager vals. + // Neither vars nor lazy vals can, because: + // + // Source: http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5.3 + // "Another problem is that the specification allows aggressive + // optimization of final fields. Within a thread, it is permissible to + // reorder reads of a final field with those modifications of a final + // field that do not take place in the constructor." + // + // A var or lazy val which is marked final still has meaning to the + // scala compiler. The word final is heavily overloaded unfortunately; + // for us it means "not overridable". At present you can't override + // vars regardless; this may change. + // + // The logic does not check .isFinal (which checks flags for the FINAL flag, + // and includes symbols marked lateFINAL) instead inspecting rawflags so + // we can exclude lateFINAL. Such symbols are eligible for inlining, but to + // avoid breaking proxy software which depends on subclassing, we do not + // emit ACC_FINAL. val finalFlag = ( ((sym.rawflags & (Flags.FINAL | Flags.MODULE)) != 0) && !sym.enclClass.isInterface && !sym.isClassConstructor - && !sym.isMutable // fix for SI-3569, it is too broad? + && !sym.isMutable // lazy vals and vars both ) mkFlags( @@ -1939,13 +1953,13 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with if (sym.hasFlag(Flags.SYNCHRONIZED)) JAVA_ACC_SYNCHRONIZED else 0 ) } - def javaFieldFlags(sym: Symbol) = { - mkFlags( + def javaFieldFlags(sym: Symbol) = ( + javaFlags(sym) | mkFlags( if (sym hasAnnotation TransientAttr) ACC_TRANSIENT else 0, if (sym hasAnnotation VolatileAttr) ACC_VOLATILE else 0, if (sym.isMutable) 0 else ACC_FINAL ) - } + ) def isTopLevelModule(sym: Symbol): Boolean = afterPickler { sym.isModuleClass && !sym.isImplClass && !sym.isNestedClass } diff --git a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala index fdde8f9990..14b3bcc8ce 100644 --- a/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala +++ b/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala @@ -31,7 +31,7 @@ trait ScalaSettings extends AbsScalaSettings protected def defaultClasspath = sys.env.getOrElse("CLASSPATH", ".") /** Enabled under -Xexperimental. */ - protected def experimentalSettings = List[BooleanSetting](YmethodInfer, overrideObjects) + protected def experimentalSettings = List[BooleanSetting](YmethodInfer, overrideObjects, overrideVars) /** Enabled under -Xfuture. */ protected def futureSettings = List[BooleanSetting]() @@ -117,6 +117,7 @@ trait ScalaSettings extends AbsScalaSettings * -Y "Private" settings */ val overrideObjects = BooleanSetting ("-Yoverride-objects", "Allow member objects to be overridden.") + val overrideVars = BooleanSetting ("-Yoverride-vars", "Allow vars to be overridden.") val Yhelp = BooleanSetting ("-Y", "Print a synopsis of private options.") val browse = PhasesSetting ("-Ybrowse", "Browse the abstract syntax tree after") val check = PhasesSetting ("-Ycheck", "Check the tree at the end of") diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala index 6b27c27652..8604366bf2 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala @@ -315,9 +315,10 @@ trait Namers extends MethodSynthesis { case DefDef(_, _, _, _, _, _) => owner.newMethod(name.toTermName, pos, flags) case ClassDef(_, _, _, _) => owner.newClassSymbol(name.toTypeName, pos, flags) case ModuleDef(_, _, _) => owner.newModule(name, pos, flags) - case ValDef(_, _, _, _) if isParameter => owner.newValueParameter(name, pos, flags) case PackageDef(pid, _) => createPackageSymbol(pos, pid) - case ValDef(_, _, _, _) => owner.newValue(name, pos, flags) + case ValDef(_, _, _, _) => + if (isParameter) owner.newValueParameter(name, pos, flags) + else owner.newValue(name, pos, flags) } } private def createFieldSymbol(tree: ValDef): TermSymbol = diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 013a74da7e..9177aca656 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -394,9 +394,14 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R overrideError("needs `override' modifier") } else if (other.isAbstractOverride && other.isIncompleteIn(clazz) && !member.isAbstractOverride) { overrideError("needs `abstract override' modifiers") - } else if (member.isAnyOverride && (other hasFlag ACCESSOR) && other.accessed.isVariable && !other.accessed.isLazy) { - overrideError("cannot override a mutable variable") - } else if (member.isAnyOverride && + } + else if (member.isAnyOverride && (other hasFlag ACCESSOR) && other.accessed.isVariable && !other.accessed.isLazy) { + // !?! this is not covered by the spec. We need to resolve this either by changing the spec or removing the test here. + // !!! is there a !?! convention? I'm !!!ing this to make sure it turns up on my searches. + if (!settings.overrideVars.value) + overrideError("cannot override a mutable variable") + } + else if (member.isAnyOverride && !(member.owner.thisType.baseClasses exists (_ isSubClass other.owner)) && !member.isDeferred && !other.isDeferred && intersectionIsEmpty(member.extendedOverriddenSymbols, other.extendedOverriddenSymbols)) { @@ -1248,9 +1253,9 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R } List(tree1) } - case Import(_, _) => Nil + case Import(_, _) => Nil case DefDef(mods, _, _, _, _, _) if (mods hasFlag MACRO) => Nil - case _ => List(transform(tree)) + case _ => List(transform(tree)) } /* Check whether argument types conform to bounds of type parameters */ diff --git a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala index 4248b6f024..94733369a8 100644 --- a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala +++ b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala @@ -259,7 +259,8 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT case sel @ Select(Super(_, mix), name) => if (sym.isValue && !sym.isMethod || sym.hasAccessorFlag) { - unit.error(tree.pos, "super may be not be used on "+ sym.accessedOrSelf) + if (!settings.overrideVars.value) + unit.error(tree.pos, "super may be not be used on "+ sym.accessedOrSelf) } else if (isDisallowed(sym)) { unit.error(tree.pos, "super not allowed here: use this." + name.decode + " instead") diff --git a/test/files/run/finalvar.check b/test/files/run/finalvar.check new file mode 100644 index 0000000000..2496293972 --- /dev/null +++ b/test/files/run/finalvar.check @@ -0,0 +1,6 @@ +(2,2,2,2,1) +(2,2,2,2) +(2,2,2,2,1001) +(2,2,2,2) +2 +10 diff --git a/test/files/run/finalvar.flags b/test/files/run/finalvar.flags new file mode 100644 index 0000000000..aee3039bec --- /dev/null +++ b/test/files/run/finalvar.flags @@ -0,0 +1 @@ +-Yoverride-vars -Yinline \ No newline at end of file diff --git a/test/files/run/finalvar.scala b/test/files/run/finalvar.scala new file mode 100644 index 0000000000..010813e520 --- /dev/null +++ b/test/files/run/finalvar.scala @@ -0,0 +1,37 @@ +object Final { + class X(final var x: Int) { } + def f = new X(0).x += 1 +} + +class A { + var x = 1 + def y0 = x + def y1 = this.x + def y2 = (this: A).x +} + +class B extends A { + override def x = 2 + def z = super.x +} + +object Test { + def main(args: Array[String]): Unit = { + Final.f + val a = new B + println((a.x, a.y0, a.y1, a.y2, a.z)) + val a0: A = a + println((a0.x, a0.y0, a0.y1, a0.y2)) + a.x = 1001 + println((a.x, a.y0, a.y1, a.y2, a.z)) + println((a0.x, a0.y0, a0.y1, a0.y2)) + + val d = new D + println(d.w) + d.ten + println(d.w) + } +} + +class C { var w = 1 ; def ten = this.w = 10 } +class D extends C { override var w = 2 } \ No newline at end of file -- cgit v1.2.3