Clean up ATN serialization: rm UUID and shifting by value of 2

[parrt]

• I think we don't need the UUID in the serialization, since it has not changed in a decade. We can bump the version number and remove the UU ID
• I did some tests and there seems to be no reason to shift the values in the serialized ATN by 2 for the purposes of improving the UTF-8 encoding for the Java target.

If you guys agree, we can make this small change for cleanup purposes. I'm happy to do it if you guys don't want to. The second fix will require changes to each target but it's trivial to fix.

[ericvergnaud]

Wouldn’t that break when old runtimes (expecting a UUID) try to read new grammars (without a UUID) ? Envoyé de mon iPhone

…

Le 30 janv. 2022 à 00:12, Terence Parr ***@***.***> a écrit :  Assigned #3515 to @ericvergnaud. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.

[KvanTTT]

It does not matter since, fortunately, generated parsers are not back-compatible because of version check on Runtime.

[KvanTTT]

Ok, I can fix the issue.

[ericvergnaud]

That just raises a warning With the proposed change they are likely to crash Envoyé de mon iPhone

…

Le 30 janv. 2022 à 12:29, Ivan Kochurkin ***@***.***> a écrit :  It does not matter since, fortunately, generated parsers are not back-compatible because of version check on Runtime. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[KvanTTT]

Yes, it will be crashed but with the clear message (Could not deserialize ATN with version 3 (expected 4)). Maybe it makes sense to add info about lexer/parser regeneration with a new tool. BTW some users don't like warning messages as well: #3278

[ericvergnaud]

We probably want to test this before making assertions ? I believe it would crash in Swift.

[parrt]

I think that right now we have three tests for compatibility: runtime version of the entire tool, ATN serialization version, and UUID. I never understood what the UUID. I don't think we need 3 checks. We probably don't even need the ATN serialization given the warning when people are mixing runtime and generated code from a different tool. We can leave the version number in there and the runtime. The runtime mismatch will give a warning I think but we can have the version number crash. But, then it begs the question why didn't the first one crash if it's not gonna work. I guess it makes sense that we could tweak the runtime libraries for multiple versions of the software and keep the ATN serialization the same. Definitely if there's a version difference in the serialization is crash.

Per @ericvergnaud I will go try this. It definitely crashed earlier when I tweak the UUID but will check with just a version number.

[parrt]

We probably want to test this before making assertions ? I believe it would crash in Swift.

It definitely crashes as we want at least for Java:

java.lang.UnsupportedOperationException: java.io.InvalidClassException: org.antlr.v4.runtime.atn.ATN; Could not deserialize ATN with version 3 (expected 4).

	at org.antlr.v4.runtime.atn.ATNDeserializer.deserialize(ATNDeserializer.java:90)
	at org.antlr.v4.runtime.misc.InterpreterDataReader.parseFile(InterpreterDataReader.java:133)
	at org.antlr.v4.test.runtime.java.TestInterpreterDataReader.testParseFile(TestInterpreterDataReader.java:24)

[KvanTTT]

It crashes with the same error in all runtimes after my changes.

[parrt]

Ok, that's good news. let me poke around with the unit tests that have no source g4 files.

[ericvergnaud]

Mmmm… Swift doesn’t check version, so that’s a bit surprising...

…

Le 30 janv. 2022 à 19:40, Ivan Kochurkin ***@***.***> a écrit : It crashes with the same error in all runtimes. — Reply to this email directly, view it on GitHub <#3515 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZNQJCSZ7JUFHVKLBKNGFDUYWAYHANCNFSM5NDOJ3EA>. You are receiving this because you were mentioned.

[parrt]

I see this in swift ATNDeserializer.swift:

if version != ATNDeserializer.SERIALIZED_VERSION {
    let reason = "Could not deserialize ATN with version \(version) (expected \(ATNDeserializer.SERIALIZED_VERSION))."
    throw ANTLRError.unsupportedOperation(msg: reason)
}

[KvanTTT]

Yes, Swift checks the version both in binary and in JSON deserializer methods. In the master Swift uses JSON serialization but I've removed it in one of the latest PR: #3513

