Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include all flow nodes made within try blocks as antecedents for catch or finally blocks #29466

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ namespace ts {
IsObjectLiteralOrClassExpressionMethod = 1 << 7,
}

let flowNodeCreated: <T extends FlowNode>(node: T) => T = identity;

const binder = createBinder();

export function bindSourceFile(file: SourceFile, options: CompilerOptions) {
Expand Down Expand Up @@ -530,6 +532,7 @@ namespace ts {
blockScopeContainer.locals = undefined;
}
if (containerFlags & ContainerFlags.IsControlFlowContainer) {
const saveFlowNodeCreated = flowNodeCreated;
const saveCurrentFlow = currentFlow;
const saveBreakTarget = currentBreakTarget;
const saveContinueTarget = currentContinueTarget;
Expand All @@ -553,6 +556,7 @@ namespace ts {
currentContinueTarget = undefined;
activeLabels = undefined;
hasExplicitReturn = false;
flowNodeCreated = identity;
bindChildren(node);
// Reset all reachability check related flags on node (for incremental scenarios)
node.flags &= ~NodeFlags.ReachabilityAndEmitFlags;
Expand All @@ -579,6 +583,7 @@ namespace ts {
currentReturnTarget = saveReturnTarget;
activeLabels = saveActiveLabels;
hasExplicitReturn = saveHasExplicitReturn;
flowNodeCreated = saveFlowNodeCreated;
}
else if (containerFlags & ContainerFlags.IsInterface) {
seenThisKeyword = false;
Expand Down Expand Up @@ -858,25 +863,25 @@ namespace ts {
return antecedent;
}
setFlowNodeReferenced(antecedent);
return { flags, expression, antecedent };
return flowNodeCreated({ flags, expression, antecedent });
}

function createFlowSwitchClause(antecedent: FlowNode, switchStatement: SwitchStatement, clauseStart: number, clauseEnd: number): FlowNode {
if (!isNarrowingExpression(switchStatement.expression)) {
return antecedent;
}
setFlowNodeReferenced(antecedent);
return { flags: FlowFlags.SwitchClause, switchStatement, clauseStart, clauseEnd, antecedent };
return flowNodeCreated({ flags: FlowFlags.SwitchClause, switchStatement, clauseStart, clauseEnd, antecedent });
}

function createFlowAssignment(antecedent: FlowNode, node: Expression | VariableDeclaration | BindingElement): FlowNode {
setFlowNodeReferenced(antecedent);
return { flags: FlowFlags.Assignment, antecedent, node };
return flowNodeCreated({ flags: FlowFlags.Assignment, antecedent, node });
}

function createFlowArrayMutation(antecedent: FlowNode, node: CallExpression | BinaryExpression): FlowNode {
setFlowNodeReferenced(antecedent);
const res: FlowArrayMutation = { flags: FlowFlags.ArrayMutation, antecedent, node };
const res: FlowArrayMutation = flowNodeCreated({ flags: FlowFlags.ArrayMutation, antecedent, node });
return res;
}

Expand Down Expand Up @@ -1080,21 +1085,49 @@ namespace ts {
function bindTryStatement(node: TryStatement): void {
const preFinallyLabel = createBranchLabel();
const preTryFlow = currentFlow;
// TODO: Every statement in try block is potentially an exit point!
const tryPriors: FlowNode[] = [];
const oldFlowNodeCreated = flowNodeCreated;
// We hook the creation of all flow nodes within the `try` scope and store them so we can add _all_ of them
// as possible antecedents of the start of the `catch` or `finally` blocks.
// Don't bother intercepting the call if there's no finally or catch block that needs the information
if (node.catchClause || node.finallyBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this condition make sense? It's only false if there is a syntax error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still parse code with syntax errors 🤷‍♂️

flowNodeCreated = node => (tryPriors.push(node), node);
}
bind(node.tryBlock);
flowNodeCreated = oldFlowNodeCreated;
addAntecedent(preFinallyLabel, currentFlow);

const flowAfterTry = currentFlow;
let flowAfterCatch = unreachableFlow;

if (node.catchClause) {
currentFlow = preTryFlow;
if (tryPriors.length) {
const preCatchFlow = createBranchLabel();
addAntecedent(preCatchFlow, currentFlow);
for (const p of tryPriors) {
addAntecedent(preCatchFlow, p);
}
currentFlow = finishFlowLabel(preCatchFlow);
}

bind(node.catchClause);
addAntecedent(preFinallyLabel, currentFlow);

flowAfterCatch = currentFlow;
}
if (node.finallyBlock) {
// We add the nodes within the `try` block to te `finally`'s antecedents if there's no catch block
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "te" should be "the"

// (If there is a `catch` block, it will have all these antecedents instead, and the `finally` will
// have the end of the `try` block and the end of the `catch` block)
if (!node.catchClause) {
if (tryPriors.length) {
for (const p of tryPriors) {
addAntecedent(preFinallyLabel, p);
}
}
}

// in finally flow is combined from pre-try/flow from try/flow from catch
// pre-flow is necessary to make sure that finally is reachable even if finally flows in both try and finally blocks are unreachable

Expand Down Expand Up @@ -1142,7 +1175,7 @@ namespace ts {
}
}
if (!(currentFlow.flags & FlowFlags.Unreachable)) {
const afterFinallyFlow: AfterFinallyFlow = { flags: FlowFlags.AfterFinally, antecedent: currentFlow };
const afterFinallyFlow: AfterFinallyFlow = flowNodeCreated({ flags: FlowFlags.AfterFinally, antecedent: currentFlow });
preFinallyFlow.lock = afterFinallyFlow;
currentFlow = afterFinallyFlow;
}
Expand Down
142 changes: 142 additions & 0 deletions tests/baselines/reference/controlFlowForCatchAndFinally.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
//// [controlFlowForCatchAndFinally.ts]
type Page = {close(): Promise<void>; content(): Promise<string>};
type Browser = {close(): Promise<void>};
declare function test1(): Promise<Browser>;
declare function test2(obj: Browser): Promise<Page>;
async function test(): Promise<string> {
let browser: Browser | undefined = undefined;
let page: Page | undefined = undefined;
try {
browser = await test1();
page = await test2(browser);
return await page.content();;
} finally {
if (page) {
await page.close(); // ok
}

if (browser) {
await browser.close(); // ok
}
}
}

declare class Aborter { abort(): void };
class Foo {
abortController: Aborter | undefined = undefined;

async operation() {
if (this.abortController !== undefined) {
this.abortController.abort();
this.abortController = undefined;
}
try {
this.abortController = new Aborter();
} catch (error) {
if (this.abortController !== undefined) {
this.abortController.abort(); // ok
}
}
}
}

//// [controlFlowForCatchAndFinally.js]
"use strict";
var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
return new (P || (P = Promise))(function (resolve, reject) {
function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
function step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
};
var __generator = (this && this.__generator) || function (thisArg, body) {
var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g;
return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
function verb(n) { return function (v) { return step([n, v]); }; }
function step(op) {
if (f) throw new TypeError("Generator is already executing.");
while (_) try {
if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t;
if (y = 0, t) op = [op[0] & 2, t.value];
switch (op[0]) {
case 0: case 1: t = op; break;
case 4: _.label++; return { value: op[1], done: false };
case 5: _.label++; y = op[1]; op = [0]; continue;
case 7: op = _.ops.pop(); _.trys.pop(); continue;
default:
if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; }
if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; }
if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; }
if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; }
if (t[2]) _.ops.pop();
_.trys.pop(); continue;
}
op = body.call(thisArg, _);
} catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; }
if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true };
}
};
function test() {
return __awaiter(this, void 0, void 0, function () {
var browser, page;
return __generator(this, function (_a) {
switch (_a.label) {
case 0:
browser = undefined;
page = undefined;
_a.label = 1;
case 1:
_a.trys.push([1, , 5, 10]);
return [4 /*yield*/, test1()];
case 2:
browser = _a.sent();
return [4 /*yield*/, test2(browser)];
case 3:
page = _a.sent();
return [4 /*yield*/, page.content()];
case 4: return [2 /*return*/, _a.sent()];
case 5:
if (!page) return [3 /*break*/, 7];
return [4 /*yield*/, page.close()];
case 6:
_a.sent(); // ok
_a.label = 7;
case 7:
if (!browser) return [3 /*break*/, 9];
return [4 /*yield*/, browser.close()];
case 8:
_a.sent(); // ok
_a.label = 9;
case 9: return [7 /*endfinally*/];
case 10: return [2 /*return*/];
}
});
});
}
;
var Foo = /** @class */ (function () {
function Foo() {
this.abortController = undefined;
}
Foo.prototype.operation = function () {
return __awaiter(this, void 0, void 0, function () {
return __generator(this, function (_a) {
if (this.abortController !== undefined) {
this.abortController.abort();
this.abortController = undefined;
}
try {
this.abortController = new Aborter();
}
catch (error) {
if (this.abortController !== undefined) {
this.abortController.abort(); // ok
}
}
return [2 /*return*/];
});
});
};
return Foo;
}());
Loading