More precise `dual` signature (prevent type errors/issues)

[kristiannotari]

What is the problem this feature would solve?

With the current dual signature from the Function module you can easily get away with wrong types, since the generics can be inferred in a very wide fashion. This can lead to subtle type errors that are not discovered at compile time, but rather at runtime.

Given this f definition:

type f_data_first = (a: number, b: number) => number
type f_data_last = (b: number) => (a: number) => number
type f = f_data_last & f_data_first

The following problems could arise with no type error / wrong type inferred:

const f_no_type_old = effect_dual(2, (a: number, b: number) => a + b) // (args: ...any[]) => any ❌
const f_wrong_type_old: f = effect_dual(2, (a: string, b: string) => a + b) // no error! ❌

What is the feature you are proposing to solve the problem?

Change the dual function type signature to be more precise over what you actually used as the implementation for the dual function.

export const dual: {
    <Impl extends (...args: readonly any[]) => any, Signature extends Impl = Impl>(
        arity: Parameters<Impl>["length"],
        body: Impl
    ): Signature
    <Impl extends (...args: readonly any[]) => any, Signature extends Impl = Impl>(
        // eslint-disable-next-line @typescript-eslint/unified-signatures
        isDataFirst: (args: IArguments) => boolean,
        body: Impl
    ): Signature
} = effect_dual


const f_new: f = dual(2, (a: number, b: number) => a + b)
const f_old: f = effect_dual(2, (a: number, b: number) => a + b)

const f_new_explicit_types = dual<f_data_first, f>(2, (a: number, b: number) => a + b)
const f_old_explicit_types = effect_dual<f_data_last, f_data_first>(2, (a: number, b: number) => a + b)

const f_no_type_new = dual(2, (a: number, b: number) => a + b) // (a: number, b: number) => number ✅
const f_no_type_old = effect_dual(2, (a: number, b: number) => a + b) // (args: ...any[]) => any ❌

const f_wrong_type_new: f = dual(2, (a: string, b: string) => a + b) // compile time error ✅
const f_wrong_type_old: f = effect_dual(2, (a: string, b: string) => a + b) // no error! ❌

What alternatives have you considered?

No response

[tim-smart]

The definition of f should be:

type f_data_first = (a: number, b: number) => number
type f_data_last = (b: number) => (a: number) => number
type f = f_data_last & f_data_first

[kristiannotari]

Yeah sorry, mistake with copy/paste. Edited the issue. The points are still valid though, since I tested them with the correct f definition.

Here's a playground repro: Playground Link

[tim-smart]

Seems to trigger a bunch of false negatives: https://www.typescriptlang.org/play/?ts=5.5.1-rc#code/JYWwDg9gTgLgBAbzgEwK4EMA2d0Gc4CmAZkQQMYwD6aWcAvnEVBCHAETGkUD0AYqgDsKwCALYAoUJFiI4AYVFFgAcwA0cAKLAYACwJR6jZqw4lyMCeIIAPafDKjc8GpgBcicXC9wAPAElwbBsYAgFkfAAKADoY9ChlXHcoAnRkUUwATxwBDIBtAF0ASjgAXgA+bIz1AGUVAXQYVGTCaxCw-ACwbBK4TswyiM9vYbjtDPcABTj0EAIQqFx-QLLctkxQ5V02fNUh4a8AIwhkcd7AvbhC91rlesbki6WulrbwuGjY+MS4ZNT0rPQOQKxXKlRqdQaTQIL1Cbz6pTOXQGF2G3G4hFwmGAAhgAFpkMBcOgDutcQJgriseS4AABGAZMAEXBkKDAMB4plUmDcQTAJQEZC43AQ+5MlHeQkAEQa6F4wAWMHcETiCXcfgAgvFULMcbgQRUjhB1oDdvtvEcTmrzsMrnAbncoeIGD1OOZqBhMFZbNB4EgXDh8PwhDARAJ3bQ6OIvXY4A4BE44CB0GAES4fEMfOr1AAhAZEJXodzq-VwbMliK4AiYfPyRQqKIKARKZSZsolxvNht1lu502+LOlgaV6vuDv1sct9VldQ15VFktl0oVCddpsqHy58QDABM6grVaIM-b3aiSbAAHkoLx0MBMPvq3v0CWtLp9FFWcodDAIkRlYV-4UQA

[kristiannotari]

You're supposed to declare the implementation and the complete signature as the generics: Playground Link

That is: dual<ImplementationSignature, CompleteSignatureIncludingImplementationSignature>(arity, implementation)

[mikearnaldi]

You're supposed to declare the implementation and the complete signature as the generics: Playground Link

That is: dual<ImplementationSignature, CompleteSignatureIncludingImplementationSignature>(arity, implementation)

it's more verbose for no valid reason imho, dual is supposed to be almost an internal utility

[tim-smart]

You're supposed to declare the implementation and the complete signature as the generics: Playground Link

That is: dual<ImplementationSignature, CompleteSignatureIncludingImplementationSignature>(arity, implementation)

Ok gotcha. This would require a rewrite of the majority of our api signatures, so this won't be adopted.

[kristiannotari]

The majority of dual signatures are in this form I think:

const f: f = dual(2, (a: number, b: number) => a + b)

Example from Array:

export const map: {
  <S extends ReadonlyArray<any>, B>(
    f: (a: ReadonlyArray.Infer<S>, i: number) => B
  ): (self: S) => ReadonlyArray.With<S, B>
  <S extends ReadonlyArray<any>, B>(self: S, f: (a: ReadonlyArray.Infer<S>, i: number) => B): ReadonlyArray.With<S, B>
} = dual(2, <A, B>(self: ReadonlyArray<A>, f: (a: A, i: number) => B): Array<B> => self.map(f))

This would require no change:

export const dual: {
    <const Impl extends (...args: readonly any[]) => any, Signature extends Impl = Impl>(
        arity: Parameters<Impl>["length"],
        body: Impl
    ): Signature
    <Impl extends (...args: readonly any[]) => any, Signature extends Impl = Impl>(
        // eslint-disable-next-line @typescript-eslint/unified-signatures
        isDataFirst: (args: IArguments) => boolean,
        body: Impl
    ): Signature
} = effect_dual
export { dual as Function_dual }

export const map: {
    <S extends ReadonlyArray<any>, B>(
      f: (a: Array.ReadonlyArray.Infer<S>, i: number) => B
    ): (self: S) => Array.ReadonlyArray.With<S, B>
    <S extends ReadonlyArray<any>, B>(self: S, f: (a: Array.ReadonlyArray.Infer<S>, i: number) => B): Array.ReadonlyArray.With<S, B>
  } = dual(2, <A, B>(self: ReadonlyArray<A>, f: (a: A, i: number) => B): Array<B> => self.map(f))

Of course it's not perfect, I mean there are still cases where you can mess up the implementation signature with the actual dual signature, but should help prevent errors in changing the implementation while not changing the signature or viceversa both for effect codebase and other libs.

The only cases where a manual change would be required is where the dual generics are explicitly set. Still, they should always pop up as compiler error, so would be easy to adjust them I think?

[kristiannotari]

You're supposed to declare the implementation and the complete signature as the generics: Playground Link

That is: dual<ImplementationSignature, CompleteSignatureIncludingImplementationSignature>(arity, implementation)

This is only required if generics are explicitly set on the dual call already. If not, the "normal" way works as before. No changes required. My wording of "you're supposed to" was indeed a bit misleading.

[tim-smart]

The majority of dual signatures are in this form I think:

The majority of our signatures are re-exported from internal implementation, which use the two dual generics.

[kristiannotari]

We could work around that changing the new dual signature to allow for the "other" signature first, and real impl later. Same principle should apply, but no internal change in signatures should be needed:

export const dual: {
    <Other extends (...args: readonly any[]) => any, Impl extends (...args: readonly any[]) => any, Signature extends Impl = Impl & ([readonly any[]] extends [Parameters<Other>] ? unknown : Other )>(
        arity: Parameters<Impl>["length"],
        body: Impl
    ): Signature
    <Other extends (...args: readonly any[]) => any, Impl extends (...args: readonly any[]) => any, Signature extends Impl = Impl & ([readonly any[]] extends [Parameters<Other>] ? unknown : Other )>(
        // eslint-disable-next-line @typescript-eslint/unified-signatures
        isDataFirst: (args: IArguments) => boolean,
        body: Impl
    ): Signature
} = effect_dual
export { dual as Function_dual }

Playground Link

