Skip to content

Commit

Permalink
Fix instanceof when subclassing Error, closes #159
Browse files Browse the repository at this point in the history
The `[Symbol.hasInstance]` property is inherited by subclasses.
Previously each class had a fixed target checks were performed
against. This caused issues where subclasses of `Error` were
treated as `Error`s themselves in `instanceof` checks leading to
incorrect results. This change uses the bound `this` as the target.
  • Loading branch information
mrbbot committed Jan 27, 2022
1 parent 3e9e9ad commit ab60cad
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 10 deletions.
14 changes: 4 additions & 10 deletions packages/runner-vm/src/instanceof.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ import vm from "vm";
import { ValueOf } from "@miniflare/shared";

// https://tc39.es/ecma262/multipage/abstract-operations.html#sec-ordinaryhasinstance
function ordinaryHasInstance(C: any, O: any): boolean {
function ordinaryHasInstance(C: unknown, O: unknown): boolean {
// 1. If IsCallable(C) is false, return false.
// - https://tc39.es/ecma262/multipage/abstract-operations.html#sec-iscallable
// - https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#table-typeof-operator-results
Expand Down Expand Up @@ -133,11 +133,11 @@ const outsideTargets = {
};

function defineHasInstance(insideTarget: ValueOf<typeof outsideTargets>) {
const outsideTarget =
outsideTargets[insideTarget.name as keyof typeof outsideTargets];
Object.defineProperty(insideTarget, Symbol.hasInstance, {
value(value: any) {
return instanceOf(value, insideTarget, outsideTarget);
const outsideTarget =
outsideTargets[this.name as keyof typeof outsideTargets];
return instanceOf(value, this, outsideTarget);
},
});
}
Expand All @@ -152,12 +152,6 @@ const defineHasInstancesScript = new vm.Script(
defineHasInstance(Promise);
defineHasInstance(RegExp);
defineHasInstance(Error);
defineHasInstance(EvalError);
defineHasInstance(RangeError);
defineHasInstance(ReferenceError);
defineHasInstance(SyntaxError);
defineHasInstance(TypeError);
defineHasInstance(URIError);
})`,
{ filename: "<defineHasInstancesScript>" }
);
Expand Down
38 changes: 38 additions & 0 deletions packages/runner-vm/test/instanceof.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,41 @@ test("calling defineHasInstances on same context multiple times doesn't throw",
defineHasInstances(ctx);
t.pass();
});

test("Error subclasses have correct instanceof behaviour", (t) => {
// https://github.com/cloudflare/miniflare/issues/159
const ctx = vm.createContext({});
defineHasInstances(ctx);
const result = vm.runInContext(
`
class CustomError extends Error {}
({
errorInstanceOfError: new Error() instanceof Error,
errorInstanceOfTypeError: new Error() instanceof TypeError,
errorInstanceOfCustomError: new Error() instanceof CustomError,
typeErrorInstanceOfError: new TypeError() instanceof Error,
typeErrorInstanceOfTypeError: new TypeError() instanceof TypeError,
typeErrorInstanceOfCustomError: new TypeError() instanceof CustomError,
customErrorInstanceOfError: new CustomError() instanceof Error,
customErrorInstanceOfTypeError: new CustomError() instanceof TypeError,
customErrorInstanceOfCustomError: new CustomError() instanceof CustomError,
})
`,
ctx
);
t.deepEqual(result, {
errorInstanceOfError: true,
errorInstanceOfTypeError: false,
errorInstanceOfCustomError: false,

typeErrorInstanceOfError: true,
typeErrorInstanceOfTypeError: true,
typeErrorInstanceOfCustomError: false,

customErrorInstanceOfError: true,
customErrorInstanceOfTypeError: false,
customErrorInstanceOfCustomError: true,
});
});

0 comments on commit ab60cad

Please sign in to comment.