Use enums more freely

[gbrail]

We have a lot of places, especially in parser-related things, where we have a pattern of integer identifiers that have to remain unique, like this:

rhino/rhino/src/main/java/org/mozilla/javascript/Token.java

Line 32 in b08de52

public static final int

Now that people are adding lots of new language features (thanks!) this is becoming a maintenance nightmare, because all it takes is two or three people modifying the same list of integers to create a merge conflict.

Fortunately, since the 1990s Java has added proper enums.

It would be wonderful if someone could take on the task of replacing these enum-like structures in Rhino with proper Java enums.

[p-bakker]

This would constitute a breaking change, for people that interact with the parser directly, no?

Do note that roughly, these are the current language features that Rhino doesn't yet support that involve new syntax:

• rest & spread Support ES2015 Spread syntax #1217, Support ES2018 Object Rest & Spread #968, Support ES2015 default values inside destructuring assignments #756, Support ES2015 Function Rest Parameters #652 (some rest support introduced in initial implementation of function rest parameter support #1451)
• modules Support ES2015 modules #945
• class ES2015 Classes and inheritance #835
• async & await Support ES2017 async (arrow) functions & “await” #395
• optional chaining operator Support ES2020 Optional Chaining Operator #937 (PR in progress)
• nullish assignment operator Support ES2021 Nullish Assignment Operator #936
• super (in Objects) Support ES2015 Method Definitions - prevent methods as constructor #1299

And stuff in the TC39 proposals pipeline (stage 2.7 & 3):

• decorators

All this to say: yes we had quite a few language features added recently with super being added for some additional operators, but likely going forward well have less conflicting PRs. Not saying we shouldn't do this, but given this resulting in a breaking change, I think we should consider not doing this before a V2.x.x

[andreabergia]

IMHO the benefits would overweight the cost. If someone has opened PRs yes, it might take a bit of refactoring, but the maintenance of some of these existing patterns (for example, the Token, which I've had to touch for some recent PR) is a lot worse.

[rbri]

fine for me, please do it

[gbrail]

I agree that the benefits outweigh the costs -- I THINK that this is technically incompatibility, but more of the sort that requires people to rebuild their code rather than just replace a JAR file, versus an incompatibility that requires that people actually change their source code. I guess trying it out would help answer that question!

…

On Mon, Sep 23, 2024 at 10:36 AM RBRi ***@***.***> wrote: fine for me, please do it — Reply to this email directly, view it on GitHub <#1639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I23FPOSKVQZ5XC4CNNTZYBGQ3AVCNFSM6AAAAABOTZL6SCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRYHEZTIOBXGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[rPraml]

I've tried to do this, but I see serveral show stoppers:

• There is Token and there is ICode - it would be necessary to keep all codes either in one enum or do a huge refactoring
• There is currentFlaggedToken which has additional bits and would require refactoring
• There is https://github.com/mozilla/rhino/blob/master/rhino/src/main/java/org/mozilla/javascript/ast/Scope.java#L118C15-L118C22 what I do not understand: Scope.type is converted in Scope.position (I think this could be a bug)

Here is the start of the PR #1647 if really someone want to continue

[rbri]

#1648 is merged, i think we can live with the current solution for a while

[p-bakker]

Closed by #1648