Hashbang lexer support #1631

nekevss · 2021-10-04T21:17:13Z

This Pull Request fixes/closes #1555.

Please see the below. There's definitely a couple ways and might needs some revisions/further functionality, but I thought it might be best to get feedback. I initially went to implement the Hashbang support in new. But it made less sense as I was dealing with Result returns, and it felt like the better approach was to implement Hashbang similar to single line and multiline comments.

It changes the following:

raskad

Looks good.

I went and had a look at the remaining failing 262 tests for this.
Some of them are failing because of other missing features. But I think I found three that should work. Maybe you could take a look at those and see why they are still failing? It may be another reason, just to be sure we are not missing anything.
https://github.com/tc39/test262/blob/10ad4c159328d12ad8d12b95a8f8aefa0e4fd49b/test/language/comments/hashbang/line-terminator-line-separator.js
https://github.com/tc39/test262/blob/10ad4c159328d12ad8d12b95a8f8aefa0e4fd49b/test/language/comments/hashbang/line-terminator-paragraph-separator.js
https://github.com/tc39/test262/blob/10ad4c159328d12ad8d12b95a8f8aefa0e4fd49b/test/language/comments/hashbang/use-strict.js

boa/src/syntax/lexer/comment.rs

jedel1043 · 2021-10-05T00:31:28Z

I think the failing tests are because the lexer is considering only '\n' and '\r' as EOL, but the spec considers a lot more characters as EOL (https://tc39.es/ecma262/#prod-LineTerminator). We should probably implement a new LineTerminator tokenizer to consume that.

nekevss · 2021-10-05T02:04:07Z

A LineTerminator tokenizer seemed to be what I was seeing in ECMAScript when I was looking through. Just wasn't sure how big of a change that was though. I can try to push forward adding something like that if you want. In the meantime, I readjusted the existing Hashbang.lex to catch the Paragraph Separator (u2029) and Line Separator (u2028) that it was missing before.

jedel1043 · 2021-10-05T02:11:06Z

A LineTerminator tokenizer seemed to be what I was seeing in ECMAScript when I was looking through. Just wasn't sure how big of a change that was though. I can try to push forward adding something like that if you want. In the meantime, I readjusted the existing Hashbang.lex to catch the Paragraph Separator (u2029) and Line Separator (u2028) that it was missing before.

Great! This PR is okay as it is, we can change it when we have a LineTerminator lexer, but if you're interested you could implement that in another PR, it should be a relatively small change :)

Razican · 2021-10-05T12:18:00Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	80,930	80,930	0
Passed	36,864	36,866	+2
Ignored	12,690	12,690	0
Failed	31,376	31,374	-2
Panics	0	0	0
Conformance	45.55%	45.55%	+0.00%

Fixed tests (2):

test/language/comments/hashbang/not-empty.js [strict mode] (previously Failed)
test/language/comments/hashbang/not-empty.js (previously Failed)

Razican · 2021-10-05T12:19:21Z

Test262 conformance changes:

Test result master count PR count difference
Total 80,930 80,930 0
Passed 36,864 36,866 +2
Ignored 12,690 12,690 0
Failed 31,376 31,374 -2
Panics 0 0 0
Conformance 45.55% 45.55% +0.00%
Fixed tests (2):
test/language/comments/hashbang/not-empty.js [strict mode] (previously Failed)
test/language/comments/hashbang/not-empty.js (previously Failed)

We should try to fix all the possible tests in the hashbang suite, I think

nekevss · 2021-10-05T17:37:06Z

Sorry if I'm missing something (still new to the test262 running and not sure where to see the results on the pull request). What tests are we still missing in the hashbang suite? If I'm reading this right, not-empty.js was fixed but not the other tests, correct?

jedel1043 · 2021-10-05T18:05:31Z

Sorry if I'm missing something (still new to the test262 running and not sure where to see the results on the pull request). What tests are we still missing in the hashbang suite? If I'm reading this right, not-empty.js was fixed but not the other tests, correct?

You can check https://github.com/boa-dev/boa/blob/master/CONTRIBUTING.md#testing to see how to run specific tests. In you case, the suit you would want to run is test/language/comments/hashbang.

jedel1043 · 2021-10-05T23:42:43Z

Test262 conformance changes:

Test result	main count	PR count	difference
Total	86,438	86,438	0
Passed	37,156	37,164	+8
Ignored	19,022	19,022	0
Failed	30,260	30,252	-8
Panics	0	0	0
Conformance	42.99%	42.99%	+0.01%

Fixed tests (8):

test/language/comments/hashbang/line-terminator-paragraph-separator.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-paragraph-separator.js (previously Failed)
test/language/comments/hashbang/not-empty.js [strict mode] (previously Failed)
test/language/comments/hashbang/not-empty.js (previously Failed)
test/language/comments/hashbang/line-terminator-line-separator.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-line-separator.js (previously Failed)
test/language/comments/hashbang/line-terminator-carriage-return.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-carriage-return.js (previously Failed)

nekevss · 2021-10-05T23:47:08Z

Looks like the consuming of the LineTerminator character was the issue. This was passing 46 tests and failed 10 for conformance of 79, when I was running the tests. The rest of the failed tests seem to be linked to hashbangs in function evaluator contexts and eval calls, which I may need some direction on where to look for that if I should be trying to close those out. Is that still in the lexer?

raskad · 2021-10-05T23:51:09Z

Looks like the consuming of the LineTerminator character was the issue. This was passing 46 tests and failed 10 for conformance of 79, when I was running the tests. The rest of the failed tests seem to be linked to hashbangs in function evaluator contexts and eval calls, which I may need some direction on where to look for that if I should be trying to close those out. Is that still in the lexer?

We currently do not have eval implemented, so you can ignore any test with an eval().

nekevss · 2021-10-06T03:37:22Z

I think that leaves most of the tests. Of the last tests left, the only one that should work that seems to not be is use-strict. I don't know what's causing this one to not pass though since anything that is inside the hashbang is considered a comment, so it shouldn't generate the DirectivePrologues as stated. Does anybody happen to have suggestions on where I should look.

raskad · 2021-10-06T17:17:22Z

I think a good method to check whats going on would be to write a test for the parser here: https://github.com/boa-dev/boa/blob/main/boa/src/syntax/parser/tests.rs

We have some methods for checking if the expected AST nodes are parsed. For example this test passes:

#[test]
fn empty_comment() {
    check_parser(
        r"
            // Some Comment;
        ",
        vec![],
    );
}

You could write a test that mirrors the 262 test and see if the AST is expected (no AST because it should be a comment).

nekevss · 2021-10-06T23:14:46Z

So I put together and ran some tests. This hashbang implementation does successfully lex the "use strict" portion of the hashbang test as far as I can tell, but it fails to parse the with statement. It looks like Keyword::With is hitting the last arm of PrimaryExpression, and throwing a ParsingError.

raskad · 2021-10-06T23:19:08Z

Ah that's right we don't have with implemented, but the test expects it to work.

With that I would say this feature is complete as all others tests depend on unimplemented features.

boa/src/syntax/lexer/comment.rs

boa/src/syntax/parser/tests.rs

jedel1043

Looks good to me :)

