Invalid parse after use of <template> as expression

[ef4]

Given the file:

import Component from '@glimmer/component';

console.log(<template></template>);

export class A extends Component {
  <template>Hello</template>
}

Glint's parse ends up off:

host/tests/integration/components/example-test.gts:6:3 - error TS1068: Unexpected token. A constructor, method, accessor, or property was expected.

6   <template>Hello</template>
    ~

(more errors follow that one, but they all seem to be fallout from the parser being off)

If I comment out the the console.log line, the whole thing parses and type checks OK.

The runtime is working fine, so I don't think this is a problem with ember-template-imports. The code it emits looks correct. This is only a type-checking problem.

[chriskrycho]

Ohhhh, interesting. Thank you!

[ef4]

I figured out how to extract the transformed intermediate representation, which makes the syntax error visible:

import Component from '@glimmer/component';

console.log(({} as typeof import("glint-environment-ember-template-imports/-private/dsl")).template(function(𝚪, χ: typeof import("glint-environment-ember-template-imports/-private/dsl")) {
  𝚪; χ;
}));

export class A extends Component {
  <static { ({} as typeof import("glint-environment-ember-template-imports/-private/dsl")).template(function(𝚪: import("glint-environment-ember-template-imports/-private/dsl").ResolveContext<A>, χ: typeof import("glint-environment-ember-template-imports/-private/dsl")) {
  𝚪; χ;
}) as unknown }
}

Looks like an off-by-one error during the handling of the first template leaves a stray < character before the static {} block of the second template.

Edit to add: for completeness and my own notes, the preprocessed-but-not-yet-transformed version is:

import Component from '@glimmer[/component]()';

console.log([___T``]);

export class A extends Component {
  [___T`Hello`]
}

[ef4]

I understand better what is failing here. I think it's probably also causing #263.

The bug starts with this code, which is doing a risky thing because the positions that it's assigning are positions in the original source code. But the node they're assigning them on is about to get appended to an AST representing the "preprocessed-but-not-yet-transformed" form that I pasted above.

Now, this might still seem OK because later when we call node.getStart(), we will use the resulting value to index into the original code. If the pos that we set reliably came back out as getStart(), all would be well.

But getStart() is trying to be smart. It tries to skip over trivia. But it looks for trivia in its own document, not the original source. In our example, it happens to be looking at a newline that appears later in the file, so it shifts its answer by one, resulting in the wrong index into the original source.

[ef4]

Ideas for the fix:

1. We could use node.pos and node.end in place of node.getStart() and node.getEnd(). I checked and that does solve my example. However, it also results in a lot of test failures, and I can't tell yet if those are trivial changes to output or meaningful ones that are actually wrong.

2. We could stop trying to use pos and getStart to convey this information, instead using something like setEmitMeta. This is safer because we're not trying to pass "wrong" position info on a node where it doesn't match the document.

3. We could layer the transformations more strictly, such that calculateTransformedSource is patching the preprocessed source, not the original source. This would allow us to use "correct" position information on the nodes, since we'd be using the positions to index into the same source code that the AST represents. But maybe there is something lossy about the preprocessing that makes this a bad idea?

[dfreeman]

Thank you for the thorough investigation here, @ef4! Based on the timestamps I'm afraid I might have been able to save you a few hours of spelunking if I'd seen this sooner, but ultimately it's probably a good thing to have some of the sins I'm committing in glint-environment-ember-template-imports to be brought to light 🙂

In the near term, I think your idea 2 is likely the most straightforward route to fixing this. That's likely how propagating this location information would have been implemented in the first place had setEmitMeta existed as a concept during the initial prototyping, rather than being layered on later.

Longer-term I think we'll aim to eliminate most of this plumbing and coordination by making .gts support first-class in @glint/transform and removing some or all of the power for environments to perform custom transformations, but for the time being the plan is to hold on that until the format is a bit more specified.

[dfreeman]

I think it's probably also causing #263.

I don't think this bug is quite the cause of #263, but it's certainly related—both are the result of a fairly naive transition from using Babel for parsing to using TypeScript's own parser, without accounting for the differences in treatment of trivia you've outlined here.