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

Strict bind, call, and apply methods on functions #27028

Merged
merged 21 commits into from
Sep 25, 2018
Merged

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Sep 11, 2018

This PR introduces a new --strictBindCallApply compiler option (in the --strict family of options) with which the bind, call, and apply methods on function objects are strongly typed and strictly checked. Since the stricter checks may uncover previously unreported errors, this is a breaking change in --strict mode.

The PR includes two new types, CallableFunction and NewableFunction, in lib.d.ts. These type contain generic method declarations for bind, call, and apply for regular functions and constructor functions, respectively. The declarations use generic rest parameters (see #24897) to capture and reflect parameter lists in a strongly typed manner. In --strictBindCallApply mode these declarations are used in place of the (very permissive) declarations provided by type Function.

interface CallableFunction extends Function {
    /**
      * Calls the function with the specified object as the this value and the elements of specified array as the arguments.
      * @param thisArg The object to be used as the this object.
      * @param args An array of argument values to be passed to the function.
      */
    apply<T, R>(this: (this: T) => R, thisArg: T): R;
    apply<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: A): R;

    /**
      * Calls the function with the specified object as the this value and the specified rest arguments as the arguments.
      * @param thisArg The object to be used as the this object.
      * @param args Argument values to be passed to the function.
      */
    call<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, ...args: A): R;

    /**
      * For a given function, creates a bound function that has the same body as the original function.
      * The this object of the bound function is associated with the specified object, and has the specified initial parameters.
      * @param thisArg The object to be used as the this object.
      * @param args Arguments to bind to the parameters of the function.
      */
    bind<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T): (...args: A) => R;
    bind<T, A0, A extends any[], R>(this: (this: T, arg0: A0, ...args: A) => R, thisArg: T, arg0: A0): (...args: A) => R;
    bind<T, A0, A1, A extends any[], R>(this: (this: T, arg0: A0, arg1: A1, ...args: A) => R, thisArg: T, arg0: A0, arg1: A1): (...args: A) => R;
    bind<T, A0, A1, A2, A extends any[], R>(this: (this: T, arg0: A0, arg1: A1, arg2: A2, ...args: A) => R, thisArg: T, arg0: A0, arg1: A1, arg2: A2): (...args: A) => R;
    bind<T, A0, A1, A2, A3, A extends any[], R>(this: (this: T, arg0: A0, arg1: A1, arg2: A2, arg3: A3, ...args: A) => R, thisArg: T, arg0: A0, arg1: A1, arg2: A2, arg3: A3): (...args: A) => R;
    bind<T, AX, R>(this: (this: T, ...args: AX[]) => R, thisArg: T, ...args: AX[]): (...args: AX[]) => R;
}

interface NewableFunction extends Function {
    /**
      * Calls the function with the specified object as the this value and the elements of specified array as the arguments.
      * @param thisArg The object to be used as the this object.
      * @param args An array of argument values to be passed to the function.
      */
    apply<T>(this: new () => T, thisArg: T): void;
    apply<T, A extends any[]>(this: new (...args: A) => T, thisArg: T, args: A): void;

    /**
      * Calls the function with the specified object as the this value and the specified rest arguments as the arguments.
      * @param thisArg The object to be used as the this object.
      * @param args Argument values to be passed to the function.
      */
    call<T, A extends any[]>(this: new (...args: A) => T, thisArg: T, ...args: A): void;

    /**
      * For a given function, creates a bound function that has the same body as the original function.
      * The this object of the bound function is associated with the specified object, and has the specified initial parameters.
      * @param thisArg The object to be used as the this object.
      * @param args Arguments to bind to the parameters of the function.
      */
    bind<A extends any[], R>(this: new (...args: A) => R, thisArg: any): new (...args: A) => R;
    bind<A0, A extends any[], R>(this: new (arg0: A0, ...args: A) => R, thisArg: any, arg0: A0): new (...args: A) => R;
    bind<A0, A1, A extends any[], R>(this: new (arg0: A0, arg1: A1, ...args: A) => R, thisArg: any, arg0: A0, arg1: A1): new (...args: A) => R;
    bind<A0, A1, A2, A extends any[], R>(this: new (arg0: A0, arg1: A1, arg2: A2, ...args: A) => R, thisArg: any, arg0: A0, arg1: A1, arg2: A2): new (...args: A) => R;
    bind<A0, A1, A2, A3, A extends any[], R>(this: new (arg0: A0, arg1: A1, arg2: A2, arg3: A3, ...args: A) => R, thisArg: any, arg0: A0, arg1: A1, arg2: A2, arg3: A3): new (...args: A) => R;
    bind<AX, R>(this: new (...args: AX[]) => R, thisArg: any, ...args: AX[]): new (...args: AX[]) => R;
}