jedel1043 · 2021-10-07T02:12:06Z

You can run cargo fmt to fix the failing check

nekevss · 2021-10-07T02:25:57Z

You can run cargo fmt to fix the failing check

Thanks! I've been trying to figure it out for a minute. There's so many features of Rust I've been using on these PRs that I hadn't on my personal projects.

nekevss · 2021-10-07T03:13:36Z

@jedel1043. Question for you. I just realized something that could pass more of the comment tests in the MultiLineComment lexer. I tried out the change and it increases the conformance from 75% to 80% in the comments test. Should I add that change to this Pull Request or would it be better to make another Pull Request?

jedel1043 · 2021-10-07T03:24:59Z

If the change is relatively small, you can add it here :)

nekevss · 2021-10-07T03:33:20Z

Changes went from:
Suite hashbang results: total: 58, passed: 46, ignored: 2, failed: 10 (panics: 0), conformance: 79.31%
Suite comments results: total: 104, passed: 78, ignored: 2, failed: 24 (panics: 0), conformance: 75.00%

To:
Suite hashbang results: total: 58, passed: 46, ignored: 2, failed: 10 (panics: 0), conformance: 79.31%
Suite comments results: total: 104, passed: 84, ignored: 2, failed: 18 (panics: 0), conformance: 80.77%

