Bug 1519097 (hashbang)

Implement the Hashbang Grammar proposal

RESOLVED FIXED in Firefox 67



3 months ago
2 months ago


(Reporter: alex.fdm, Assigned: Waldo)


(Blocks 1 bug, {dev-doc-needed})

63 Branch
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)




(1 attachment, 1 obsolete attachment)



3 months ago

The proposal is already in Stage 3.


Comment 1

3 months ago
Posted 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 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.


3 months ago
Assignee: nobody → jwalden
Priority: -- → P2


2 months ago
Alias: hashbang

Comment 2

2 months ago

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

2 months ago

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



2 months ago
Blocks: 1531202

Comment 4

2 months ago
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)


2 months ago
Attachment #9036062 - Attachment is obsolete: true
Attachment #9047249 - Flags: review?(arai.unmht) → review+

Comment 5

2 months ago
Pushed by jwalden@mit.edu:
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

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}

Flags: needinfo?(jwalden)

Comment 7

2 months ago
Pushed by jwalden@mit.edu:
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

Comment 8

2 months ago
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67


2 months ago
Flags: needinfo?(jwalden)

Comment 9

2 months ago

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

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