'evolve' Breaks w/ Partial Structs

[anthonyjoeseph]

🐛 Bug report

evolve, as currently defined, exhibits strange behavior re: partial fields

Current Behavior

Given:

interface A {
  a?: 'a';
  b?: 'b';
  c?: 'c';
}

const a: A = {};
const b = F.pipe(
  a,
  evolve({
    a: fromNullable,
    b: fromNullable,
    c: fromNullable,
  })
);

runtime value of b = {}

Expected behavior

runtime value of b = {
  a: O.none,
  b: O.none,
  c: O.none
}

Reproducible example

TS Playground

Suggested solution(s)

New implementation for evolve (keeping the same type signature)

TS Playground

Additional context

Originally posted by @kalda341 here

Your environment

Which versions of fp-ts are affected by this issue? Did this work in previous versions of fp-ts?

Software	Version(s)
fp-ts	2.11.5
TypeScript	4.5.4

[qlonik]

Hey, it seems that the suggested solution would have a similar issue that some non-evolved fields won't be copied to the new object. In the playground, where you have const out = {}, it should probably be const out = {...a}

[anthonyjoeseph]

That's a great point, I had totally missed that, and great idea for a simple fix!

I think that requires a change to the type signature as well - here's an updated playground to reflect that

[kalda341]

I've also noticed that Eq is incorrect for partial structs. Maybe it's worth creating a separate partial struct module?

[anthonyjoeseph]

I'm not sure what you mean - how would Eq.struct be used with partial fields? Something like this?

import * as Eq from 'fp-ts/Eq'
import * as N from 'fp-ts/number'

const partialEq = Eq.struct({
  a: N.Eq,
  b: N.Eq
})

declare const partialObj: { a?: number; b: number }
const appliedEq = partialEq.equals({ b: 123 }, partialObj) // type error

[kalda341]

More like used like this:

const partialEq = Eq.struct<{
  a?: number;
  b?: number;
}>({
  a: N.Eq,
  b: N.Eq
})

I've been playing around with the following:

export const partialStructEq = <A>(eqs: {
  [K in keyof Required<A>]: Eq.Eq<Exclude<A[K], undefined>>;
}): Eq.Eq<{
  readonly [K in keyof A]: A[K];
}> =>
  Eq.fromEquals((first, second) => {
    for (const key in eqs) {
      const v1 = first[key];
      const v2 = second[key];

      if (v1 === undefined && v2 === undefined) {
        continue;
      } else if (v1 === undefined || v2 === undefined) {
        return false;
      } else if (
        !eqs[key].equals(
          v1 as Exclude<A[Extract<keyof A, string>], undefined>,
          v2 as Exclude<A[Extract<keyof A, string>], undefined>
        )
      ) {
        return false;
      }
    }
    return true;
  });

This allows checking equality on optional fields, without having to handle the undefined case. It also requires providing an Eq for each field, which the regular Eq.struct doesn't handle. I'm not 100% sure why I need the type cast, but I hacked this together pretty quickly.

