[JS] Fix equality of PrimitiveKClassImpl (KT-87307)#6510
Open
Sergej Jaskiewicz (broadwaylamb) wants to merge 5 commits into
Open
[JS] Fix equality of PrimitiveKClassImpl (KT-87307)#6510Sergej Jaskiewicz (broadwaylamb) wants to merge 5 commits into
PrimitiveKClassImpl (KT-87307)#6510Sergej Jaskiewicz (broadwaylamb) wants to merge 5 commits into
Conversation
|
~~This MR fixes the regression, but the root cause is not yet documented as required by the guideline This will allow our automation to copy the RCA into the issue comments and automatically apply the root-cause-defined tag.~~ |
Code Owners
|
Sergej Jaskiewicz (broadwaylamb)
force-pushed
the
rr/broadwaylamb/js-long-kclass-eq
branch
from
e356ec3 to
ecceaf4
Compare
This identifier is replaced with the actual backend name at source preprocessing time, before the test file is fed to the compiler.
Make `PrimitiveKClassImpl` comparable with any `KClassImpl`, not just `PrimitiveKClassImpl`. This fixes a regression that was introduced in 6d14904. ^KT-87307 Fixed ^RCA: **Root cause:** In the commit 6d14904 we started lowering `Long::class` to `kotlin.reflect.js.internal.PrimitiveClasses.longClass` unconditionally, not just when compiling `Long` to `BigInt` `PrimitiveClasses.longClass` has the type `PrimitiveKClassImpl`, and `42L::class` would have the type `SimpleKClassImpl` when compiling `Long` as a regular class, so comparing them would always yield `false`, because of how equality was defined in `PrimitiveKClassImpl`. We had a test for this, but it didn't catch the regression because the relevant logic of computing `42L::class` was optimized away into just `Long::class` by the compiler, which made the test useless. **Preventive steps taken:** the test was improved to be more robust. **Future steps:** raise awareness about this class of issues among compiler engineers, so that they write more robust tests, e.g. by wrapping literals into blackhole-like functions that are guaranteed to not be optimized away. **Additional notes:** As mentioned, there already was an autotest, but the compiler has outsmarted us.
Sergej Jaskiewicz (broadwaylamb)
force-pushed
the
rr/broadwaylamb/js-long-kclass-eq
branch
from
ecceaf4 to
32a5455
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
KT-87307
Make
PrimitiveKClassImplcomparable with anyKClassImpl, not justPrimitiveKClassImpl. This fixes a regression that was introduced in 6d14904.