Strumenta / antlr-kotlin

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

Unneeded @Suppress("UNSAFE_CALL") causes build warning #200

Open JPercival opened 1 week ago

JPercival commented 1 week ago

When compiling a parser generated by antlr-kotlin 1.0.1 using Kotlin 2.0.20 and Gradle 8.11 the Kotlin compiler emits this warning:

 [ERROR_SUPPRESSION] This code uses error suppression for 'UNSAFE_CALL'. While it might compile and work, the compiler behavior is UNSPECIFIED and WON'T BE PRESERVED. Please report your use case to the Kotlin issue tracker instead: https://kotl.in/issue

The Suppression comes from this resource file that defines the template for semantic predicates:

https://github.com/Strumenta/antlr-kotlin/blob/master/antlr-kotlin-target/src/main/resources/org/antlr/v4/tool/templates/codegen/Kotlin/Kotlin.stg#L381-L382

The notes indicate that the function body references the _localctx parameter which is a nullable type. In practice, the generated functions seem to always reference context instead. As an example from the parser I've attached:

 @Suppress("UNSAFE_CALL")
    private fun expr_sempred(_localctx: ExprContext?, predIndex: Int): Boolean {
        when (predIndex) {
            0 -> return (precpred(context!!, 5))
            1 -> return (precpred(context!!, 4))
        }

        return true
    }

Attached is a Gradle project with a made-up arithmetic grammar to reproduce the error. Build with ./gradlew build.

A wild guess is that the suppression was added earlier in development and it's no longer needed, but it may also simply be that the grammars I've tried never hit a path that would cause a _localctx reference to be generated.

issue-repro.zip

ftomassetti commented 1 week ago

Thank you for reporting this

lppedd commented 1 week ago

It's not really a bug, but more of a deliberate choice to avoid null checking in custom actions.

As you can see by the comment above in the .stg file, there is an alternative which I'll switch to as soon as error suppression isn't allowed anymore.

At the moment you can ignore it, unless it somehow messes up your pipeline. In that case I'll consider refactoring sooner.

JPercival commented 1 week ago

Not urgent, just an annoying warning in the build pipeline that can't be suppressed. Thanks for looking at this!

JPercival commented 1 week ago

The difference between what the comment says and what I actually see in the compiled output is that _localctx is never referenced at all in my testing. IOW, it may already be the case that the suppression is unneeded and it's not clear why you'd swap to _localctx!!.<a.name> over the existing context. Hopefully that helps clarify my report! :)

lppedd commented 1 week ago

IIRC the MySQL grammar we use for benchmarking has got actions that reference the parameter.

I'll have a look soon tho, don't have much free time lately.

lppedd commented 5 days ago

I think I remember why we have that UNSAFE_CALL. Let's take this grammar.

assignment : ID ASSIGN expression { $text == "something" }?;

$text == "something" is translated by ANTLR to _input.getText(_localctx.start, _input.LT(-1)) == "something". Notice the _localctx.start: without the UNSAFE_CALL suppression that piece of code would result in an error.

Now, my comment

An alternative is to change RetValueRef ...

doesn't seem to hold true anymore, as my experiments show that we could modify other rules, such as:

ThisRulePropertyRef_start
ThisRulePropertyRef_stop
ThisRulePropertyRef_text
ThisRulePropertyRef_ctx
ThisRulePropertyRef_parser

I will keep investigating, but any other idea or thought is welcomed.

You can also find the parser rule attributes at page 276 of the ANTL4 Reference book.