Generated BaseVisitor does not handle nullable types

[simonolander]

Let's say I want a visitor that traverses the tree until it finds a certain kind of node whose text also can be parsed to an Int. If no such node is found, null is produced.

I would like to write the following class:

class MaybeIntVisitor : MyBaseVisitor<Int?>() {
    override fun visitMaybeInt(ctx: MyParser.MaybeIntContext): Int? {
        return ctx.text.toIntOrNull()
    }

    override fun shouldVisitNextChild(
        node: RuleNode,
        currentResult: Int?,
    ) = currentResult == null
}

Sadly, it's not possible, because all the visitSomething methods that I'm not overriding assert that the result is non-null.

public open class MyBaseVisitor<T> : AbstractParseTreeVisitor<T>(), MyVisitor<T> {

    // ...

    override fun visitMaybeInt(ctx: MyParser.MaybeIntContext): T {
        return this.visitChildren(ctx)!! // overridden by MaybeIntVisitor
    }

    override fun visitSomeOtherThing(ctx: MyParser.SomeOtherThingContext): T {
        return this.visitChildren(ctx)!! // <- throws NPE
    }
}

This causes a NullPointerException to be thrown.

Could you generate the base visitor in such a way as to make nullable types possible?

[ftomassetti]

Hi @simonolander , thank you for opening this issue. I personally never use visitors so I am not be the best person to comment on this, but a first look your use case make sense, so we may eventually support it. Of course, if you are willing to put forward a PR that could accelerate things :)

[simonolander]

Hi @ftomassetti, thanks for the quick response!

I personally never use visitors

How do you typically use the library? If I'm using it in a niche way, I would like to know.

if you are willing to put forward a PR

I might, but the project is a bit daunting.

Skimming through the code, it seems the assertions originate from Kotlin.stg:213, but I think this change would cascade to many files in org.antlr.v4.kotlinruntime.tree.

There are many classes that are parameterized T, but whose methods return a T?, effectively hiding that T might already be nullable. Perhaps this stems from the fact that AbstractParseTreeVisitor implements

protected open fun defaultResult(): T? = null

and since the default is null, everything is forced to allow for the possibility.

Are you pulling org/antlr/v4/kotlinruntime from somewhere, or are you writing it yourselves?

One potential solution is to change AbstractParseTreeVisitor to instead declare

protected abstract fun defaultResult(): T

and leave the implementation to whoever wants to extend AbstractParseTreeVisitor. What do you think of that idea?

[lppedd]

One potential solution is to change AbstractParseTreeVisitor to instead declare an abstract defaultResult

This looks like the best approach we can follow.

However this is going to break every existing usage of antlr-kotlin, as AbstractParseTreeVisitor is the base class for all generated visitors.

We can partially mitigate it by adding a default implementation for defaultResult that throws an error:

// From Kotlin.stg
public open class <file.grammarName>BaseVisitor\<T> : AbstractParseTreeVisitor\<T>(), <file.grammarName>Visitor\<T> {
    override fun defaultResult(): T {
        throw NotImplementedError()
    }

That could be removed in a subsequent release, when we think most people have updated their code, and the class could be made abstract to ensure the function is always overridden. Edit: let's skip this part and go straight to declaring the class abstract. Doesn't really make sense to have that intermediate step.

I've implemented this idea very quickly now and it seems it can work, but I'm not yet sure 100%.

[ftomassetti]

@simonolander a solution to this has been included in the latest release (1.0.0-RC4)

[simonolander]

Looks good, well done on the fast work!