Strumenta / antlr-kotlin

Support for Kotlin as a target for ANTLR 4
Apache License 2.0
221 stars 47 forks source link

Adjust nullability for predicates and actions #158

Closed lppedd closed 7 months ago

lppedd commented 7 months ago

Fixes: #157

ftomassetti commented 7 months ago

It looks good, but in general it is very hard for me to review changes to the template. Perhaps having a bit more tests would be useful here

lppedd commented 7 months ago

@ftomassetti in this case I've pretty much replicated what the Swift template does.

But yes, if we can, having some other tests would be better. At some point we'll get there.

KvanTTT commented 6 months ago

FYI, I'm not sure if it's a correct patch. If I applies it, some tests from ANTLR 5 repository starts to fail (NullPointerException). I fixed the problem by adding @Suppress("UNSAFE_CALL") to private fun <r.name>_sempred(_localctx: <r.ctxType>?, predIndex: Int): Boolean. Using suppressing is also not the best solution, but at least it works (it could be fixed later).

lppedd commented 6 months ago

That's strange. Theoretically you should never have a null context at that point.

lppedd commented 6 months ago

I'll have a better look after porting back the template fixes.

KvanTTT commented 6 months ago

I recommend trying out tests in the ANTLR 5 runtime (my branch regarding integration until it's merged) 😊

lppedd commented 6 months ago

I'll try. It's just strange that it happens with Kotlin and not with Swift, I've used the exact same nullabilities.

lppedd commented 6 months ago

@KvanTTT managed to reproduce also with a MySQL grammar. I'll check what's happening.

lppedd commented 6 months ago

@KvanTTT so I was wrong, there are situations where a null context is possible.
In case of the MySQL grammar, a LexerCustomAction is executed, which has an execute method like:

override fun execute(lexer: Lexer): Unit =
  lexer.action(null, ruleIndex, actionIndex)

The lexer in that case is MySQLLexer, which overrides the action method:

override fun action(_localctx: RuleContext?, ruleIndex: Int, actionIndex: Int) {
    when (ruleIndex) {
        21 -> LOGICAL_OR_OPERATOR_action(_localctx as RuleContext, actionIndex)
        70 -> INT_NUMBER_action(_localctx as RuleContext, actionIndex) // <-- NPE 
        ...
KvanTTT commented 6 months ago

Yes, and such situations are also possible in testsuite.