From 9c84e7d895e1ee6aa8f1276e5a1e9d9168bde809 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 8 Nov 2021 18:14:29 +0100 Subject: [PATCH] Address code review comments --- rfcs/01-js-grammar.ungram | 249 +++++++++++++++++++------------------- 1 file changed, 124 insertions(+), 125 deletions(-) diff --git a/rfcs/01-js-grammar.ungram b/rfcs/01-js-grammar.ungram index 368bf36bf163..c982813d3a4c 100644 --- a/rfcs/01-js-grammar.ungram +++ b/rfcs/01-js-grammar.ungram @@ -14,12 +14,12 @@ JsScript = interpreter: 'js_shebang'? // rslint: shebang directives: 'js_directive'* // Or a directive node? - stmts: JsStatement* // Called items + stmts: AnyJsStatement* // Called items JsModule = interpreter: 'js_shebang'? directives: 'js_directive'* - stmts: JsStatement* + stmts: AnyJsStatement* // --------------------- Unknowns --------------------- // Hack to make ungrammar happy. Let's hope they never ban recursive types xD @@ -38,7 +38,7 @@ JsUnknownAssignmentTarget = SyntaxElement* // --------------------- Statements --------------------- // RSLint -JsStatement = JsBlockStatement // BlockStmt +AnyJsStatement = JsBlockStatement // BlockStmt | JsBreakStatement // BreakStmt | JsClassDeclaration // ClassDecl | JsDebuggerStatement // DebuggerStmt @@ -65,7 +65,7 @@ JsStatement = JsBlockStatement // BlockStmt | JsUnknownStatement JsBlockStatement = - '{' stmts: JsStatement* '}' + '{' stmts: AnyJsStatement* '}' JsBreakStatement = 'break' @@ -82,41 +82,40 @@ JsDebuggerStatement = // RSLint uses Condition for: (test) JsDoWhileStatement = - 'do' body: JsStatement - 'while' '(' test: JsExpression ')' ';'? + 'do' body: AnyJsStatement + 'while' '(' test: AnyJsExpression ')' ';'? JsEmptyStatement = ';' JsExpressionStatement = - expression: JsExpression ';'? + expression: AnyJsExpression ';'? // Difference to RSLint: no ForInit, ForTest and ForUpdate: RSLint needed the extra kinds so that it could keep the test/update apart JsForStatement = 'for' '(' init: JsForInit? init_semicolon: ';' - test: JsExpression? test_semicolon: ';' - update: JsExpression? + test: AnyJsExpression? test_semicolon: ';' + update: AnyJsExpression? ')' - body: JsStatement + body: AnyJsStatement -JsForInit = JsExpression | JsVariableDeclaration +JsForInit = AnyJsExpression | JsVariableDeclaration JsForInStatement = - 'for' '(' left: JsForLeft 'in' right: JsExpression ')' - body: JsStatement + 'for' '(' left: JsForLeft 'in' right: AnyJsExpression ')' + body: AnyJsStatement -// Separate `ForAwait?` JsForOfStatement = 'for' 'await'? '(' left: JsForLeft 'of' - right: JsExpression + right: AnyJsExpression ')' - body: JsStatement + body: AnyJsStatement -JsForLeft = JsVariableDeclaration | JsAssignmentTarget +JsForLeft = JsVariableDeclaration | AnyJsAssignmentTarget JsFunctionDeclaration = 'function' '*'? @@ -129,36 +128,37 @@ JsFunctionDeclaration = JsFunctionBody = '{' directives: 'directive'* - body: JsStatement* + body: AnyJsStatement* '}' JsIfStatement = - 'if' '(' test: JsExpression ')' - consequence: JsStatement + 'if' '(' test: AnyJsExpression ')' + consequence: AnyJsStatement else_clause: JsElseClause? JsElseClause = 'else' - alternate: JsStatement + alternate: AnyJsStatement JsLabeledStatement = label: 'js_identifier' ':' - body: JsStatement + body: AnyJsStatement JsReturnStatement = - 'return' argument: JsExpression? ';'? + 'return' argument: AnyJsExpression? ';'? JsSwitchStatement = - 'switch' '(' discriminant: JsExpression ')' + 'switch' '(' discriminant: AnyJsExpression ')' cases: JsSwitchCase* // Introduce a case clause / default because the optionality of the tokens depend on which variant it is +// An alternative would be to have preDefaultCases, defaultCases, postDefaultCases JsSwitchCase = JsCaseClause | JsDefaultClause -JsCaseClause = 'case' test: JsExpression ':' -JsDefaultClause = 'default' ':' +JsCaseClause = 'case' test: AnyJsExpression ':' consequent: AnyJsStatement* +JsDefaultClause = 'default' ':' consequent: AnyJsStatement* JsThrowStatement = - 'throw' argument: JsExpression ';'? + 'throw' argument: AnyJsExpression ';'? JsTryStatement = 'try' @@ -191,21 +191,20 @@ JsFinallyClause = JsVariableDeclarationStatement = declaration: JsVariableDeclaration ';'? -// TODO unable to type: at least one child. Add support for + (at least one) to ungrammar or split into declarator: JsVariableDeclarator, other_declarators: JsVariableDeclarator* JsVariableDeclaration = kind_token: ('var' | 'let' | 'const') - declarators: JsVariableDeclarator* + declarators: (JsVariableDeclarator (',' JsVariableDeclarator)*) JsVariableDeclarator = - id: JsBinding '=' init: JsExpression? + id: JsBinding '=' init: AnyJsExpression? JsWhileStatement = - 'while' '(' test: JsExpression ')' - body: JsStatement + 'while' '(' test: AnyJsExpression ')' + body: AnyJsStatement JsWithStatement = - 'with' '(' object: JsExpression ')' - body: JsStatement + 'with' '(' object: AnyJsExpression ')' + body: AnyJsStatement // --------------------- EXPRESSIONS --------------------- @@ -216,7 +215,7 @@ JsWithStatement = // PrivatePropAccess, -> MemberExpression // } // RSLint Name -JsExpression = JsArrayExpression // ArrayExpr +AnyJsExpression = JsArrayExpression // ArrayExpr | JsArrowFunctionExpression // ArrowExpr | JsAssignmentExpression // AssignExpr | JsAwaitExpression // AwaitExpr @@ -254,44 +253,44 @@ JsArrayExpression = '[' // Not 100% correct because array holes must have a trailing comma. But enforcing // this would make it too awkward to use - elements: (JsArrayElement (',' JsArrayElement)* ','?) + elements: (JsArrayElement (',' AnyJsArrayElement)* ','?) ']' // Doesn't exist in Rome classic nor RSLint. Required so that we can refer the ',' token -JsArrayElement = JsSpread | JsExpression | JsArrayHole +AnyJsArrayElement = JsSpread | AnyJsExpression | JsArrayHole JsArrowFunctionExpression = 'async'? - parameters: JsArrowFunctionParameters + parameters: AnyJsArrowFunctionParameters '=>' body: JsStatementOrExpression -JsArrowFunctionParameters = JsParameterList | JsParameter +AnyJsArrowFunctionParameters = JsParameterList | AnyJsParameter JsAssignmentExpression = - left: JsAssignmentTarget + left: AnyJsAssignmentTarget operator: ('=' | '+=' | '-=' | '*=' | '/=' | '%=' | '<<=' | '>>=' | '>>>=' | '|=' | '^=' | '&=' | '**=' | '&&=' | '||=' | '??=') - right: JsExpression + right: AnyJsExpression JsAwaitExpression = - 'await' argument: JsExpression + 'await' argument: AnyJsExpression JsBinaryExpression = - left: JsExpression + left: AnyJsExpression operator: ('==' | '!=' | '===' | '**' | '!==' | '<' | '<=' | '>' | '>=' | '<<' | '>>' | '>>>' | '+' | '-' | '*' | '/' | '%' | '|' | '^' | '&' | 'in' | 'instanceof') - right: JsExpression + right: AnyJsExpression // Rome classic adds super to callee but this isn't needed because Rome adds super to the expression union??? JsCallExpression = - callee: JsExpression + callee: AnyJsExpression argument_list: JsArgumentList JsConditionalExpression = - test: JsExpression + test: AnyJsExpression '?' - consequent: JsExpression + consequent: AnyJsExpression ':' - alternate: JsExpression + alternate: AnyJsExpression JsDoExpression = 'do' @@ -306,33 +305,33 @@ JsFunctionExpression = body: JsFunctionBody JsLogicalExpression = - left: JsExpression + left: AnyJsExpression operator: ('||' | '&&' | '??') - right: JsExpression + right: AnyJsExpression // Rome classic defines object as expr | super but super is already expression? JsMemberExpression = - object: JsExpression - member: JsMember + object: AnyJsExpression + member: AnyJsMember -JsMember = JsStaticMember | JsComputedMember +AnyJsMember = JsStaticMember | JsComputedMember -JsStaticMember = '.' name: JsStaticMemberName +JsStaticMember = '.' name: AnyJsStaticMemberName // Rome classic defines callee as expr | super but super is an expression? JsNewExpression = 'new' - callee: JsExpression + callee: AnyJsExpression argument_list: JsArgumentList // TODO Better to have a OptionalExpression type to support arrays, members, calls, template literal, private identifier JsOptionalCallExpression = - callee: JsExpression + callee: AnyJsExpression optional: '?.' argument_list: JsArgumentList JsParenthesizedExpression = - '(' expression: JsExpression ')' + '(' expression: AnyJsExpression ')' JsReferenceIdentifierExpression = name: 'js_identifier' @@ -342,38 +341,38 @@ JsReferenceIdentifierExpression = // - specify that the parser never generates a sequence expression if there are less than 2 expressions. In that case, expressions* would be safe. `a,` would be parsed as unknown expression // - Model as `BinaryExpression` where `,` is the operator. Results in deeper trees but at least, can be modelled correctly. a, b, c -> seq(left: seq(a, b), c) JsSequenceExpression = - left: JsExpression + left: AnyJsExpression comma: ',' - second: JsExpression + second: AnyJsExpression JsSuperExpression = 'super' // example`test` JsTaggedTemplateExpression = - tag: JsExpression + tag: AnyJsExpression literal: JsTemplateLiteral JsThisExpression = 'this' JsUnaryExpression = operator: ('-' | '+' | '!' | '~' | 'typeof' | 'void' | 'delete')? - argument: JsExpression + argument: AnyJsExpression // Pre/Post are needed to make the `operator` accessor mandatory. The only alternative would be to -// `JsUpdateExpression = **internal**_prefix: (...)? argument: JsExpression **internal**_postfix: (...)?` and then +// `JsUpdateExpression = **internal**_prefix: (...)? argument: AnyJsExpression **internal**_postfix: (...)?` and then // define a public facing `operator` method that returns either one of them or missing if the operator isn't present at all // (which is unlikely, because the parser would then just parse the argument) JsPreUpdateExpression = operator: ('--' | '++') - operand: JsSimpleAssignmentTarget + operand: AnyJsSimpleAssignmentTarget JsPostUpdateExpression = - operand: JsSimpleAssignmentTarget + operand: AnyJsSimpleAssignmentTarget operator: ('--' | '++') JsYieldExpression = 'yield' '*'? - argument: JsExpression? + argument: AnyJsExpression? // --------------------- LITERALS --------------------- @@ -404,10 +403,10 @@ JsNullLiteral = value: 'null' // Rome uses two different lists: expressions and quasis JsTemplateLiteral = left_tick: '`' - elements: JsTemplateElement* + elements: AnyJsTemplateElement* right_tick: '`' -JsTemplateElement = JsStringTemplateElement | JsExpression +AnyJsTemplateElement = JsStringTemplateElement | AnyJsExpression JsStringTemplateElement = value: 'js_string_template_element' @@ -420,19 +419,19 @@ JsClassDeclaration = 'class' id: JsBindingIdentifier extends_clause: JsExtendsClause? - '{' members: JsClassMember* '}' + '{' members: AnyJsClassMember* '}' JsExtendsClause = 'extends' - super_class: JsExpression + super_class: AnyJsExpression JsClassExpression = 'class' id: JsBindingIdentifier? extends_clause: JsExtendsClause? - '{' members: JsClassMember* '}' + '{' members: AnyJsClassMember* '}' -JsClassMember = // RSLint +AnyJsClassMember = // RSLint JsConstructorClassMember // Constructor | JsPropertyClassMember // ClassProp | JsPrivatePropertyClassMember // PrivateProp @@ -450,37 +449,37 @@ JsConstructorClassMember = parameter_list: JsParameterList body: JsFunctionBody -JsClassMemberName = JsObjectMemberName | JsPrivateClassMemberName +AnyJsClassMemberName = AnyJsObjectMemberName | JsPrivateClassMemberName JsPropertyClassMember = 'static' - key: JsClassMemberName + key: AnyJsClassMemberName value: JsPropertyClassMemberInitializer? ';' JsPropertyClassMemberInitializer = - '=' expression: JsExpression + '=' expression: AnyJsExpression // TODO share methods etc with objects? JsMethodClassMember = 'static'? 'async'? '*'? - key: JsClassMemberName + key: AnyJsClassMemberName parameter_list: JsParameterList body: JsFunctionBody JsGetterClassMember = 'static'? 'get' - key: JsClassMemberName + key: AnyJsClassMemberName '(' ')' body: JsFunctionBody JsSetterClassMember = 'static'? 'set' - key: JsClassMemberName - '(' value: JsParameter ')' + key: AnyJsClassMemberName + '(' value: AnyJsParameter ')' body: JsFunctionBody JsPrivateClassMemberName = '#' id: 'js_identifier' @@ -502,20 +501,20 @@ JsPrivatePropertyClassMember = // --------------------- OBJECTS --------------------- // We should implement helpers that convert the content to a valid name, e.g. the name 0x02 = '2' and not '0x02' -JsStaticMemberName = JsIdentifier | JsStringLiteral | JsNumberLiteral +AnyJsStaticMemberName = JsIdentifier | JsStringLiteral | JsNumberLiteral -JsComputedMember = '[' name: JsExpression ']' +JsComputedMember = '[' name: AnyJsExpression ']' -JsObjectMemberName = JsStaticMemberName | JsComputedMember +AnyJsObjectMemberName = AnyJsStaticMemberName | JsComputedMember // members = properties in Rome classic JsObjectExpression = '{' - members: (JsObjectMember (',' JsObjectMember)* ','?) + members: (AnyJsObjectMember (',' AnyJsObjectMember)* ','?) '}' // RSLint -JsObjectMember = JsMethodObjectMember // Method, shared with class (allows static, object doesn't allow private names, trailing comma) +AnyJsObjectMember = JsMethodObjectMember // Method, shared with class (allows static, object doesn't allow private names, trailing comma) | JsGetterObjectMember // Getter, shared with class | JsSetterObjectMember // Setter, shared with class | JsPropertyObjectMember // LiteralProp? @@ -523,37 +522,37 @@ JsObjectMember = JsMethodObjectMember // Method, shared with class (al | JsUnknownMember JsPropertyObjectMember = - name: JsObjectMemberName + name: AnyJsObjectMemberName ':' - value: JsExpression + value: AnyJsExpression JsMethodObjectMember = 'async'? '*'? - name: JsObjectMemberName + name: AnyJsObjectMemberName parameter_list: JsParameterList body: JsFunctionBody JsGetterObjectMember = 'get' - name: JsObjectMemberName + name: AnyJsObjectMemberName '(' ')' body: JsFunctionBody JsSetterObjectMember = 'set' - name: JsObjectMemberName - '(' value: JsParameter ')' + name: AnyJsObjectMemberName + '(' value: AnyJsParameter ')' body: JsFunctionBody // --------------------- MODULES --------------------- JsExport = - 'export' declaration: JsExportDeclaration + 'export' declaration: AnyJsExportDeclaration ';'? -JsExportDeclaration = JsFunctionDeclaration | JsClassDeclaration | JsVariableDeclarationStatement +AnyJsExportDeclaration = JsFunctionDeclaration | JsClassDeclaration | JsVariableDeclarationStatement JsExportFrom = 'export' @@ -577,7 +576,7 @@ JsExportDefault = JsExportDefaultArgument = JsClassDeclaration | JsFunctionDeclaration | JsExportDefaultExpressionArgument // You may be tempted to use `ExpressionStatement` here. Don't! the `DefaultExpressionArgument` would appear // wherever the user queries for all expression statements which isn't what we want. -JsExportDefaultExpressionArgument = expression: JsExpression ';'? +JsExportDefaultExpressionArgument = expression: AnyJsExpression ';'? JsExportAllFrom = 'export' @@ -593,7 +592,7 @@ JsImportModule = JsImport = 'import' - clause: JsImportClause + clause: AnyJsImportClause 'from' module_specifier: JsStringLiteral ';'? @@ -604,13 +603,13 @@ JsImport = // - namespace: import * from 'a'; // - default + namespace: import x, * as a from 'a' // - default + named: import x, { a } from 'a' -JsImportClause = JsImportDefaultBinding | JsNamespaceImportClause | JsNamedImportClause +AnyJsImportClause = JsImportDefaultBinding | JsNamespaceImportClause | JsNamedImportClause // Rome wraps the local name in JSImportSpecifierLocal JsImportDefaultBinding = local_name: JsBindingIdentifier // The comma is required if followed by a {} but otherwise forbidden. Introducing another node just to - // enforce this in namspace import clause / named import clause feels overkill + // enforce this in namespace import clause / named import clause feels overkill trailing_comma: ','? // Flatten the 'as' + name or use 'JsImportBinding'? @@ -636,7 +635,7 @@ JsImportBinding = JsImportCall = 'import' '(' - argument: JsExpression + argument: AnyJsExpression ')' // --------------------- Bindings --------------------- @@ -647,34 +646,34 @@ JsImportCall = // let { x, y: [a] } = { x: 1, y: [0] }, // x, introduces the new bindings x and a // ``` -JsBinding = JsObjectBinding | JsArrayBinding | JsBindingIdentifier | JsUnknownBinding +AnyJsBinding = JsObjectBinding | JsArrayBinding | JsIdentifierBinding | JsUnknownBinding JsDefaultValueClause = '=' - value: JsExpression + value: AnyJsExpression // let x = OR function(test) {} // ^ ^^^^ -JsBindingIdentifier = name: 'js_identifier' +JsIdentifierBinding = name: 'js_identifier' // let [a, , b] = test // ^^^^^^^^ JsArrayBinding = '[' - elements: (JsArrayBindingElement (',' JsArrayBindingElement)* ','?) + elements: (AnyJsArrayBindingElement (',' AnyJsArrayBindingElement)* ','?) rest: JsArrayRestBinding? ']' -JsArrayBindingElement = JsBindingWithDefault | JsArrayHole +AnyJsArrayBindingElement = JsBindingWithDefault | JsArrayHole // Used in function parameters or for array elements in array bindings JsBindingWithDefault = - binding: JsBinding + binding: AnyJsBinding default_value: JsDefaultValueClause JsArrayRestBinding = '...' - binding: JsBinding + binding: AnyJsBinding // let {a, b, ...rest } = @@ -697,9 +696,9 @@ JsShorthandPropertyBinding = // let { x: a } or let { x: a = "test" } // JsDestructuredPropertyBinding JsPropertyBinding = - member: JsMember + member: AnyJsMember ':' - binding: JsBinding + binding: AnyJsBinding default_value: JsDefaultValueClause? @@ -708,9 +707,9 @@ JsObjectRestBinding = '...' binding: JsBindingIdentifier // --------------------- Assignment targets --------------------- -JsSimpleAssignmentTarget = JsMemberAssignmentTarget | JsIdentifierAssignmentTarget | JsUnknownAssignmentTarget +AnyJsSimpleAssignmentTarget = JsMemberAssignmentTarget | JsIdentifierAssignmentTarget | JsUnknownAssignmentTarget -JsAssignmentTarget = JsSimpleAssignmentTarget | JsArrayAssignmentTarget | JsObjectAssignmentTarget +AnyJsAssignmentTarget = AnyJsSimpleAssignmentTarget | JsArrayAssignmentTarget | JsObjectAssignmentTarget // b = "test" // ^ @@ -719,36 +718,36 @@ JsIdentifierAssignmentTarget = name: 'js_identifier' // a.b = a['b'] = "test" // ^^^ ^^^^^^ JsMemberAssignmentTarget = - object: JsExpression - member: JsMember + object: AnyJsExpression + member: AnyJsMember // [a, {b}, ...rest] = test // ^^^^^^^^^^^^^^^^^ JsArrayAssignmentTarget = '[' - elements: (JsArrayAssignmentTargetElement (',' JsArrayAssignmentTargetElement)* ','?) + elements: (AnyJsArrayAssignmentTargetElement (',' AnyJsArrayAssignmentTargetElement)* ','?) rest: JsArrayAssignmentRest? ']' -JsArrayAssignmentTargetElement = JsAssignmentTargetWithDefault | JsArrayHole +AnyJsArrayAssignmentTargetElement = JsAssignmentTargetWithDefault | JsArrayHole JsAssignmentTargetWithDefault = - target: JsAssignmentTarget + target: AnyJsAssignmentTarget default_value: JsDefaultValueClause JsArrayAssignmentRest = '...' - target: JsAssignmentTarget + target: AnyJsAssignmentTarget // {a, b: x, ...rest} = // ^^^^^^^^^^^^^^^^^^ JsObjectAssignmentTarget = '{' - properties: (JsPropertyAssignmentTarget (',' JsPropertyAssignmentTarget)* ','?) + properties: (AnyJsPropertyAssignmentTarget (',' AnyJsPropertyAssignmentTarget)* ','?) rest: JsObjectRestAssignmentTarget? '}' -JsPropertyAssignmentTarget = JsShorthandPropertyAssignmentTarget | JsObjectPropertyAssignmentTarget +AnyJsPropertyAssignmentTarget = JsShorthandPropertyAssignmentTarget | JsObjectPropertyAssignmentTarget // { x } or { x = "test" } JsShorthandPropertyAssignmentTarget = @@ -758,19 +757,19 @@ JsShorthandPropertyAssignmentTarget = // let { x: a } or let { x: a = "test" } // JsDestructuredPropertyBinding JsObjectPropertyAssignmentTarget = - member: JsMember + member: AnyJsMember ':' - target: JsAssignmentTarget + target: AnyJsAssignmentTarget default_value: JsDefaultValueClause? -JsObjectRestAssignmentTarget = '...' target: JsSimpleAssignmentTarget +JsObjectRestAssignmentTarget = '...' target: AnyJsSimpleAssignmentTarget // --------------------- Auxilary --------------------- // The hole token will always be missing. Needed to make ungrammar happy JsArrayHole = hole: ''? -JsStatementOrExpression = JsStatement | JsExpression +JsStatementOrExpression = AnyJsStatement | AnyJsExpression // We can implement various helpers on the function union to replace `FunctionHead` (without paying for the abstraction) JsFunction = JsFunctionDeclaration @@ -779,24 +778,24 @@ JsFunction = JsFunctionDeclaration JsParameterList = '(' - parameters: (JsParameter (',' JsParameter)* ','?) + parameters: (AnyJsParameter (',' AnyJsParameter)* ','?) rest: JsRestParameter? ')' -JsParameter = JsBinding | JsBindingWithDefault +AnyJsParameter = AnyJsBinding | JsBindingWithDefault -JsRestParameter = '...' binding: JsBinding +JsRestParameter = '...' binding: AnyJsBinding JsArgumentList = '(' - arguments: (JsArgument (',' JsArgument)* ','?) + arguments: (AnyJsArgument (',' AnyJsArgument)* ','?) ')' -JsArgument = JsExpression | JsSpread +AnyJsArgument = AnyJsExpression | JsSpread JsIdentifier = name: 'js_identifier' JsSpread = - '...' argument: JsExpression + '...' argument: AnyJsExpression