Wasm

[igoriakovlev]

This is WasmJs target implementation.

We update kotlin version to 1.9.20 and implement WasmJs target similar to JS target implementation.

The experimental repository dependency will be removed before merging and replaced with maven central as soon as kotlinx.serialization for WasmJs target become published.

[dkhalanskyjb]

If this lazy works around some bug, please accompany each of its usages with a comment with the corresponding issue number.

[igoriakovlev]

This bug observable only for wasm target (which seems is more strict for nullability in this case). Could you please to suggest how to file such bug and where.

[dkhalanskyjb]

I don't know how nullability comes into play here: these are all not null and don't even depend on one another.

I'd file this under https://kotl.in/issue, specifying that the behavior is different on wasm and crashes the code that works on all other platforms.

[igoriakovlev]

The initialisation sequence reads null value from not-null uninitialised property. Here, I put the code into lazy block to prevent reading uninitialised property. The bug exists at all platforms but seems it is observable only on wasm.

[dkhalanskyjb]

I feel we're speaking past each other.

The bug exists at all platforms but seems it is observable only on wasm.

The way I'm reading this, if this bug never surfaces on other platforms, then we can't call it a bug. Kotlin/Wasm behaves differently, making code that always works correctly on other platforms buggy. This is a problem with Kotlin/Wasm and needs to be reported.

Or did you mean that you can cause this code to misbehave on other platforms somehow, we just never encountered it, and there actually is a bug in the library code that's fixed by lazy? In what circumstances can it happen?

Either way, could you elaborate on what the bug is? At which point is a null value read from a non-null property?

[igoriakovlev]

I do not think this is Kotlin/Wasm compiler bug.

[dkhalanskyjb]

Ok, where is this difference coming from then? Is the serialization library using expect/actual in this part to have different code on different platforms and uses a different implementation from everything else on Kotlin/Wasm?

[igoriakovlev]

The fact that at all backends except wasm the behaviour is different does not mean that this is a kotlin/wasm bug, especially for the code that uses frontend suppressors.

But anyway, I will review what is happening in detail here on the next week and try to localise exact problem.

[igoriakovlev]

I found the difference of implementation for associated objects in the wasm that triggered the problem.
It was a difference of associated objects initialization that was not specified and tested.
In a matter of same behavior for different targets I have filed a bug for kotlin/wasm stdlib.
As far, to workaround this difference, I put lazy for all serialiser constructor initialisers in DateTime with a comment.

[dkhalanskyjb]

Good job figuring this one out!

[dkhalanskyjb]

The jsTry change is extremely intrusive and pollutes all JS code. If I understand correctly what's going on, this is a measure to support the exceptions thrown from pure JS (not Kotlin/JS) code, which seems incompatible with how Kotlin/WASM treats exceptions. Is this correct?

If so, we'd need to remember to surround all the JS code we call with such checks. It is easy to miss, and I don't even know what will happen if we do miss this. How badly will something break? This question needs to be answered in form of a comment somewhere in the code, no matter what solution we end up with.

Here's an idea of how this could be done in a more robust manner:

// jsAndWasmShared
internal expect class JodaTimeLocalDate {
  fun plusDays(days: Number): Number
  // ...
}

class LocalDateTime(value: JodaTimeLocalDate) // instead of `value: jtLocalDate`

// js
internal actual typealias JodaTimeLocalDate = jtLocalDate

// wasm
internal actual value class JodaTimeLocalDate(val value: jtLocalDate) {
  fun plusDays(days: Number): Number = JodaTimeLocalDate(jsTry { value.plusDays(days) })
  // ...
}

This way, the shared source set will stay almost the same, and all WASM-specific logic will be in one place where it's easy to review and spot if something's wrong.

What do you think? Is this approach viable?

[igoriakovlev]

Yeah, when you commonize a code between two platforms with different interop it always looks tricky. This is a price for commonization.