[ericvergnaud]

Fine then Envoyé de mon iPhone

…

Le 30 janv. 2022 à 22:03, Ivan Kochurkin ***@***.***> a écrit :  Yes, Swift checks the version both in binary and in JSON deserializer methods. Now Swift uses JSON serialization but I've removed it in one of the latest PR: #3513 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[parrt]

Actually, concerning #3513, why not use the binary string in the parser file like others? Now Swift build is much more complicated.

[KvanTTT]

I've introduced multiline string serialization into Swift in that PR like other runtimes. It removes a lot of serializing code and decreases the size of the building output. I don't know exactly why Swift target authors introduced JSON serialization (maybe because there were a couple of errors in binary serialization), it looks excess.

[luisespino]

absolutely stellar support!

bendgk Absolutely, despite having other jobs, and still continue with the project.

[parrt]

Not rollback, just split on dev and master (create dev on current master and hard reset master on 4.9.3)

Yeah, that definitely makes sense but then I should change the default PR branch to dev. Hmm... I should think about this before I make a mistake. Have to run off at the moment...

[parrt]

I think I did it right!

[parrt]

Hmm... I just set the default branch to dev. @bendgk does the Go fetch

$ go get github.com/antlr/antlr4/runtime/Go/antlr

now just go to dev instead of master? If so, we have not changed anything except made the repository more complicated!!!

Maybe we can change the documentation to

$ go get github.com/antlr/antlr4/runtime/Go/antlr@master

I think this works with go 1.11. This seems to confirm it.

[parrt]

I started updating the README when I ran into this issue of the default branch.

@jcking, @mike-lischke, @marcospassos, @ericvergnaud Please note the change to the repository, which might of been a mistake ha ha

Now I'm wondering whether I should try to undo the damage and not push this change to the README. I'll wait to hear from the Go Target users.

[luisespino]

I'll wait to hear from the Go Target users.

@parrt Hi, I am working with ANTLR and Go. I fixed my problem by downloading the ZIP and making a local package. Because if you use: go get github.com/antlr/antlr4/runtime/Go/antlr it downloads the default branch, that is, the dev. Which still causes problem when using the visitor by version of deserialize ATN. I think the Master branch should be the default branch for use the import ("github.com/antlr/antlr4/runtime/Go/antlr") .

Greetings.

[parrt]

Hi! Well, can you try the

go get github.com/antlr/antlr4/runtime/Go/antlr@master

??? Hopefully that will make everything work properly

