Closed Bug 1519097 (hashbang) Opened 5 years ago Closed 5 years ago

Implement the Hashbang Grammar proposal


(Core :: JavaScript Engine, enhancement, P2)

63 Branch



Tracking Status
firefox67 --- fixed


(Reporter: alex.fdm, Assigned: Waldo)


(Blocks 1 open bug, )


(Keywords: dev-doc-complete)


(1 file, 1 obsolete file)

The proposal is already in Stage 3.

Attached patch Patch, lacking tests (obsolete) — Splinter Review
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 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.
Assignee: nobody → jwalden
Priority: -- → P2
Alias: hashbang 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.

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. :-|

Blocks: 1531202
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.
Attachment #9047249 - Flags: review?(arai.unmht)
Attachment #9036062 - Attachment is obsolete: true
Attachment #9047249 - Flags: review?(arai.unmht) → review+
Pushed by
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

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



[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/ /builds/worker/workspace/build/src/obj-spider/_virtualenvs/init/bin/python -u /builds/worker/workspace/build/src/js/src/tests/
[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/' --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}

Flags: needinfo?(jwalden)
Pushed by
Relax the meaning of 'poisoned' SourceUnits to allow SourceUnits initialized with null units and zero length to be considered not-poisoned.  r=bustage
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: needinfo?(jwalden)

I added discussion of hashbang syntax to 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. :-)

(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:

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.

OK, I think we're done with the docs for this feature. I gave :Waldo's work a copy edit:

I also added a note about it to the main MDN JS guide:

And finally, I submitted a PR to add a data point to our browser compat data:

Please let me know if this looks OK; thanks!

You need to log in before you can comment on or make changes to this bug.