kaitai_struct icon indicating copy to clipboard operation
kaitai_struct copied to clipboard

Extended nullabillity support for Kotlin and other null-safe languages

Open xpathexception opened this issue 9 months ago • 7 comments

Hello!

I'm currently developing Kotlin language support in order to bring Kaitai Struct to the Kotlin Multiplatform world. For now I've implemented most of the things and the implementation is able to pass 231 out of 233 tests on JVM and MacOS ARM64 targets.

But there's an issue with 7 of them related to kotlin's Null Safety concept.

There are at least two main operators in Kotlin exist to provide null-safety support - Non-null Assertion Operator (!!) and Safe Calls Operator (?.). The main issue is with the safe call operator - in order to use it and in order to be able to perform safe call chaining, both compiler and translator should be aware of nullability of each type.

I've came up with two approaches which could help resolve this issue. Let's pick NavParentRecursive and it's test as an example.

The most interesting part of the test looks like this:

assertEquals(actual = r.next().value(), expected = 1)
assertEquals(actual = r.next().parentValue(), expected = 255)
assertNull(r.next().next())

We don't care about null

Using this approach we can simply make every attributeReader return non-nullable type by requiring attribute being not null explicitly. Nullability in instances can be handled in the same way.

More on this

Pros:

  • easy to implement
  • already works

Cons:

  • on the call site it is unclear will reading fail or not
  • nullability is basically unmanageable
  • need to adjust tests by replacing assertNull with assertFails
    /* attributeDeclaration */
    /* isNullable: false */
    private var value: IntU1 by Delegates.notNull()  // avoiding default values for primitive types
    fun value(): IntU1 = value

    /* attributeDeclaration */
    /* isNullable: true */
    private var next: NavParentRecursive? = null
    fun next(): NavParentRecursive = requireNotNull(next)
    
    private fun _read() {
        this.value = this._io.readU1()
        if (value() == 255.toIntU1()) {
            this.next = NavParentRecursive(_io = this._io, _parent = this, _root = _root)
        }
    }

    /* instanceDeclaration */
    private var parentValue: IntU1? = null
    private var f_parentValue = false

    fun parentValue(): IntU1? {
        if (f_parentValue) { return this.parentValue }
        if (value() != 255.toIntU1()) {
            this.parentValue = ((_parent() as NavParentRecursive).value()).toIntU1()
        }

        f_parentValue = true
        return this.parentValue
    }

Giving that adjusting test as follows:

    assertFails { r.next().next() }

Nullable types should be supported at DataType level

Using this approach wi need to be able to propagate nullability to the resulting type of every expression.

More on this

Pros:

  • explicitly nullable types would be easier to use on the call site
  • looks much safer than the other way
  • expected behaviour in Kotlin
  • tests will receive support automatically

Cons:

  • looks like it requires adjusting the whole DataType layer
  • requires to develop a set of rules for cases like DataType? %op% DataType in order to resolve resulting nullability
  • requires special handling for most of the operators
  • requires adjusting compiler and translator contracts and implementations

Giving that if we declare attribute reader's type nullable like this:

    /* attributeDeclaration */
    /* isNullable: true */
    private var next: NavParentRecursive? = null
    fun next(): NavParentRecursive? = next

compiler should be able to generate something like this:

assertEquals(actual = r.next()?.value(), expected = 1)
assertEquals(actual = r.next()?.parentValue(), expected = 255)
assertNull(r.next()?.next())

In order to do so we need to be able to know if the lvalue nullable or not and propagate nullability to the right side. Moreover there's one more caveat: if expression is used as parameter, we need to know if target function accepts nullable parameters and perform non-null assertion in some cases.

As the bottom line, I believe it would be much better to extend nullability support, at least for the Kotlin implementation.

I'm not sure that approach with adjusting the whole DataType system with, for example isNullable flag is the proper way to implement null safety support. Therefore I'm looking for some help with this using that or alternative approach.

In short, there are very few things needs to be done:

  • a way to resolve nullability of expression
  • a way to resolve nullability of a combined type
  • a way to provide nullability with the DataType itself
  • ability to translate expressions depending on operand's type nullability

xpathexception avatar May 09 '24 15:05 xpathexception

