Skip to content

Commit 1b9d48f

Browse files
PerryvwPerryvw
authored
Fix promise then chaining and disallow using try/catch in 5.1 in async and generator functions (#1150)
* Make await throw if awaited thing is a rejected promise * fix almost all tests * Also fix lambas in async * Fix bug in try/catch adding extra return twice * Fix prettier * Add failing tests * Add diagnostic to prevent 5.1 users from trying to use try/catch in async function * Diagnostic for try/catch in generator or async function in 5.1 * Fix missing case in promise then
1 parent 8f70f50 commit 1b9d48f

File tree

8 files changed

+165
-28
lines changed

8 files changed

+165
-28
lines changed

‎src/lualib/Promise.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,16 @@ class __TS__Promise<T> implements Promise<T> {
7070
onFulfilled?: FulfillCallback<T, TResult1>,
7171
onRejected?: RejectCallback<TResult2>
7272
): Promise<TResult1 | TResult2> {
73-
const { promise, resolve, reject } = __TS__PromiseDeferred<TResult1 | TResult2>();
73+
const { promise, resolve, reject } = __TS__PromiseDeferred<T | TResult1 | TResult2>();
74+
75+
const isFulfilled = this.state === __TS__PromiseState.Fulfilled;
76+
const isRejected = this.state === __TS__PromiseState.Rejected;
7477

7578
if (onFulfilled) {
7679
const internalCallback = this.createPromiseResolvingCallback(onFulfilled, resolve, reject);
7780
this.fulfilledCallbacks.push(internalCallback);
7881

79-
if (this.state === __TS__PromiseState.Fulfilled) {
82+
if (isFulfilled) {
8083
// If promise already resolved, immediately call callback
8184
internalCallback(this.value);
8285
}
@@ -89,13 +92,23 @@ class __TS__Promise<T> implements Promise<T> {
8992
const internalCallback = this.createPromiseResolvingCallback(onRejected, resolve, reject);
9093
this.rejectedCallbacks.push(internalCallback);
9194

92-
if (this.state === __TS__PromiseState.Rejected) {
95+
if (isRejected) {
9396
// If promise already rejected, immediately call callback
9497
internalCallback(this.rejectionReason);
9598
}
9699
}
97100

98-
return promise;
101+
if (isFulfilled) {
102+
// If promise already resolved, also resolve returned promise
103+
resolve(this.value);
104+
}
105+
106+
if (isRejected) {
107+
// If promise already rejected, also reject returned promise
108+
reject(this.rejectionReason);
109+
}
110+
111+
return promise as Promise<TResult1 | TResult2>;
99112
}
100113
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch
101114
public catch<TResult = never>(onRejected?: (reason: any) => TResult | PromiseLike<TResult>): Promise<T | TResult> {

‎src/transformation/utils/typescript/nodes.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import * as ts from "typescript";
2+
import { findFirstNodeAbove } from ".";
23
import { TransformationContext } from "../../context";
34

45
export function isAssignmentPattern(node: ts.Node): node is ts.AssignmentPattern {
@@ -25,6 +26,26 @@ export function isInDestructingAssignment(node: ts.Node): boolean {
2526
);
2627
}
2728

29+
export function isInAsyncFunction(node: ts.Node): boolean {
30+
// Check if node is in function declaration with `async`
31+
const declaration = findFirstNodeAbove(node, ts.isFunctionLike);
32+
if (!declaration) {
33+
return false;
34+
}
35+
36+
return declaration.modifiers?.some(m => m.kind === ts.SyntaxKind.AsyncKeyword) ?? false;
37+
}
38+
39+
export function isInGeneratorFunction(node: ts.Node): boolean {
40+
// Check if node is in function declaration with `async`
41+
const declaration = findFirstNodeAbove(node, ts.isFunctionDeclaration);
42+
if (!declaration) {
43+
return false;
44+
}
45+
46+
return declaration.asteriskToken !== undefined;
47+
}
48+
2849
/**
2950
* Quite hacky, avoid unless absolutely necessary!
3051
*/

‎src/transformation/visitors/errors.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import * as ts from "typescript";
2+
import { LuaTarget } from "../..";
23
import * as lua from "../../LuaAST";
34
import { FunctionVisitor } from "../context";
5+
import { unsupportedForTarget } from "../utils/diagnostics";
46
import { createUnpackCall } from "../utils/lua-ast";
57
import { ScopeType } from "../utils/scope";
8+
import { isInAsyncFunction, isInGeneratorFunction } from "../utils/typescript";
69
import { transformScopeBlock } from "./block";
710
import { transformIdentifier } from "./identifier";
811
import { isInMultiReturnFunction } from "./language-extensions/multi";
@@ -11,6 +14,18 @@ import { createReturnStatement } from "./return";
1114
export const transformTryStatement: FunctionVisitor<ts.TryStatement> = (statement, context) => {
1215
const [tryBlock, tryScope] = transformScopeBlock(context, statement.tryBlock, ScopeType.Try);
1316

17+
if (context.options.luaTarget === LuaTarget.Lua51 && isInAsyncFunction(statement)) {
18+
context.diagnostics.push(unsupportedForTarget(statement, "try/catch inside async functions", LuaTarget.Lua51));
19+
return tryBlock.statements;
20+
}
21+
22+
if (context.options.luaTarget === LuaTarget.Lua51 && isInGeneratorFunction(statement)) {
23+
context.diagnostics.push(
24+
unsupportedForTarget(statement, "try/catch inside generator functions", LuaTarget.Lua51)
25+
);
26+
return tryBlock.statements;
27+
}
28+
1429
const tryResultIdentifier = lua.createIdentifier("____try");
1530
const returnValueIdentifier = lua.createIdentifier("____returnValue");
1631

‎src/transformation/visitors/return.ts‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
canBeMultiReturnType,
1515
} from "./language-extensions/multi";
1616
import { invalidMultiFunctionReturnType } from "../utils/diagnostics";
17-
import { findFirstNodeAbove } from "../utils/typescript";
17+
import { isInAsyncFunction } from "../utils/typescript";
1818

1919
function transformExpressionsInReturn(
2020
context: TransformationContext,
@@ -96,16 +96,6 @@ export function createReturnStatement(
9696
return lua.createReturnStatement(results, node);
9797
}
9898

99-
function isInAsyncFunction(node: ts.Node): boolean {
100-
// Check if node is in function declaration with `async`
101-
const declaration = findFirstNodeAbove(node, ts.isFunctionLike);
102-
if (!declaration) {
103-
return false;
104-
}
105-
106-
return declaration.modifiers?.some(m => m.kind === ts.SyntaxKind.AsyncKeyword) ?? false;
107-
}
108-
10999
function isInTryCatch(context: TransformationContext): boolean {
110100
// Check if context is in a try or catch
111101
let insideTryCatch = false;

‎test/unit/builtins/async-await.spec.ts‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { ModuleKind, ScriptTarget } from "typescript";
2-
import { awaitMustBeInAsyncFunction } from "../../../src/transformation/utils/diagnostics";
2+
import { LuaTarget } from "../../../src";
3+
import { awaitMustBeInAsyncFunction, unsupportedForTarget } from "../../../src/transformation/utils/diagnostics";
34
import * as util from "../../util";
45

56
const promiseTestLib = `
@@ -385,8 +386,9 @@ test("async function can forward varargs", () => {
385386

386387
// https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1105
387388
describe("try/catch in async function", () => {
388-
test("await inside try/catch returns inside async function", () => {
389-
util.testModule`
389+
util.testEachVersion(
390+
"await inside try/catch returns inside async function",
391+
() => util.testModule`
390392
export let result = 0;
391393
async function foo(): Promise<number> {
392394
try {
@@ -398,11 +400,17 @@ describe("try/catch in async function", () => {
398400
foo().then(value => {
399401
result = value;
400402
});
401-
`.expectToEqual({ result: 4 });
402-
});
403+
`,
404+
// Cannot execute LuaJIT with test runner
405+
{
406+
...util.expectEachVersionExceptJit(builder => builder.expectToEqual({ result: 4 })),
407+
[LuaTarget.Lua51]: builder => builder.expectToHaveDiagnostics([unsupportedForTarget.code]),
408+
}
409+
);
403410

404-
test("await inside try/catch throws inside async function", () => {
405-
util.testModule`
411+
util.testEachVersion(
412+
"await inside try/catch throws inside async function",
413+
() => util.testModule`
406414
export let reason = "";
407415
async function foo(): Promise<number> {
408416
try {
@@ -414,11 +422,19 @@ describe("try/catch in async function", () => {
414422
foo().catch(e => {
415423
reason = e;
416424
});
417-
`.expectToEqual({ reason: "an error occurred in the async function: test error" });
418-
});
425+
`,
426+
{
427+
...util.expectEachVersionExceptJit(builder =>
428+
builder.expectToEqual({ reason: "an error occurred in the async function: test error" })
429+
),
430+
[LuaTarget.Lua51]: builder => builder.expectToHaveDiagnostics([unsupportedForTarget.code]),
431+
}
432+
);
419433

420-
test("await inside try/catch deferred rejection uses catch clause", () => {
421-
util.testModule`
434+
util.testEachVersion(
435+
"await inside try/catch deferred rejection uses catch clause",
436+
() =>
437+
util.testModule`
422438
export let reason = "";
423439
let reject: (reason: string) => void;
424440
@@ -433,6 +449,12 @@ describe("try/catch in async function", () => {
433449
reason = e;
434450
});
435451
reject("test error");
436-
`.expectToEqual({ reason: "an error occurred in the async function: test error" });
437-
});
452+
`,
453+
{
454+
...util.expectEachVersionExceptJit(builder =>
455+
builder.expectToEqual({ reason: "an error occurred in the async function: test error" })
456+
),
457+
[LuaTarget.Lua51]: builder => builder.expectToHaveDiagnostics([unsupportedForTarget.code]),
458+
}
459+
);
438460
});

‎test/unit/builtins/promise.spec.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,48 @@ test("promise is instanceof promise", () => {
709709
util.testExpression`Promise.resolve(4) instanceof Promise`.expectToMatchJsResult();
710710
});
711711

712+
test("chained then on resolved promise", () => {
713+
util.testFunction`
714+
Promise.resolve("result1").then(undefined, () => {}).then(value => log(value));
715+
Promise.resolve("result2").then(value => "then1", () => {}).then(value => log(value));
716+
Promise.resolve("result3").then(value => undefined, () => {}).then(value => log(value ?? "undefined"));
717+
Promise.resolve("result4").then(value => "then2").then(value => [value, "then3"]).then(([v1, v2]) => log(v1, v2));
718+
719+
return allLogs;
720+
`
721+
.setTsHeader(promiseTestLib)
722+
.expectToEqual(["result1", "then1", "undefined", "then2", "then3"]);
723+
});
724+
725+
test("chained catch on rejected promise", () => {
726+
util.testFunction`
727+
Promise.reject("reason1").then(() => {}).then(v => log("resolved", v), reason => log("rejected", reason));
728+
Promise.reject("reason2").then(() => {}, () => "reason3").then(v => log("resolved", v));
729+
Promise.reject("reason4").then(() => {}, () => undefined).then(v => log("resolved", v ?? "undefined"));
730+
731+
return allLogs;
732+
`
733+
.setTsHeader(promiseTestLib)
734+
.expectToEqual(["rejected", "reason1", "resolved", "reason3", "resolved", "undefined"]);
735+
});
736+
737+
// Issue 2 from https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1105
738+
test("catch after then catches rejected promise", () => {
739+
util.testFunction`
740+
Promise.reject('test error')
741+
.then(result => {
742+
log("then", result);
743+
})
744+
.catch(e => {
745+
log("catch", e);
746+
})
747+
748+
return allLogs;
749+
`
750+
.setTsHeader(promiseTestLib)
751+
.expectToEqual(["catch", "test error"]);
752+
});
753+
712754
describe("Promise.all", () => {
713755
test("resolves once all arguments are resolved", () => {
714756
util.testFunction`

‎test/unit/functions/generators.spec.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { LuaTarget } from "../../../src/CompilerOptions";
2+
import { unsupportedForTarget } from "../../../src/transformation/utils/diagnostics";
13
import * as util from "../../util";
24

35
test("generator parameters", () => {
@@ -147,3 +149,22 @@ test("hoisting", () => {
147149
}
148150
`.expectToMatchJsResult();
149151
});
152+
153+
util.testEachVersion(
154+
"generator yield inside try/catch",
155+
() => util.testFunction`
156+
function* generator() {
157+
try {
158+
yield 4;
159+
} catch {
160+
throw "something went wrong";
161+
}
162+
}
163+
return generator().next();
164+
`,
165+
// Cannot execute LuaJIT with test runner
166+
{
167+
...util.expectEachVersionExceptJit(builder => builder.expectToMatchJsResult()),
168+
[LuaTarget.Lua51]: builder => builder.expectToHaveDiagnostics([unsupportedForTarget.code]),
169+
}
170+
);

‎test/util.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,19 @@ export function testEachVersion<T extends TestBuilder>(
6565
}
6666
}
6767

68+
export function expectEachVersionExceptJit<T>(
69+
expectation: (builder: T) => void
70+
): Record<tstl.LuaTarget, ((builder: T) => void) | boolean> {
71+
return {
72+
[tstl.LuaTarget.Universal]: expectation,
73+
[tstl.LuaTarget.Lua51]: expectation,
74+
[tstl.LuaTarget.Lua52]: expectation,
75+
[tstl.LuaTarget.Lua53]: expectation,
76+
[tstl.LuaTarget.Lua54]: expectation,
77+
[tstl.LuaTarget.LuaJIT]: false, // Exclude JIT
78+
};
79+
}
80+
6881
const memoize: MethodDecorator = (_target, _propertyKey, descriptor) => {
6982
const originalFunction = descriptor.value as any;
7083
const memoized = new WeakMap();

0 commit comments

Comments
 (0)