[K/JS]: Implement support for companion blocks and extensions#6165
[K/JS]: Implement support for companion blocks and extensions#6165Andrii Rublov (seclerp) wants to merge 6 commits into
Conversation
Code Owners
|
|
/dry-run |
|
THIS IS A DRY RUN Quality gate is triggered at https://buildserver.labs.intellij.net/build/968015790 — use this link to get full insight. Quality gate was triggered with the following revisions:
Quality gate finished successfully. |
| private fun processDeclarationContainer(container: IrClass) { | ||
| if (container.isExpect || container.isEffectivelyExternal()) return | ||
| val builder = context.irBuiltIns.createIrBuilder(container.symbol, SYNTHETIC_OFFSET, SYNTHETIC_OFFSET) | ||
| val staticDeclarationsByFields = buildMap { |
There was a problem hiding this comment.
TBH I don't understand why we need this map. We can filter out static fields in the loop below, where we populate initializers. As for the staticDeclarationsByFields.isNotEmpty() check — it could be replaced with container.declarations.any { /*whatever logic we use when building the map*/ } (hoisted out of the loop, obviously).
The benefit of this is that we don't have to waste memory on the map.
There was a problem hiding this comment.
Not fixed
| container.initEntryInstancesFun?.let { initEntries -> | ||
| // Replace the call to a _initEntries function to a static_init call. This ensures touching enum entries will trigger | ||
| // static initializers too. | ||
| container.parent.transformChildrenVoid(object : IrElementTransformerVoidWithContext() { |
There was a problem hiding this comment.
This looks really inefficient to me. Basically, if container.parent is an IrFile with a large number of enum classes in it, then for each such class we traverse the whole file again looking for IrCalls. This will be slow. Usually, we would have two lowerings for such cases: one would transform declarations, and another — their usages.
However, the whole static initializer generation logic seems awfully similar to the initEntryInstancesFun generation logic in EnumClassCreateInitializerLowering, so we could try to commonize it I guess?
And I don't mean it in the "extract it into a common routine" sense, I mean that we can try to delete EnumClassCreateInitializerLowering entirely, and instead only rely on generating static initializers for all static members alike, be it fields or enum entries.
There was a problem hiding this comment.
Yes, it totally makes sense to me too. Initially, after starting working on static initializers in common, I dug into enum initializer lowering and it looked exactly as a specific case of static initializers, due to a fact that we only had them as statics and haven't needed to commonize them before.
I will try to replace them with JsStaticInitializersLowering in that PR. In case it would take more time will also make a corresponding task for it. Thanks!
|
Regarding static initialization blocks - we discussed it with Artem Kobzar (@JSMonk) and for now we will keep it as |
Regarding this one, I'll make a separate PR for such a test, as I did with other tests previously. |
| // _getInstance then calls static_init instead. | ||
| declaration.objectGetInstanceFunction?.let { getInstance -> | ||
| val body = getInstance.body as? IrBlockBody ?: return@let | ||
| body.statements.let { statements -> |
There was a problem hiding this comment.
nit: maybe using let is a bit too much here, val statements = body.statements reads better
|
|
||
| override fun visitClass(declaration: IrClass) { | ||
| for (function in declaration.functions) { | ||
| if (declaration.staticInitializer != function) continue |
There was a problem hiding this comment.
It seems like the whole for loop can be replaced with just declaration.staticInitializer?.let { /*add parent static initializer calls*/ }?
| } | ||
|
|
||
| private fun createStaticInitCalledField(irClass: IrClass): IrField = context.irFactory.buildField { | ||
| name = Name.identifier("${irClass.name.identifier}$$STATIC_INIT_CALLED_PROPERTY_NAME") |
There was a problem hiding this comment.
Actually, I think that prefixing the field name with the name of the class is not needed, since it's generated inside the class anyway.
There was a problem hiding this comment.
It is indeed generated inside the class, but there might be cases of lifting such declaration up, to the top level, which will introduce weird static_init_N styled names, like we for example have for Companion_N. It is clearly visible in todays tests
WDYT about thinking of updating lifting lowerings to somehow reflect the original container name in them instead of wiping it out?
| startOffset = SYNTHETIC_OFFSET | ||
| endOffset = SYNTHETIC_OFFSET | ||
| this.origin = origin | ||
| name = Name.identifier(container.name.identifier + '$' + STATIC_INIT_FUNCTION_NAME) |
There was a problem hiding this comment.
Same here.
| // If static_init exists, it always contains at least 1 static field initializer | ||
| // TODO: The 'origin' has not been taken into account here because currently enum cases initializers do not set it to | ||
| // STATIC_FIELD_INITIALIZER | ||
| val initStartIndex = body.statements.indexOfFirst { it is IrSetField } |
There was a problem hiding this comment.
Do we have a test when an enum class implements some interface with a companion block? If not, let's add it to make sure the initialization order is correct.
| private fun processDeclarationContainer(container: IrClass) { | ||
| if (container.isExpect || container.isEffectivelyExternal()) return | ||
| val builder = context.irBuiltIns.createIrBuilder(container.symbol, SYNTHETIC_OFFSET, SYNTHETIC_OFFSET) | ||
| val staticDeclarationsByFields = buildMap { |
There was a problem hiding this comment.
Not fixed
The PR adds support for companion blocks and extensions feature for the Kotlin/JS backend, following the specification defined in the corresponding KEEP.
Since Kotlin/JS already supports proper lowering for static function and property declarations, main work in this PR has been made around static properties initializers in classes, especially preserving initialization order defined by a KEEP from above.
In short, main rules are the following:
companionblock orcompanion object:The implementation of such an ordering guarantee is split between 2 new passes -
JsStaticInitializersLoweringandJsStaticInitializersInheritanceLowering.JsStaticInitializersLoweringcreates a new static function attached to a class, calledstatic_init. The body of that function contains all initializer statements in a proper order. Thestatic_initfunction is always called once per class.There are 2 cases with special handling:
_getInstancefunction created byObjectDeclarationLowering, while_getInstancenow delegates tostatic_initcall. This also ensures that accessing the companion instance will trigger other static initializers.JsStaticInitializersInheritanceLoweringinjects parentstatic_initcall into the beginning of the childstatic_initbody. It is done in a separate lowering due to a fact that not all classes would havestatic_initadded to them, so we first need to create them where needed and only after that make proper child-parent propagations.A simplified codegen example:
Produces:
Fixes KT-85459