[Port from snakeyaml-engine] Allow to avoid dumping anchors #272
Conversation
| // TODO: rewrite Collections.newSetFromMap(new IdentityHashMap<Node, Boolean>()); | ||
| // Regular set is incorrect! | ||
| private val recursive: MutableSet<Node> = mutableSetOf() |
There was a problem hiding this comment.
There was a problem hiding this comment.
TODO: check if
IdentityHashCode is used there.
There was a problem hiding this comment.
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.
Co-authored-by: Andrey Somov <[email protected]>
krzema12
force-pushed
the
port-dereference-aliases
branch
from
75e6ed6 to
bd8ffe0
Compare
|
|
||
| data class TestClass(val value: String) | ||
|
|
||
| class IdentitySetTest : FunSpec({ |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
It turned out that there's a bug here:
WeakMap has a has method instead of contains. But even if I fix it, I'm getting further issues with set.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
https://kotlinlang.slack.com/archives/CDFP59223/p1716543360983169
maybe rename contains to has?
There was a problem hiding this comment.
Yes, this helps with the first problem, but it uncovers another one. I'll describe this next problem better.
There was a problem hiding this comment.
Extracting this problem and fixing it to a separate PR: #273
Before this change, we simply stored a hash of the latest commit in snakeyaml-engine that we ported over to snakeyaml-engine-kmp. The idea was to port the changes one after another, assuming it would be easy. However, given our latest problems to port one of the commits (#272) so that it works on all targets, it's not good to block on this single change with porting other changes. One idea would be to note with better granularity which commits have been ported. However, it would cost us quite a bit to add this mechanism, instead of focusing on actually porting the changes. That's why I propose changing the semantics of the counter displayed in the README: show how many changes weren't analyzed by us. In other words, as soon as we acknowledge a given upstream change by creating a proper issue in our project, we can assume this change is "analyzed". Then, in our own pace, we can take care of porting the changes, possibly doing it in parallel. Thanks to this approach, changes that are harder to port will naturally be ported later, and won't block porting easy changes, which is good for the users of the lib. I've created these issues under a new label to track the changes to port: https://github.com/krzema12/snakeyaml-engine-kmp/issues?q=is%3Aissue+is%3Aopen+label%3A%22port+from+snakeyaml-engine%22 You can cross-reference it with https://raw.githubusercontent.com/krzema12/snakeyaml-engine-kmp/refs/heads/commits-to-upstream-badge/log-diff-between-repos.txt
Ports https://bitbucket.org/snakeyaml/snakeyaml-engine/commits/1af7dcd2c9c64a9d7cd9de3cd5f64bb2fc2aab6a
Part of #280