AST naming convention

[ematipico]

This is a follow up of #1868

Now that we have a good coverage of the JS syntax and we managed to express all the syntax, I wanted to open a small discussion to propose some changes to the current names of our AST nodes.

Here's a list of the AST nodes that I found incorrect/incoherent and a possible proposal of change/question:

• JsUnknownImportAssertionEntry
• JsUnknownNamedImportSpecifier
  This should have Import at the end;

---

• JsElseClause
• JsAnySwitchClause
• JsDefaultClause
  Probably these are exceptions? Should we add Auxiliary at the end?

---

• JsCatchDeclaration
• JsVariableDeclaration
• JsForVariableDeclaration
  We don't have the Declaration conception. Should it be Statement? Or Auxiliary?

---

• JsSpread
• JsArrayHole
  Should we add Auxiliary at the end?

---

• JsReferenceIdentifier
  Should this be Binding at the end?

---

• JsAssignmentWithDefault
  Maybe this should become JsWithDefaultAssignment?

---

• JsArrayAssignmentPatternRestElement
• JsObjectAssignmentPatternShorthandProperty
• JsObjectAssignmentPatternProperty
• JsObjectAssignmentPatternRest
• JsArrayBindingPatternRestElement
  Are these Assignment or Pattern? They should have their concept at the end of their name.

---

• JsObjectBindingPatternProperty
• JsObjectBindingPatternRest
• JsObjectBindingPatternShorthandProperty
  Probably, we should move BindingPattern at the end`

---

• JsAnyModuleItem
  Probably, Module should be at the end.

---

• AnyJsImportClause
• JsImportBareClause
• JsImportNamedClause
• JsImportDefaultClause
• JsImportNamespaceClause
• TsImportRequireClause
• TsImportAliasClause
• JsImportAssertion
• JsDefaultImportSpecifier
  Probably, we should move Import at the end of their name.

---

• JsAnyExportClause
• JsExportFunctionClause
• JsExportDefaultFunctionClause
• JsExportClassClause
• JsExportDefaultClassClause
• JsExportVariableClause
• JsExportDefaultExpressionClause
• JsExportNamedClause
• JsExportFromClause
• JsExportNamedFromClause
  Probably, we should move Export at the end of their name

---

Maybe we should add Auxiliary at the end of each name? Just a proposal, I don't think

[MichaReiser]

Thanks @ematipico for going through all the names and assembling this list!

I'll create separate comments to make the discussion easier

Maybe we should add Auxiliary at the end of each name? Just a proposal, I don't think

I don't think it adds any clarity to people reading code using the AST nodes and just adds some more characters to type. It isn't important for authors/readers to know that this is an auxiliary node. This is different from the Statement postfix because it tells in which context the node is valid.

The overall rule is that the postfix specifies to what group a node belongs. For example:

• BinaryExpression: It's an expression
• JsUnknownImportAssertionEntry: It's an ImportAssertionEntry
• JsExportFunctionClause: It's an ExportClause but not an export (it's part of an export but not an export)

These go often along with a corresponding union:

• *Expression -> JsAnyExpression
• *AssertionEntry -> JsAnyAssertionEntry

[ematipico]

The thing is, we have too many concepts/groups at the moment, and I find personally difficult to understand when something is a group or when something might be a type/mistake. Should we document them somewhere? A discussion is not enough, I think. Maybe a README.md inside the parser crate would help.

In case of ImportAssertionEntry we have just one group. The formatter will generate only one folder with one node. Is it something that we want?

[ematipico]

Follow up: how do we know if something falls under a "group" when something is an auxiliary? ImportAssertionEntry is very niche. Having Import at the end will simplify the code generation.

[ematipico]

It isn't important for authors/readers to know that this is an auxiliary node

It's important for us developers, I think. If we make categories (expressions, statements) and elements (entry, item) etc. official, somewhere. This will help with:

• choosing a correct name for new nodes
• implement a code generation that is coherent with the official internal documentation
• understand which nodes are auxiliary which are following the naming convention

[MichaReiser]

In case of ImportAssertionEntry we have just one group. The formatter will generate only one folder with one node. Is it something that we want?

There are two things at play here. The postfix should, when possible, match the name of the union. However, there's no 1:1 mapping to groups. For example, I would group all nodes related to assignment into an assignment folder. That includes multiple nodes and unions.

Follow up: how do we know if something falls under a "group" when something is an auxiliary? ImportAssertionEntry is very niche. Having Import at the end will simplify the code generation.

I prefer more readable names over the complexity of the code generation. We'll write these names over and over again but the code generator only once. Ideally, we would be able to attribute the group names inside of the ungrammer file. For example, with a

/// @collection: statements
JsIfStatement = ... 

Having the match is just a simple way of achieving this today.

[MichaReiser]

• JsUnknownImportAssertionEntry
• JsUnknownNamedImportSpecifier

These are AssertionEntry and or ImportSpecifier and not imports. In my view, the naming is correct here.

[ematipico]

But isn't the category Import?

(copied from the original discussion)

Category = 'Expression' | 'Statement' | 'Type' | `BindingPattern` | etc
Element  = 'Element' | 'Member' | 'Clause' | 'Entry' | 'Specifier' | etc

Would you want to code gen the formatter by "element" too? In this case (let's not count the Any): assertion/entry/file.rs and import/specifier/file.rs

[MichaReiser]

• JsElseClause
• JsAnySwitchClause
• JsDefaultClause

JsDefaultClause and JsCaseClause could be renamed to JsDefaultSwitchClause and JsCaseSwitchClause. JsElseClause is just an auxiliary node

[MichaReiser]

• JsCatchDeclaration
• JsVariableDeclaration
• JsForVariableDeclaration

I wouldn't postfix these with Statements because they aren't statements. I'm open to other names but don't think they are problematic. These are just helper nodes that don't add up to any other collection.

[ematipico]

It seems to be a recurrent pattern. I think the name is fine

[MichaReiser]

• JsSpread
• JsArrayHole

Simply auxiliary nodes. Don't belong to a bigger scheme.

[MichaReiser]

JsReferenceIdentifier

This is an identifier referencing a name from the scope and not a binding of a new identifier. It's an auxiliary node that is necessary because there are places where an identifier references a name besides the ReferenceIdentifierExpression.

[MichaReiser]

JsAssignmentWithDefault
Maybe this should become JsWithDefaultAssignment?

Postfixing the node with Assignment makes the impression that the node itself is an assignment which isn't the case. The node wrapps an assignment with a default value, but the assignment is only the inner assignment.

[ematipico]

On the other hand, for me it's difficult to understand that this node is an auxiliary 🤔 From what I understood, it doesn't fall under any category.

Or, it might seems that it falls under the category AssignmentWithDefault.

[MichaReiser]

Agree, it's a one off node (together with the BindingWithDefault) to model assignments with a default inside arrays. However, it belongs to the assignment group because that's conceptually the closest.

[MichaReiser]

• JsArrayAssignmentPatternRestElement
• JsObjectAssignmentPatternShorthandProperty
• JsObjectAssignmentPatternProperty
• JsObjectAssignmentPatternRest
• JsArrayBindingPatternRestElement
• Are these Assignment or Pattern? They should have their concept at the end of their name.

These are neither assignments nor patterns but elements of an array or object assignment. But I agree, the names are a bit out of place:

• JsArrayAssignmentPatternRestElement -> JsRestArrayAssignmentPatternElement
• JsObjectAssignmentPatternShorthandProperty -> JsShorthandPropertyObjectAssignmentPatternMember
• JsObjectAssignmentPatternProperty -> JsPropertyObjectAssignmentPatternMember
• JsObjectAssignmentPatternRest -> JsRestObjectAssignmentPatternMember

But that might be a bit of a mouthful. Also, JsArrayHole doesn't match the naming exactly but just having another name to make the name match doesn't seem worth it.

• JsObjectBindingPatternProperty
• JsObjectBindingPatternRest
• JsObjectBindingPatternShorthandProperty
  Probably, we should move BindingPattern at the end`