This might be related: https://github.com/kaitai-io/kaitai_struct/issues/141

Looking forward to Kotlin Multiplatform support!

Skaldebane avatar Jul 25 '24 17:07 Skaldebane

@xpathexception Can open a pull request with your work on the Kotlin support?

Since there's no direct support for nullability, I think shipping the current implementation (where nulls are ignored) may be okay, until there's support in Kaitai itself. This affects other languages like C# as well (and I'm not sure how the Rust folks are handling this).

Another path to take is to err on the safe side and mark literally everything as nullable, but that would be very inconvenient and useless to the end-user of the generated code, just more annoying to avoid (i.e. a catch-all try/catch vs. 100s of ? or !! symbols) without much benefit over just throwing an NPE.

Skaldebane avatar Jul 25 '24 18:07 Skaldebane

@Skaldebane I had no plans on publishing my work (i.e. opening PR) until this issue resolved somehow because of couple of reasons:

  • in it's current state it is more of an experiment, than a production-ready implementation - I believe it will take a couple more iterations to improve it and bring it up to Kaiati's standards
  • the implementation itself is complicated and undocumented - I've made numerous amount of design decisions that needed to be properly documented and proven
  • I had to rewrite and modify runtime library due to Kotlin's structure and Kotlin Multiplatform limitations especially - we have no built-in IO and FS support (considering kotlinx-io not being mature enough and having it's own limitations), so I had to rely on okio and did some black magic in order to make substreams/seeking work at least at first glance
  • personally I don't like the approach of throwing NPEs - it will make code on the caller side too complex and fragile - definitely, not the way it supposed to be

Giving all of that, I can't see Kotlin Multiplatform support as a part of the main, at least in it's current state and without interest and support from maintainers.

And finally answering your question. It will take some time and effort to wrap things up and publish them. I can try to do so if you're willing to help with further improvement and take a part in this adventure :)

xpathexception avatar Jul 28 '24 06:07 xpathexception

@xpathexception Thanks for the thorough response!

I also dislike throwing NPEs, but if they're the only way forward in the current state of things, then I guess we'll have to make do with that. We may be able to take inspiration from the way this works in the C# or Rust implementation.

As for inclusion in main, I think (after cleaning things) that should be possible, but while marking it as "entry-level support", like Rust and Go are marked in Kaitai's website.

While I don't currently have much time to help with this (due to some life circumstances), I'm quite interested and will surely hit you up when I'm ready to hop on this (hopefully very soon). I never worked with Scala or code generation before, but I can help with the runtime part and getting it working on all KMP platforms at the very least.

Skaldebane avatar Aug 10 '24 23:08 Skaldebane

@Skaldebane I've managed to collect all the things I've done as of now.

There are three parts: compiler branch, tests branch and runtime repository.

I've synced all of it with the latest changes, but haven't tried to compile. It is hard to recall latest state and roadmaps I had in mind, so it will require some effort to make all of that at least compile.

Tests itself were compiling, but they're will require some manual adjustments because of nullability issues/readers implementations inconsistency. I've tried to mimic original tests buildscripts in ruby, but had no luck, so they're may be broken at all.

There may also be a set of "homemade feature-flags" because I've tried some different approaches. I have to say that this code was not meant to be shared at this moment and may be very ugly in a bunch of ways - it is still just a half-working POC.

At last, I've decided to stop working on kotlin support as a part of kaitai struct at all, but rather started implementation of compiler, tests and runtime in pure kotlin. I'm working on it for more than a month now and it has advanced very well (I've even managed to add more or less proper nullability support), but I'll keep it private for some time until some major issues are resolved. Maybe I'll make a public roadmap of it somewhere in my repos, I still haven't decided on it.

xpathexception avatar Aug 14 '24 22:08 xpathexception

@xpathexception , just for your reference, you may be interested to look at my compiler in Rust to get some inspiration. Recently, I have resumed active work on it.

Mingun avatar Aug 15 '24 04:08 Mingun

@xpathexception That's great to hear! The pure-Kotlin implementation sounds great, excited to see how that turns out.

I'd be happy to help if you need assistance with any part of the work.

Skaldebane avatar Aug 15 '24 15:08 Skaldebane