(We can't use the last release branch as the default because then PRs will all be created from that and not our current development branch.) :(

[parrt]

Ok, I have pushed the documentation changes to the master and dev branches. I did a reset hard on master after cutting the dev branch from our latest changes. Then, hi force pushed master to upstream antlr/antlr4. Hope that's right and that Go can use the @master module query. :)

[luisespino]

@parrt use @master in go get works fine, thanks for the support!

[parrt]

Oh, @KvanTTT and @ericvergnaud we need to change the continuous integration script so that they use dev not master. I just fixed the github CI for dev but I'm not sure how to do that for circleci.

[ericvergnaud]

from Circle-CI GitHub We will check the default branch and update it for your project on each commit pushed. If you change the default branch it will be updated on the next commit push to CircleCI

…

Le 8 févr. 2022 à 03:21, Terence Parr ***@***.***> a écrit : Oh, @KvanTTT <https://github.com/KvanTTT> and @ericvergnaud <https://github.com/ericvergnaud> we need to change the continuous integration script so that they use dev not master. I just fixed the github CI for dev but I'm not sure how to do that for circleci. — Reply to this email directly, view it on GitHub <#3515 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZNQJEI3VC3XJ2RUAUZXJ3U2B44JANCNFSM5NDOJ3EA>. You are receiving this because you were mentioned.

[mike-lischke]

Odd, my fork (https://github.com/mike-lischke/antlr4) does not show the dev branch, only master. Does this mean all forks have to re-fork again?

[ericvergnaud]

No, you need to go to GitHub and change your default brach from master to dev

[mike-lischke]

I cannot switch the default branch if dev is not even available:

[KvanTTT]

now just go to dev instead of master? If so, we have not changed anything except made the repository more complicated!!!

It looks like branches switching didn't solve any problem. Go users can use @release or @4.9.2 branches instead of @master as well. Maybe roll back everything back and add info about @release to Go?

BTW, maybe it also makes sense to rename master to main.

[KvanTTT]

Now every merge request contains a lot of unrelated changes and everyone should update the default branch in forks (if it's possible). I suggest rolling everything back and adding info about @release or @stable branch to Go.

go get github.com/antlr/antlr4/runtime/Go/antlr@release

[parrt]

Damn. Ok your suggestion makes sense. Anybody else think we should keep this new branching mechanism? Unfortunately I think it causes more problems than solves

On Tue, Feb 8, 2022 at 2:09 AM Ivan Kochurkin ***@***.***> wrote: Now every merge request contains a lot of unrelated changes and everyone should update the default branch in forks. I suggest rolling back everything back and adding info about @Release branch to Go. — Reply to this email directly, view it on GitHub <#3515 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABLUWI5R5DOKM5BV5Z5YQDU2DTWTANCNFSM5NDOJ3EA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Dictation in use. Please excuse homophones, malapropisms, and nonsense.

[mike-lischke]

Maybe it's just a temporary problem? Using a master/main branch as well as a dev branch (and many more) is common practice and I'd stay with at least these two, if possible.

[KvanTTT]

I'm not sure everyone will rebase their branch to dev to get rid of a lot of unrelated commits (take a look at any recent PR). Also, naming depends on a repository, it's possible to use master/main with dev as well as release with master.

[parrt]

We do have a lot of PRs in the queue. Can those easily reset to compare with dev not master? Looks like people can click edit then reset to compare to dev. Hm...affects many people to switch now. @ericvergnaud what do you think? @jcking ?

[parrt]

Ok, talked to @jcking just now; it won't cause any problems internally for the galactic overlords. I also like the idea of having a separate master and development branch, although it is a bit awkward that the default branch hass to be set to the development branch. In an ideal world the master branch would be the latest stable version and the default but PRs would pull from a development branch.

Anyway, in a nutshell, I'm going to leave it as is. Existing PRs can't easily tweak to base upon dev not master so let's proceed as we have it now.

Note: Watch out for git push origin dev not master haha

[parrt]

Also wondering if default branch should be master but keep dev branch. People in future would need to fork then create PRs from dev. As it is now, if we change the documentation or readme during development, we are in a fact changing the landing page or documentation that is visible by default. This is despite the fact that the software is not Yep an updated. For example, we have now removed the case insensitive parsing document but have yet to release the software.

Yes, I think most people are consumers and not developers of antlr and so I think the default brand should be master which will be the last stable release. This means we don't have to update the Go documentation for example but I will update the contributing-to-antlr doc.

[KvanTTT]

It might push some users away if they have seen the date of the last update of master. Also, probably, some new users will make pull requests to the master branch by default, that's not good. In the documentation, it's possible to add since version for new features and they are quite rare.

[parrt]

This is a tricky situation. I think we can simply put a first line on the readme that says they're looking at the last stable release not the development branch. Definitely some people will make a mistake and edit the master branch but of course we will see this in the PR during evaluation and I will update the documentation. With luck they can simply merge their changes with dev and then the PR Will look like it came originally from dev.

[parrt]

I've actually switched master back to being the default now and will update documentation/readme next

[parrt]

Ok, until we can stabilize these PRs and branches etc... Please be careful with pulling and pushing :) I have updated the readme and contributing doc: https://github.com/antlr/antlr4/blob/master/CONTRIBUTING.md

[KvanTTT]

My name is missing in Authors and major contributors in both branches :) It was added some time ago by you.

[parrt]

Oh! Damn. Sorry! Will fix asap. i edited wrong branch then cherry-picked.

[parrt]

Fixed @KvanTTT !!