jedel1043 · 2021-10-07T03:45:20Z

Test262 conformance changes:

Test result	main count	PR count	difference
Total	86,438	86,438	0
Passed	37,442	37,452	+10
Ignored	19,022	19,022	0
Failed	29,974	29,964	-10
Panics	0	0	0
Conformance	43.32%	43.33%	+0.01%

Fixed tests (14):

test/language/comments/multi-line-asi-paragraph-separator.js [strict mode] (previously Failed)
test/language/comments/multi-line-asi-paragraph-separator.js (previously Failed)
test/language/comments/multi-line-asi-carriage-return.js [strict mode] (previously Failed)
test/language/comments/multi-line-asi-carriage-return.js (previously Failed)
test/language/comments/multi-line-asi-line-separator.js [strict mode] (previously Failed)
test/language/comments/multi-line-asi-line-separator.js (previously Failed)
test/language/comments/hashbang/line-terminator-paragraph-separator.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-paragraph-separator.js (previously Failed)
test/language/comments/hashbang/not-empty.js [strict mode] (previously Failed)
test/language/comments/hashbang/not-empty.js (previously Failed)
test/language/comments/hashbang/line-terminator-line-separator.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-line-separator.js (previously Failed)
test/language/comments/hashbang/line-terminator-carriage-return.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-carriage-return.js (previously Failed)

Broken tests (4):

test/language/literals/regexp/S7.8.5_A1.2_T1.js [strict mode] (previously Passed)
test/language/literals/regexp/S7.8.5_A1.2_T1.js (previously Passed)
test/language/comments/S7.4_A2_T2.js [strict mode] (previously Passed)
test/language/comments/S7.4_A2_T2.js (previously Passed)

jedel1043 · 2021-10-07T03:46:25Z

boa/src/syntax/lexer/comment.rs

-                return Err(Error::syntax(
-                    "unterminated multiline comment",
-                    cursor.pos(),
-                ));


Some tests now fail because this error is missing on the new commit.

Ah shoot, I'll work that back in.

boa/src/syntax/lexer/comment.rs

jedel1043 · 2021-10-07T04:29:01Z

Test262 conformance changes:

Test result	main count	PR count	difference
Total	86,438	86,438	0
Passed	37,442	37,456	+14
Ignored	19,022	19,022	0
Failed	29,974	29,960	-14
Panics	0	0	0
Conformance	43.32%	43.33%	+0.02%

Fixed tests (14):

test/language/comments/multi-line-asi-paragraph-separator.js [strict mode] (previously Failed)
test/language/comments/multi-line-asi-paragraph-separator.js (previously Failed)
test/language/comments/multi-line-asi-carriage-return.js [strict mode] (previously Failed)
test/language/comments/multi-line-asi-carriage-return.js (previously Failed)
test/language/comments/multi-line-asi-line-separator.js [strict mode] (previously Failed)
test/language/comments/multi-line-asi-line-separator.js (previously Failed)
test/language/comments/hashbang/line-terminator-paragraph-separator.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-paragraph-separator.js (previously Failed)
test/language/comments/hashbang/not-empty.js [strict mode] (previously Failed)
test/language/comments/hashbang/not-empty.js (previously Failed)
test/language/comments/hashbang/line-terminator-line-separator.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-line-separator.js (previously Failed)
test/language/comments/hashbang/line-terminator-carriage-return.js [strict mode] (previously Failed)
test/language/comments/hashbang/line-terminator-carriage-return.js (previously Failed)

jedel1043 · 2021-10-07T04:29:28Z

No regressing tests :)

nekevss added 6 commits October 3, 2021 22:43

initial hashbang sup logic in Lexer new()

7408fe7

implemented basic concept for hashbang lexing

edd14a0

minor changes - missed semi-colon

e6c0a3e

cleaning calls in Hashbang.lex()

2eb3e74

move hashbang lex from new() to next()

65278fa

add comment to to hashbang block

318e9d0

raskad requested changes Oct 4, 2021

View reviewed changes

boa/src/syntax/lexer/comment.rs Outdated Show resolved Hide resolved

Hashbang to HashbangComment - more comments added

820b065

changed Hashbang lex - added support for u2028 and u2029

6eb79c3

removing debug print statement

0eef980

nekevss added 2 commits October 5, 2021 19:08

