Skip to content

Commit

Permalink
Error if an abstract function that might throw is called from within …
Browse files Browse the repository at this point in the history
…a try

This is a follow up to facebookarchive#1142. If we call an unknown function it might throw
which will take a different control Flow. facebookarchive#1264 is the follow up to do this
properly but since we don't have a use case right now, we can just issue
a recoverable error if this happens.
  • Loading branch information
sebmarkbage committed Jan 5, 2018
1 parent 9e402ad commit aadf322
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 2 deletions.
2 changes: 2 additions & 0 deletions scripts/test-error-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function runTest(name: string, code: string): boolean {
let recover = code.includes("// recover-from-errors");
let additionalFunctions = code.includes("// additional functions");
let delayUnsupportedRequires = code.includes("// delay unsupported requires");
let abstractEffects = code.includes("// abstract effects");

let expectedErrors = code.match(/\/\/\s*expected errors:\s*(.*)/);
invariant(expectedErrors);
Expand All @@ -73,6 +74,7 @@ function runTest(name: string, code: string): boolean {
errorHandler: errorHandler.bind(null, recover ? "Recover" : "Fail", errors),
serialize: true,
initializeMoreModules: false,
abstractEffectsInAdditionalFunctions: abstractEffects,
};
if (additionalFunctions) (options: any).additionalFunctions = ["global.additional1", "global['additional2']"];
prepackFileSync([name], options);
Expand Down
13 changes: 12 additions & 1 deletion src/evaluators/CallExpression.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,26 @@ function EvaluateCall(
}

if (func instanceof AbstractValue) {
let loc = ast.callee.type === "MemberExpression" ? ast.callee.property.loc : ast.callee.loc;
if (!Value.isTypeCompatibleWith(func.getType(), FunctionValue)) {
let loc = ast.callee.type === "MemberExpression" ? ast.callee.property.loc : ast.callee.loc;
let error = new CompilerDiagnostic("might not be a function", loc, "PP0005", "RecoverableError");
if (realm.handleError(error) === "Fail") throw new FatalError();
} else if (func.kind === "conditional") {
return callBothFunctionsAndJoinTheirEffects(func.args, ast, strictCode, env, realm);
} else {
// Assume that it is a safe function. TODO #705: really?
}
if (realm.isInPureTryStatement) {
// TODO(1264): We should be able to preserve error handling on abstract throw
// but currently we just issue a recoverable error instead.
let diag = new CompilerDiagnostic(
"Possibly throwing function call inside try/catch",
loc,
"PP0021",
"RecoverableError"
);
if (realm.handleError(diag) !== "Recover") throw new FatalError();
}
return generateRuntimeCall();
}
invariant(func instanceof ConcreteValue);
Expand Down
14 changes: 13 additions & 1 deletion src/evaluators/TryStatement.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,19 @@ import type { BabelNodeTryStatement } from "babel-types";
import invariant from "../invariant.js";

export default function(ast: BabelNodeTryStatement, strictCode: boolean, env: LexicalEnvironment, realm: Realm): Value {
let blockRes = env.evaluateCompletionDeref(ast.block, strictCode);
let wasInPureTryStatement = realm.isInPureTryStatement;
if (realm.createdObjectsTrackedForLeaks) {
// TODO(1264): This is used to issue a warning if we have abstract function calls in here.
// We might not need it once we have full support for handling potential errors. Even
// then we might need it to know whether we should bother tracking error handling.
realm.isInPureTryStatement = true;
}
let blockRes;
try {
blockRes = env.evaluateCompletionDeref(ast.block, strictCode);
} finally {
realm.isInPureTryStatement = wasInPureTryStatement;
}

let handlerRes = blockRes;
let handler = ast.handler;
Expand Down
2 changes: 2 additions & 0 deletions src/realm.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export class Realm {
this.isReadOnly = false;
this.useAbstractInterpretation = !!opts.serialize || !!opts.residual;
this.trackLeaks = !!opts.abstractEffectsInAdditionalFunctions;
this.isInPureTryStatement = false;
if (opts.mathRandomSeed !== undefined) {
this.mathRandomGenerator = seedrandom(opts.mathRandomSeed);
}
Expand Down Expand Up @@ -194,6 +195,7 @@ export class Realm {
useAbstractInterpretation: boolean;
trackLeaks: boolean;
debugNames: void | boolean;
isInPureTryStatement: boolean; // TODO(1264): Remove this once we implement proper exception handling in abstract calls.
timeout: void | number;
mathRandomGenerator: void | (() => number);
strictlyMonotonicDateNow: boolean;
Expand Down
36 changes: 36 additions & 0 deletions test/error-handler/try-and-call-abstract-function.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// additional functions
// abstract effects
// expected errors: [{location: {"start":{"line":26,"column":11},"end":{"line":26,"column":21},"identifierName":"abstractFn","source":"test/error-handler/try-and-call-abstract-function.js"}, errorCode: "PP0021", severity: "RecoverableError", message: "Possibly throwing function call inside try/catch"}]

let abstractFn = global.__abstract ? __abstract('function', '(function() { return true; })') : function() { return true; };

function concreteFunction() {
return true;
}

function additional1() {
let value;
try {
// This is ok.
value = concreteFunction();
} catch (x) {
value = false;
}
// This is ok.
return abstractFn(value);
}

function additional2() {
try {
// This is not ok.
return abstractFn();
} catch (x) {
return false;
}
}

inspect = function() {
let ret1 = additional1();
let ret2 = additional2();
return JSON.stringify({ ret1, ret2 });
}

0 comments on commit aadf322

Please sign in to comment.