Removes spurious js.AsInstanceOf around the js.LinkTimeIf #5210
Conversation
| case Apply(fun, List(cond, thenp, elsep)) | ||
| if fun.symbol == jsDefinitions.LinkingInfo_linkTimeIf && | ||
| thenp.tpe <:< targetTpe && elsep.tpe <:< targetTpe => | ||
| val genObj = genExpr(obj).asInstanceOf[js.LinkTimeIf] |
There was a problem hiding this comment.
The original change already looks good to me, but maybe we can match against genExpr(obj) and abort if it's not typed js.LinkTimeIf? That would display informative error when the compiler generates an unexpected IR by accident.
There was a problem hiding this comment.
Indeed, that would provide more context (especially with a message that includes the current position) in case something fails in the future.
There was a problem hiding this comment.
IMO we should also have a test in the compiler tests' OptimizationTest. I think the "impl pattern" should be tested there, with a hasNot assertion that makes sure there is no js.AsInstanceOf in the produced code.
| case Apply(fun, List(cond, thenp, elsep)) | ||
| if fun.symbol == jsDefinitions.LinkingInfo_linkTimeIf && | ||
| thenp.tpe <:< targetTpe && elsep.tpe <:< targetTpe => | ||
| val genObj = genExpr(obj).asInstanceOf[js.LinkTimeIf] |
There was a problem hiding this comment.
Indeed, that would provide more context (especially with a message that includes the current position) in case something fails in the future.
| /* This is an optimization for `linkTimeIf(cond)(thenp)(elsep).asInstanceOf[T]`. | ||
| * If both `thenp` and `elsep` are subtypes of `T`, the `asInstanceOf` | ||
| * is redundant and can be removed. | ||
| */ |
There was a problem hiding this comment.
Let's elaborate a bit more on why this is useful:
| /* This is an optimization for `linkTimeIf(cond)(thenp)(elsep).asInstanceOf[T]`. | |
| * If both `thenp` and `elsep` are subtypes of `T`, the `asInstanceOf` | |
| * is redundant and can be removed. | |
| */ | |
| /* This is an optimization for `linkTimeIf(cond)(thenp)(elsep).asInstanceOf[T]`. | |
| * If both `thenp` and `elsep` are subtypes of `T`, the `asInstanceOf` | |
| * is redundant and can be removed. The optimizer already routinely performs | |
| * this optimization. However, that comes too late for the module instance | |
| * field alias analysis performed by `IncOptimizer`. In that case, while the | |
| * desugarer removes the `LinkTimeIf`, the extra `AsInstanceOf` prevents | |
| * aliasing the field. Removing the cast ahead of time in the compiler allows | |
| * field aliases to be recognized in the presence of `LinkTimeIf`s. | |
| */ |
| } | ||
|
|
||
| @Test def implPattern(): Unit = { | ||
| sealed abstract class ArrayImpl { |
There was a problem hiding this comment.
This abstract class and its two objects should static for the pattern to be the one we are interested in. You can put them in the companion object of LinkTimeIfTest. We often do that.
When B<:A and C<:A, `linkTimeIf[A](cond){ B }{ C }` would
generate an `.asInstanceOf[A]` that casts the resulting
value to a supertype.
However, this cast is redundant,
as the linker will prune one branch at linktime,
and the remaining expression is already known to be a compatible type.
This commit eliminates the surrounding `.asInstanceOf` around the
`linkTimeIf[T]` when both `thenp` and `elsep` are surely subtypes of `T`.
The optimizer already routinely performs
this optimization. However, that comes too late for the module instance
field alias analysis performed by `IncOptimizer`. In that case, while the
desugarer removes the `LinkTimeIf`, the extra `AsInstanceOf` prevents
aliasing the field. Removing the cast ahead of time in the compiler allows
field aliases to be recognized in the presence of `LinkTimeIf`s.
context: scala-js#5168
Co-authored-by: Sébastien Doeraene <sjrdoeraene@gmail.com>
a3a0b43 to
9bb267c
Compare
Cherry-picking a change from #5168 (comment) :)
When B<:A and C<:A,
linkTimeIf[A](cond){ B }{ C }would generate an.asInstanceOf[A]that casts the resulting value to a supertype.However, this cast is redundant,
as the linker will prune one branch at linktime,
and the remaining expression is already known to be a compatible type.
This commit eliminates the surrounding
.asInstanceOfaround thelinkTimeIf[T]when boththenpandelsepare surely subtypes ofT.context: #5168