Anyway, the specifics aren't super relevant - my point is more that most of the functions in fp-ts for structs don't handle optional fields elegantly (io-ts too, I can't count the number of times this specific question has come up: gcanti/io-ts#629).

[anthonyjoeseph]

Agreed that Eq.struct<{ a?: number }>({}) should not be allowed, and is unexpected behavior - all keys should be required

However, under this implementation

const keyExists = { a: 123, b: undefined }
const keyMissing = { a: 123 }

partialEq.equals(keyExists, keyMissing) === true

Which may not always be desired, since 'b' in keyExists === true and 'b' in keyMissing === false

Which actually raises a thorn in the original question - should keys always be copied, even if the original object doesn't have them? The word 'evolve' could be seen to imply that it would only affect what's already there.

monocle-ts handles the undefined/missing key disparity by having separate key and atKey operators, but we're not really able to have separate eq's per field in this way

I hate to say it, but maybe a more robust solution is disallowing partial fields in struct functions?

Either that or some interface like this (which is admittedly ugly)?

declare const withUndefined: <A>(eq: Eq.Eq<A>) => Eq.Eq<A | undefined>

const partialEq = Eq.struct<{
  a: number;
  b?: number;
}>({
  a: eqNumber,
  b: {
    eq: withUndefined(eqNumber),
    failWhenMissing: false
  }
})

[anthonyjoeseph]

There would theoretically also have to be a couple versions of withUndefined, an inclusive one where undefined === undefined, and an exclusive one where undefined !== undefined, since it can have a variety of meaning in different contexts

// A 'User' can be anonymous
interface User { email: string; name: string | undefined }
 
// might use inclusive here -- anonymous users have the same 'name'
const sameUser = (a: User, b: User): boolean => ...

// might use exclusive here -- an index-out-of-bounds is different from an anonymous user
declare const users: Users[]
const userName = (index: number): string | undefined => users[index]?.name
const sameName = (indexA: number, indexB: number): boolean => ...

[anthonyjoeseph]

Another idea is that we could take inspiration from monocle-ts atKey, and use an Option<Option<A>> type - the outer Option is none if the key is missing, and the inner Option is none if the value itself is undefined. So something like this:

declare const handleB: Eq<Option<Option<number>>>

const partialEq = Eq.struct<{
  a: number;
  b?: number;
}>({
  a: eqNumber,
  b: handleB
})

Imo that type, while complicated and possibly confusing, minimizes data loss and allows the user flexibility in all cases. And it's probably a preferred solution over preventing partial fields entirely.

The same idea would make the evolve function look like so:

interface A { a: number | undefined; b?: number; }

const a: A = { a: undefined };
const b = F.pipe(
  a,
  evolve({
    a: O.fromNullable,
    b: (b: O.Option<O.Option<number>>) => ...,
  })
);

[kalda341]

I actually quite like this, but I think it's a little arbitrary to represent undefined with an option in this case. It is nice to be able to treat individual fields differently, but I do wonder how often it would be necessary.

I think this is devolving into a discussion about value of undefined vs missing key? It would be amazing to have some consistency on this within fp-ts (and ideally the rest of the ecosystem). Personally I hate treating them differently, especially in functional programming in Javascript due to the difficulty of removing a key from a struct without mutating the original value. The treatment of undefined is probably my biggest gripe with JS in general.

Typescript (by default: https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#exact-optional-property-types) treats the two as more or less the same in many situations, which makes me wonder whether we should do the same.
Alternatively it may be a good idea to specify that that compiler flag should be enabled on projects with fp-ts?

[kalda341]

You've also got me thinking about how I'm contradicting myself with how I want Eq and evolve implemented (Eq functions shouldn't be passed undefined, but evolve should). We haven't even talked about what we want for Semigroup, Monoid, etc, which are also not totally clear.

[anthonyjoeseph]

I think a lot of fp-ts users would agree w/ you that js's treatment and use of undefined is frustrating - that's why we've avoided it in favor of Option, and to some extent pretended it wasn't there (thus this issue)

Oh wow I didn't know about 'exact optional property types'. That could even motivate the use of Option instead of undefined in these cases - a missing key really isn't an undefined value at all. Imo that flag is proof that some developers want to treat 'missing key' as separate from 'undefined'

Maybe it makes sense to conditionally use a type of Option<A | undefined> or Option<A> depending on whether that property is enabled. Something like this, maybe? Playground (excludedOptionalPropertyTypes enabled), Playground (excludedOptionalPropertyTypes disabled)

[kalda341]

Really interesting that you can actually test whether the flag is enabled. That might be a good solution, however I wouldn't know for sure until I have some time to play around with some implementations.

[anthonyjoeseph]

You've got a good instinct, it turns out what I wrote is impossible to achieve at runtime, since you can't know whether a key is partial or not at the value level.

I think the simplest solution here is to take inspiration from io-ts and have separate 'struct' and 'partial' functions, and merge these together thru the Eq monoid - playground

As for monoid and semigroup, we might introduce an 'intersection' combinator to allow the same pattern (also similar to io-ts), which could be useful otherwise - playground

Evolve is a bit more complicated - we have to be able to separate partial and unpartial fields & handle them separately - Playground

The evolve use-case seems uncommon enough that I'm not sure how useful it would end up being, and it looks like a bit of a nightmare to maintain. Maybe it's best to simply ignore missing keys in this case - something like the fix proposed earlier

[kalda341]

Your Eq and Semigroup implementations look very, very good, and I think keeping the struct and partial functions separate is a pretty clean way of doing it.

Your evolve implementation really tested my Typescript comprehension, and I agree that it might end up a maintenance nightmare. By ignore, you mean forbidding optional keys? That could be a decent solution, and it may not be worth the extra effort of supporting them for evolve.

Thanks so much for putting so much effort into this (much more than even me!). It's really appreciated.

[anthonyjoeseph]

That's great, I'm glad you like them! Ok it sounds like we have

Just a note: this already exists for Eq, inside the module io-ts. Maybe 'semigroup' and 'monoid' also belong there? Although it's hard to see how those are 'decoding' or 'encoding' values in a runtime type system