Refactor MemberExpression AST structure #1799

MichaReiser · 2021-11-18T09:22:08Z

Summary

Refactors the AST tree structure for MemberExpressions as part of #1725

Renames BracketExpr to JsComputedMemberExpression
Renames DotExpr to JsStaticMemberExpression
Introduces the new JsReferenceIdentifierMember and JsReferencePrivateMember which are references to member names (analog to JsReferenceIdentifierExpression that references an identifier and JsBindingIdentifier that defines an identifier)
Merge PrivatePropAccess into JsStaticMemberExpression (enabled by introducing JsReferenceMember
Introduce SuperExpression so that super.test() works
Add new check that verifies that calling super() only works inside of constructor (I leave checking if we're inside of a subclass to another PR).
Delete SuperCall as this can now be modelled using CallExpr
Deleted some no longer used nodes/kinds

Test Plan

Verified the coverage and that all tests are still passing. Added new integration tests.

The coverage is increasing because of the added check if super is called from inside a constructor.

github-actions · 2021-11-18T09:28:38Z

Test262 comparison coverage results on ubuntu-latest

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

github-actions · 2021-11-18T09:30:55Z

Test262 comparison coverage results on windows-latest

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

Fixed 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

MichaReiser · 2021-11-18T09:32:03Z

crates/rslint_parser/src/syntax/expr.rs

-			let err = p
-				.err_builder("optional chaining cannot contain private identifiers")
-				.primary(range, "");
+	}


This isn't true. err?.#test works perfectly fine.

MichaReiser · 2021-11-18T09:33:49Z

xtask/js.ungram

+JsAnyReferenceMember =
+	JsReferenceIdentifierMember
+	| JsReferencePrivateMember
+
+// A reference to a member
+// a.b
+//   ^
+// let { b: c } = a
+//       ^
+JsReferenceIdentifierMember =
+	name: 'ident'
+
+// A reference to a private member
+// a.#b
+//   ^^
+// let { #b: c } = a
+//       ^^
+JsReferencePrivateMember =
+	'#'
+	name: 'ident'
+
+


I'm not entirely sure about the naming here. Should these be postfixed with JsAnyReferenceMemberName since these are names and not members? Might sound a bit weird in the case of JsReferenceIdentifierMemberName

@jamiebuilds wdyt?

I don't know that I would call this a "reference" since it's not actually looking anything up in scope. Accessing a member uses completely different logic that could be affected by getters/proxies/etc, so relating it to scope references seems incorrect.

I like the idea of using JsStaticMember (or JsLiteralMember)

JsStaticMember(Name?) -- or -- JsLiteralMember(Name?) JsPrivateMember(Name?)

We could use the *Name suffix for all miscellaneous "identifier-like" nodes

We could use "literal" to mean "any value that can be determined from the source text alone" which would make "static" less overloaded since it's another language feature

That's an excellent point and something I haven't considered. My main crux is that the potential of name clashes.

Classes and Object have Js*ClassMember and Js*ObjectMember, and it might be a nice addition in the future to have a JsAnyMember that is a union over all possible members. Using JsStaticMember and JsPrivateMember would then not be part of that union which is confusing from a naming perspective

Classes and Object permit identifiers, string literals, and number literals as member names which are why this PR introduces a JsLiteralMemberName. The problem is, string literals and number literals are not permitted in MemberExpression's, which is why they need two different types (with different names, calling both *LiteralMemberName` might be confusing if they are not interchangeable

That's why I opted for the Reference prefix to distinguish the use cases clearly, but, as mentioned, I'm only somewhat happy with it.

An alternative that I considered is to prefix the member names used for classes and object expressions with Binding because they introduce new bindings. That's probably more true for classes that have their own scope, and each member defines a new binding in that scope. Objects may define members, but they don't introduce a new binding to the scope (they bind the members on the object, but I don't think that's the meaning of Binding we're using).

Overall, my problem is that these names are similar but have different semantics nonetheless.

I talked about the naming with @jamiebuilds . We don't have a better name yet but we also came up with some more renames for other structures. So I think it's best to move on with this PR and fix the name when renaming all other fields/ nodes.

cloudflare-workers-and-pages · 2021-11-18T09:36:38Z

Deploying with Cloudflare Pages

Latest commit:	`3aff5af`
Status:	✅ Deploy successful!
Preview URL:	https://6ecaed9f.tools-8rn.pages.dev

View logs

crates/rslint_parser/src/syntax/expr.rs

MichaReiser requested review from ematipico and jamiebuilds November 18, 2021 09:22

MichaReiser commented Nov 18, 2021

View reviewed changes

MichaReiser mentioned this pull request Nov 18, 2021

☂️ AST Façade Improvements #1725

Closed

47 tasks

MichaReiser changed the title ~~Refactor MemberExpression tree structure~~ Refactor MemberExpression AST structure Nov 18, 2021

MichaReiser mentioned this pull request Nov 18, 2021

Rename *Literal nodes to *LiteralExpression #1800

Merged

ematipico reviewed Nov 18, 2021

View reviewed changes

crates/rslint_parser/src/syntax/expr.rs Show resolved Hide resolved

crates/rslint_parser/src/syntax/expr.rs Show resolved Hide resolved

crates/rslint_parser/src/syntax/expr.rs Show resolved Hide resolved

MichaReiser force-pushed the feature/member-expression branch from bc542cc to 3da3c66 Compare November 22, 2021 09:54

MichaReiser requested a review from ematipico November 22, 2021 09:54

MichaReiser added 4 commits November 22, 2021 12:54

MemberExpression

d6e42ae

SuperExpression

a508d19

Add explicit error for optional chaining with super

8eb6f6b

Document Slot types in Cursor

3aff5af

MichaReiser force-pushed the feature/member-expression branch from 3da3c66 to 3aff5af Compare November 22, 2021 12:02

MichaReiser mentioned this pull request Nov 22, 2021

Print empty slots in debug output #1806

Merged

ematipico approved these changes Nov 22, 2021

View reviewed changes

MichaReiser merged commit d0b9b60 into main Nov 22, 2021

MichaReiser deleted the feature/member-expression branch November 22, 2021 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MemberExpression AST structure #1799

Refactor MemberExpression AST structure #1799

MichaReiser commented Nov 18, 2021

github-actions bot commented Nov 18, 2021 •

edited

Loading

github-actions bot commented Nov 18, 2021 •

edited

Loading

MichaReiser Nov 18, 2021

MichaReiser Nov 18, 2021

MichaReiser Nov 18, 2021

jamiebuilds Nov 18, 2021

MichaReiser Nov 18, 2021

MichaReiser Nov 19, 2021

cloudflare-workers-and-pages bot commented Nov 18, 2021 •

edited

Loading

Refactor MemberExpression AST structure #1799

Refactor MemberExpression AST structure #1799

Conversation

MichaReiser commented Nov 18, 2021

Summary

Test Plan

github-actions bot commented Nov 18, 2021 • edited Loading

Test262 comparison coverage results on ubuntu-latest

github-actions bot commented Nov 18, 2021 • edited Loading

Test262 comparison coverage results on windows-latest

MichaReiser Nov 18, 2021

Choose a reason for hiding this comment

MichaReiser Nov 18, 2021

Choose a reason for hiding this comment

MichaReiser Nov 18, 2021

Choose a reason for hiding this comment

jamiebuilds Nov 18, 2021

Choose a reason for hiding this comment

MichaReiser Nov 18, 2021

Choose a reason for hiding this comment

MichaReiser Nov 19, 2021

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Nov 18, 2021 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented Nov 18, 2021 •

edited

Loading

github-actions bot commented Nov 18, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 18, 2021 •

edited

Loading