Code Monkey home page Code Monkey logo

Comments (16)

tim-smart avatar tim-smart commented on July 18, 2024 1

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.

from effect.

tim-smart avatar tim-smart commented on July 18, 2024 1

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.

from effect.

tim-smart avatar tim-smart commented on July 18, 2024

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

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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

from effect.

tim-smart avatar tim-smart commented on July 18, 2024

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

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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

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

from effect.

mikearnaldi avatar mikearnaldi commented on July 18, 2024

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

from effect.

tim-smart avatar tim-smart commented on July 18, 2024

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.

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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?

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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.

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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)

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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.

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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)))
})

from effect.

mikearnaldi avatar mikearnaldi commented on July 18, 2024 
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

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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

from effect.

kristiannotari avatar kristiannotari commented on July 18, 2024

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

from effect.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.