-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Conversation
# Conflicts: # src/compiler/diagnosticMessages.json
src/compiler/checker.ts
Outdated
if (resolved === anyFunctionType || resolved.callSignatures.length || resolved.constructSignatures.length) { | ||
const symbol = getPropertyOfObjectType(globalFunctionType, name); | ||
const functionType = resolved === anyFunctionType ? globalFunctionType : | ||
resolved.callSignatures.length ? globalCallableFunctionType : |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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); |
Yes, your example fails when 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. |
The second argument to |
It's very important that it be an ArrayLike, since people .apply with NodeLists, arrays, and arguments objects. |
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? |
There was a problem hiding this 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?
src/compiler/checker.ts
Outdated
@@ -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 : |
There was a problem hiding this comment.
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”.
There was a problem hiding this 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?
src/compiler/checker.ts
Outdated
@@ -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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/compiler/checker.ts
Outdated
@@ -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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There is currently no type-safe way to use an (myFunction as Function).apply(undefined, arguments);
myFunction.apply(undefined, arguments as any); Note that you can still function printArguments() {
Array.prototype.forEach.call(arguments, (item: any) => console.log(item));
} This is permitted because we only strictly check the |
@felixfbecker There is a lot of code out there today that uses |
@ahejlsberg |
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! 😸 |
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. |
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; |
Is it possible to extend this functionality so that the compiler can detect errors of this type?
Launching this code generates an error:
|
@szagi3891 Your example is covered by #7968; AFAICT it has nothing to do with the current issue because you aren't using |
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:
=>
Bypasses this for now, but ouch. |
It's no worse than it was before, but yes, you're going to need a roundtrip through |
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. |
This PR introduces a new
--strictBindCallApply
compiler option (in the--strict
family of options) with which thebind
,call
, andapply
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
andNewableFunction
, in lib.d.ts. These type contain generic method declarations forbind
,call
, andapply
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 typeFunction
.Note that the overloads of
bind
include up to four bound arguments beyond thethis
argument. (In the real world code we inspected in researching this PR, practically all uses ofbind
supplied only thethis
argument, and a few cases supplied one regular argument. No cases with more arguments were observed.)Some examples:
Fixes #212. (Hey, just a short four years later!)