Cleanup v1 AST #1568

chancancode · 2024-02-23T23:44:49Z

See the commit message for each individual commits

packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts

chancancode · 2024-02-24T00:17:54Z

packages/@glimmer/syntax/lib/v1/visitor-keys.ts

-  // v2 new nodes
-  NamedBlock: ['attributes', 'modifiers', 'children', 'comments'],
-  SimpleElement: ['attributes', 'modifiers', 'children', 'comments'],
-  Component: ['head', 'attributes', 'modifiers', 'children', 'comments'],


I "ensured these stays in sync with typing" as instructed by the comment on line 3

I don't think these are meant to be here, v2 nodes won't appear in the v1 AST and defining visitors for them doesn't do anything, and in any case, these aren't even in sync with what is currently in the "v2" AST

NullVoxPopuli · 2024-02-26T01:44:29Z

packages/@glimmer/syntax/lib/generation/printer.ts

@@ -320,7 +320,7 @@ export default class Printer {
      return;
    }

-    this.buffer += mustache.escaped ? '{{' : '{{{';
+    this.buffer += mustache.trusting ? '{{{' : '{{';


good change

NullVoxPopuli · 2024-02-26T01:47:18Z

packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts

  }

-  if (options && options.plugins && options.plugins.ast) {
+  if (options.plugins && options.plugins.ast) {


Suggested change

if (options.plugins && options.plugins.ast) {

if (options.plugins?.ast) {

🤷

NullVoxPopuli · 2024-02-26T01:50:59Z

packages/@glimmer/syntax/lib/traversal/walker.ts

      case 'ElementNode':
-        return visitors.ElementNode(this, node, callback);
+        walkBody(this, node.children, callback);
+        return;
      case 'BlockStatement':


TIL:

{{#if a}} hi {{else if b}} hello {{else}} hola {{/if}}

everything that's not the first block is in "inverse"

except that the second if is a nested block statement and the else is the inverse of the second block statementn

kinda goofy 🙈

NullVoxPopuli · 2024-02-26T02:04:22Z

packages/@glimmer/syntax/test/parser-node-test.ts

@@ -285,47 +343,36 @@ test('Element modifiers', () => {
  );
 });

-test('Element paths', (assert) => {


I didn't see a replacement test for this scenario -- why was the test removed? (or did I miss an equiv test?)

NullVoxPopuli · 2024-02-26T02:04:45Z

packages/@glimmer/syntax/test/loc-node-test.ts

-test('html elements with paths', () => {
-  let ast = parse(`
-    <Foo as |bar|>
-      <bar.x.y class='bar'/>


I didn't see a replacement test for this scenario -- why was the test removed? (or did I miss an equiv test?)

Because I only pushed the part one of two (the block statement part) not the element one so far

`{ type: "Program" }` isn't a real AST node you actually encounter, it was inherited from the Handlebars AST, but it has been split into AST.Template and AST.Block in Glimmer for a long time now. More throughly deprecate the `Program` visitor and builders. Also rename `blockParams` on `Template` to `locals`, and make both the array and the property immutable. We are only providing this information so that AST consumers can make decisions on what names are referring to outer local variables, but the compiler does not refer to it at all, and always use the original `options.locals` as the source of truth, so if any AST plugin tried to mutate this, it would not have worked. Keeping `blockParams` around as a deprecated alias for the time being. Consumers that need to support older versions should be able to feature detect this (such as with `"locals" in node`, or even just short-circuiting `node.locals ?? node.blockParams` to avoid triggering the deprecation warning.

These were marked as `@deprecated` in the code for a long time, but we never actaully issued deprecation warnings at runtime. These features were available for a very long time, so consumers shouldn't have a problem switching to the recommended properties at this point.

Note that this is referring to the Handlebars `{{> ... }}` syntax, not the (also defunct) Ember `{{partial ...}}` feature. This had never worked in any modern versions of Ember (maybe v1+) and the compiler doesn't do anything with it.

This reverts commit c7f954b, reversing changes made to e934005.

This reverts commit e934005, reversing changes made to 648ce18.

`PathHead` is not a real AST node, its visitor will never be called

Other than mustache, none of the other "callable" nodes allow a literal in the "callee" (node.path) position: {{log ("hi")}} ~~~~ illegal <div {{"hi"}} /> ~~~~ illegal {{#"hi"}}...{{/"hi"}} ~~~~ illegal Further more, in mustache positions this is only allowed if there are no other arguments: {{"hi" 1 2 3}} ~~~~ illlegal Most of these syntaxes already generate syntax errors in the parser but is allowed in the v1 AST, so it's possible for a AST plugin to force these illegal conditions in a transform but should ultimately get caught and rejected elsewhere. This commit fixes the type to reflect that these conditions are illegal/impossible and make sure we catch all of these edge cases in the v2 normalization step.

These probably once existed to normalize across all the possible nodes that call go into a `mustache.path` position, such that you can always do `node.path.original` on a mustache. In my opinion, this is of dubious utility in the first place – the `original` property on these can have a variety of different types, it's unclear what you would do with the generic value you acquired in this manner. Code like `if (node.path.original === "component")` will likely misclassify `{{"component"}}` as `{{component}}`. Further more, with the introduction of the `{{(sexp)}}` syntax, the original assumption no longer holds anyway, as sub-expression nodes do not have an `original` property. Deprecating this property and turning it into a getter/setter for `value`, ensuring they stay in sync. In the future, we may want to reclaim this original property to store the _actual_ original syntax in the form of a string. For example, today, if you had `{{1.0}}` in the source code, it gets parsed into the JavaScript number/value `1` and the original "raw" format is irrecoverably lost, which is a problem for printer and codemods type use cases.

The parser builders are the defacto constructors for the v1 AST nodes. As the nodes get increasingly more eloborate for backwards compatibility and such, it's important that we share the logic for constructing these nodes as much as possible so we don't miss any cases. Also standardize the signatures to use the object argument style, you should be able to clone a node by passing it back into the same builder method which it originated from.

This makes printing-esq tasks easier when working with `path.head` and restores feature parity with `path.original` which is a very common way to work with PathExpression nodes

The original commit has a few issues: - Type safety - Added a new AST node, which is unnecessary since we already have `VarHead`, but is also a breaking change and is kind of breakage we have been careful to avoid in this area thus far - The parsing is robust against a few classes of edge cases This rework the same feature (on the `Block`/`BlockStatement` side) and adds more test coverage for the feature. Also renamed the field to `block.params`.

The previous/existing "parsing" code tries to piggy back on letting the HTML tokenizer/parser parse out the block params syntax into attribute nodes and tries to recover the information after the fact This is fundamentally broken and suffers from some wild edge cases like: ``` <div as="" |foo="bar|>...</div> // => <div as |foo|>...</div> ``` This is especially problematic in light of wanting to retain the source spans of the block params nodes. This commit rework the block params parsing and integrate it into the normal parsing pass in the appropiate time.

Provide source spans for the open and close tags without adding new AST nodes, also parse the element's tag name into a path which has the required source span.

patricklx · 2024-02-27T22:20:40Z

I'm liking the changes. Much better to do it at tokenizer/parser level

It turns out to be important that AST plugins can mutate the list of local variables in the root `Template` node. Instead, use *that* list of local variables as the ultimate source of truth after AST plugin ran.

The rename is motivated by: 1. It's confusing for the root node to have "block params" 2. All other AST nodes that has `blockParams` also has a `params` field that exposes the source location as `VarHead`s There is some uncertainty in: 1. Whether there are a lot of existing AST consumers that uses it (shouldn't be a big deal in itself, as it's just a deprecation) 2. Whether the direction of using `Template.locals` as the source of truth for outside local variables will ultimately prevail When we have more certainty on those and if this rename is still desired, it should be possible to revert this commit

The refactor in #1568 slightly changed the semantics of `path.parts` in that it didn't previously include `this` or the leading `@`. This commit restores the previous semantics.

chancancode force-pushed the ast-refactor branch from 330baf9 to 05d8bdc Compare February 23, 2024 23:55

chancancode commented Feb 23, 2024

View reviewed changes

packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts Show resolved Hide resolved

chancancode force-pushed the ast-refactor branch from 05d8bdc to 69a11b1 Compare February 24, 2024 00:14

chancancode commented Feb 24, 2024

View reviewed changes

chancancode force-pushed the ast-refactor branch 5 times, most recently from 584ef8a to eee6eea Compare February 24, 2024 05:32

chancancode changed the title ~~Deprecate Program in v1 AST~~ v1 AST refactors Feb 24, 2024

chancancode changed the title ~~v1 AST refactors~~ Cleanup v1 AST Feb 24, 2024

chancancode force-pushed the ast-refactor branch 3 times, most recently from 0cfb213 to 342810c Compare February 25, 2024 21:43

NullVoxPopuli reviewed Feb 26, 2024

View reviewed changes

chancancode added 11 commits February 25, 2024 19:40

Add .tsbuildinfo to .gitignore

d3ee00c

Drop the defunct PartialStatementNode from v1 AST

5d52308

Note that this is referring to the Handlebars `{{> ... }}` syntax, not the (also defunct) Ember `{{partial ...}}` feature. This had never worked in any modern versions of Ember (maybe v1+) and the compiler doesn't do anything with it.

Revert "Merge pull request #1552 from patricklx/param-node"

db16ab9

This reverts commit c7f954b, reversing changes made to e934005.

Revert "Merge pull request #1553 from patricklx/html-tag-split"

6f5b20f

This reverts commit e934005, reversing changes made to 648ce18.

Use SourceSpan consistently

f018137

Sync v1 AST nodes with visitor-keys

cd71711

`PathHead` is not a real AST node, its visitor will never be called

chancancode added 2 commits February 25, 2024 22:27

Add v1 AST PathHead.original

6070862

This makes printing-esq tasks easier when working with `path.head` and restores feature parity with `path.original` which is a very common way to work with PathExpression nodes

chancancode force-pushed the ast-refactor branch from 342810c to 3104daf Compare February 26, 2024 06:42

chancancode force-pushed the ast-refactor branch from 78d98b6 to 13aeaa0 Compare February 27, 2024 07:13

chancancode mentioned this pull request Feb 27, 2024

add modifiers to debug render tree #1559

Merged

Second attempt at landing #1553

68509ac

Provide source spans for the open and close tags without adding new AST nodes, also parse the element's tag name into a path which has the required source span.

chancancode added 2 commits March 4, 2024 20:16

Revert making Template.locals readonly

31cc612

It turns out to be important that AST plugins can mutate the list of local variables in the root `Template` node. Instead, use *that* list of local variables as the ultimate source of truth after AST plugin ran.

wycats merged commit 38366b3 into main Mar 7, 2024
6 checks passed

wycats deleted the ast-refactor branch March 7, 2024 16:30

NullVoxPopuli added internal enhancement and removed internal labels Mar 8, 2024

chancancode mentioned this pull request Mar 11, 2024

use new glimmer ElementNode sub nodes typed-ember/glint#696

Open

chancancode mentioned this pull request Mar 28, 2024

[BUGFIX] Ensure legacy path.parts matches existing semantics #1583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup v1 AST #1568

Cleanup v1 AST #1568

chancancode commented Feb 23, 2024 •

edited

chancancode Feb 24, 2024

NullVoxPopuli Feb 26, 2024

NullVoxPopuli Feb 26, 2024

NullVoxPopuli Feb 26, 2024 •

edited

NullVoxPopuli Feb 26, 2024

NullVoxPopuli Feb 26, 2024

chancancode Feb 26, 2024

patricklx commented Feb 27, 2024

	if (options.plugins && options.plugins.ast) {
	if (options.plugins?.ast) {

Cleanup v1 AST #1568

Cleanup v1 AST #1568

Conversation

chancancode commented Feb 23, 2024 • edited

chancancode Feb 24, 2024

Choose a reason for hiding this comment

NullVoxPopuli Feb 26, 2024

Choose a reason for hiding this comment

NullVoxPopuli Feb 26, 2024

Choose a reason for hiding this comment

NullVoxPopuli Feb 26, 2024 • edited

Choose a reason for hiding this comment

NullVoxPopuli Feb 26, 2024

Choose a reason for hiding this comment

NullVoxPopuli Feb 26, 2024

Choose a reason for hiding this comment

chancancode Feb 26, 2024

Choose a reason for hiding this comment

patricklx commented Feb 27, 2024

chancancode commented Feb 23, 2024 •

edited

NullVoxPopuli Feb 26, 2024 •

edited