Skip to content
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

#1005 - HKT restructuring #1055

Closed
wants to merge 73 commits into from
Closed

#1005 - HKT restructuring #1055

wants to merge 73 commits into from

Conversation

isthatcentered
Copy link
Contributor

@isthatcentered isthatcentered commented Jan 22, 2022

(kinda) Getting close, I still have to:

  • Migrate the transformers (<- that's not a useful link)
  • Adress some todos
  • Compare with zio prelude and see what differences there are/what could be interesting
  • I'd love to add some snippets for each typeclass, I feel like people often miss how much type classes like Contravariant/Ord/Const/... are amazing

@isthatcentered isthatcentered linked an issue Jan 22, 2022 that may be closed by this pull request
@mikearnaldi
Copy link
Member

The whole MixVariance/InitialVariance thing should be removed

@mikearnaldi
Copy link
Member

Variance is now fixed per parameter and doesn't take into account a fixed type (at least for now)

Parameters like S that are considered invariant should not mix at all, meaning in a signature you should not have S1 and S2 at all

@mikearnaldi
Copy link
Member

Also ideally every parameter has a meaning, it almost feel like we could get away with:

S = State (Invariant)
I = Category Input (Contravariant)
R = Requirements (Contravariant)
E = Error (Covariant)
A = Output (Covariant)

Any fixed type can be supported by transformers by having parametric URIs like:

interface KeyValueT<Key, Value, F extends HKT> {
  type: KeyValue<Key, Value, Kind<F, this[...]>
}

@mikearnaldi
Copy link
Member

I am not even too sure we want to keep the invariant S around, you could always have a:

interface StateT<S, F> {
   type: (s: S) => Kind<F, this["I"], this["R"], this["E"], [S, this["A"]]>
}

and would even allow mixing multiple different states

@isthatcentered
Copy link
Contributor Author

isthatcentered commented Feb 5, 2022

I am not even too sure we want to keep the invariant S around, you could always have a:

interface StateT<S, F> {
   type: (s: S) => Kind<F, this["I"], this["R"], this["E"], [S, this["A"]]>
}

and would even allow mixing multiple different states

Oh, that would be cool, the simpler the Kind signature the better!

And If we go down this road, should we keep the I parameter then? The parameters would kind of lose their "usage" meaning (requirements/error/output) but we still expose a Contravariant input, and a covariant output (+ error Covariant output for convenience even though it kind of goes against the idea).

Edit:
I might have said something dumb here, I have never used the I before. I'll give it a go on xpure this should make me sweat enough to fix that :D

@isthatcentered
Copy link
Contributor Author

I am not even too sure we want to keep the invariant S around, you could always have a:

interface StateT<S, F> {
   type: (s: S) => Kind<F, this["I"], this["R"], this["E"], [S, this["A"]]>
}

and would even allow mixing multiple different states

I gave it a try but it seems that would force us to fix the S type no?

import { pipe, tuple } from "../../Function/index.js"
import * as PV2 from "../../PreludeV2"

interface StateT<S, F> extends PV2.HKT {
  readonly type: (
    s: S
  ) => PV2.Kind<F, this["X"], this["I"], this["R"], this["E"], readonly [this["A"], S]>
}

export function monad<F extends PV2.HKT>(M: PV2.Monad<F>) {
  return <S>() =>
    PV2.instance<PV2.Monad<StateT<S, F>>>({
      any: () => (s) =>
        pipe(
          M.any(),
          M.map((m) => tuple(m, s))
        ),
      flatten: (ffa) => (x) =>
        pipe(
          x,
          ffa,
          PV2.DSL.chainF(M)(([f, us]) => f(us))
        ),
      map: (f) => (fa) => (x) =>
        pipe(
          x,
          fa,
          M.map(([a, s]) => tuple(f(a), s))
        )
    })
}

(Also, If Sgoes away, it should probably take X with it, now that I'm re-reading this)

@mikearnaldi
Copy link
Member

I gave it a try but it seems that would force us to fix the S type no?

That is the objective, S needs to be invariant anyway and given the lack of generics in constants it end up being forced to appear in every signature even things like unit that are then forced to be functions like unit<S>()

Even with S invariant you can't have a computation that has both state A and state B because you have a single S, "fixing" the param allows you to have many StateT<A, StateT<B, Bla>> they won't conflict and usually when you use state you really want to use a fixed state

@mikearnaldi
Copy link
Member

Oh, that would be cool, the simpler the Kind signature the better!

And If we go down this road, should we keep the I parameter then? The parameters would kind of lose their "usage" meaning (requirements/error/output) but we still expose a Contravariant input, and a covariant output (+ error Covariant output for convenience even though it kind of goes against the idea).

Edit: I might have said something dumb here, I have never used the I before. I'll give it a go on xpure this should make me sweat enough to fix that :D

Thought of I to model the input type of the category typeclass, that basically encodes I => A and A => B enables I => B

@isthatcentered
Copy link
Contributor Author

Notable changes:

  • Removed IndexFor and used params for all {...}WithIndex
  • Removed all implementXWithIndex functions

@shroomist
Copy link

is anything blocking this? I'm really looking forward to #1055

@mikearnaldi
Copy link
Member

Not really I am planning it for when system next is ready given it is a breaking change

import * as DSL from "../PreludeV2/DSL/index.js"
import * as P from "../PreludeV2/index.js"

interface StateT<A, B> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to do this otherwise type inference for A param was not working when using chain. Please let me know If there Is there a better way to fix this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very strange indeed, also to avoid errors down the line it is good to export all the used symbols

@@ -1,22 +0,0 @@
import { pipe } from "@effect-ts/system/Function"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't see any difference with example 10 besides InvariantT which, from what I understand, can be deleted with the new HKT system. (I'll test this assumption further though)

@@ -1,75 +0,0 @@
import * as M from "@effect-ts/system/Collections/Immutable/Map"
import * as Tp from "@effect-ts/system/Collections/Immutable/Tuple"
Copy link
Contributor Author

@isthatcentered isthatcentered Mar 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted example since I couldn't see any difference with example 11 after the move to PV2

@mikearnaldi
Copy link
Member

Currently updating all the examples in core/tests (I used a different test file while moving the instances) and, while I'm at It I'm also adding explicit types and expectations (It's always nice when you're reading the code on github and can't execute the code). Renaming/categorizing the examples would be cool too.

Few issues I'll have to investigate tomorrow:

  1. P.anyDSLMethod() fails with TypeError in tests (no clue why)
    Ex:
// effect/instances.ts
import * as P from "../../src/PreludeV2/index.js

export const { match, matchIn, matchMorph, matchTag, matchTagIn } =
  DSL.matchers<EffectF>()

// test.spect.ts
someMethodImportingEffectFiles() // 💥 TypeError

Can be fixed by accessing it via DSL root rather than prelude root

// effect/instances.ts
import * as DSL from "../../src/PreludeV2/DSL/index.js

export const { match, matchIn, matchMorph, matchTag, matchTagIn } =
  DSL.matchers<EffectF>()

// test.spect.ts
someMethodImportingEffectFiles() // 👌 All fine
  1. Transformers instance factory names (Ex: fail(F): Fail<EitherT<F>> can clash with the already exported function with that same name. I renamed Uppercased them (applicative(F) -> Applicative(F)). If anyone has a better idea please don't hesitat^^
  2. Transformers provide types don't match implementation. After provide, the nested HKT Rparam has a type of
    unknown (aka, provided too), but we only provide the transformer. I'm assuming that's fine since this is the current implementation but I wanted to point It out just in case.
  3. I'll have to go back to XPure tomorrow, usage feels weird now that every dsl method has to take a State parameter be struct<S>()(record).
  1. Sounds really strange but it would be a blocker if not solved.

  2. I don't understand this, what are the functions already exported with the same name, can you point to the code of 2 things that would conflict? function names only make sense in a single module not in general so "fail" in the context of Effect or on the context of OptionT may mean different things, I don't like the uppercase naming for functions and it would conflict with type names. The convention could be something like getApplicative but I do prefer simply applicative.

  3. please provide a code example I don't understand what you are referring to.

  4. Just remove the param S from the HKT and drop support for XPure. If someone wants to use State you can only use the parametric one with a fixed state

@mikearnaldi
Copy link
Member

mikearnaldi commented Mar 28, 2022

True, as an added bonus I think the full example appears when you hover over the method!

I have the opposite opinion here, the reason there are no inline examples in jsdoc in effect is because I truly don't like to see them in the on-hover (I have removed the few that were left from the port of fp-ts modules). I think we will find a middle ground with paka where @schickling is planning embeddable code in docs

@isthatcentered
Copy link
Contributor Author

  1. Sounds really strange but it would be a blocker if not solved.
  2. I don't understand this, what are the functions already exported with the same name, can you point to the code of 2 things that would conflict? function names only make sense in a single module not in general so "fail" in the context of Effect or on the context of OptionT may mean different things, I don't like the uppercase naming for functions and it would conflict with type names. The convention could be something like getApplicative but I do prefer simply applicative.
  3. please provide a code example I don't understand what you are referring to.
  4. Just remove the param S from the HKT and drop support for XPure. If someone wants to use State you can only use the parametric one with a fixed state
  1. I'll investigate and report, this is suspicious, If this is really an issue how come no one has surfaced It before.

  2. Actually after thinking a bit more about this the only time this can happen is with XPure, now that It accepts an S parameter, we have the original fail method + the fail method for making an instance of Fail. Can't happen with other transformers since they only expose instances. This is solved by your 4th point so back to the original naming which felt much better!

  3. One example would be XReader

export function Provide<F extends P.HKT>(_: P.Monad<F>) {
  return HKT.instance<FX.Provide<XReaderTF<F>>>({
    provide:
      <R>(r: R) =>
      <X, I, E, A>(fa: XR.XReader<R, HKT.Kind<F, X, I, R, E, A>>) => {
        return pipe(
          fa,
          XR.provideSome(() => r)
        ) as XR.XReader<unknown, HKT.Kind<F, X, I, unknown, E, A>> // <- Cast required, the result is typed XR.XReader<unknown, HKT.Kind<F, X, I, unknown, E, A>>
      }
  })
}
  1. I'll rephrase just to be sure, do you mean:
    a. Remove the instances in XPure/index.ts, If someone wants to use S they can default to XState.
    b. Modify XPure to drop state handling, people can use StateT if needed.

@isthatcentered
Copy link
Contributor Author

So, the DSL import issue definitely comes from something I've done. Not sure what yet but I'll find it

@isthatcentered
Copy link
Contributor Author

isthatcentered commented Mar 28, 2022

Annnd It's fixed, my IDE is drunk, I had imports of @effect-ts/core inside core.

@@ -207,57 +26,70 @@ export class Ix<I, O, A> {
constructor(readonly value: A) {}
}

export interface IndexedT<F extends P.HKT, _I, _O> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikearnaldi I removed the initial Indexed representation and exposed directly an IndexedT, let me know if that's an issue and I'll rework it :).

(also, this type is pretty cool and simple! I got lost in the types with the old version but the new system makes what happens very explicit)

@@ -48,7 +48,6 @@ export * as IO from "./IO/index.js"
export * as Id from "./Id/index.js"
export * as Identity from "./Identity/index.js"
export * as IndexedT from "./IndexedT/index.js"
export * as InvariantT from "./InvariantT/index.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikearnaldi Removed InvariantT, unless I'm mistaken this isn't needed with the new system

@isthatcentered
Copy link
Contributor Author

@mikearnaldi I think we're there, I'll have to take at everything again for the peace of mind but everything compiles, and all tests are passing (Ill update the PR with all the notable changes).

@isthatcentered
Copy link
Contributor Author

Moved to #1162 for a cleaner commit history (I did not want to kill the history for this PR)

@mikearnaldi mikearnaldi deleted the refactor/hkt branch April 9, 2022 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal for HKT restructuring
4 participants