Skip to content

Commit

Permalink
Don't error calling possibly non-functions in pure functions
Browse files Browse the repository at this point in the history
As outlined in facebookarchive#1264 we know
that it is safe to throw in an pure function since then the return value
will be ignored and therefore its state doesn't matter. The only time
it matters, is if there is a try/catch around it which we already warn
about.

This means that we don't have to error when calling a non-function.

I also plan to add more of these cases like this. So I took the liberty to
also move this category of tests into a separate folder.
  • Loading branch information
sebmarkbage committed Jan 11, 2018
1 parent 9949c16 commit 414bb13
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 4 deletions.
6 changes: 5 additions & 1 deletion scripts/test-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -556,11 +556,15 @@ function run(args) {
//only run specific tests if desired
if (!test.name.includes(args.filter)) continue;
const isAdditionalFunctionTest = test.name.includes("additional-functions");
const isPureFunctionTest = test.name.includes("pure-functions");
const isCaptureTest = test.name.includes("Closure") || test.name.includes("Capture");
const isSimpleClosureTest = test.file.includes("// simple closures");
// Skip lazy objects mode for certain known incompatible tests, react compiler and additional-functions tests.
const skipLazyObjects =
test.file.includes("// skip lazy objects") || isAdditionalFunctionTest || test.name.includes("react");
test.file.includes("// skip lazy objects") ||
isAdditionalFunctionTest ||
isPureFunctionTest ||
test.name.includes("react");

let flagPermutations = [
[false, false, undefined, isSimpleClosureTest],
Expand Down
9 changes: 7 additions & 2 deletions src/evaluators/CallExpression.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,13 @@ 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 error = new CompilerDiagnostic("might not be a function", loc, "PP0005", "RecoverableError");
if (realm.handleError(error) === "Fail") throw new FatalError();
if (!realm.isInPureScope()) {
// If this is not a function, this it might throw which can change the state of the program.
// If this is called from a pure function it is safe, unless in a try/catch which has a
// separate error below.
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 {
Expand Down
2 changes: 1 addition & 1 deletion src/evaluators/TryStatement.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import invariant from "../invariant.js";

export default function(ast: BabelNodeTryStatement, strictCode: boolean, env: LexicalEnvironment, realm: Realm): Value {
let wasInPureTryStatement = realm.isInPureTryStatement;
if (realm.createdObjectsTrackedForLeaks) {
if (realm.isInPureScope()) {
// 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.
Expand Down
4 changes: 4 additions & 0 deletions src/realm.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ export class Realm {
}
}

isInPureScope() {
return !!this.createdObjectsTrackedForLeaks;
}

// Evaluate the given ast in a sandbox and return the evaluation results
// in the form of a completion, a code generator, a map of changed variable
// bindings and a map of changed property bindings.
Expand Down
26 changes: 26 additions & 0 deletions test/serializer/pure-functions/AbstractCallUnknownType.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// additional functions
// abstract effects

let obj = global.__abstract ? __abstract('object', '({foo: function() { return 1; }})') : {foo: function() { return 1; }};
if (global.__makeSimple) {
__makeSimple(obj);
}
let condition = global.__abstract ? __abstract('boolean', 'true') : true;

function additional1() {
return obj.foo();
}

function additional2() {
function fn() {
return 5;
}
let fnOrString = condition ? fn : 'string';
return fnOrString();
}

inspect = function() {
let ret1 = additional1();
let ret2 = additional2();
return ret1 + ret2;
}

0 comments on commit 414bb13

Please sign in to comment.