Fix #15701: Implement js.dynamicImport for dynamic module loading. #15720
Conversation
Forward port of the compiler changes in scala-js/scala-js@a640f15
In dotc, `private val`s do not generate a getter method, so always relying on the getter to generate the `JSNativeMemberDef` and reading it is not correct. We now generate `JSNativeMemberDef`s both for `ValDef`s and for `DefDef`s that are not Accessors. For reading, we support both `Select`s of fields and `Apply`s of methods.
This ensures that the module-only features are tested.
| * that can be dynamically loaded. Moving members across that boundary changes | ||
| * the dynamic and static dependencies between ES modules, which is forbidden. | ||
| */ | ||
| ctx.settings.scalajs.value && encClass.isSubClass(jsdefn.DynamicImportThunkClass) |
There was a problem hiding this comment.
Is this enough to guarantee that no member is ever moved across a DynamicImportThunk class boundary?
Scala 2 did not move things around like that, so this is new code compared to the commit we're porting.
| val applySym = newSymbol(cls, nme.apply, Method, MethodType(Nil, Nil, defn.AnyType), coord = span).entered | ||
| val newBody = transform(body).changeOwnerAfter(currentOwner, applySym, thisPhase) | ||
| val applyDefDef = DefDef(applySym, newBody) |
There was a problem hiding this comment.
Is this the correct way to completely move a tree, and everything that it contains, inside the new def apply(): Any method?
| val cls = newNormalizedClassSymbol(currentOwner, tpnme.ANON_CLASS, Synthetic | Final, | ||
| List(jsdefn.DynamicImportThunkType), coord = span) | ||
| val constr = newConstructor(cls, Synthetic, Nil, Nil).entered |
There was a problem hiding this comment.
I kind of wanted to use tpe.AnonClass here, but it only creates forwarder methods, whereas I need a new method (apply) that actually contains the tree body. Is there a better way to do this?
There was a problem hiding this comment.
I am not aware of a more suitable method. But maybe we can factor out the useful parts of tpd.AnonClass and call them from both AnonClass and here?
There was a problem hiding this comment.
Indeed, there was some opportunity for refactoring. In fact, it extended to two other places in the codebase which followed the same pattern. I have added a commit that factors out the common pattern from those 4 places. Does it look good to you?
|
@odersky May I ask you to review this one? This is mostly a port, but I have pointed out a few places where I'm not sure what I'm doing. The most critical is the change in lambdalift's The most important tests for these features are in https://github.com/scala-js/scala-js/blob/v1.10.1/test-suite/js/src/test/require-multi-modules/org/scalajs/testsuite/jsinterop/SJSDynamicImportTest.scala and are brought in by the last commit of this PR. Without the change in lambdalift, the test |
| val cls = newNormalizedClassSymbol(currentOwner, tpnme.ANON_CLASS, Synthetic | Final, | ||
| List(jsdefn.DynamicImportThunkType), coord = span) | ||
| val constr = newConstructor(cls, Synthetic, Nil, Nil).entered |
There was a problem hiding this comment.
I am not aware of a more suitable method. But maybe we can factor out the useful parts of tpd.AnonClass and call them from both AnonClass and here?
| val applySym = newSymbol(cls, nme.apply, Method, MethodType(Nil, Nil, defn.AnyType), coord = span).entered | ||
| val newBody = transform(body).changeOwnerAfter(currentOwner, applySym, thisPhase) | ||
| val applyDefDef = DefDef(applySym, newBody) |
There are several places where we create typed anonymous classes. All followed the same pattern, which we now factored out in a new `tpe.AnonClass` overload.
| val parents1 = | ||
| if (parents.head.classSymbol.is(Trait)) { | ||
| val head = parents.head.parents.head | ||
| if (head.isRef(defn.AnyClass)) defn.AnyRefType :: parents else head :: parents | ||
| } | ||
| else parents | ||
| val cls = newNormalizedClassSymbol(owner, tpnme.ANON_CLASS, Synthetic | Final, parents1, | ||
| coord = fns.map(_.span).reduceLeft(_ union _)) | ||
| val cls = newNormalizedClassSymbol(owner, tpnme.ANON_CLASS, Synthetic | Final, parents1, coord = coord) |
There was a problem hiding this comment.
Nitpick: If it's the same name, consider just writing it once. I.e. coord instead of coord = coord.
There was a problem hiding this comment.
I can't. There are other parameters with default values in the middle, namely selfInfo and privateWithin. So we need coord =.
No description provided.