Add toplevel await #149

sandersn · 2024-12-09T23:13:23Z

No incremental reparsing, although the non-incremental reparsing only reparses the statements that might contain await.
No TransformFlags, just an approximation in the parser.

I haven't added all the code to turn off PossibleTopLevelAwait because I'd like some agreement that this approach is good enough. I made enough changes to pass our test suite, in ClassDeclaration and ImportDeclaration parsing, but there are 16-17 more places to save/overwrite the toplevel await tracking bool.

That means in the current commit, for example, a toplevel FunctionDeclaration that contains await somewhere inside will reparse the FunctionDeclaration, even though the old compiler would not. It's possible that reparsing this doesn't change anything anyway, but it's inefficient to reparse it when it's not needed.

1. No incremental reparsing, although the non-incremental reparsing only reparses the statements that might contain await. 2. No TransformFlags, just an approximation in the parser. I haven't added all the code to turn off PossibleTopLevelAwait because I'm not convinced my approach is right. There are 16-17 more places to save/overwrite the toplevel await tracking bool. I made enough changes to pass our test suite, that's all. It's also possible that reparsing, say, a FunctionDeclaration, doesn't change anything anyway, but it's inefficient to reparse it when it's not needed.

internal/compiler/parser.go

sandersn · 2024-12-09T23:17:37Z

internal/compiler/parser.go

+}
+
+func (p *Parser) reparseTopLevelAwait(sourceFile *ast.SourceFile) *ast.Node {
+	statements := []*ast.Statement{}


this part is a simplified port from tsc. It's simplified because it drops the attempt to use the incremental parser (which doesn't exist in tsgo).

internal/compiler/parser.go

1. Move possible-await cache to Parser. 2. Ignore "await" identifiers in the same places that tsc does, but implemented in the parser instead. This also caches fewer statements in the possible-await cache than the previous commit.

sandersn · 2024-12-10T18:13:10Z

