Javascript: migrate prototypical Tree objects to es6 classes

[carocad]

This PR is a follow up on #2746.

The (original) scope was to refactor the Tree object declarations to:

• use es6 classes/inheritance instead of direct prototype manipulation. Es6 class declarations are really just syntactic sugar over prototype inheritance so this change should have no effect on the runtime itself.
• use const/let on variable declarations to improve variable scoping
• refactor comments to be JsDoc compatible which most IDEs can leverage to provide better developer feedback

However, during the refactoring a couple of problems arised which increased the scope to also include:

• change the js test suite string templates to avoid using antlr4.tree.ParseTreeListener.call(this). This is a consecuence of this PR refactor. Classes unlike functions cannot be "called" only instantiated. A possible substitution for this would be new antlr..... However a closer inspection showed that this is an interface with no inner members therefore it makes no sense to instantiate an objec and throw it away inmediately.
• expose some "classes" as module.export = MyClass. This is a follow up on this comment from @ericvergnaud to make the JS target closer to the Java one. As I was already touching these files it made sense to do the change inmediately.

Note to @ericvergnaud :

• During this refactoring I found out that the antlr4 implementation has a bug here. There is a cyclic dependency which is not being resolved correctly. This causes the const ATN = require .... to return an empty object which means that ATN.INVALID_ALT_NUMBER returns undefined as opposed to 0. Since fixing that bug would require quite some refactoring I left it there. This bug has been there even before I started the refactoring. See Fix errors in JavaScript run-time. #2496 (comment)

[ericvergnaud]

INVALID_ALT_NUMBER is a member of ATN, so I think this should be: ...).ATN.INVALID_ALT_NUMBER;

[carocad]

this is fine as it is because I changed the export of ATN.js to be module.exports = ATN; which means that the object itself is exported. This is precisely the equivalent of export default.

[ericvergnaud]

so it's fine now? I thought you said it was undefined?

[carocad]

sorry for the misunderstanding. The situation is as follows:

• the ATN module is exported directly so its members are directly available as shown above.
• the cyclic dependency prevents the ATN module from being correctly imported, therefore node just returns an empty object {} which results in INVALID_ALT_NUMBER being undefined. However, that has nothing to do with the way the object is exposed but rather with the cyclic dependency itself.

[carocad]

hey @ericvergnaud I think it is more appropiate to continue the discussion over here from: #2749 (comment)

So I have good news and bad news:

good: I think I have a fix for the failing tests. I basically did a rollback of the require.js polyfill but on a mock directory together with the fs module. This allows a browser to "require" the complete antlr library and the node.js ones as well. I testes it manually and it "worked". I havent pushed the changes upstream yet due to the problem below.

bad: I tried to run the tests on different browsers but in all cases they fail with empty output 😞 . Eventually I started doubting the tests themselves and rolled back to commit 2d7f727 and tried the tests; and they also fail 😨 .

Therefore right now I am stuck. There are no documentation on the browser test suite (so I dont know if I need a special setup) and even the tests before the refactoring started are failing for me. Any hints/suggestion are welcomed.

[ericvergnaud]

@carocad I will try and look into this by end of next week

[ericvergnaud]

Ok, I've been looking into this, and from what I can see:

• the browser testing framework stopped working a loooong time ago (probably end 2016)
• nobody has complained since
• nobody's using the runtime js files 'as is' anymore i.e. they're always packaged
• the new package does the es2015 conversion (correct?)
• the runtime does not rely on any browser runtime behavior
  From there, I think it's ok to simply drop these tests suites.
  I will do that in a separate PR.

[carocad]

Ok, I've been looking into this

Thanks for looking into this :)

the new package does the es2015 conversion (correct?)

I just double checked this and for full disclosure here is the webpack processed antlr4 runtime. It seems that it is still using classes and other "newer" stuff. Therefore I think I would need to modify the webpack configuration (separate PR?).

Btw, how do we proceed with this PR then ? Is it ok as it is ?

[ericvergnaud]

do we still need a fix for the INVALID_ALT_NUMBER?

[carocad]

do we still need a fix for the INVALID_ALT_NUMBER?

definitely, but as a mention before that would require quite some refactoring on the way that several modules are imported and exported. Therefore I wanted to tackle that separatedly as it is not a problem that arised due to this PR but rather it has been there from the very beginning.

[ericvergnaud]

how about the following:
const INVALID_ALT_NUMBER = require(...). INVALID_ALT_NUMBER || 0;
that would ensure the correct value for now and I can look into it later...

[carocad]

For transparency here is the cyclic dependency cycle. I got it using madge

$ madge --circular src/antlr4/index.js 
Processed 47 files (1.1s) 

✖ Found 3 circular dependencies!

1) LL1Analyzer.js > PredictionContext.js > RuleContext.js > atn/ATN.js
2) RuleContext.js > tree/Trees.js > ParserRuleContext.js
3) RuleContext.js > tree/Trees.js

and the complete dependency graph starting at antlr4/index.js. The red parts are the cyclic dependencies.

[ericvergnaud]

For transparency here is the cyclic dependency cycle. I got it using madge

$ madge --circular src/antlr4/index.js 
Processed 47 files (1.1s) 

✖ Found 3 circular dependencies!

1) LL1Analyzer.js > PredictionContext.js > RuleContext.js > atn/ATN.js
2) RuleContext.js > tree/Trees.js > ParserRuleContext.js
3) RuleContext.js > tree/Trees.js

and the complete dependency graph starting at antlr4/index.js. The red parts are the cyclic dependencies.

My usual workaround in ES2015 is as follows:

in imported script

var imported = null;

exports.resolve = function() {
    imported = require(...);
} 

in importing script

var stuffScript = require("some-script");
var stuff = stuffScript.stuff;
stuffScript.resolve() ;

not sure how this can be achieved in ES6 though...

[ericvergnaud]

@parrt blessed