Disparity between encoder and FF decoder

[RReverser]

Currently trying to encode JavaScript that uses ES6 const syntax succeeds, but then trying to load the result in Firefox fails with:

SyntaxError: BinAST Parsing Error: Const is not supported in this preview release

While the encoder is in theory generic and shouldn't be tied to what's implemented on the Firefox side, this can currently lead to pretty bad cases where we serve a transformed JavaScript that consequently fails in Firefox with no reasonable fallback.

I'm not sure how much work is there to finalise the Firefox decoder side, but should we consider restricting the encoder to transform only features supported on both sides and throw an error otherwise?

Also, are there other known cases that are supported in the encoder but not decoder?

cc @xtuc @Yoric @arai-a

[Yoric]

Actually, there are quite a few things that are not implemented Firefox side yet. Search for instances of raiseError in the auto-generated parser code: https://searchfox.org/mozilla-central/source/js/src/frontend/BinASTParser.cpp . Most of these should be easy to fix, it's just a matter of priority – there are currently two devs working on BinAST on SpiderMonkey (@arai-a and myself) and we're currently both prioritizing stuff that we need to demonstrate BinAST performance. We were hoping that @efaust would be the third person, but his employer has put him on other stuff.

So, unfortunately, no ETA just yet.

While I don't have time to write that code myself just yet (maybe in August/September?), I'd be happy to review code that adds an option to restrict to what is currently implemented in Firefox.

[RReverser]

Search for instances of raiseError in the auto-generated parser code: searchfox.org/mozilla-central/source/js/src/frontend/BinASTParser.cpp .

@Yoric Quickly looking at these instances, is it safe to say that ES5 is fully supported and it's just some ES6+ features that are not implemented?

[arai-a]

almost all ES5 feature are supported, except Function.prototype.toString() and block-scoped functions.

[RReverser]

@arai-a Hmm, will block-scoped function also throw a syntax error or just silently work differently than in the original JavaScript?

[arai-a]

It throws syntax error, due to scope mismatch.
(it's not explicitly blocked, but it doesn't work)

[RReverser]

Sounds good (well, at least it's not a silent breakage). I'll look into filtering out AST nodes in the encoder for now then.

[RReverser]

Looks like at least lazy getters and setters (which are part of ES5) are also unsupported. Looking carefully for more, but it's hard to find all the cases from just C++ code... Having a separate list for tracking could be nice.

[RReverser]

@arai-a is there any reason not to fallback to parsing lazy methods/getters/setters as eager in Firefox until a proper implementation is added?

[arai-a]

mostly no reason I think, just that they got low priority,
and we started experiment about laziness with function declaration/expression and others are just kept unsupported.

[RReverser]

and others are just kept unsupported

Yes, but currently encoder lazifies all of them, which, like with other mentioned cases, can lead to producing BinaryAST that won't be actually parseable.

[RReverser]

This issue is just particularly high-priority for CDN-wide testing, because, if we enable encoder by default on CDNjs or unpkg.com, then it can currently break lots of assets with no way of us knowing about that or being able to fix it. I'm currently trying to filter out nodes by looking at Firefox C++ code, but it still feels pretty risky...

[arai-a]

for multipart, we can treat lazy function as eager function, given the function body is stored in-place.
so, we can read lazy function's content by not-skipping to the next offset.

but that doesn't apply for context format, that stores lazy functions in the latter part of the file, separate from the enclosing script.
(... and now I realized that, the underlying token streams are different between multipart and context, so the parser code cannot be shared around that...)

so, if treating lazy getter as eager getter in multiplart format is high-priority, I think supporting that now is reasonable.
(we'll need to write different code for context format anyway)

do you have any deadline?

[RReverser]

so, if treating lazy getter as eager getter in multiplart format is high-priority

No, it's not particularly that part that is high-priority but the general "we have things produced by the encoder that are not recognised by decoder" issue. I guess it won't go away until BinASTParser supports all the nodes...