Rename Pattern to Bindings and share parsing logic with Assignment Target #1839

MichaReiser · 2021-12-01T08:33:20Z

Sorry, it seems this escalated quite a bit. I wasn't aware, that patterns are used in so many different places in our codebase

Summary

#1805 separated assignment targets out from patterns. This PR now renames the "remaining" patterns to bindings. Bindings are different from assignment targets because they bind a new name in the current scope and assign a value to it rather than assigning a value to an existing name.

This PR further restructures Patterns as specified in #1719.

Renames Pattern to JsAnyBinding
Replaces AssignPattern with JsBindingWithDefault
Deletes PatternOrExpr
Replace SinglePattern with JsIdentifierBinding
Replace RestPattern with JsArrayRestBinding and JsObjectRestBinding. Remove it from the Pattern union (only permitted inside of arrays/objects)
Rename ObjectPattern to JsObjectBinding
Replace KeyValuePattern with JsShorthandPropertyBinding and JsPropertyBinding
Rename ArrayPattern to JsArrayBinding
Remove ExprPattern
Unify SpreadElement and JsSpread
Deleted InitializedProp (not valid JS nor TS syntax???)

This PR extracts the common logic for parsing Patterns from assignment_target as traits into pattern.rs that I then used to parse bindings.

Part of #1725

Test Plan

Verified coverage, added new test cases.

All libs are now parsing without panicking. Performance is roughly the same

github-actions · 2021-12-01T08:41:24Z

Test262 comparison coverage results on ubuntu-latest

Test result	`main` count	This PR count	Difference
Total	17608	17608	0
Passed	17068	17084	✅ ⏫ +16
Failed	539	523	✅ ⏬ -16
Panics	1	1	0
Coverage	96.93%	97.02%	+0.09%

github-actions · 2021-12-01T08:43:42Z

Test262 comparison coverage results on windows-latest

Test result	`main` count	This PR count	Difference
Total	17608	17608	0
Passed	17068	17084	✅ ⏫ +16
Failed	539	523	✅ ⏬ -16
Panics	1	1	0
Coverage	96.93%	97.02%	+0.09%

Fixed tests (16):

xtask/src/coverage/test262/test\language\expressions\arrow-function\syntax\early-errors\arrowparameters-bindingidentifier-no-arguments.js
xtask/src/coverage/test262/test\language\expressions\arrow-function\syntax\early-errors\arrowparameters-bindingidentifier-no-eval.js
xtask/src/coverage/test262/test\language\expressions\async-arrow-function\await-as-param-ident-nested-arrow-parameter-position.js
xtask/src/coverage/test262/test\language\expressions\async-arrow-function\await-as-param-rest-nested-arrow-parameter-position.js
xtask/src/coverage/test262/test\language\expressions\async-arrow-function\await-as-param-nested-arrow-parameter-position.js
xtask/src/coverage/test262/test\language\expressions\async-arrow-function\early-errors-arrow-await-in-formals-default.js
xtask/src/coverage/test262/test\language\expressions\async-arrow-function\early-errors-arrow-await-in-formals.js
xtask/src/coverage/test262/test\language\expressions\class\class-name-ident-await-module.js
xtask/src/coverage/test262/test\language\expressions\class\class-name-ident-let.js
xtask/src/coverage/test262/test\language\expressions\object\cover-initialized-name.js
xtask/src/coverage/test262/test\language\future-reserved-words\let-strict.js
xtask/src/coverage/test262/test\language\reserved-words\await-module.js
xtask/src/coverage/test262/test\language\statements\class\class-name-ident-await-module.js
xtask/src/coverage/test262/test\language\statements\class\class-name-ident-let.js
xtask/src/coverage/test262/test\language\statements\labeled\value-await-module.js
xtask/src/coverage/test262/test\language\statements\let\syntax\identifier-let-allowed-as-lefthandside-expression-strict.js

cloudflare-workers-and-pages · 2021-12-01T08:58:27Z

Deploying with Cloudflare Pages

Latest commit:	`5eaade0`
Status:	✅ Deploy successful!
Preview URL:	https://2a0145de.tools-8rn.pages.dev

View logs

MichaReiser · 2021-12-01T08:48:00Z

crates/rslint_parser/src/syntax/decl.rs

-
-	let mut opt = None;
-
-	if p.at(T![?]) {


I removed the TypeScript handling for now. The recovery logic here is incorrect and we need to re-think the logic now that assign pattern is no longer a thing

crates/rslint_parser/src/syntax/decl.rs

MichaReiser · 2021-12-01T08:50:15Z

crates/rslint_parser/src/syntax/expr.rs

@@ -898,42 +913,42 @@ pub fn primary_expr(p: &mut Parser) -> Option<CompletedMarker> {
 			} else {
 				// `async a => {}` and `async (a) => {}`
 				if p.state.potential_arrow_start
-					&& token_set![T![ident], T![yield], T!['(']].contains(p.nth(1))
+					&& token_set![T![ident], T![yield], T![await], T!['(']].contains(p.nth(1))


Fixes an issue where async await => {} isn't recognized as a arrow function

MichaReiser · 2021-12-01T08:50:50Z

crates/rslint_parser/src/syntax/expr.rs

 					{
-						let mut guard = p.with_state(ParserState {
+						let in_async_p = &mut *p.with_state(ParserState {


the parameters must be parsed with async set to true to detect if e.g. await is a valid or invalid identifier.

MichaReiser · 2021-12-01T08:52:05Z

crates/rslint_parser/src/syntax/object.rs

@@ -156,14 +156,6 @@ fn parse_object_member(p: &mut Parser) -> ParsedSyntax {
 				parse_method_object_member_body(p);
 				Present(m.complete(p, JS_METHOD_OBJECT_MEMBER))
 			} else if let Some(mut member_name) = member_name {
-				// test object_expr_assign_prop
-				// let b = { foo = 4, foo = bar }


This isn't valid JS nor TS syntax...???

No, it's not a valid syntax

ematipico

It's a lot of code and it will take time for me to review. For now I leave some feedback and question:

we should add new tests to the formatter for array binding. It seems we don't have them, other than simple.js;
why did you use traits in this case? what's the endgame? The documentation doesn't explain that part and I can see you implemented the traits just once, which means that it could have been a struct or an enum;

crates/rome_formatter/src/ts/bindings/array_binding.rs

crates/rslint_parser/src/parser/parsed_syntax.rs

crates/rslint_parser/src/syntax/assignment_target.rs

crates/rslint_parser/src/syntax/binding.rs

crates/rslint_parser/src/syntax/decl.rs

crates/rslint_parser/src/syntax/expr.rs

ematipico · 2021-12-01T10:43:17Z

crates/rslint_parser/src/syntax/object.rs

@@ -156,14 +156,6 @@ fn parse_object_member(p: &mut Parser) -> ParsedSyntax {
 				parse_method_object_member_body(p);
 				Present(m.complete(p, JS_METHOD_OBJECT_MEMBER))
 			} else if let Some(mut member_name) = member_name {
-				// test object_expr_assign_prop
-				// let b = { foo = 4, foo = bar }


No, it's not a valid syntax

crates/rslint_parser/src/syntax/pattern.rs

MichaReiser · 2021-12-01T11:45:38Z

It's a lot of code and it will take time for me to review. For now I leave some feedback and question:

Thanks for taking the time!

* we should add new tests to the formatter for array binding. It seems we don't have them, other than `simple.js`;

I'm not sure if that code is final. I just moved the files from the pattern to the bindings folder and renamed them. I don't know why git doesn't recognize the moves. I would leave it for now because this PR isn't touching the formatter code and we need to revisit all implementations anyway.

* why did you use traits in this case? what's the endgame? The documentation doesn't explain that part and I can see you implemented the traits just once, which means that it could have been a `struct` or an `enum`;

The traits are implemented twice: Once for Bindings and once for AssignmentTarget. For example, ObjectAssignmentTarget and ArrayAssignmentTarget both implement the ParseObjectPattern trait.

ematipico · 2021-12-02T08:59:26Z

crates/rslint_parser/src/syntax/binding.rs

+// let let = 5;
+// const let = 5;
+// let a, a;
+pub(crate) fn parse_identifier_binding(p: &mut Parser) -> ConditionalParsedSyntax {


Maybe we should start documenting those methods that are ConditionalParsedSyntax and explain why they return this type (and have it as a rule inside the CONTRIBUTING.md file). To be more precise, we know that ConditionalParsedSyntax can be used for different reasons (languages features, super language, browser version, etc.), although by looking at method, it's difficult to know. Having a documentation with some snippet might help?

We should explain what's the syntax that can differ.

Yes, that makes sense. I also need to revisit the ConditionalParsedSyntax. The way it currently works (where Absent values can be invalid too`) is super awkward to use.

I'm also still figuring this out. So far I thought it's a good idea to make the or_invalid_to_unknown conversion right inside of the parse function but it turned out that is dangerous.

For example, the class or function id, a single arrow function parameter or an object rest pattern don't support JsAnyBinding and, therefore, returning a JsUnknownBinding is invalid in these cases. So it seems, all we can do in these cases is to bubble up the error until we come to a place where we can handle it.

This is a bit annoying. An alternative would be to just add a diagnostic and keep the binding as is. This would certainly be easier but we then no longer have a strict rule when to use unknown:

any syntax error (additional syntax) -> Content is inside of an Unknown node

missing syntax -> Empty slot, handled in the AST facade

We need to get some more experience how to handle and we may come back to this change.

I bumped into this too while refactoring class.rs, where some function uses or_invalid_to_unknown under to hood but it is "hidden" and we don't know what's happening

crates/rslint_parser/src/syntax/binding.rs

crates/rslint_parser/src/syntax/class.rs

crates/rslint_parser/src/syntax/decl.rs

crates/rslint_parser/src/syntax/pattern.rs

MichaReiser · 2021-12-02T11:22:46Z

Update on await. It's a valid identifier in scripts (outside of async) functions but not in modules.

I added one more check to disallow let as an identifier in strict mode (see coverage improvements)

MichaReiser force-pushed the feature/bindings branch from f2dba6c to 914f0c2 Compare December 1, 2021 08:58

MichaReiser commented Dec 1, 2021

View reviewed changes

MichaReiser requested review from ematipico and jamiebuilds December 1, 2021 09:06

MichaReiser marked this pull request as ready for review December 1, 2021 09:06

MichaReiser mentioned this pull request Dec 1, 2021

☂️ AST Façade Improvements #1725

Closed

47 tasks

ematipico reviewed Dec 1, 2021

View reviewed changes

MichaReiser requested a review from ematipico December 1, 2021 14:53

MichaReiser mentioned this pull request Dec 1, 2021

📎 Coverage Report: Compare results for each tests #1844

Closed

MichaReiser added 17 commits December 2, 2021 07:44

Rename Pattern to JsAnyBinding

e9f37c1

Rename SinglePattern to JsIdentifierBinding

2341a3d

Rename SinglePattern to JsIdentifierBinding

e47a66e

Fix parsing of await () functions, update ast snapshots

45894b4

Rename KeyValuePattern to JsPropertyBinding

2880228

Rename ObjectPattern to JsObjectBinding

7097e82

Introduce JsShorthandPropertyBinding

1b17862

Introduce JsObjectRestBinding

47f8a44

Add init to object property bindings

7d0f099

Use trait for object binding parsing

2ca63f9

Use trait for object assignment target

7141e43

Update snapshots after merging

75f68a3

Change pattern to return ParsedSyntax and rename to parse_binding

18abc2b

Extract ArrayPattern trait

df511a0

Rename ArrayPattern to JsArrayBinding, Remove AssignPattern

463b3fa

Use ArrayPattern trait for ArrayBinding

2f7a375

Factor out traits into pattern file.

2ac5c01

MichaReiser added 10 commits December 2, 2021 07:48

Remove parameters argument for bindings (TS only)

3bf3ad1

Delete PatternOrExpr

456f104

Remove no longer needed syntax

47f5757

Pre-review cleanup

8e51c48

Remove ts types from identifier binding

dafc355

Fix formatting

611013b

Address code review comments.

1eb3245

Include JsBindingWithDefault in constructor parameter.

4d1ae58

Update tests after rebase

8d9cd98

Add tests for variable declaration statements

fb9f941

MichaReiser force-pushed the feature/bindings branch from 969b05d to fb9f941 Compare December 2, 2021 07:21

MichaReiser added 6 commits December 2, 2021 09:48

Fix unknown bindings in class id, shorthand bindings, etc. positions

0fc9f91

Fix lint errors

0e3d76c

Revert changes to tests

f0e7984

Fix snapshots

229d5d3

Fix silly bug in conditional parsed syntax

57f4dca

Clippy

2470732

ematipico reviewed Dec 2, 2021

View reviewed changes

MichaReiser added 8 commits December 2, 2021 11:16

Address code review comments

34ca967

More documentation

c5faf6b

More documentation 2

d5cf625

Fix type of class decl inside of stmt

c091862

Disallow await inside of modules

51cc285

Formatting...

85d3514

Disallow let in strict mode

12ebeac

More identifier tests

5eaade0

ematipico approved these changes Dec 2, 2021

View reviewed changes

MichaReiser merged commit 9f2cdc0 into main Dec 2, 2021

MichaReiser deleted the feature/bindings branch December 2, 2021 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Pattern to Bindings and share parsing logic with Assignment Target #1839

Rename Pattern to Bindings and share parsing logic with Assignment Target #1839

MichaReiser commented Dec 1, 2021 •

edited

Loading

github-actions bot commented Dec 1, 2021 •

edited

Loading

github-actions bot commented Dec 1, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 1, 2021 •

edited

Loading

MichaReiser Dec 1, 2021

MichaReiser Dec 1, 2021

MichaReiser Dec 1, 2021

MichaReiser Dec 1, 2021

ematipico Dec 1, 2021

ematipico left a comment

ematipico Dec 1, 2021

MichaReiser commented Dec 1, 2021

ematipico Dec 2, 2021

MichaReiser Dec 2, 2021

MichaReiser Dec 2, 2021

ematipico Dec 2, 2021

MichaReiser commented Dec 2, 2021

Rename Pattern to Bindings and share parsing logic with Assignment Target #1839

Rename Pattern to Bindings and share parsing logic with Assignment Target #1839

Conversation

MichaReiser commented Dec 1, 2021 • edited Loading

Summary

Test Plan

github-actions bot commented Dec 1, 2021 • edited Loading

Test262 comparison coverage results on ubuntu-latest

github-actions bot commented Dec 1, 2021 • edited Loading

Test262 comparison coverage results on windows-latest

cloudflare-workers-and-pages bot commented Dec 1, 2021 • edited Loading

Deploying with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Dec 2, 2021

MichaReiser commented Dec 1, 2021 •

edited

Loading

github-actions bot commented Dec 1, 2021 •

edited

Loading

github-actions bot commented Dec 1, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 1, 2021 •

edited

Loading