Same as for assignment

[ematipico]

These are neither assignments nor patterns but elements of an array or object assignment

We need to find a way to mark these nodes as auxiliary. The carry the name of a concept and they are misleading. Should we just to add a comment in the ungram, which will mark them as auxiliary nodes?

[MichaReiser]

I fear that this might even be more misleading and you would need to mark most of the nodes in the grammar as auxiliary.

Why I think it's misleading is that I would still put these files into the assignment directory because they are only used by assignment nodes. However, there's also an auxilary directory for nodes like JsInitializerClause that are used by a number of different nodes that don't share any common concept.

I also wouldn't mark the "auxiliary" nodes that we put into the "auxiliary" directory with any sort of comment in the grammar (except the big AUXILARY title at the top) because I consider the match inside of the formatter codegen as an extension to the grammar, the same as kinds_src. Marking the nodes as auxiliary and inside the grammar file leads to duplication and the comments probably will come out of sync after some time.

I would propose to implement the match outside of the formatter codegen as a general utility function that maps a node name to a node group. This can then be used across multiple code gens.

[ematipico]

A developer that creates a new node needs to know where that node will end up. They might need to update the match inside the codegen too. Do you agree that we should have something inside a README.md that explains how to name new nodes inside a grammar and how to categories them? Some guidelines at least

The discussion created by Jamie is good, but it's a discussion, that eventually can be lost. Having it inside a markdown might be better.

[MichaReiser]

JsAnyModuleItem
Probably, Module should be at the end.

The items aren't a module but a ModuleItem. You could argue that JsImport and JsExport should be named JsImportModuleItem and JsExportModuleItem (or we rename it to JsAnyModuleStatement and these are then called JsImportModuleStatement or simply JsImportStatement). I personally would prefer to call JsImport JsImportStatement and simply tread JsAnyModuleStatement as a supertype of JsAnyStatement which it is

[MichaReiser]

• AnyJsImportClause
• JsImportBareClause
• JsImportNamedClause
• JsImportDefaultClause
• JsImportNamespaceClause
• TsImportRequireClause
• TsImportAliasClause
• JsImportAssertion
• JsDefaultImportSpecifier
• Probably, we should move Import at the end of their name.

Same for exports:

I think you're right that it would be more consistent if the Clauses are named JsBareImportClause, JsNamedImportClause to match with the union. However, I do think that the naming of JsImportAssertion and JsDefaultImportSpecifier is alright.

[Conaclos]

Some feedback about the AST:

• TsBigIntLiteralType and TsBigintType have not the same case (BigInt and Bigint). This should be normalized to one or the other.
• Why JsUndefinedLiteralExpression does not exist?

[jeysal]

1. Agreed about bigint consistency.
2. undefined is not a literal but a property of the globalThis object, afaik.

[Conaclos]

undefined is not a literal but a property of the globalThis object, afaik.

You are right, I thought that undefined was no longer a property of the global scope since ES6. ES6 just makes undefined immutable.