Skip to content

Fix code generation by partially backporting ANTLR5 changes #167

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Fix code generation by partially backporting ANTLR5 changes #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2898fc
refactor: fix nullability for TokenStream.getText parameters
lppedd Jan 31, 2024
e77b7ec
refactor: remove unnecessary overload for ATNDeserializer.decodeIntsE…
lppedd Jan 31, 2024
f2f9754
refactor: fix code generation
lppedd Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/BufferedTokenStream.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright 2017-present Strumenta and contributors, licensed under Apache 2.0.
// Copyright 2024-present Strumenta and contributors, licensed under BSD 3-Clause.

package org.antlr.v4.kotlinruntime

import com.strumenta.antlrkotlin.runtime.assert
Expand Down Expand Up @@ -474,8 +473,12 @@ public open class BufferedTokenStream(tokenSource: TokenSource) : TokenStream {
override fun getText(ctx: RuleContext): String =
getText(ctx.sourceInterval)

override fun getText(start: Token, stop: Token): String? =
getText(Interval.of(start.tokenIndex, stop.tokenIndex))
override fun getText(start: Token?, stop: Token?): String? =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be moved to TokenStream? Or become an extension method for TokenStream? In this way we would avoid the duplication we have in UnbufferedTokenStream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ftomassetti we could, but the better approach imo is to stick with simple code that is aligned with ANTLR4 Java, and ANTLR5, even if there is a small amount of duplication.

if (start != null && stop != null) {
getText(Interval.of(start.tokenIndex, stop.tokenIndex))
} else {
""
}

/**
* Get all tokens from lexer until `EOF`.
Expand Down
1 change: 0 additions & 1 deletion antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/CommonTokenStream.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright 2017-present Strumenta and contributors, licensed under Apache 2.0.
// Copyright 2024-present Strumenta and contributors, licensed under BSD 3-Clause.

package org.antlr.v4.kotlinruntime

/**
Expand Down
2 changes: 1 addition & 1 deletion antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/DummyTokenStream.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,6 @@ internal object DummyTokenStream : TokenStream {
override fun getText(ctx: RuleContext): String =
throw UnsupportedOperationException()

override fun getText(start: Token, stop: Token): String =
override fun getText(start: Token?, stop: Token?): String =
throw UnsupportedOperationException()
}
3 changes: 1 addition & 2 deletions antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/TokenStream.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright 2017-present Strumenta and contributors, licensed under Apache 2.0.
// Copyright 2024-present Strumenta and contributors, licensed under BSD 3-Clause.

package org.antlr.v4.kotlinruntime

import org.antlr.v4.kotlinruntime.misc.Interval
Expand Down Expand Up @@ -134,5 +133,5 @@ public interface TokenStream : IntStream {
* @throws UnsupportedOperationException If this stream does not support
* this method for the specified tokens
*/
public fun getText(start: Token, stop: Token): String?
public fun getText(start: Token?, stop: Token?): String?
}
9 changes: 6 additions & 3 deletions ...-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/UnbufferedTokenStream.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright 2017-present Strumenta and contributors, licensed under Apache 2.0.
// Copyright 2024-present Strumenta and contributors, licensed under BSD 3-Clause.

package org.antlr.v4.kotlinruntime

