feat: unknown bindings and unknown patterns #1804

ematipico · 2021-11-19T13:49:40Z

Summary

This PR adds more test cases and adds the correct unknown nodes inside existing test cases:

created new test cases to cover existing errors
eval, argments and yield will emit JS_UNKNOWN_BINDING
when parsing a pattern, we decide the unknown node based on the state of the parser
an incorrect usage of let will emit a JS_UNKNOWN_BINDING
an incorrect object pattern can generate a JS_UNKNOWN_PATTERN
```
 let { 5 } = foo;
```
an incorrect parameters list will generate a JS_UNKNOWN_PATTERN
```
 function foo(true) {}
```

incorrect functions will generate JS_UNKNOWN_BINDING

 function {}

   JS_PARAMETER_LIST@23..38
     LIST@23..37
       JS_UNKNOWN_BINDING@23..24
         L_CURLY@23..24 "{"
       JS_UNKNOWN_BINDING@24..25
         R_CURLY@24..25 "}"
       WHITESPACE@25..26 "\n"
       ERROR@26..34
         FUNCTION_KW@26..34 "function"
       WHITESPACE@34..35 " "
       JS_UNKNOWN_BINDING@35..36
         STAR@35..36 "*"
       ERROR@36..37
         L_PAREN@36..37 "("
     R_PAREN@37..38 ")"

updated the test suite with all the correct tests, extracted directly from the source code (comments)

Please refers to the new tests in the PR for more details. Make sure to search for JS_UNKNOWN_BINDING and JS_UNKNOWN_PATTERN.

Test Plan

cargo lint
cargo test
cargo format

cloudflare-pages · 2021-11-19T13:52:36Z

Deploying with Cloudflare Pages

Latest commit:	`053b5fc`
Status:	⚡️ Build in progress...

View logs

github-actions · 2021-11-19T13:56:35Z

Test262 comparison coverage results on ubuntu-latest

Test result	`main` count	This PR count	Difference
Total	17608	17608	0
Passed	16787	16771	❌ ⏬ -16
Failed	820	836	❌ ⏫ +16
Panics	1	1	0
Coverage	95.34%	95.25%	-0.09%

github-actions · 2021-11-19T13:59:03Z

Test262 comparison coverage results on windows-latest

Test result	`main` count	This PR count	Difference
Total	17608	17608	0
Passed	16787	16771	❌ ⏬ -16
Failed	820	836	❌ ⏫ +16
Panics	1	1	0
Coverage	95.34%	95.25%	-0.09%

Failed tests (16):

xtask/src/coverage/test262/test\language\expressions\async-arrow-function\early-errors-arrow-body-contains-super-call.js
xtask/src/coverage/test262/test\language\expressions\async-arrow-function\early-errors-arrow-formals-contains-super-call.js
xtask/src/coverage/test262/test\language\expressions\async-function\early-errors-expression-body-contains-super-call.js
xtask/src/coverage/test262/test\language\expressions\async-function\early-errors-expression-formals-contains-super-call.js
xtask/src/coverage/test262/test\language\expressions\function\early-body-super-call.js
xtask/src/coverage/test262/test\language\expressions\function\early-params-super-call.js
xtask/src/coverage/test262/test\language\expressions\object\method-definition\early-errors-object-method-body-contains-super-call.js
xtask/src/coverage/test262/test\language\expressions\object\method-definition\early-errors-object-method-formals-contains-super-call.js
xtask/src/coverage/test262/test\language\expressions\object\method-definition\name-super-call-body.js
xtask/src/coverage/test262/test\language\expressions\object\method-definition\name-super-call-param.js
xtask/src/coverage/test262/test\language\statements\async-function\early-errors-declaration-body-contains-super-call.js
xtask/src/coverage/test262/test\language\statements\async-function\early-errors-declaration-formals-contains-super-call.js
xtask/src/coverage/test262/test\language\statements\class\definition\early-errors-class-method-body-contains-super-call.js
xtask/src/coverage/test262/test\language\statements\class\definition\early-errors-class-method-formals-contains-super-call.js
xtask/src/coverage/test262/test\language\statements\function\early-body-super-call.js
xtask/src/coverage/test262/test\language\statements\function\early-params-super-call.js

yassere · 2021-11-19T18:41:24Z

Were those new tests added manually into the /inline directory? I think those tests are intended to be extracted.

ematipico · 2021-11-19T18:49:02Z

Were those new tests added manually into the /inline directory? I think those tests are intended to be extracted.

What do you mean by "extracted"?

Yes, I created manually the tests. Myself and Micha have been adding new tests in the /inline folder every time we change things in the parser, and it worked as intended - they are like snapshot tests.

yassere · 2021-11-19T19:03:32Z

The tests themselves would work as intended, but I think the idea is that you add the test cases "inline" to comments right next to the parser code they're testing and then the codegen extracts those comments into the inline directory.

So for example, these ok tests come from here and these err tests come from here.

ematipico · 2021-11-19T19:06:55Z

Damn! I wished there was some documentation somewhere! If there was, I couldn't find it anywhere!

Thank you for letting me know, I will change that right away!

ematipico · 2021-11-19T19:46:15Z

@MichaReiser I restored your tests by adding the new cases as commented code.

MichaReiser · 2021-11-19T22:13:08Z

@MichaReiser I restored your tests by adding the new cases as commented code.

Thanks ema. How can I generate the inline tests? Is it a macro?

ematipico · 2021-11-20T02:07:24Z

@MichaReiser I restored your tests by adding the new cases as commented code.

Thanks ema. How can I generate the inline tests? Is it a macro?