Note that the overloads of bind include up to four bound arguments beyond the this argument. (In the real world code we inspected in researching this PR, practically all uses of bind supplied only the this argument, and a few cases supplied one regular argument. No cases with more arguments were observed.)

Some examples:

// Compile with --strictBindCallApply

declare function foo(a: number, b: string): string;

let f00 = foo.bind(undefined);  // (a: number, b: string) => string
let f01 = foo.bind(undefined, 10);  // (b: string) => string
let f02 = foo.bind(undefined, 10, "hello");  // () => string
let f03 = foo.bind(undefined, 10, 20);  // Error

let c00 = foo.call(undefined, 10, "hello");  // string
let c01 = foo.call(undefined, 10);  // Error
let c02 = foo.call(undefined, 10, 20);  // Error
let c03 = foo.call(undefined, 10, "hello", 30);  // Error

let a00 = foo.apply(undefined, [10, "hello"]);  // string
let a01 = foo.apply(undefined, [10]);  // Error
let a02 = foo.apply(undefined, [10, 20]);  // Error
let a03 = foo.apply(undefined, [10, "hello", 30]);  // Error

class C {
    constructor(a: number, b: string) {}
    foo(this: this, a: number, b: string): string { return "" }
}

declare let c: C;
declare let obj: {};

let f10 = c.foo.bind(c);  // (a: number, b: string) => string
let f11 = c.foo.bind(c, 10);  // (b: string) => string
let f12 = c.foo.bind(c, 10, "hello");  // () => string
let f13 = c.foo.bind(c, 10, 20);  // Error
let f14 = c.foo.bind(undefined);  // Error

let c10 = c.foo.call(c, 10, "hello");  // string
let c11 = c.foo.call(c, 10);  // Error
let c12 = c.foo.call(c, 10, 20);  // Error
let c13 = c.foo.call(c, 10, "hello", 30);  // Error
let c14 = c.foo.call(undefined, 10, "hello");  // Error

let a10 = c.foo.apply(c, [10, "hello"]);  // string
let a11 = c.foo.apply(c, [10]);  // Error
let a12 = c.foo.apply(c, [10, 20]);  // Error
let a13 = c.foo.apply(c, [10, "hello", 30]);  // Error
let a14 = c.foo.apply(undefined, [10, "hello"]);  // Error

let f20 = C.bind(undefined);  // new (a: number, b: string) => C
let f21 = C.bind(undefined, 10);  // new (b: string) => C
let f22 = C.bind(undefined, 10, "hello");  // new () => C
let f23 = C.bind(undefined, 10, 20);  // Error

C.call(c, 10, "hello");  // void
C.call(c, 10);  // Error
C.call(c, 10, 20);  // Error
C.call(c, 10, "hello", 30);  // Error

C.apply(c, [10, "hello"]);  // void
C.apply(c, [10]);  // Error
C.apply(c, [10, 20]);  // Error
C.apply(c, [10, "hello", 30]);  // Error

Fixes #212. (Hey, just a short four years later!)

if (resolved === anyFunctionType || resolved.callSignatures.length || resolved.constructSignatures.length) {
const symbol = getPropertyOfObjectType(globalFunctionType, name);
const functionType = resolved === anyFunctionType ? globalFunctionType :
resolved.callSignatures.length ? globalCallableFunctionType :
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we prefer callable to newable if a given type is both?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I have a convincing argument for either preference. All I know is that it is very rare to have both and the two sets of signatures presumably align such that calling without new just causes the function to allocate an instance (as is the case with Array<T>, for example). I also know that having a combined list of overloads causes error reporting to be poor for one side because we always use the last arity-matching signature as the error exemplar.

Copy link
Member

Choose a reason for hiding this comment

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

Can we not use both sets of overloads for resolution, and if both fail, simply prefer one set or the other just when reporting errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not easily. We assume that all overloads are in a single list of signatures, so we'd somehow have to merge them or declare a third type (CallableNewableFunction) in lib.d.ts that has the combined sets of overloads.

@ChiriVulpes
Copy link

ChiriVulpes commented Sep 11, 2018

Correct me if I'm wrong, but won't the bind method overloads fail on functions with variable arguments? I use this kind of thing in my codebases... (not often, but they do exist)

function foo(...numbers: number[]) {}
const bar = [1, 2, 3];
foo.bind(undefined, ...bar);
@ahejlsberg
Copy link
Member Author

Correct me if I'm wrong, but won't the bind method overloads fail on functions with variable arguments?

Yes, your example fails when bar is an array (but succeeds if bar is a tuple with four or less number elements). I think you're right, we need an extra overload to handle binding of functions with rest parameter arrays. That is:

interface CallableFunction extends Function {
    // ...
    bind<T, AX, R>(this: (this: T, ...args: AX[]) => R, thisArg: T, ...args: AX[]): (...args: AX[]) => R;
}

interface NewableFunction extends Function {
    // ...
    bind<AX, R>(this: new (...args: AX[]) => R, thisArg: any, ...args: AX[]): new (...args: AX[]) => R;
}

Should be pretty easy to add.

@Jessidhia
Copy link

The second argument to apply is not actually an array but an ArrayLike, though I'm not sure how important that is on real world code; almost always when it's not an array it's just an arguments object.

@ljharb
Copy link
Contributor

ljharb commented Sep 12, 2018

It's very important that it be an ArrayLike, since people .apply with NodeLists, arrays, and arguments objects.

@felixfbecker
Copy link
Contributor

Some part of me wishes this was not behind a flag. What’s an example of a usage of apply/bind that is correct but would break with this?

If the two interfaces are only “injected” if the flag is on, how can libraries reference them without making an assumption on the consumer’s flag seeting?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

One initial comment, plus can you elaborate on the breaks that result from having this on everywhere as @felixfbecker requested? Or was that discussed during the last design meeting when I was out?

@@ -7366,8 +7369,12 @@ namespace ts {
if (symbol && symbolIsValue(symbol)) {
return symbol;
}
if (resolved === anyFunctionType || resolved.callSignatures.length || resolved.constructSignatures.length) {
const symbol = getPropertyOfObjectType(globalFunctionType, name);
const functionType = resolved === anyFunctionType ? globalFunctionType :
Copy link
Member

Choose a reason for hiding this comment

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

This should be put into a function and anyFunctionType’s declaration needs a comment saying “Use this function instead”.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Why is the variance change needed?

@@ -13752,15 +13759,17 @@ namespace ts {
if (!inferredType) {
const signature = context.signature;
if (signature) {
if (inference.contraCandidates && (!inference.candidates || inference.candidates.length === 1 && inference.candidates[0].flags & TypeFlags.Never)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The variance change is there to ensure we infer the most specific type possible when we have both co- and contra-variant candidates. Furthermore, knowing that an error will result when the co-variant inference is not a subtype of the contra-variant inference, we now prefer the contra-variant inference because it is likely to have come from an explicit type annotation on a function. This improves our error reporting.

@@ -28543,6 +28555,8 @@ namespace ts {
globalArrayType = getGlobalType("Array" as __String, /*arity*/ 1, /*reportErrors*/ true);
globalObjectType = getGlobalType("Object" as __String, /*arity*/ 0, /*reportErrors*/ true);
globalFunctionType = getGlobalType("Function" as __String, /*arity*/ 0, /*reportErrors*/ true);
globalCallableFunctionType = strictBindCallApply && getGlobalType("CallableFunction" as __String, /*arity*/ 0, /*reportErrors*/ true) || globalFunctionType;
Copy link
Member

Choose a reason for hiding this comment

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

Comment here is also a good idea.



==== tests/cases/conformance/types/rest/genericRestArityStrict.ts (1 errors) ====
==== tests/cases/conformance/types/rest/genericRestArityStrict.ts (2 errors) ====
// Repro from #25559

declare function call<TS extends unknown[]>(
handler: (...args: TS) => void,
...args: TS): void;

call((x: number, y: number) => x + y);
Copy link
Member

Choose a reason for hiding this comment

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

Is this improved error because of the variance inference change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

error TS2318: Cannot find global type 'NewableFunction'.


!!! error TS2318: Cannot find global type 'CallableFunction'.
Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn’t get an error here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to update the private lib.d.ts used by the tests as I suspect that'll cause some really noisy baseline changes.

Copy link
Member

Choose a reason for hiding this comment

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

OK, just wanted to make sure this error wouldn't show up in some public-facing scenario.

@ahejlsberg
Copy link
Member Author

The second argument to apply is not actually an array but an ArrayLike, though I'm not sure how important that is on real world code; almost always when it's not an array it's just an arguments object.

There is currently no type-safe way to use an ArrayLike<T> as the argument list in apply. I think this is fine since, as you point out, the typical use case is the arguments object which is inherently untyped anyway. To use an array-like with apply in --strictBindCallApply mode you need to cast the function object to type Function or cast the argument array to any:

(myFunction as Function).apply(undefined, arguments);
myFunction.apply(undefined, arguments as any);

Note that you can still call or apply methods of Array<T> with an ArrayLike<T> as the thisArg:

function printArguments() {
    Array.prototype.forEach.call(arguments, (item: any) => console.log(item));
}

This is permitted because we only strictly check the thisArg if the function on which you're invoking call or apply has an explicit this parameter. We might someday also tighten this up with a --strictThis flag, although there are several non-trivial issues with doing that. We're tracking this in #7968.

@ahejlsberg
Copy link
Member Author

@felixfbecker There is a lot of code out there today that uses apply with the arguments object to intercept functions. A substantial portion of our real world code suites break with this type of error when the stricter checking is enabled. It's technically correct to report the issue (the affected code is unsafe and an explicit cast is merited), but without a compiler switch it becomes a gating issue for adopting the latest compiler. The main point of --strict mode is to signal that you're ok with such breakage. But conversely, when you're not --strict we take it as a signal to minimize breakage.

@ljharb
Copy link
Contributor

ljharb commented Sep 12, 2018

@ahejlsberg { length: number }?

@SlurpTheo
Copy link

(Extra points for the added subtlety of spreading a string into individual characters!)

Ha/yes -- I've never wanted that to have happened in my code when it has; be great to have some sort of warning about it! 😸

@mattmccutchen
Copy link
Contributor

mattmccutchen commented Oct 12, 2018

It looks like the new feature doesn't work well for generic or overloaded functions. Silly example:

function foo<T>(name: string, arg: T): T {
    console.log(name);
    return arg;
}

// Type of `fooResult` is `{}`. :(
let fooResult = foo.bind(undefined, "Matt")("TypeScript");

function bar(name: string, arg: number): number;
function bar(name: string, arg: string): string;
function bar(name: string, arg: string | number) {
    console.log(name);
    return (typeof arg === "number") ? arg + 1 : arg + "1";
}

// Error: Argument of type '5' is not assignable to parameter of type 'string'.
let barResult = bar.bind(undefined, "Matt")(5);

It would be good to document this as a limitation in the "What's new in TypeScript" entry.

@NN---
Copy link

NN--- commented Oct 21, 2018

Is there a reason to not use 'unknown' instead of 'any' ?

    bind<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T): (...args: A) => R;
@szagi3891
Copy link

@ahejlsberg

Is it possible to extend this functionality so that the compiler can detect errors of this type?

class A {
    constructor(
        readonly name: string
    ) {}

    show() {
        console.info(`Name: ${this.name}`);
    }
}

const a = new A('aaa');
const showFn = a.show;

showFn();

Launching this code generates an error:

TypeError: this is undefined
@mattmccutchen
Copy link
Contributor

mattmccutchen commented Nov 8, 2018

@szagi3891 Your example is covered by #7968; AFAICT it has nothing to do with the current issue because you aren't using bind, call, or apply.

@DrMiaow
Copy link

DrMiaow commented Dec 3, 2018

What is the solution in the case of using generic or overloaded functions?

Is there going to be an updated version that will handle generic or overloaded functions? I'm trying to decide if I should pin to the previous working version of Typescript until you release a working version.

Update:

      const factoryFunction = event.bind.apply(event, args)
      return new factoryFunction()

=>

      const factoryFunction = (event as any).bind.apply(event, args)
      return new factoryFunction()

Bypasses this for now, but ouch.

@Jessidhia
Copy link

It's no worse than it was before, but yes, you're going to need a roundtrip through as any. Or maybe better, as Function.

@balupton
Copy link

balupton commented Dec 5, 2018

It looks like the new feature doesn't work well for generic or overloaded functions.
#27028 (comment)

Is that also why the following doesn't work?

export function binder<
	Method extends (this: Context, ...args: Args) => Result,
	Context,
	Args extends any[],
	Result
>(method: Method, context: Context, ...args: Args) {
	return method.bind(context)
}

const context = { local: 'string value' }

function fn(this: typeof context, arg: string) {}

binder(fn, context) // [ts] Argument of type '(this: { local: string; }, arg: string) => void' is not assignable to parameter of type '(this: { local: string; }) => {}'. [2345]

It would be nice if the error code 2345 was switched to something more specific about this.

I ask because I am trying to figure out how to model the below in TypeScript (yet the above seems a hurdle in its accomplishment):

function binder (method, context, …args) {
	const unbounded = method.unbounded || method
	const bounded = method.bind(context, ...args)
	Object.defineProperty(bounded, 'unbounded', {
		value: unbounded,
		enumerable: false,
		configurable: false,
		writable: false
	})
	return bounded
}

The latter seems impossible to model in TypeScript correctly.

@glen-84
Copy link

glen-84 commented Dec 18, 2018

@DrMiaow Another option is @ts-ignore, but it's not ideal without #19139.

bung87 added a commit to bung87/vscode-rails that referenced this pull request Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet