More precise dual signature (prevent type errors/issues)
#2967
Comments
|
The definition of 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 |
|
Yeah sorry, mistake with copy/paste. Edited the issue. The points are still valid though, since I tested them with the correct Here's a playground repro: Playground Link |
|
You're supposed to declare the implementation and the complete signature as the generics: Playground Link That is: |
it's more verbose for no valid reason imho, dual is supposed to be almost an internal utility |
Ok gotcha. This would require a rewrite of the majority of our api signatures, so this won't be adopted. |
|
The majority of dual signatures are in this form I think: const f: f = dual(2, (a: number, b: number) => a + b)Example from 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? |
This is only required if generics are explicitly set on the |
The majority of our signatures are re-exported from internal implementation, which use the two |
|
We could work around that changing the new 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 }(haven't battle tested it yet) |
|
If you want to play around, you can update the dual signature in the Function module, run |
|
Just for info, it works 90% of the cases, there are a couple of points where it breaks due to being inferred as |
|
Example from 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)))
}) |
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 |
|
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 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 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_dualIf you all are ok with this change I can go ahead and submit the PR with the ~20 affected functions changed this way. |
|
I submitted the #3068 pr to see if this can land safely without any disruptions. |
What is the problem this feature would solve?
With the current
dualsignature from theFunctionmodule 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
fdefinition:The following problems could arise with no type error / wrong type inferred:
What is the feature you are proposing to solve the problem?
Change the
dualfunction type signature to be more precise over what you actually used as the implementation for the dual function.What alternatives have you considered?
No response
The text was updated successfully, but these errors were encountered: