Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Port from snakeyaml-engine] Allow to avoid dumping anchors #272

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[Port from snakeyaml-engine] Allow to avoid dumping anchors #272

wants to merge 1 commit into from
+242 −3

Conversation

krzema12
Copy link
Owner

@krzema12 krzema12 commented Nov 13, 2024 •
edited
Loading

krzema12
krzema12 commented Nov 13, 2024
View reviewed changes
src/commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/serializer/Serializer.kt Outdated
Comment on lines 38 to 40
// TODO: rewrite Collections.newSetFromMap(new IdentityHashMap<Node, Boolean>());
// Regular set is incorrect!
private val recursive: MutableSet<Node> = mutableSetOf()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This has to be fixed before merging. From a quick skim, I haven't found any set in Kotlin that would do identity-based equality check, so I think we need to implement it ourselves.

Copy link
Owner Author

@krzema12 krzema12 Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: check if

snakeyaml-engine-kmp/src/commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/representer/BaseRepresenter.kt

Line 225 in 6c52b61

private class AnchorNodeMap : MutableMap<IdentityHashCode, Node> {
can be a solution to this problem. I noticed that IdentityHashCode is used there.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up implementing our own multiplatform IdentitySet based on utils that the KMP API delivers. It looks sane to me, but definitely is a subject of review here.

@krzema12 krzema12 force-pushed the port-dereference-aliases branch from 75e6ed6 to bd8ffe0 Compare November 14, 2024 08:04
krzema12
krzema12 commented Nov 14, 2024
View reviewed changes
src/commonTest/kotlin/it/krzeminski/snakeyaml/engine/kmp/serializer/IdentitySetTest.kt

data class TestClass(val value: String)

class IdentitySetTest : FunSpec({
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests fail for Wasm target with a mysterious

 JsException: Exception was thrown while running JavaScript code
     at kotlin.captureStackTrace(file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:16)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.captureStackTrace__externalAdapter(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.Throwable.<init>(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.Throwable.<init>(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.js.JsException.<init>(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.wasm.internal.throwJsException(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlinx.coroutines.intrinsics.startUndispatchedOrReturnIgnoreTimeout(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlinx.coroutines.setupTimeout(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at 8.doResume(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlinx.coroutines.withTimeoutOrNull(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://youtrack.jetbrains.com/issue/KT-68185

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked for help: https://kotlinlang.slack.com/archives/CDFP59223/p1731593368886219

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for a failing test:

     TypeError: _this.contains is not a function
         at it.krzeminski.snakeyaml.engine.kmp.internal.contains_$external_fun (file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:4069:101)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.internal.identityHashCode (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[16416]:0x192c25)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySet.add (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[16281]:0x1913da)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySetTest$<init>$lambda$slambda$lambda.invoke (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26212]:0x23704c)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySetTest$<init>$lambda$slambda$lambda.invoke (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26213]:0x237077)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.js.__callFunction_(()->Unit) (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[10224]:0x1393a7)
         at file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:125:121                                        
         at it.krzeminski.snakeyaml.engine.kmp.serializer.jsCatch (file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:4109:89)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.jsCatch__externalAdapter (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26210]:0x236fee)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySetTest$<init>$lambda$slambda.doResume (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26217]:0x237199)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turned out that there's a bug here:

snakeyaml-engine-kmp/src/wasmJsMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/internal/system.wasm.kt

Line 17 in bd8ffe0

operator fun contains(key: JsReference<Any>): Boolean

WeakMap has a has method instead of contains. But even if I fix it, I'm getting further issues with set.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @aSemy in case you have any useful insight there. I see you contributed this part, and it looks like it's being exercised in the tests only now (which is weird since I thought BaseRepresenter is covered fairly well). Do you remember how you tackled implementing such internals in Wasm? Any good reference impls you know? I see korlibs still doesn't have it implemented: https://github.com/korlibs/korlibs-datastructure/blob/b3f589fe2ae7e90b878fce3f586e1f8caba07baf/korlibs-datastructure/src%40wasm/korlibs/datastructure/internal/InternalJs.kt#L25
Ideally we reproduce any issues related to identityHashCode with a new test on the main branch, and fix it there, then this PR should just work. I'll try to handle it, but I'm wondering of you have some useful thoughts/hints here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://kotlinlang.slack.com/archives/CDFP59223/p1716543360983169

maybe rename contains to has?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this helps with the first problem, but it uncovers another one. I'll describe this next problem better.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting this problem and fixing it to a separate PR: #273

This was referenced Dec 13, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aSemy aSemy aSemy left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@krzema12 @aSemy