Skip to content

Commit 0bc8c06

Browse files
sjrdsjrd
authored
Merge pull request #5020 from sjrd/enforce-ctor-discipline
Check that constructor calls follow a chaining discipline.
2 parents 0521de4 + 20e334b commit 0bc8c06

File tree

9 files changed

+242
-44
lines changed

9 files changed

+242
-44
lines changed

‎compiler/src/main/scala/org/scalajs/nscplugin/PrepJSInterop.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ abstract class PrepJSInterop[G <: Global with Singleton](val global: G)
475475
*/
476476
val superCtorCall = gen.mkMethodCall(
477477
Super(clsSym, tpnme.EMPTY),
478-
ObjectClass.primaryConstructor, Nil, Nil)
478+
DynamicImportThunkClass.primaryConstructor, Nil, Nil)
479479

480480
// class $anon extends DynamicImportThunk
481481
val clsDef = ClassDef(clsSym, List(

‎ir/shared/src/main/scala/org/scalajs/ir/Serializers.scala

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ object Serializers {
4141
*/
4242
final val IRMagicNumber = 0xCAFE4A53
4343

44+
// For deserialization hack
45+
private final val DynamicImportThunkClass =
46+
ClassName("scala.scalajs.runtime.DynamicImportThunk")
47+
4448
def serialize(stream: OutputStream, classDef: ClassDef): Unit = {
4549
new Serializer().serialize(stream, classDef)
4650
}
@@ -1042,6 +1046,7 @@ object Serializers {
10421046

10431047
private[this] var enclosingClassName: ClassName = _
10441048
private[this] var thisTypeForHack: Option[Type] = None
1049+
private[this] var patchDynamicImportThunkSuperCtorCall: Boolean = false
10451050

10461051
def deserializeEntryPointsInfo(): EntryPointsInfo = {
10471052
hacks = new Hacks(sourceVersion = readHeader())
@@ -1218,9 +1223,24 @@ object Serializers {
12181223
case TagApply =>
12191224
Apply(readApplyFlags(), readTree(), readMethodIdent(), readTrees())(
12201225
readType())
1226+
12211227
case TagApplyStatically =>
1222-
ApplyStatically(readApplyFlags(), readTree(), readClassName(),
1223-
readMethodIdent(), readTrees())(readType())
1228+
val flags = readApplyFlags()
1229+
val receiver = readTree()
1230+
val className0 = readClassName()
1231+
val method = readMethodIdent()
1232+
val args = readTrees()
1233+
val tpe = readType()
1234+
1235+
val className = {
1236+
if (patchDynamicImportThunkSuperCtorCall && method.name.isConstructor)
1237+
DynamicImportThunkClass
1238+
else
1239+
className0
1240+
}
1241+
1242+
ApplyStatically(flags, receiver, className, method, args)(tpe)
1243+
12241244
case TagApplyStatic =>
12251245
ApplyStatic(readApplyFlags(), readClassName(), readMethodIdent(),
12261246
readTrees())(readType())
@@ -1510,6 +1530,15 @@ object Serializers {
15101530
val superClass = readOptClassIdent()
15111531
val parents = readClassIdents()
15121532

1533+
if (hacks.use17 && kind.isClass) {
1534+
/* In 1.18, we started enforcing the constructor chaining discipline.
1535+
* Unfortunately, we used to generate a wrong super constructor call in
1536+
* synthetic classes extending `DynamicImportThunk`, so we patch them.
1537+
*/
1538+
patchDynamicImportThunkSuperCtorCall =
1539+
superClass.exists(_.name == DynamicImportThunkClass)
1540+
}
1541+
15131542
/* jsSuperClass is not hacked like in readMemberDef.bodyHack5. The
15141543
* compilers before 1.6 always use a simple VarRef() as jsSuperClass,
15151544
* when there is one, so no hack is required.

‎linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala

Lines changed: 102 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ private final class ClassDefChecker(classDef: ClassDef,
236236
}
237237

238238
private def checkMethodDef(methodDef: MethodDef): Unit = withPerMethodState {
239-
val MethodDef(flags, MethodIdent(name), _, params, _, body) = methodDef
239+
val MethodDef(flags, MethodIdent(name), _, params, _, optBody) = methodDef
240240
implicit val ctx = ErrorContext(methodDef)
241241

242242
val namespace = flags.namespace
@@ -251,7 +251,7 @@ private final class ClassDefChecker(classDef: ClassDef,
251251
if (!methods(namespace.ordinal).add(name))
252252
reportError(i"duplicate ${namespace.prefixString}method '$name'")
253253

254-
if (body.isEmpty && namespace != MemberNamespace.Public)
254+
if (optBody.isEmpty && namespace != MemberNamespace.Public)
255255
reportError("Abstract methods may only be in the public namespace")
256256

257257
// ClassInitializer
@@ -299,11 +299,17 @@ private final class ClassDefChecker(classDef: ClassDef,
299299
}
300300

301301
// Body
302-
val thisType = if (static) NoType else instanceThisType
303-
val bodyEnv = Env.fromParams(params)
304-
.withThisType(thisType)
305-
.withInConstructor(isConstructor)
306-
body.foreach(checkTree(_, bodyEnv))
302+
for (body <- optBody) {
303+
val thisType = if (static) NoType else instanceThisType
304+
val bodyEnv = Env.fromParams(params)
305+
.withThisType(thisType)
306+
.withInConstructor(isConstructor)
307+
308+
if (isConstructor)
309+
checkConstructorBody(body, bodyEnv)
310+
else
311+
checkTree(body, bodyEnv)
312+
}
307313
}
308314

309315
private def checkJSConstructorDef(ctorDef: JSConstructorDef): Unit = withPerMethodState {
@@ -324,14 +330,10 @@ private final class ClassDefChecker(classDef: ClassDef,
324330
.withHasNewTarget(true)
325331
.withInConstructor(true)
326332

327-
val envJustBeforeSuper = body.beforeSuper.foldLeft(startEnv) { (prevEnv, stat) =>
328-
checkTree(stat, prevEnv)
329-
}
333+
val envJustBeforeSuper = checkBlockStats(body.beforeSuper, startEnv)
330334
checkTreeOrSpreads(body.superCall.args, envJustBeforeSuper)
331335
val envJustAfterSuper = envJustBeforeSuper.withThisType(instanceThisType)
332-
body.afterSuper.foldLeft(envJustAfterSuper) { (prevEnv, stat) =>
333-
checkTree(stat, prevEnv)
334-
}
336+
checkBlockStats(body.afterSuper, envJustAfterSuper)
335337
}
336338

337339
private def checkJSMethodDef(methodDef: JSMethodDef): Unit = withPerMethodState {
@@ -510,6 +512,80 @@ private final class ClassDefChecker(classDef: ClassDef,
510512
}
511513
}
512514

515+
private def checkConstructorBody(body: Tree, bodyEnv: Env): Unit = {
516+
/* If the enclosing class is `jl.Object`, the `body` cannot contain any
517+
* delegate constructor call.
518+
*
519+
* Otherwise:
520+
*
521+
* - Let `stats` be the list of flattened statements in `body`.
522+
* - There exists a unique `stat` in the `stats` list that is an
523+
* `ApplyStatically(_, This(), cls, someConstructor, args)`, called the
524+
* delegate constructor call.
525+
* - There is no such `ApplyStatically` anywhere else in the body.
526+
* - `cls` must be the enclosing class or its direct superclass.
527+
*
528+
* After the optimizer, there may be no delegate constructor call at all.
529+
* This frequently happens as the optimizer inlines super constructor
530+
* calls. If there is one, `cls` can be any class (it must still be some
531+
* class in the superclass chain for the types to align, but this is not
532+
* checked here).
533+
*/
534+
535+
implicit val ctx = ErrorContext(body)
536+
537+
val bodyStats = body match {
538+
case Block(stats) => stats
539+
case Skip() => Nil
540+
case _ => body :: Nil
541+
}
542+
543+
if (isJLObject) {
544+
checkBlockStats(bodyStats, bodyEnv)
545+
} else {
546+
val (beforeDelegateCtor, rest) = bodyStats.span {
547+
case ApplyStatically(_, This(), _, MethodIdent(ctor), _) =>
548+
!ctor.isConstructor
549+
case _ =>
550+
true
551+
}
552+
553+
if (rest.isEmpty) {
554+
if (!postOptimizer)
555+
reportError(i"Constructor must contain a delegate constructor call")
556+
557+
checkBlockStats(bodyStats, bodyEnv)
558+
} else {
559+
val (delegateCtorCall: ApplyStatically) :: afterDelegateCtor = rest
560+
val ApplyStatically(_, receiver, cls, MethodIdent(ctor), args) = delegateCtorCall
561+
562+
val envJustBeforeDelegate = checkBlockStats(beforeDelegateCtor, bodyEnv)
563+
564+
checkApplyArgs(ctor, args, envJustBeforeDelegate)
565+
566+
checkTree(receiver, envJustBeforeDelegate) // check that the This itself is valid
567+
568+
if (!postOptimizer) {
569+
if (!(cls == classDef.className || classDef.superClass.exists(_.name == cls))) {
570+
implicit val ctx = ErrorContext(delegateCtorCall)
571+
reportError(
572+
i"Invalid target class $cls for delegate constructor call; " +
573+
i"expected ${classDef.className}" +
574+
classDef.superClass.fold("")(s => i" or ${s.name}"))
575+
}
576+
}
577+
578+
checkBlockStats(afterDelegateCtor, envJustBeforeDelegate)
579+
}
580+
}
581+
}
582+
583+
private def checkBlockStats(stats: List[Tree], env: Env): Env = {
584+
stats.foldLeft(env) { (prevEnv, stat) =>
585+
checkTree(stat, prevEnv)
586+
}
587+
}
588+
513589
private def checkTreeOrSpreads(trees: List[TreeOrJSSpread], env: Env): Unit = {
514590
trees.foreach {
515591
case JSSpread(items) => checkTree(items, env)
@@ -520,15 +596,19 @@ private final class ClassDefChecker(classDef: ClassDef,
520596
private def checkTrees(trees: List[Tree], env: Env): Unit =
521597
trees.foreach(checkTree(_, env))
522598

599+
private def checkApplyArgs(methodName: MethodName, args: List[Tree], env: Env)(
600+
implicit ctx: ErrorContext): Unit = {
601+
val paramRefsCount = methodName.paramTypeRefs.size
602+
if (args.size != paramRefsCount)
603+
reportError(i"Arity mismatch: $paramRefsCount expected but ${args.size} found")
604+
checkTrees(args, env)
605+
}
606+
523607
private def checkTree(tree: Tree, env: Env): Env = {
524608
implicit val ctx = ErrorContext(tree)
525609

526-
def checkApplyGeneric(methodName: MethodName, args: List[Tree]): Unit = {
527-
val paramRefsCount = methodName.paramTypeRefs.size
528-
if (args.size != paramRefsCount)
529-
reportError(i"Arity mismatch: $paramRefsCount expected but ${args.size} found")
530-
checkTrees(args, env)
531-
}
610+
def checkApplyGeneric(methodName: MethodName, args: List[Tree]): Unit =
611+
checkApplyArgs(methodName, args, env)
532612

533613
val newEnv = tree match {
534614
case VarDef(ident, _, vtpe, mutable, _) =>
@@ -545,9 +625,7 @@ private final class ClassDefChecker(classDef: ClassDef,
545625
case Skip() =>
546626

547627
case Block(stats) =>
548-
stats.foldLeft(env) { (prevEnv, stat) =>
549-
checkTree(stat, prevEnv)
550-
}
628+
checkBlockStats(stats, env)
551629

552630
case Labeled(label, _, body) =>
553631
checkDeclareLabel(label)
@@ -645,6 +723,8 @@ private final class ClassDefChecker(classDef: ClassDef,
645723
checkApplyGeneric(method, args)
646724

647725
case ApplyStatically(_, receiver, _, MethodIdent(method), args) =>
726+
if (method.isConstructor)
727+
reportError(i"Illegal constructor call")
648728
checkTree(receiver, env)
649729
checkApplyGeneric(method, args)
650730

‎linker/shared/src/test/scala/org/scalajs/linker/AnalyzerTest.scala

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ class AnalyzerTest {
192192
val classDefs = Seq(
193193
classDef("A", kind = kindSub,
194194
superClass = Some("B"),
195-
methods = requiredMethods("A", kindSub),
195+
methods = requiredMethods("A", kindSub, "B"),
196196
jsConstructor = requiredJSConstructor(kindSub)),
197197
classDef("B", kind = kindBase,
198198
superClass = validParentForKind(kindBase),
@@ -321,7 +321,7 @@ class AnalyzerTest {
321321
)))(EOH, UNV)
322322
)),
323323
classDef("B", superClass = Some("A"),
324-
methods = List(trivialCtor("B")))
324+
methods = List(trivialCtor("B", "A")))
325325
)
326326

327327
val analysis = computeAnalysis(classDefs,
@@ -350,7 +350,7 @@ class AnalyzerTest {
350350
methods = List(trivialCtor("A"))),
351351
classDef("B", superClass = Some("A"),
352352
methods = List(
353-
trivialCtor("B"),
353+
trivialCtor("B", "A"),
354354
MethodDef(EMF, fooMethodName, NON, Nil, IntType, Some(int(5)))(EOH, UNV)
355355
))
356356
)
@@ -771,12 +771,12 @@ class AnalyzerTest {
771771
)),
772772
classDef("B", superClass = Some("A"), interfaces = List("I2"),
773773
methods = List(
774-
trivialCtor("B"),
774+
trivialCtor("B", "A"),
775775
MethodDef(EMF, fooMethodName, NON, Nil, IntType, Some(int(5)))(EOH, UNV)
776776
)),
777777
classDef("C", superClass = Some("B"),
778778
methods = List(
779-
trivialCtor("C"),
779+
trivialCtor("C", "B"),
780780
MethodDef(EMF, barMethodName, NON, Nil, IntType, Some(int(5)))(EOH, UNV)
781781
))
782782
)

‎linker/shared/src/test/scala/org/scalajs/linker/BaseLinkerTest.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ class BaseLinkerTest {
5555
"Sub",
5656
kind = ClassKind.Class,
5757
superClass = Some("Base"),
58-
methods = List(trivialCtor("Sub"))
58+
methods = List(trivialCtor("Sub", "Base"))
5959
),
6060
mainTestClassDef(
6161
consoleLog(Apply(EAF, New("Sub", NoArgConstructorName, Nil), fooName, Nil)(IntType))

‎linker/shared/src/test/scala/org/scalajs/linker/IRCheckerTest.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ class IRCheckerTest {
133133
superClass = Some("C"),
134134
interfaces = List("B"),
135135
methods = List(
136-
trivialCtor("D"),
136+
trivialCtor("D", "C"),
137137
MethodDef(
138138
EMF.withNamespace(MemberNamespace.PublicStatic),
139139
testMethodName,

‎linker/shared/src/test/scala/org/scalajs/linker/OptimizerTest.scala

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -503,10 +503,12 @@ class OptimizerTest {
503503
// def this() = {
504504
// this.x = null
505505
// this.y = 5
506+
// this.jl.Object::<init>()
506507
// }
507508
MethodDef(EMF.withNamespace(Constructor), NoArgConstructorName, NON, Nil, NoType, Some(Block(
508509
Assign(Select(thisFor("Foo"), FieldName("Foo", "x"))(witnessType), Null()),
509-
Assign(Select(thisFor("Foo"), FieldName("Foo", "y"))(IntType), int(5))
510+
Assign(Select(thisFor("Foo"), FieldName("Foo", "y"))(IntType), int(5)),
511+
trivialSuperCtorCall("Foo")
510512
)))(EOH, UNV),
511513

512514
// def method(): Int = this.y

0 commit comments

Comments
 (0)