It's an xtask: cargo xtask codegen.

This will generate the JS files out of the comments.

Then we run the command to update the .rast files.

I guess we should start documenting all these findings

…ng-nodes-2

yassere

This looks good to me, but I haven't personally worked much with the internals of this part of the parser.

crates/rslint_parser/src/syntax/object.rs

crates/rslint_parser/src/syntax/pat.rs

crates/rslint_parser/src/syntax/object.rs

MichaReiser

Looks good and thanks for recovering the tests.

The one thing I would change is to not distinguish between binding and pattern and only use JS_UNKNOWN_PATTERN because the current grammar doesn't make the distinction as well (that's what I've been working on when extracting assignment targets, and then later introduce bindings).

We may need to give this a second pass once we have refined our approach to error recovery.

crates/rslint_parser/src/syntax/pat.rs

MichaReiser · 2021-11-23T12:28:12Z

crates/rslint_parser/src/syntax/pat.rs

 		p.error(err);
 	}

 	if p.at(T![await]) && p.state.in_async {
 		let err = p
 			.err_builder("Illegal use of `await` as an identifier in an async context")
 			.primary(p.cur_tok().range, "");
-
+		kind_to_change = JS_UNKNOWN_BINDING;


There were some more unknown bindings ;)

Sorry @MichaReiser , I make this change later in another PR

ematipico added 6 commits November 19, 2021 10:00

feat: pass error kind via parameter (#1788)

f5c5b46

feat: unknown statements (#1794)

f43dfcc

chore: rebase

397e567

fix: test and format

e53bcd5

chore: new error test cases

79d644f

feat: unknown binding

6792b69

ematipico added 2 commits November 19, 2021 11:24

feat: correctly flag unknown binding inside patterns

ab35618

feat: correct unknown pattern

d32218c

ematipico changed the title ~~feat: unknown bindings~~ feat: unknown bindings and unknown patterns Nov 19, 2021

chore: format

c2835fd

ematipico marked this pull request as ready for review November 19, 2021 17:25

ematipico requested review from MichaReiser, xunilrj and yassere November 19, 2021 17:26

feat: better pattern coverage

a3f1fd2

chore: code gen tests

8c62e9a

chore: restored tests

33fa14f

ematipico force-pushed the fature/unknown-binding-nodes-2 branch from ce04d04 to 33fa14f Compare November 19, 2021 20:08

ematipico force-pushed the feature/unknown-nodes-errors branch 2 times, most recently from 24629d5 to da8e59a Compare November 22, 2021 12:24

Merge branch 'feature/unknown-nodes-errors' into fature/unknown-bindi…

d745c25

…ng-nodes-2

yassere reviewed Nov 22, 2021

View reviewed changes

crates/rslint_parser/src/syntax/object.rs Outdated Show resolved Hide resolved

crates/rslint_parser/src/syntax/pat.rs Outdated Show resolved Hide resolved

crates/rslint_parser/src/syntax/object.rs Outdated Show resolved Hide resolved

MichaReiser approved these changes Nov 23, 2021

View reviewed changes

crates/rslint_parser/src/syntax/pat.rs Outdated Show resolved Hide resolved

chore: code review

053b5fc

ematipico merged commit 39523fe into feature/unknown-nodes-errors Nov 23, 2021

ematipico deleted the fature/unknown-binding-nodes-2 branch November 23, 2021 12:16

MichaReiser reviewed Nov 23, 2021

View reviewed changes

ematipico added a commit that referenced this pull request Nov 23, 2021

feat: unknown bindings and unknown patterns (#1804)

abc0044

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unknown bindings and unknown patterns #1804

feat: unknown bindings and unknown patterns #1804

ematipico commented Nov 19, 2021 •

edited

cloudflare-pages bot commented Nov 19, 2021 •

edited

github-actions bot commented Nov 19, 2021 •

edited

github-actions bot commented Nov 19, 2021 •

edited

yassere commented Nov 19, 2021

ematipico commented Nov 19, 2021 •

edited

yassere commented Nov 19, 2021

ematipico commented Nov 19, 2021 •

edited

ematipico commented Nov 19, 2021

MichaReiser commented Nov 19, 2021

ematipico commented Nov 20, 2021

yassere left a comment

MichaReiser left a comment

MichaReiser Nov 23, 2021

ematipico Nov 23, 2021

feat: unknown bindings and unknown patterns #1804

feat: unknown bindings and unknown patterns #1804

Conversation

ematipico commented Nov 19, 2021 • edited

Summary

Test Plan

cloudflare-pages bot commented Nov 19, 2021 • edited

Deploying with Cloudflare Pages

github-actions bot commented Nov 19, 2021 • edited

Test262 comparison coverage results on ubuntu-latest

github-actions bot commented Nov 19, 2021 • edited

Test262 comparison coverage results on windows-latest

yassere commented Nov 19, 2021

ematipico commented Nov 19, 2021 • edited

yassere commented Nov 19, 2021

ematipico commented Nov 19, 2021 • edited

ematipico commented Nov 19, 2021

MichaReiser commented Nov 19, 2021

ematipico commented Nov 20, 2021

yassere left a comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Nov 23, 2021

Choose a reason for hiding this comment

ematipico Nov 23, 2021

Choose a reason for hiding this comment

ematipico commented Nov 19, 2021 •

edited

cloudflare-pages bot commented Nov 19, 2021 •

edited

github-actions bot commented Nov 19, 2021 •

edited

github-actions bot commented Nov 19, 2021 •

edited

ematipico commented Nov 19, 2021 •

edited

ematipico commented Nov 19, 2021 •

edited