Skip to content

Commit

Permalink
Fix scala#17344: Make implicit references to this above dynamic impor…
Browse files Browse the repository at this point in the history
…ts explicit.

Implicit references to the `this` of an outer class are made
explicit by the typer, and they need to be for `explicitOuter` to
do its job correctly.

When we desugar a `js.dynamicImport`, we move the code inside a
synthetic inner class. If it contains implicit references to an
enclosing class, we must make them explicit at that point.
  • Loading branch information
sjrd committed Apr 26, 2023
1 parent 7a6dc76 commit 7c96eed
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 1 deletion.
33 changes: 32 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/sjs/PrepJSInterop.scala
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,24 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
}
}

private var dynamicImportEnclosingClasses: Set[Symbol] = Set.empty

private def enterDynamicImportEnclosingClass[A](cls: Symbol)(body: => A): A = {
val saved = dynamicImportEnclosingClasses
dynamicImportEnclosingClasses = saved + cls
try
body
finally
dynamicImportEnclosingClasses = saved
}

private def hasImplicitThisPrefixToDynamicImportEnclosingClass(tpe: Type)(using Context): Boolean =
tpe match
case tpe: ThisType => dynamicImportEnclosingClasses.contains(tpe.cls)
case TermRef(prefix, _) => hasImplicitThisPrefixToDynamicImportEnclosingClass(prefix)
case _ => false
end hasImplicitThisPrefixToDynamicImportEnclosingClass

/** DefDefs in class templates that export methods to JavaScript */
private val exporters = mutable.Map.empty[Symbol, mutable.ListBuffer[Tree]]

Expand Down Expand Up @@ -297,10 +315,15 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP

assert(currentOwner.isTerm, s"unexpected owner: $currentOwner at ${tree.sourcePos}")

val enclosingClass = currentOwner.enclosingClass

// new DynamicImportThunk { def apply(): Any = body }
val dynamicImportThunkAnonClass = AnonClass(currentOwner, List(jsdefn.DynamicImportThunkType), span) { cls =>
val applySym = newSymbol(cls, nme.apply, Method, MethodType(Nil, Nil, defn.AnyType), coord = span).entered
val newBody = transform(body).changeOwnerAfter(currentOwner, applySym, thisPhase)
val transformedBody = enterDynamicImportEnclosingClass(enclosingClass) {
transform(body)
}
val newBody = transformedBody.changeOwnerAfter(currentOwner, applySym, thisPhase)
val applyDefDef = DefDef(applySym, newBody)
List(applyDefDef)
}
Expand All @@ -310,6 +333,14 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
.appliedToTypeTree(tpeArg)
.appliedTo(dynamicImportThunkAnonClass)

// #17344 Make `ThisType`-based references to enclosing classes of `js.dynamicImport` explicit
case tree: Ident if hasImplicitThisPrefixToDynamicImportEnclosingClass(tree.tpe) =>
def rec(tpe: Type): Tree = (tpe: @unchecked) match // exhaustive because of the `if ... =>`
case tpe: ThisType => This(tpe.cls)
case tpe @ TermRef(prefix, _) => rec(prefix).select(tpe.symbol)

rec(tree.tpe).withSpan(tree.span)

// Compile-time errors and warnings for js.Dynamic.literal
case Apply(Apply(fun, nameArgs), args)
if fun.symbol == jsdefn.JSDynamicLiteral_applyDynamic ||
Expand Down
12 changes: 12 additions & 0 deletions project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,18 @@ object Build {
org.scalajs.jsenv.Input.Script(f) +: (Test / jsEnvInput).value
},

Test / unmanagedSourceDirectories ++= {
val linkerConfig = scalaJSStage.value match {
case FastOptStage => (Test / fastLinkJS / scalaJSLinkerConfig).value
case FullOptStage => (Test / fullLinkJS / scalaJSLinkerConfig).value
}

if (linkerConfig.moduleKind != ModuleKind.NoModule && !linkerConfig.closureCompiler)
Seq(baseDirectory.value / "test-require-multi-modules")
else
Nil
},

(Compile / managedSources) ++= {
val dir = fetchScalaJSSource.value
(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.scalajs.testsuite.jsinterop

import org.junit.Assert.*
import org.junit.Test

import org.scalajs.junit.async._

import scala.scalajs.js
import scala.scalajs.js.annotation.*

class SJSDynamicImportTestScala3 {
import scala.concurrent.ExecutionContext.Implicits.global

@Test def implicitThisReferenceInDynamicImport_Issue17344(): AsyncResult = await {
class Foo() {
def foo(): Int = 1
}
class Bar(foo: Foo) {
def bar(): js.Promise[Int] = js.dynamicImport(foo.foo())
}

val bar = new Bar(new Foo())
val promise = bar.bar()

promise.toFuture.map { r =>
assertEquals(1, r)
}
}
}

0 comments on commit 7c96eed

Please sign in to comment.