internal/compiler/parser.go


 func (p *Parser) parseClassDeclarationOrExpression(pos int, hasJSDoc bool, modifiers *ast.ModifierList, kind ast.Kind) *ast.Node {
 	saveContextFlags := p.contextFlags
+	saveHasAwaitIdentifier := p.statementHasAwaitIdentifier


these are all the save/restores that approximate TransformFlags propagation in tsc.

internal/compiler/parser.go

rbuckton · 2024-12-10T20:54:07Z

internal/compiler/parser.go

+}
+
+func (p *Parser) containsPossibleTopLevelAwait(node *ast.Node) bool {
+	return !(node.Flags&ast.NodeFlagsAwaitContext != 0) && p.getPossibleAwait(node)


Suggested change

return !(node.Flags&ast.NodeFlagsAwaitContext != 0) && p.getPossibleAwait(node)

return node.Flags&ast.NodeFlagsAwaitContext == 0 && p.getPossibleAwait(node)

Ugh, yes, I saw that as I pasted it and then forgot to go fix it.

internal/compiler/parser.go

internal/parser/parser.go

DanielRosenwasser · 2024-12-18T19:05:09Z

That means in the current commit, for example, a toplevel FunctionDeclaration that contains await somewhere inside will reparse the FunctionDeclaration, even though the old compiler would not. It's possible that reparsing this doesn't change anything anyway, but it's inefficient to reparse it when it's not needed.

That might actually have some effects on things. What errors do you issue on the following cases?

// @filename: a.ts
function foo(x = await(10)) {
}

// @filename: b.ts
async function bar(x = await(10)) {
}

// @filename: c.ts
export {};
export function fooExported(x = await(10)) {
}

// @filename: d.ts
export {};
export async function barExported(x = await(10)) {
}

Weirdly, we don't have any test case where await in a parameter initializer is syntactically ambiguous as both an AwaitExpression and as a CallExpression like in these examples. Would you be able to add it?

DanielRosenwasser

I haven't really looked at the diagnostic stitching code yet, but I've left some comments on style and how we run through the statement list that you might want to consider.

internal/parser/parser.go

Instead of tracking await-possible statements and searching to reconstruct spans, build spans during parsing. Then reparsing can read the spans directly, which simplifies code quite a bit. The downside is that I need to provide indices in parseList. Because Go isn't Javascript, I decided to copy parseList to parseListIndex. Two other options: change parseList to always provide indices then - Add ignored indices to parseStatement et al. - Add a parseList that passes an adapter func which ignores indices. I don't have a strong opinion about which to use, although I lean slightly to adding a second parameter to parseStatement et al. The current code isn't tested because of the way AST baselines aren't in main. I'll push a followup commit with fixes.

sandersn · 2024-12-19T17:37:05Z

The new commit simplifies tracking a lot compared to Strada. Instead of tracking await-possible statements and searching to
reconstruct spans, it builds spans during parsing. Then reparsing can read the spans directly, which simplifies code quite a bit.

The downside is that I need to provide indices in parseList. Because Go isn't Javascript, I decided to copy parseList to parseListIndex. Two other options: change parseList to always provide indices then

Add ignored indices to parseStatement, parseHeritageClause et al.
Add a parseList that passes an adapter func which ignores indices.

If adapter funcs aren't optimised away, I'd say the current code is best. I tried adding _ int parameters to parseStatement, et al and didn't like it at all--normal calls to parseStatement now need to pass -1 and there are lots of those calls.

I've tested on our current test cases but I need to try Daniel's cases he suggested on this PR.

jakebailey · 2024-12-19T17:44:03Z

If adapter funcs aren't optimised away, I'd say the current code is best. I tried adding _ int parameters to parseStatement, et al and didn't like it at all--normal calls to parseStatement now need to pass -1 and there are lots of those calls.

You can largely assume that simple one-liners that aren't generic are inlineable. If you wanted to double check, there are flags that you can pass to the compiler that would show that information.

rbuckton · 2024-12-19T17:50:16Z

The downside is that I need to provide indices in parseList.

Since parseTopLevelStatement is only ever called for statements at the top of a SourceFile, we could just have a topLevelStatementIndex field that we increment in parseTopLevelStatement, and reset it to 0 if we reuse a Parser

rbuckton · 2024-12-19T17:56:07Z

Weirdly, we don't have any test case where await in a parameter initializer is syntactically ambiguous as both an AwaitExpression and as a CallExpression like in these examples. Would you be able to add it?

Not Weirdly. await cannot be an AwaitExpression in a parameter initializer. Per https://tc39.es/ecma262/#sec-async-function-definitions-static-semantics-early-errors, it is a Syntax Error if the parameters contain AwaitExpression.

sandersn · 2024-12-19T18:06:38Z

@DanielRosenwasser re your test cases:

a: no (parse) errors, await is legal in a non-module (no reparsing either).
b. Same.
c. no (parse) errors, there is a reparse but it doesn't change anything. Not sure if this is legal in a module.
d. Same.

@rbuckton Are (c) and (d) correct? await(10) is parseable as a function call even in a module, but should it be parsed that way?

rbuckton · 2024-12-19T19:06:13Z

@rbuckton Are (c) and (d) correct? await(10) is parseable as a function call even in a module, but should it be parsed that way?

await will always be parsed in the Await context of the function:

(c) should be an error because a module is always parsed in strict mode, and await is illegal as an identifier in strict mode, per https://tc39.es/ecma262/#sec-identifiers-static-semantics-early-errors, though we could choose to parse it as an Identifier and issue a grammar error.

(d) should be an error because it will be parsed as an AwaitExpression, but would be a syntax error per https://tc39.es/ecma262/#sec-async-function-definitions-static-semantics-early-errors, which we could also treat as a grammar error.

There are two other scenarios to consider:

// @filename: e.ts
function fooStrict(x = await(10)) {
  "use strict"
}

// @filename: f.ts
(function () {
  "use strict"
  function fooStrict(x = await(10)) {
  }
})()

These should be grammar errors because await is parsed as an Identifier, but fails https://tc39.es/ecma262/#sec-identifiers-static-semantics-early-errors since the function is strict-mode code.

Please note that V8 seems to have a bug here, as it incorrectly allows await as an identifier in strict-mode code, in violation of https://tc39.es/ecma262/#sec-identifiers-static-semantics-early-errors, though it correctly forbids yield as an identifier in strict mode per the same rule.

rbuckton · 2024-12-19T19:44:41Z

These should be grammar errors because await is parsed as an Identifier, but fails https://tc39.es/ecma262/#sec-identifiers-static-semantics-early-errors since the function is strict-mode code.

Sorry, this was an incorrect interpretation of 13.1.1. 13.1.1 only disallows await if the goal symbol is Module.

To clarify:

(a) should parse as a call expression for the id await
(b) should parse as an AwaitExpression, but error due to 15.8.1
(c) should parse as a call expression for the id await, but error due to 13.1.1 because we are parsing a Module.
(d) should parse as an AwaitExpression, but error due to 15.8.1
(e) and (f) should be ignored as they were based on a faulty interpretation of 13.1.1

sandersn · 2024-12-20T14:20:48Z

Ah, I see the difference. a,b,c,d,e,f all give grammar errors after parsing correctly. I think the PR's code is working the same as the old code -- by the end of parsing it produces exactly the same trees and exactly the same errors (none).

rbuckton · 2025-01-07T16:58:00Z

Ah, I see the difference. a,b,c,d,e,f all give grammar errors after parsing correctly. I think the PR's code is working the same as the old code -- by the end of parsing it produces exactly the same trees and exactly the same errors (none).

I take it the grammar errors for this are not yet ported in the checker, or are they missing/broken in both compilers?

sandersn · 2025-01-07T18:07:47Z

The grammar errors are present and correct in Strada.
Errors for (b) and (d) and two of (e)'s are ported. But this PR ports just the parser code.

jakebailey · 2025-01-07T18:51:03Z

internal/parser/parser.go

 	p.parseExpected(ast.KindImportKeyword)
 	afterImportPos := p.nodePos()
 	// We don't parse the identifier here in await context, instead we will report a grammar error in the checker.
+	saveHasAwaitIdentifier := p.statementHasAwaitIdentifier


I wish we didn't have all of this manual saving/restoring in favor of some sort of defer but...

DanielRosenwasser · 2025-01-07T18:59:34Z

internal/parser/parser.go

+			result.ScriptKind = p.scriptKind
+		}
+	}
+	p.possibleAwaitSpans = []int{}