import com.strumenta.antlrkotlin.runtime.System
Expand Down Expand Up @@ -112,8 +111,12 @@ public open class UnbufferedTokenStream(override val tokenSource: TokenSource, b
override fun getText(ctx: RuleContext): String =
getText(ctx.sourceInterval)

override fun getText(start: Token, stop: Token): String =
getText(Interval.of(start.tokenIndex, stop.tokenIndex))
override fun getText(start: Token?, stop: Token?): String =
if (start != null && stop != null) {
getText(Interval.of(start.tokenIndex, stop.tokenIndex))
} else {
""
}

override fun consume() {
if (LA(1) == Token.EOF) {
Expand Down
13 changes: 4 additions & 9 deletions antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/atn/ATNDeserializer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,14 @@ public open class ATNDeserializer(deserializationOptions: ATNDeserializationOpti
for (state in atn.states) {
if (state is BlockStartState) {
// We need to know the end state to set its start state
if (state.endState == null) {
throw IllegalStateException()
}
val endState = state.endState ?: throw IllegalStateException()

// Block end states can only be associated to a single block start state
if (state.endState!!.startState != null) {
if (endState.startState != null) {
throw IllegalStateException()
}

state.endState!!.startState = state
endState.startState = state
}

if (state is PlusLoopbackState) {
Expand Down Expand Up @@ -538,13 +536,10 @@ public open class ATNDeserializer(deserializationOptions: ATNDeserializationOpti
LexerActionType.TYPE -> LexerTypeAction(data1)
}

public open fun decodeIntsEncodedAs16BitWords(data16: CharArray): IntArray =
decodeIntsEncodedAs16BitWords(data16, false)

/**
* Convert a list of chars (16 uint) that represent a serialized and compressed list of ints for an ATN.
*/
public open fun decodeIntsEncodedAs16BitWords(data16: CharArray, trimToSize: Boolean): IntArray {
public open fun decodeIntsEncodedAs16BitWords(data16: CharArray, trimToSize: Boolean = false): IntArray {
// Will be strictly smaller, but we waste bit of space to avoid copying during initialization of parsers
val data = IntArray(data16.size)
var i = 0
Expand Down
9 changes: 1 addition & 8 deletions antlr-kotlin-target/src/main/kotlin/org/antlr/v4/codegen/target/KotlinTarget.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package org.antlr.v4.codegen.target
import org.antlr.v4.codegen.CodeGenerator
import org.antlr.v4.codegen.Target
import org.antlr.v4.codegen.UnicodeEscapes
import org.antlr.v4.tool.Grammar

public class KotlinTarget(codeGenerator: CodeGenerator) : JavaTarget(codeGenerator) {
private companion object {
Expand Down Expand Up @@ -70,6 +69,7 @@ public class KotlinTarget(codeGenerator: CodeGenerator) : JavaTarget(codeGenerat
addAll(kotlinKeywords)
add("rule")
add("parserRule")
add("ruleIndex")
}

override fun getVersion(): String =
Expand All @@ -92,13 +92,6 @@ public class KotlinTarget(codeGenerator: CodeGenerator) : JavaTarget(codeGenerat
override fun appendUnicodeEscapedCodePoint(codePoint: Int, sb: StringBuilder): Unit =
UnicodeEscapes.appendEscapedCodePoint(sb, codePoint, "Java")

override fun getTokenTypeAsTargetLabel(g: Grammar, ttype: Int): String {
// All tokens are namespaced inside a Tokens object.
// Here we simply force the qualification
val label = super.getTokenTypeAsTargetLabel(g, ttype)
return "Tokens.$label"
}

override fun getTargetStringLiteralFromANTLRStringLiteral(
generator: CodeGenerator,
literal: String,
Expand Down
53 changes: 29 additions & 24 deletions antlr-kotlin-target/src/main/resources/org/antlr/v4/tool/templates/codegen/Kotlin/Kotlin.stg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import org.antlr.v4.kotlinruntime.atn.ATN.Companion.INVALID_ALT_NUMBER
import org.antlr.v4.kotlinruntime.dfa.*
import org.antlr.v4.kotlinruntime.misc.*
import org.antlr.v4.kotlinruntime.tree.*
import kotlin.jvm.JvmField

<parser>
>>
Expand Down Expand Up @@ -293,7 +294,7 @@ public open class <parser.name>(input: TokenStream) : <superClass; null="Parser"

override fun sempred(_localctx: RuleContext?, ruleIndex: Int, predIndex: Int): Boolean {
when (ruleIndex) {
<parser.sempredFuncs.values:{f|<f.ruleIndex> -> return <f.name>_sempred(_localctx as <f.ctxType>, predIndex)}; separator="\n">
<parser.sempredFuncs.values:{f|<f.ruleIndex> -> return <f.name>_sempred(_localctx as <f.ctxType>?, predIndex)}; separator="\n">
}

return true
Expand Down Expand Up @@ -322,7 +323,7 @@ dumpActions(recog, argFuncs, actionFuncs, sempredFuncs) ::= <<

override fun action(_localctx: RuleContext?, ruleIndex: Int, actionIndex: Int) {
when (ruleIndex) {
<recog.actionFuncs.values:{f|<f.ruleIndex> -> <f.name>_action(_localctx as <f.ctxType>, actionIndex)}; separator="\n">
<recog.actionFuncs.values:{f|<f.ruleIndex> -> <f.name>_action(_localctx, actionIndex)}; separator="\n">
}
}

Expand All @@ -332,7 +333,7 @@ override fun action(_localctx: RuleContext?, ruleIndex: Int, actionIndex: Int) {

override fun sempred(_localctx: RuleContext?, ruleIndex: Int, predIndex: Int): Boolean {
when (ruleIndex) {
<recog.sempredFuncs.values:{f|<f.ruleIndex> -> return <f.name>_sempred(_localctx as <f.ctxType>, predIndex)}; separator="\n">
<recog.sempredFuncs.values:{f|<f.ruleIndex> -> return <f.name>_sempred(_localctx, predIndex)}; separator="\n">
}

return true
Expand All @@ -346,7 +347,7 @@ override fun sempred(_localctx: RuleContext?, ruleIndex: Int, predIndex: Int): B
* overriding implementation impossible to maintain.
*/
RuleActionFunction(r, actions) ::= <<
private fun <r.name>_action(_localctx: <r.ctxType>, actionIndex: Int) {
private fun <r.name>_action(_localctx: <r.ctxType>?, actionIndex: Int) {
when (actionIndex) {
<actions:{index|
<index> -> {
Expand All @@ -356,11 +357,15 @@ private fun <r.name>_action(_localctx: <r.ctxType>, actionIndex: Int) {
}
>>

/* This generates a private method since the predIndex is generated, making an
* overriding implementation impossible to maintain.
*/
// This generates a private method since the predIndex is generated, making an
// overriding implementation impossible to maintain.
//
// We suppress UNSAFE_CALL as in "_localctx.something" _localctx is null,
// but we should be able to compile anyway.
// An alternative is to change RetValueRef to "_localctx!!.<a.name>".
RuleSempredFunction(r, actions) ::= <<
private fun <r.name>_sempred(_localctx: <r.ctxType>, predIndex: Int): Boolean {
@Suppress("UNSAFE_CALL")
private fun <r.name>_sempred(_localctx: <r.ctxType>?, predIndex: Int): Boolean {
when (predIndex) {
<actions:{index|<index> -> return (<actions.(index)>)}; separator="\n">
}
Expand Down Expand Up @@ -412,11 +417,11 @@ LeftRecursiveRuleFunction(currentRule, args, code, locals, ruleCtx, altLabelCtxs
<ruleCtx>
<altLabelCtxs:{l | <altLabelCtxs.(l)>}; separator="\n">

<if(currentRule.modifiers)><currentRule.modifiers:{f | <f> }><else>public<endif> fun <currentRule.name>(<args; separator=", ">): <currentRule.ctxType> {
return <currentRule.name>(0<currentRule.args:{a | , <a.name>}>)
<if(currentRule.modifiers)><currentRule.modifiers:{f | <f> }><else>public<endif> fun <currentRule.escapedName>(<args; separator=", ">): <currentRule.ctxType> {
return <currentRule.escapedName>(0<currentRule.args:{a | , <a.name>}>)
}

private fun <currentRule.name>(_p: Int<args:{a | , <a>}>): <currentRule.ctxType> {
private fun <currentRule.escapedName>(_p: Int<args:{a | , <a>}>): <currentRule.ctxType> {
var _parentctx = context
var _parentState = state
var _localctx = <currentRule.ctxType>(context, _parentState<currentRule.args:{a | , <a.name>}>)
Expand Down Expand Up @@ -621,11 +626,11 @@ offsetShift(shiftAmount, offset) ::= <%

// produces more efficient bytecode when bits.tokens contains at most two items
bitsetInlineComparison(s, bits) ::= <%
<bits.tokens:{t | <s.varName> == <t.name>}; separator=" || ">
<bits.tokens:{t | <s.varName> == Tokens.<t.name>}; separator=" || ">
%>

cases(tokens) ::= <<
<tokens:{t | <t.name>}; separator=", "> -> >>
<tokens:{t | Tokens.<t.name>}; separator=", "> -> >>

InvokeRule(r, argExprsChunks) ::= <<
this.state = <r.stateNumber>
Expand All @@ -640,10 +645,10 @@ _ctx = <r.escapedName>(<if(r.ast.options.p)><r.ast.options.p><if(argExprsChunks)
MatchToken(m) ::= <<
this.state = <m.stateNumber>
<if(m.labels)>
_token = match(<m.name>)
_token = match(Tokens.<m.name>)
<m.labels:{l | <labelref(l)> = _token}; separator="\n">
<else>
match(<m.name>)
match(Tokens.<m.name>)
<endif>
>>

Expand Down Expand Up @@ -722,13 +727,13 @@ ActionText(t) ::= "<t.text>"
ActionTemplate(t) ::= "<t.st>"
ArgRef(a) ::= "_localctx.<a.name>"
LocalRef(a) ::= "_localctx.<a.name>"
RetValueRef(a) ::= "_localctx.<a.name>"
RetValueRef(a) ::= "_localctx.<a.escapedName>"
QRetValueRef(a) ::= "<ctx(a)>.<a.dict>!!.<a.name>"

/** How to translate $tokenLabel */
TokenRef(t) ::= "<ctx(t)>.<t.name>"
LabelRef(t) ::= "<ctx(t)>.<t.name>"
ListLabelRef(t) ::= "<ctx(t)>.<ListLabelName(t.name)>"
ListLabelRef(t) ::= "<ctx(t)>.<ListLabelName(t.escapedName)>"
SetAttr(s, rhsChunks) ::= "<ctx(s)>.<s.name> = <rhsChunks>"

TokenLabelType() ::= "<file.TokenLabelType; null={Token}>"
Expand All @@ -744,13 +749,13 @@ TokenPropertyRef_int(t) ::= "(<ctx(t)>.<t.label>?.text!!.toInt() ?: 0)"

RulePropertyRef_start(r) ::= "(<ctx(r)>.<r.label>?.start)"
RulePropertyRef_stop(r) ::= "(<ctx(r)>.<r.label>?.stop)"
RulePropertyRef_text(r) ::= "(<ctx(r)>.<r.label>?.let { _input.getText(it.start!!, it.stop!!) })"
RulePropertyRef_text(r) ::= "(<ctx(r)>.<r.label>?.let { _input.getText(it.start, it.stop) })"
RulePropertyRef_ctx(r) ::= "<ctx(r)>.<r.label>"
RulePropertyRef_parser(r) ::= "this"

ThisRulePropertyRef_start(r) ::= "_localctx.start"
ThisRulePropertyRef_stop(r) ::= "_localctx.stop"
ThisRulePropertyRef_text(r) ::= "_input.getText(_localctx.start!!, _input.LT(-1)!!)"
ThisRulePropertyRef_text(r) ::= "_input.getText(_localctx.start, _input.LT(-1))"
ThisRulePropertyRef_ctx(r) ::= "_localctx"
ThisRulePropertyRef_parser(r) ::= "this"

Expand Down Expand Up @@ -801,11 +806,11 @@ StructDecl(struct, ctorAttrs, attrs, getters, dispatchMethods, interfaces, exten
public open class <struct.name> : <if(contextSuperClass)><contextSuperClass><else>ParserRuleContext<endif><if(interfaces)> : <interfaces; separator=", "><endif> {
override val ruleIndex: Int = Rules.<struct.derivedFromName; format="cap">

<attrs:{a | public var <a>}; separator="\n">
<attrs:{a | @JvmField public var <a>}; separator="\n">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change may break existing code, which I think is acceptable, but I just wonder if it could make sense to first replicate the tests we have in ANTLR 5, so that we are more confident this change will work on all platforms before introducing it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this avoid the platform clash on the JVM, but does it work also on other platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change may break existing code

I think that's not something you have to take into account, as I really doubt a Java consumer is using this Kotlin target.

so that we are more confident this change will work on all platforms before introducing it

I have tested with code specifically written to reproduce conflicting signatures, and all supported platforms compiled and run correctly.

Also, this avoid the platform clash on the JVM, but does it work also on other platforms?

The problem is specific to the JVM. In JS/TS you end up with code similar to:

class Example {
    get foo(): number;
    getFoo(): string;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piacenti ahh no, this is what I did test, your case is a new one.

<getters:{g | <g>}; separator="\n">

public constructor(parent: ParserRuleContext?, invokingState: Int<ctorAttrs:{a | , <a>}>) : super(parent, invokingState) {
<struct.ctorAttrs:{a | this.<a.name> = <a.name>}; separator="\n">
<struct.ctorAttrs:{a | this.<a.escapedName> = <a.escapedName>}; separator="\n">
}

<! Don't need copy unless we have subclasses !>
Expand All @@ -814,7 +819,7 @@ public open class <struct.name> : <if(contextSuperClass)><contextSuperClass><els

public fun copyFrom(ctx: <struct.name>) {
super.copyFrom(ctx)
<struct.attrs:{a | this.<a.name> = ctx.<a.name>}; separator="\n">
<struct.attrs:{a | this.<a.escapedName> = ctx.<a.escapedName>}; separator="\n">
}
<endif>
<dispatchMethods; separator="\n\n">
Expand All @@ -825,7 +830,7 @@ public open class <struct.name> : <if(contextSuperClass)><contextSuperClass><els

AltLabelStructDecl(struct, attrs, getters, dispatchMethods) ::= <<
public open class <struct.name> : <currentRule.name; format="cap">Context {
<attrs:{a | public var <a>}; separator="\n">
<attrs:{a | @JvmField public var <a>}; separator="\n">
<getters:{g | <g>}; separator="\n">

public constructor(ctx: <currentRule.name; format="cap">Context) {
Expand Down Expand Up @@ -959,7 +964,7 @@ public open class <lexer.name>(input: CharStream) : <superClass; null="Lexer">(i
public object Modes {
public const val DEFAULT_MODE: Int = 0
<if(rest(lexer.modes))>
<rest(lexer.modes):{m | public const val <m>: Int = <i>}; separator="\n">
<rest(lexer.modes):{m | public const val <m; format="upper">: Int = <i>}; separator="\n">
<endif>
}

Expand Down