Removing byte consumption from hashbang.lex break statement

a4491b3

removing comment linked to byte consumption

acbf3c9

Added some hashbang tests to parser/tests.rs

5688a7c

raskad reviewed Oct 6, 2021

View reviewed changes

boa/src/syntax/lexer/comment.rs Outdated Show resolved Hide resolved

nekevss added 2 commits October 6, 2021 19:58

removed line separator hashbang test

44186df

switched loop if let to while let

2dfa9d4

comment out failing use strict with test for now

48c5eac

jedel1043 reviewed Oct 7, 2021

View reviewed changes

boa/src/syntax/parser/tests.rs Outdated Show resolved Hide resolved

nekevss added 2 commits October 6, 2021 21:53

changed comment out to ignore

57fd8cd

adding new line to end of file

257db03

jedel1043 approved these changes Oct 7, 2021

View reviewed changes

running cargo fmt

62d1e74

MultiLineComment/Hashbang match update

756fc46

cargo fmt update

f765c43

jedel1043 reviewed Oct 7, 2021

View reviewed changes

boa/src/syntax/lexer/comment.rs Outdated Show resolved Hide resolved

Rework to include unterminated error

0b3ec3c

raskad approved these changes Oct 7, 2021

View reviewed changes

raskad merged commit 69d9f62 into boa-dev:main Oct 7, 2021

raskad added this to the v0.14.0 milestone Oct 7, 2021

nekevss deleted the hashbang-lex-support branch October 7, 2021 19:50

RageKnify added the enhancement New feature or request label Jan 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hashbang lexer support #1631

Hashbang lexer support #1631

nekevss commented Oct 4, 2021 •

edited by jedel1043

raskad left a comment

jedel1043 commented Oct 5, 2021 •

edited

nekevss commented Oct 5, 2021

jedel1043 commented Oct 5, 2021

Razican commented Oct 5, 2021

Razican commented Oct 5, 2021

Test262 conformance changes:

nekevss commented Oct 5, 2021

jedel1043 commented Oct 5, 2021

jedel1043 commented Oct 5, 2021

nekevss commented Oct 5, 2021

raskad commented Oct 5, 2021

nekevss commented Oct 6, 2021

raskad commented Oct 6, 2021

nekevss commented Oct 6, 2021

raskad commented Oct 6, 2021

jedel1043 left a comment

jedel1043 commented Oct 7, 2021

nekevss commented Oct 7, 2021

nekevss commented Oct 7, 2021

jedel1043 commented Oct 7, 2021

nekevss commented Oct 7, 2021

jedel1043 commented Oct 7, 2021

jedel1043 Oct 7, 2021

nekevss Oct 7, 2021

jedel1043 commented Oct 7, 2021

jedel1043 commented Oct 7, 2021

Hashbang lexer support #1631

Hashbang lexer support #1631

Conversation

nekevss commented Oct 4, 2021 • edited by jedel1043

raskad left a comment

Choose a reason for hiding this comment

jedel1043 commented Oct 5, 2021 • edited

nekevss commented Oct 5, 2021

jedel1043 commented Oct 5, 2021

Razican commented Oct 5, 2021

Test262 conformance changes:

Razican commented Oct 5, 2021

Test262 conformance changes:

nekevss commented Oct 5, 2021

jedel1043 commented Oct 5, 2021

jedel1043 commented Oct 5, 2021

Test262 conformance changes:

nekevss commented Oct 5, 2021

raskad commented Oct 5, 2021

nekevss commented Oct 6, 2021

raskad commented Oct 6, 2021

nekevss commented Oct 6, 2021

raskad commented Oct 6, 2021

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 commented Oct 7, 2021

nekevss commented Oct 7, 2021

nekevss commented Oct 7, 2021

jedel1043 commented Oct 7, 2021

nekevss commented Oct 7, 2021

jedel1043 commented Oct 7, 2021

Test262 conformance changes:

jedel1043 Oct 7, 2021

Choose a reason for hiding this comment

nekevss Oct 7, 2021

Choose a reason for hiding this comment

jedel1043 commented Oct 7, 2021

Test262 conformance changes:

jedel1043 commented Oct 7, 2021

nekevss commented Oct 4, 2021 •

edited by jedel1043

jedel1043 commented Oct 5, 2021 •

edited