- Notifications
You must be signed in to change notification settings - Fork396
Removes spurious js.AsInstanceOf around the js.LinkTimeIf#5210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The original change already looks good to me, but maybe we can match againstgenExpr(obj) and abort if it's not typedjs.LinkTimeIf? That would display informative error when the compiler generates an unexpected IR by accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO we should also have a test in the compiler tests'OptimizationTest. I think the "impl pattern" should be tested there, with ahasNot assertion that makes sure there is nojs.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 latefor the module instance | |
| * field alias analysis performed by `IncOptimizer`.In thatcase,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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This abstract class and its twoobjects should static for the pattern to be the one we are interested in. You can put them in the companion object ofLinkTimeIfTest. We often do that.
When B<:A and C<:A, `linkTimeIf[A](cond){ B }{ C }` wouldgenerate an `.asInstanceOf[A]` that casts the resultingvalue 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 performsthis optimization. However, that comes too late for the module instancefield alias analysis performed by `IncOptimizer`. In that case, while thedesugarer removes the `LinkTimeIf`, the extra `AsInstanceOf` preventsaliasing the field. Removing the cast ahead of time in the compiler allowsfield aliases to be recognized in the presence of `LinkTimeIf`s.context:scala-js#5168Co-authored-by: Sébastien Doeraene <sjrdoeraene@gmail.com>a3a0b43 to9bb267cCompare1501c94 intoscala-js:mainUh oh!
There was an error while loading.Please reload this page.
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