Rather than an int array, you could make this a struct {} but I guess it's fine.

sandersn added 2 commits December 9, 2024 15:03

Merge branch 'main' into add-toplevel-await-notest

3c6e158

jakebailey reviewed Dec 9, 2024

View reviewed changes

internal/compiler/parser.go Show resolved Hide resolved

sandersn commented Dec 9, 2024

View reviewed changes

improve variable names

bd07a14

jakebailey reviewed Dec 9, 2024

View reviewed changes

internal/compiler/parser.go Outdated Show resolved Hide resolved

sandersn added 4 commits December 10, 2024 09:28

Address PR comments

2314f3d

1. Move possible-await cache to Parser. 2. Ignore "await" identifiers in the same places that tsc does, but implemented in the parser instead. This also caches fewer statements in the possible-await cache than the previous commit.

Merge branch 'main' into add-toplevel-await-notest

03c9200

hereby format

725609c

undo stray edit

4ba013b

sandersn commented Dec 10, 2024

View reviewed changes

sandersn requested a review from rbuckton December 10, 2024 20:04

rbuckton reviewed Dec 10, 2024

View reviewed changes

switch possible-await statements to Set+remove sync

f9f9fcf

jakebailey reviewed Dec 10, 2024

View reviewed changes

internal/compiler/parser.go Outdated Show resolved Hide resolved

address PR comments

fbc5d49

jakebailey reviewed Dec 12, 2024

View reviewed changes

internal/compiler/parser.go Outdated Show resolved Hide resolved

sandersn added 2 commits December 13, 2024 06:42

Merge branch 'main' into add-toplevel-await-notest

6bd9284

clear possibleAwaitStatement on every parse

e910e89

DanielRosenwasser reviewed Dec 13, 2024

View reviewed changes

internal/parser/parser.go Outdated Show resolved Hide resolved

Merge branch 'main' into add-toplevel-await-notest

7c7dafc

DanielRosenwasser reviewed Dec 18, 2024

View reviewed changes

internal/parser/parser.go Outdated Show resolved Hide resolved

internal/parser/parser.go Outdated Show resolved Hide resolved

sandersn added 2 commits December 18, 2024 14:06

Merge branch 'main' into add-toplevel-await-notest

a5411aa

sandersn added 2 commits December 19, 2024 10:09

parseList delegates to parseListIndex

d1bc536

Merge branch 'main' into add-toplevel-await-notest

2dc8eb1

rbuckton approved these changes Jan 7, 2025

View reviewed changes

jakebailey approved these changes Jan 7, 2025

View reviewed changes

DanielRosenwasser reviewed Jan 7, 2025

View reviewed changes

DanielRosenwasser approved these changes Jan 7, 2025

View reviewed changes

sandersn merged commit 6f1526c into microsoft:main Jan 7, 2025
12 checks passed

sandersn deleted the add-toplevel-await-notest branch January 7, 2025 19:19

jakebailey mentioned this pull request Jan 27, 2025

Top-level await doesn't parse #135

Closed

	return !(node.Flags&ast.NodeFlagsAwaitContext != 0) && p.getPossibleAwait(node)
	return node.Flags&ast.NodeFlagsAwaitContext == 0 && p.getPossibleAwait(node)

Add toplevel await #149

Add toplevel await #149

Uh oh!

Conversation

sandersn commented Dec 9, 2024

Uh oh!

Uh oh!

sandersn Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sandersn Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rbuckton Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

sandersn Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielRosenwasser commented Dec 18, 2024

Uh oh!

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sandersn commented Dec 19, 2024

Uh oh!

jakebailey commented Dec 19, 2024

Uh oh!

rbuckton commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Dec 19, 2024

Uh oh!

rbuckton commented Dec 19, 2024

Uh oh!

rbuckton commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jan 7, 2025

Uh oh!

sandersn commented Jan 7, 2025

Uh oh!

jakebailey Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rbuckton commented Dec 19, 2024 •

edited

Loading

rbuckton commented Dec 19, 2024 •

edited

Loading

rbuckton commented Dec 19, 2024 •

edited

Loading

sandersn commented Dec 20, 2024 •

edited

Loading