Fix Function.prototype.apply.bind

[goodmind]

Fixes #5681

const fn = (a: number) => {}
export const fnApply = fn.apply.bind(fn) // no error

Also adds argument to Function$Prototype$Apply

declare var fnApply: Function$Prototype$Apply<typeof fn>

[mroch]

i think this is the only place we'd allow a sometimes-parameterized generic. the most similar concept is having a default, but in that case you'd have to use an empty <> (https://flow.org/en/docs/types/generics/#toc-adding-defaults-to-parameterized-generics).

this feels like a new behavior that we should consider carefully. also, we should consider how it interacts with annotating this, because it might be closer to Function$Prototype$Apply<this=T> or whatever syntax we end up with for that -- we've gone back and forth about annotating this for functions as either function f(this: T, ...) or function f<this = T>(...) (cc @gabelevi )

[goodmind]

I like first one because it mirrors TypeScript

[goodmind]

How can I make this support <>?

[goodmind]

@mroch you can mimic second one with first one
type F<T> = (this: T) => void

[samwgoldman]

A silly question: What significance does binding the apply function itself have? Can you share an example where the observable effect of checking is different?

If it is significant, I think the solution of adding a this type to the internal representation of ApplyT is reasonable. However, I don't think we should add a type parameter to the annotation syntax, or at least not until the overall this-type story is sorted.

[goodmind]

@samwgoldman it would be possible to apply $Compose or other custom functions on type-level

[samwgoldman]

Can you please share an example? :)

[mroch]

function spread(fn) {
  return Function.apply.bind( fn, null );
}

spread(fn)([1, 2, 3]) // fn(1, 2, 3)

[goodmind]

https://github.com/lttb/flown/blob/master/src/Reduce/index.js

type Apply<Fn, Args> = $Call<Function$Prototype$Apply<Fn>, null, Args>
// basically $Call with tuple

declare function reduce<Fn, A, T>(Fn, A, T): $Call<
  Apply<
    $ComposeReverse,
    $TupleMap<T, Fn>,
  >
, A>

export type _Reduce = typeof reduce

export type Reduce<Fn, A, T> = $Call<_Reduce, Fn, A, T>

[goodmind]

type MergeAll<T> = Reduce<<X, V>(V) => X => { ...$Exact<X>, ...$Exact<V> }, {}, T>

declare function mergeAll<A>(A): MergeAll<A>

const x = mergeAll([{ a: 1 }, { b: 1 }, { c: 1 }, { a: 2 }])

;(x: { a: 2, b: 1, c: 1 })

/**
 * $ExpectError
 *
 * the type of 'c' is incompatible with number
 */
;(x: { a: 2, b: 1, c: '1' })

[samwgoldman]

@goodmind I don't see how these examples are related to the linked issue, or Function.prototype.apply/bind. I would much prefer to focus on the problem at hand, since there's enough work involved in supporting the use case from Marshall's example.

1. I still don't think we should add a type argument to Function$Prototype$Apply. Even if it makes some things expressible, it's putting the cart before the horse.
2. Per Marshall's example, adding the this type to the internal FunApplyT repr isn't enough, we also need to add the partial argument list.
3. The implementation here does not look correct (will comment inline) so some tests are definitely needed.

[samwgoldman]

This doesn't look right. Look at the FunT _, BindT _ rule for guidance.

1. The call_this_t field of the BindT use is the this type corresponding to the call of bind. E.g., x in x.bind(...). You want the first argument instead.
2. Looking at FunT _, BindT _ as a guide, you'll notice some complexity missing from here. Specifically, the constraints involving ResolveSpreadsToMultiflowPartial. This is needed to figure out the full argument list in the presence of spread arguments.

[goodmind]

What first argument? This is forwarding From

declare var regularApply: $Function$Prototype$Apply;
const b = regularApply.bind(fn) // would be `$Function$Prototype$Apply<typeof fn>`

to $Function$Prototype$Apply<typeof fn>

[samwgoldman]

If you write a test, I think you will see what I mean. You want the first argument passed to the bind call, which is not call_this_t.

[samwgoldman]

Ah, sorry, my mistake -- I was confusing BindT with FunProtoBindT. You're right that call_this_t is the first parameter.

You do still need to do something with call_args_tlist, though.

[goodmind]

You mean something like this?

const appliedFn = Function.apply.bind(x); // ok
const reappliedFn = appliedFn.bind(y); // wrong
reappliedFn(null, [""]); // no error

[goodmind]

@samwgoldman I think you were right 😀

[goodmind]

@samwgoldman would this mean adding second argument with applied parameters? I think one argument is already too much

[goodmind]

@samwgoldman now it should work, I didn't expose partial apply to arguments itself, only to use_t

[samwgoldman]

I still don't think we should add a type argument to the type annotation.

[goodmind]

@samwgoldman how it would work then for this case

type Apply<Fn, Args> = $Call<Function$Prototype$Apply<Fn>, null, Args>

[samwgoldman]

We don't need it to fix #5681. If you want to support another case, I think that you should write up a more thorough proposal. For example, why should we support that?

[goodmind]

@samwgoldman maybe I can remove argument if you don't want it and decide later how to make my case work? (or write proposal)

[samwgoldman]

Yeah. I would want to have a full understanding of your use case before adding something. It's possible that another solution would be better. I'm not opposed to adding something later, but we need to follow a process to make sure that we're adding the right things for the right reasons.

[goodmind]

@samwgoldman removed it

[goodmind]

Probably related issue, methods doesn't have function prototype
https://flow.org/try/#0MYGwhgzhAECC0G8BQ1oQC5nQS2NAZgBRgBc0AdgK4C2ARgKYBOAlIitAL5JdKwB0+PmAAOwkAE9CVECAA00ANoByJQF1mQA