antlr / antlr5

BSD 3-Clause "New" or "Revised" License
57 stars 5 forks source link

Integrate Kotlin runtime #6

Closed KvanTTT closed 10 months ago

KvanTTT commented 10 months ago

I've merged Strumenta's Kotlin Runtime repository (git history is preserved) and integrated it in out ANTLR runtime test infrastructure (runtime-testsuite). Fixes #1

To make it possible to run tests, I had to fix Kotlin.stg template, fix some runtime files, write Kotlin.test.stg and other wrapping code. Also, I had to remove expect/actual functions because they don't work reliable with Maven, and expect/actual within one module are prohibited since Kotlin 2.0.

Generally, everything is not so bad as I thought before, and currently we have 35 failed tests out of 356. It's worth mention, kotlinc cmd tools should be configured.

Remaining problems in tests:

error: extension property cannot be initialized because it has no backing field.
        public var Tokens.INT: Token? = null

Subsystem to fix: code generation.

error: platform declaration clash: The following declarations have the same JVM signature (getExpr()LTParser$ExprContext;):
    fun `<get-expr>`(): TParser.ExprContext? defined in TParser.SContext
    fun getExpr(): TParser.ExprContext defined in TParser.SContext
        public var expr: ExprContext? = null
        ^

Subsystem to fix: code generation.

Subsystem to fix: code generation (probably only Kotlin.stg).

Subsystem to fix: Kotlin runtime.

KvanTTT commented 10 months ago

@ericvergnaud is it possible to set up GitHub Actions for this repository (probably some option in GitHub UI). I don't see Actions in this repository and CI is not triggered despite the fact .github/workflows/hosted.yml file exists in the repository (as it exists in antlr4).

I want to make sure Kotlin GitHub Action is configured correctly and Kotlin runtime tests work.

ericvergnaud commented 10 months ago

I've enabled actions

ericvergnaud commented 10 months ago

Don't forget to -s your commits

lppedd commented 10 months ago

Very nice, thanks!

Happy to see we don't have that many failing tests too.

KvanTTT commented 10 months ago

Don't forget to -s your commits

I have signature (if I understand -s correctly) on all my commits. But I merged a lot of other commits without signature from external Kotlin repository. Probably it affects DCO bot as well.

KvanTTT commented 10 months ago

Interesting, GitHub CI is not activated for this PR anyway.

KvanTTT commented 10 months ago

@ericvergnaud it could be caused by merge conflicts. Sorry, but may I drop all your commits in ANTLR5 from dev since ebb511a04a60ae5a605aba65471c07dd854e9303 (your latest commit in antlr4)? I started my Kotlin integration branch on this commit and we have definitely working build on this commit unlike the current dev.

I suggest making renaming and versions upgrading a bit later, I'm not sure it's very improtant for now. But I'll keep your already introduced commits separately in another branch and nothing will be loosed (probably you also have this branch unremoved). Also, next time I'm going to squash them to get rid of non-informative commits.

ericvergnaud commented 10 months ago

Sure, whatever works is fine with me, provided we don't lose any changes

ftomassetti commented 10 months ago

Thank you @KvanTTT : that is amazing news. I am running the tests locally and try to see how I can help get some of these tests fixed

KvanTTT commented 10 months ago

Now Kotlin tests work on CI at least on Ubuntu: https://github.com/antlr/antlr5/actions/runs/7643931630/job/20827098759?pr=6#step:24:4436 (it shows 36 failed tests instead of 35 as I wrote before, but locally I tested on K2 and it doesn't fail on a big grammar unlike the current K1).

KvanTTT commented 10 months ago

The bad news: Kotlin tests are very slow, it's especially actual for remote servers. But I have an idea how performance can be significantly improved. I'll try it later (it looks like the full runtime is being compiled on every test, it's very wasteful, much better to compile the runime only once and use result class files in tests as we do for other runtimes).

ftomassetti commented 10 months ago

I have prepared a PR on this PR (#8).

It brings down the tests failing from 36 to 3.

They are:

KvanTTT commented 10 months ago

It brings down the tests failing from 36 to 3.

Great news!

LexerExec.AtnStatesSizeMoreThan65535: we have an OutOfMemoryError here, but if I understood correctly what @KvanTTT wrote, it will disappear when using K2

Yes, I meant exactly that. This test is green if switch to K2.

Sets.UnicodeEscapedSMPRangeSetMismatch: this one is tricky, as if we compile to WASM, I guess we cannot use Java specific classes for dealing with Unicode, so I am not sure how to solve this one

I'll take a look what we can do with Unicode text once again. If I fails, I think Unicode tests can be ignored.

KvanTTT commented 10 months ago

So, I've fixed all remaining tests, now build status is completely green:

Also, I've fixed testsuite running on Windows CI.

Finally, I've integrated Strumenta's Kotlin runtime specific tests. I'm not sure if all of them are valuable considering the fact now we have much better coverage, but we can remove them later if it's needed.

@ericvergnaud I think it's ready for review. If everything is ok, just Approve the PR and I'll merge it by myself (it's not a regular PR since it contains a merge from unrelated histories and it's better to merge it manually).

ericvergnaud commented 10 months ago

Wow! Amazing! I'll look into that tomorrow morning 😃 !

KvanTTT commented 10 months ago

I leaved untouched unused kt files in antlr-kotlin-runtime/src:

It we are going to implement KMP features later (with Native, Wasm targets), these files can be used and the git history will be preserved as currently it's preserved for Kotlin runtime files (git is able to track files moving from directory to directory).

KvanTTT commented 10 months ago

Merged manually (force-pushed to dev)! Now we can use standard and safe GitHub merging methods like Rebase and merge or Squash and merge.

ftomassetti commented 10 months ago

Great to see this one merged!