Yes. In kotlin.wasm platform we have no ability (so far) to catch exceptions thowed from javascript code. This is why I put special wrapper to catch these exceptions inside js world and rethrow it as wasm exceptions in wasm world. For js platform its just a nop.
When I was writing this I had a simple mental rule - every time you want to try-catch exception from js library you should wrap jsTry this block.

Your suggest is indeed more readable from a perspective of jsTry but one more indirection level makes a mess and boilerplate.

There is several cons for this:

1. Now we have two Instant types - DateTime.Instant and jsJoda.Instant external. With this approach we will have three similar types - DateTime.Instant, jsJoda.Instant and WasmWrapper.Instant.
2. Everywhere we use joda externals we need now make 2 unbox operations, like jtDuration.between(other.value.jodaInstant, this.value.jodaInstant).
3. Everywhere we pass jsJoda declarations to DateTime declarations we need to pack twice.
4. You need to overlap almost all Joda external hierarchy.
5. It does not solves major problem, because you still need to know which methods should be wrapped with jsTry and which is not.

Now you can formulate a common rule - any place I put try-catch for js exception should be wrapped with jsTry also. Not sure that wrappers can do better.

But:
If you still think that it can do better, I can try to do that but for now it does't seem any better for me.

[dkhalanskyjb]

Your suggest is indeed more readable from a perspective of jsTry but one more indirection level makes a mess and boilerplate.

In the common code, we replace one no-op indirection (jsTry) with another (a type alias), a more readable one. I think the number of indirections stays the same.

Now we have two Instant types

We had two Instant types: the user-visible facade (kotlinx.datetime.Instant) and the implementation (Instant from JsJoda). Now we have three Instant types: the user-visible facade (kotlinx.datetime.Instant), the adapter of platform-specific code (kotlinx.datetime.JodaTimeInstant), and the platform-specific implementation (no-op type alias on JS, a wrapper on WASM). All three entities seem to have their places.

You need to overlap almost all Joda external hierarchy.

The good thing is that this only needs to be done once, and this code only needs to be rarely touched. On the other hand, we edit the shared code fairly often. Its clarity is worth a lot.

2 unbox operations

pack twice

Won't value class work here? Also, is this only a concern performance-wise?

It does not solves major problem, because you still need to know which methods should be wrapped with jsTry and which is not.

It does: now we need to know to wrap only in the place of definition, not on every call.

[igoriakovlev]

You cant make typealias actual in js for these external declarations. You will be forced to hold this fierarchy both for js and wasm.

[ilya-g]

I'm ok with the current jsTry approach, we could refactor it later if we find it disadvantageous.

[dkhalanskyjb]

Some miscommunication probably happened: I agreed not to have this refactoring. Sorry if I worded this incorrectly!

[igoriakovlev]

Ohh, I missed this post. :(
So, need I revert this?

[ilya-g]

Yes, please do, if you want this review to be completed sooner than later

[dkhalanskyjb]

@ilya-g, why do you believe that this approach would take longer to review? With this separation, we barely have to look at the shared code; the changes there are trivial. We only have to look through a small list of definitions and check that they are wrapped.

What does make this PR difficult to review now is the amount of code that's touched but not meaningfully changed. This needs to be fixed. After that, the changes would be self-contained.

[igoriakovlev]

I reduced unnecessary touched code. If you still want me to remove wrappers then I will.

[ilya-g]

master is updated to Kotlin 1.9.21, please rebase on it.

[ilya-g]

While it disables the compilation, the information about the corresponding shared source set is still present in kotlin-project-structure-metadata.json embedded into common klib jar.
So it needs to be checked what effects it would have in IDE in projects having similar shared source sets.

[igoriakovlev]

I used a such approach for all libraries and did't find any problems with it so far. The only problem that I found so far on the K1 IDE now reports error that it is no actual for JS and Wasm targets (in a same time it found these actuals in the shared sourceSet).

[dkhalanskyjb]

Good job figuring this one out!