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.

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

Extended nullabillity support for Kotlin and other null-safe languages #1111

Open
xpathexception opened this issue May 9, 2024 · 7 comments

Comments

@xpathexception
Copy link

xpathexception commented May 9, 2024

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
@Skaldebane
Copy link

This might be related: #141

Looking forward to Kotlin Multiplatform support!

@Skaldebane
Copy link

@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.

@xpathexception
Copy link
Author

@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 :)

@Skaldebane
Copy link

Skaldebane commented Aug 10, 2024

@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.

@xpathexception
Copy link
Author

@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.

@Mingun
Copy link

Mingun commented Aug 15, 2024

@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.

@Skaldebane
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants