1519097 - (hashbang) Implement the Hashbang Grammar proposal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Alexandre Folle de Menezes]

The proposal is already in Stage 3.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch, lacking tests

This should do the trick as far as the current spec proposal goes.  However, it doesn't have any tests, and test262 hasn't had tests landed yet that we could just import, so I'm not sure what to do now.  I eyeballed all the tests in https://github.com/tc39/test262/pull/1983 and am pretty sure this passes all of them.  It doesn't seem a good use of time to write our own tests, that would basically just duplicate what test262 will eventually land, so I guess maybe this is in limbo until test262 gets things going.

Comment 2

[Jeff Walden [:Waldo]]

https://github.com/tc39/test262/pull/2065 got merged earlier today, so after a refresh of our test262 checkout and a test of this patch against its tests (making sure to flip on the test262 hashbang feature when importing), this should be ready to go.

I do not immediately have time to push on this, but if people got the test262 steps done for me I could probably get this landed sooner. But it's not the highest-priority thing for me to do just because, right now.

Comment 3

[Jeff Walden [:Waldo]]

Bug 1530879 updates our test262 import considerably -- right up to the point where hashbang tests start to be imported. Unfortunately hashbang tests are...complicated to import, because the traditional "// ..." comment format used to annotate tests is not something we can apply to raw tests that necessarily are supposed to be run unaltered.

I don't see an immediate obvious solution to this problem, and given how much pain it was to update through three months of changes to just before the hashbang tests, I am not eager to try to think through how to update through the last couple weeks to pick up these tests. :-|

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Implement the Hashbang Grammar proposal in JavaScript to allow a '#!' line at the very start of a script/module to be treated as a single-line comment

Okay, with bug 1531199 in place, we can implement this.

Sort of.

All the test262 tests that check for #! and false versions of it in literal source text (that are not negative tests) have to be skipped because of bug 1531202.  Given how simple the "match hashbang" code is, I'm not worried about missing testing of the goofy-escaped versions.  And while I'm not *happy* about missing testing of hashbang-not-quite-at-start (after whitespace, after multiline comment, etc.), I think it would be safe to ship this without those tests.  So, how about it?

I could probably add non262 versions of some of those skipped hashbang tests, if it's really desired, tho.  Just, for now I'm going to try the less-effort approach.

It is perhaps debatable whether BOM skipping should happen in the shell any more -- and whether BOM skipping should be allowed *along* with this feature whose position necessarily conflicts with it.  I've left the skipping in place just because it changes less shell behavior, but maybe we could remove it in a followup.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb343e7c6cc
Implement the Hashbang Grammar proposal in JavaScript to allow a '#!' line at the very start of a script/module to be treated as beginning a single-line comment.  r=arai

Comment 6

[Stefan Hindli [:stefan_hindli]]

Backed out 2 changesets (Bug 1531199, Bug 1519097) for spidermonkey build bustages in /builds/worker/workspace/build/src/js/src/frontend/TokenStream.h:1176 CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=231146881

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231146881&repo=mozilla-inbound&lineNumber=21091

[task 2019-03-01T02:16:09.132Z] Considering argument_and_var.js
[task 2019-03-01T02:16:09.136Z] Considering eval_fake_in_function.js
[task 2019-03-01T02:16:09.140Z] Considering var_in_fun.js
[task 2019-03-01T02:16:09.144Z] Considering eval_fake_2.binjs
[task 2019-03-01T02:16:09.144Z] Testing jsapi-tests/binast/parser/multipart/unit/eval_fake_2.js
[task 2019-03-01T02:16:09.144Z] Got the same AST when parsing jsapi-tests/binast/parser/multipart/unit/eval_fake_2.js
[task 2019-03-01T02:16:09.148Z] Considering toplevel_var.binjs
[task 2019-03-01T02:16:09.148Z] Testing jsapi-tests/binast/parser/multipart/unit/toplevel_var.js
[task 2019-03-01T02:16:09.149Z] Got the same AST when parsing jsapi-tests/binast/parser/multipart/unit/toplevel_var.js
[task 2019-03-01T02:16:09.149Z] runTestFromPath: entering directory 'jsapi-tests/binast/parser/multipart/spidermonkey/'
[task 2019-03-01T02:16:09.149Z] runTestFromPath: entering directory 'jsapi-tests/binast/parser/multipart/spidermonkey/ecma_2/'
[task 2019-03-01T02:16:09.149Z] runTestFromPath: entering directory 'jsapi-tests/binast/parser/multipart/spidermonkey/ecma_2/instanceof/'
[task 2019-03-01T02:16:09.153Z] Considering instanceof-001.binjs
[task 2019-03-01T02:16:09.153Z] Testing jsapi-tests/binast/parser/multipart/spidermonkey/ecma_2/instanceof/instanceof-001.js
[task 2019-03-01T02:16:09.153Z] Got the same AST when parsing jsapi-tests/binast/parser/multipart/spidermonkey/ecma_2/instanceof/instanceof-001.js
[task 2019-03-01T02:16:09.157Z] Considering regress-7635.js
[task 2019-03-01T02:16:09.161Z] Considering instanceof-001.js
[task 2019-03-01T02:16:09.165Z] Considering instanceof-002.js
[task 2019-03-01T02:16:09.169Z] Considering shell.binjs
[task 2019-03-01T02:16:09.169Z] Testing jsapi-tests/binast/parser/multipart/spidermonkey/ecma_2/instanceof/shell.js
[task 2019-03-01T02:16:09.169Z] Assertion failure: ptr (shouldn't be using if poisoned), at /builds/worker/workspace/build/src/js/src/frontend/TokenStream.h:1176
[task 2019-03-01T02:16:09.185Z] in directory /builds/worker/workspace/build/src/obj-spider, running ['setarch', 'x86_64', '-R', 'make', 'check-jstests']
[task 2019-03-01T02:16:09.193Z] make -C js/src check-jstests
[task 2019-03-01T02:16:09.252Z] make[1]: Entering directory '/builds/worker/workspace/build/src/obj-spider/js/src'
[task 2019-03-01T02:16:09.252Z] ../../dist/bin/run-mozilla.sh /builds/worker/workspace/build/src/obj-spider/_virtualenvs/init/bin/python -u /builds/worker/workspace/build/src/js/src/tests/jstests.py
[task 2019-03-01T02:16:09.252Z] --no-progress --format=automation --timeout 300
[task 2019-03-01T02:16:09.252Z] --args='--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so' --exclude-file=/builds/worker/workspace/build/src/js/src/devtools/automation/arm64-jstests-slow.txt
[task 2019-03-01T02:16:09.252Z] ../../dist/bin/js
[task 2019-03-01T02:16:19.295Z] {"action": "suite_start", "pid": 12704, "source": "jstests", "tests": [], "thread": "main", "time": 1551406579.294903}

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f57b355891
Relax the meaning of 'poisoned' SourceUnits to allow SourceUnits initialized with null units and zero length to be considered not-poisoned.  r=bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea769ff9a9a
Implement the Hashbang Grammar proposal in JavaScript to allow a '#!' line at the very start of a script/module to be treated as beginning a single-line comment.  r=arai

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/b7f57b355891
https://hg.mozilla.org/mozilla-central/rev/3ea769ff9a9a

Comment 9

[Jeff Walden [:Waldo]]

I added discussion of hashbang syntax to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar as one place that definitely should mention this, using somewhat tentatively-suggesting wording. (Don't want people going too willy-nilly with it in advance of its being widely implemented, and also in advance of browser-compat tables being updated for it.)

I'm certain there are other places that should be updated too, but I wouldn't know where they all are, so docs authors, help a brotha' out. :-)

Comment 10

[Chris Mills [:cmills]]

(In reply to Jeff Walden [:Waldo] from comment #9)

I'm certain there are other places that should be updated too, but I wouldn't know where they all are, so docs authors, help a brotha' out. :-)

OK, we're getting to this finally ;-)

I've made sure it is mentioned in the Fx67 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/67#JavaScript

MDN writers, looks like we need to added this to BCD for lexical grammar, copy edit :Waldo's work (much appreciated, thanks!) and make sure it is mentioned in other places as appropriate.

Comment 11

[Chris Mills [:cmills]]

OK, I think we're done with the docs for this feature. I gave :Waldo's work a copy edit:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Hashbang_comments

I also added a note about it to the main MDN JS guide:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Grammar_and_types#Comments

And finally, I submitted a PR to add a data point to our browser compat data:
https://github.com/mdn/browser-compat-data/pull/4094

Please let me know if this looks OK; thanks!