(haven't battle tested it yet)

[tim-smart]

If you want to play around, you can update the dual signature in the Function module, run pnpm check and see how it will affect the codebase.

[kristiannotari]

Just for info, it works 90% of the cases, there are a couple of points where it breaks due to being inferred as any before, which is not possible anymore. I tried working around it but no luck. One can think about changing those places to an explicit type signature in the dual generics, since there are not that many. Before I dive in, would be cool to have something like 20 functions using dual being changed in a pr to make it so they have precise types specified in the dual signature instead of it being inferred as the generic function with any? I can submit it if so.

[kristiannotari]

Example from Array.split:

export const split: {
  (n: number): <A>(self: Iterable<A>) => Array<Array<A>>
  <A>(self: Iterable<A>, n: number): Array<Array<A>>
} = dual(2, <A>(self: Iterable<A>, n: number) => { // this is (...args: any[]) => any really, that's how it works now
  const input = fromIterable(self)
  return chunksOf(input, Math.ceil(input.length / Math.floor(n)))
})

This is how I'd solve it:

export const split: {
  (n: number): <A>(self: Iterable<A>) => Array<Array<A>>
  <A>(self: Iterable<A>, n: number): Array<Array<A>>
} = dual<
    (n: number): <A>(self: Iterable<A>) => Array<Array<A>>, 
    <A>(self: Iterable<A>, n: number): Array<Array<A>>
>(2, <A>(self: Iterable<A>, n: number) => { // this is (...args: any[]) => any really, that's how it works now
  const input = fromIterable(self)
  return chunksOf(input, Math.ceil(input.length / Math.floor(n)))
})

eventually making it so they're not duplicated:

declare namespace split {
    type DataLast = (n: number): <A>(self: Iterable<A>) => Array<Array<A>>
    type DataFirst = <A>(self: Iterable<A>, n: number): Array<Array<A>>
    interface split extends DataLast, DataFirst {}
}
export const split: split.split = dual<
    split.DataLast, 
    split.DataFirst
>(2, <A>(self: Iterable<A>, n: number) => { // this is (...args: any[]) => any really, that's how it works now
  const input = fromIterable(self)
  return chunksOf(input, Math.ceil(input.length / Math.floor(n)))
})

[mikearnaldi]

declare namespace split {
    type DataLast = (n: number): <A>(self: Iterable<A>) => Array<Array<A>>
    type DataFirst = <A>(self: Iterable<A>, n: number): Array<Array<A>>
    interface split extends DataLast, DataFirst {}
}
export const split: split.split = dual<
    split.DataLast, 
    split.DataFirst
>(2, <A>(self: Iterable<A>, n: number) => { // this is (...args: any[]) => any really, that's how it works now
  const input = fromIterable(self)
  return chunksOf(input, Math.ceil(input.length / Math.floor(n)))
})

would be problematic in the re-exports as we can't export internal types

[kristiannotari]

An alternative could be to reuse the term type directly, since its type is already declared. No duplicates and no issues so far, and you can even omit types for the passed implementation, since it's already 1:1 inferred by the generic used as the DataFirst parameter.

export const split: {
  (n: number): <A>(self: Iterable<A>) => Array<Array<A>>
  <A>(self: Iterable<A>, n: number): Array<Array<A>>
} = dual<typeof split, <A>(self: Iterable<A>, n: number) => Array<Array<A>>>(2, (self, n) => {
  return [[]]
})

even with more signatures:

export const findLast: {
  <A, B>(f: (a: NoInfer<A>, i: number) => Option.Option<B>): (self: Iterable<A>) => Option.Option<B>
  <A, B extends A>(refinement: (a: NoInfer<A>, i: number) => a is B): (self: Iterable<A>) => Option.Option<B>
  <A>(predicate: (a: NoInfer<A>, i: number) => boolean): (self: Iterable<A>) => Option.Option<A>
  <A, B>(self: Iterable<A>, f: (a: A, i: number) => Option.Option<B>): Option.Option<B>
  <A, B extends A>(self: Iterable<A>, refinement: (a: A, i: number) => a is B): Option.Option<B>
  <A>(self: Iterable<A>, predicate: (a: A, i: number) => boolean): Option.Option<A>
} = dual<typeof findLast, <A>(self: Iterable<A>, f: ((a: A, i: number) => boolean) | ((a: A, i: number) => Option.Option<A>)) => Option.Option<A>>(
  2,
  (self, f) => {
    return Option.none()
  }
)

Given this change, the generic parameter for DataLast is better to be called Other or something, since it contains all the signatures except (or including, no problems with including) the actual implementation (which is DataFirst, but should be called Impl really).

export const dual: {
  <
    Other extends (...args: Array<any>) => any,
    Impl extends (...args: Array<any>) => any,
    Signature extends Impl = Impl & ([ReadonlyArray<any>] extends [Parameters<Other>] ? unknown : Other)
  >(
    arity: Parameters<Impl>["length"],
    body: Impl
  ): Signature
  <
    Other extends (...args: Array<any>) => any,
    Impl extends (...args: Array<any>) => any,
    Signature extends Impl = Impl & ([ReadonlyArray<any>] extends [Parameters<Other>] ? unknown : Other)
  >(
    isDataFirst: (args: IArguments) => boolean,
    body: Impl
  ): Signature
} = effect_dual

If you all are ok with this change I can go ahead and submit the PR with the ~20 affected functions changed this way. Here's the playground for reference: Playground Link

[kristiannotari]

I submitted the #3068 pr to see if this can land safely without any disruptions.