Rewrite Equal to use the equality check from ReadonlyEquivalent exclusively

[trevorade]

Fixes #29
Fixes #26
Fixes #5

Note: I extracted a very small part of this PR to #20

This is a breaking change as I opted to remove the types that were no longer needed. They are exported though so it's likely some people depend on them. I can add these back as desired.

This took a lot of tinkering. This topic and this equality check is discussed extensively at microsoft/TypeScript#27024

The main three edge-cases this implementation worked around are:

1. Explicitly handling any separately
2. Supporting identity unions
3. Supporting identity intersections

The only remaining known issue with this implementation is:

  // @ts-expect-error This is the bug.
  expectTypeOf<{foo: number} & {bar: string}>().toEqualTypeOf<{foo: number; bar: string}>()

@shicks and I could not find a tweak to the Equality check to make this work.

Instead, I added a workaround in the shape of a new .simplified modifier that works similar to .not:

  // The workaround is the new optional .simplified modifier.
  expectTypeOf<{foo: number} & {bar: string}>().simplified.toEqualTypeOf<{foo: number; bar: string}>()

I'm not entirely sure what to do with documenting .simplified because it's something you should never use unless you need it. The simplify operation tends to lose information about the types being tested (e.g., functions become {} and classes lose their constructors). I'll definitely update this PR to reference the .simplified modifier but I wanted to get a review on this approach first. One option would be to keep around all the DeepBrand stuff and to have .deepBranded or something being the modifier instead. That would have the benefit of preserving all the exported types making this less of a breaking change.

[trevorade]

Just an anecdote, this PR is compatible with all of the type-fest build tests (in our repo, we have an automation step which rewrites the tsd expectations to use expectTypeOf).

My previous reverted PR ran afoul of one of merge-deep.ts test cases which had too much nesting. This PR does not have that problem.

[papb]

Hi @trevorade! I am against .simplified, for a few reasons (pick the one that convinces you the most, I guess).

1. I like to consider { a: 1 } & { b: 2 } identical to { a: 1, b: 2 }. Not just "simplifiedly"-identical. Really identical. I have not seen to this day any observable difference between the two. Therefore, I think expectTypeOf<{ a: 1 } & { b: 2 }>().toEqualTypeOf({ a: 1, b: 2 }) must work, if not, it is a bug.

2. With this PR, given a type T, there is not a single correct way of testing the equality between the types { a: 1 } & { b: () => true } and { a: 1, b: () => T }. If T is true, .simplified must be used. If T is anything else, .simplified must not be used.

3. I may agree with what you said that "you should never use unless you need it", but the only way for someone to "know" they need it is if they've previously manually analyzed the types, in which case the test becomes meaningless at best (since it is just "masking" an actual manual test someone did - and can get outdated).

4. From what I've seen from the source code of your PR, it seems your implementation of Simplified<T> is not recursive, so it wouldn't catch { a: { x: 1 } & { y: 2 } } and { a: { x: 1, y: 2 } }. Even though this example can be fixed easily, I don't see any easy way to cover all complex recursive cases, such as intersections in return types of functions, high-order functions, the special thisArg, and so on.

5. With this PR, expectTypeOf<A>().simplified.toEqualTypeOf<B>() would pass for type A = () => true and type B = Record<never, number>. To me this is unacceptable, and makes .simplified impossible to be trusted, therefore I would never write a test using it.

[papb]

1. I like to consider { a: 1 } & { b: 2 } identical to { a: 1, b: 2 }. Not just "simplifiedly"-identical. Really identical. I have not seen to this day any observable difference between the two. Therefore, I think expectTypeOf<{ a: 1 } & { b: 2 }>().toEqualTypeOf({ a: 1, b: 2 }) must work, if not, it is a bug.

By the way, I would think we had a test for this already, but it seems we don't (since I didn't see you commenting out this test on the PR changes)?

[papb]

Also, maybe I should have asked this before anything else, but why did you want to make this change? Is it just to work around #5?

[shicks]

@papb I agree that Simplify as written definitely has some trustability problems. But I'd also point out that DeepBrand is not without issues as well. We've patched a number of them, but the fact remains that it's a very kludgy hodgepodge that's both very inefficient (we found massive speed-ups with using the new direct comparison version) and yet still has potential holes (i.e. readonly properties in parameters/returns of overloaded functions simply can't be handled correctly).

I could see making the DeepBrand-based version available when needed (instead of just simplified). It unfortunately means we can't delete this very complicated bit of code that's only used for the one thing - but it would provide an alternative when the direct comparison has problems. I do strongly believe the direct equality check is the best default, since it's basically just deferring the check directly to the type checker, rather than hacking something together.

[mmkal]

Sorry for the delay in getting to this, hoping to find time to dive in in the next week.

@shicks when you say direct equality check what do you mean exactly? TS doesn't have that built in as far as I'm aware, all we really have to play with is extends. Could you give an example of how you'd want it to look?

Definitely agreed with the general concern with how DeepBrand has grown and grown, slowly taking over everything. And every edge case seems to add to its complexity, making it harder to maintain and most likely slowing the compiler down. It started as a way to distinguish between hairy types like any/unknown/never - which I do think is really important and should be the default behaviour. This library wouldn't be doing its job if it let anys make things go green secretly.

[mmkal]

A general thought that's based on the comments rather than the code. I agree with @papb that .simplified is a confusing API we should try to avoid if at all possible. @shicks it sounded like your answer was about the Simplified helper type, which I would think is more to do with the internal implementation of this library than the API. Internal implementation I'm very open to bigger changes in.

[trevorade]

I need to find some time to update this draft. I think what I'd want to do would be to keep around DeepBrand but have it not be the default. We could also un-revert my previous PR which improves DeepBrand (the problems there were not real problems, just an excessive amount of type nesting in the particular test I was looking at). Retaining DeepBrand also has the benefit of keeping the API of exported types stable.

Accessing the DeepBrand variant could be done with something like:

expectTypeOf<Foo>().branded.toEqualTypeOf<Bar>();

Honestly, nearly all examples we could find worked fine with this PR's new equality check with the exception of the intersection case noted above. This PR is more performant and covers several cases DeepBrand does not. So I think it's still a win.

TS really just needs an equality check in their typing system 😛

[trevorade]

Updated to keep around DeepBrand. Removed Simplified. Added a .branded chain.

This has the same ergonomic issues as the .simplified chain.

Main benefit here is that all the other exported types beyond expectTypeOf are now unchanged with this commit. expectTypeOf is a breaking change though in the off chance that people are checking one of the few cases where this new version fails.

As an alternative to the .branded chain, how would people feel about adding expectBrandedTypeOf as an exported function?

[trevorade]

@mmkal, @papb Please take another look.

[mmkal]

I really like this. I still need to go through the implementation, but I have very high confidence based on the tests, and not being (properly) breaking makes it much easier to say "yes" to. This is definitely a net-good change.

A couple of comments/requests:

• could you add a simplified version of the .branded test to usage.test.ts? maybe with inline comments so they appear on the readme, including a link to the typescript issue? Or could even put a markdown link as the rest title and it'll render nicely on the readme. I like the pattern of treating usage.test.ts as features that users should know about, and types.test.ts being edge-cases that should "just work". We could still keep the detailed test you have in types.test.ts if there's a simplified version that'd be more presentable (e.g. no need to test expectTypeOf<...>() and expectTypeOf(...) in usage.test.ts)
• I'm fine with this kind of very small breaking change. The lib is v0 anyway so not even a semver violation to publish as a new minor.
• could you update the PR description so it reflects what this change now does? I've set the squashed-commit to use the PR body as git commit message so will be a useful breadcrumb for people who update and get hit by new failures.

[mmkal]

Some minor comments on the implementation part

[mmkal]

🚀🚀🚀

[trevorade]

Outdated though...

If you did the .Equal not branded, it would work

[mmkal]

Is there a link to a stack overflow/blog post/docs page that explains this trick or something similar? And/or an explanation of why we can't just use T. It's befuddling(-looking) enough to warrant a comment.

[mmkal]

Maybe we should merge in a separate PR that renames B to Positive or something? I feel like I've seen that elsewhere on one of the PRs recently, am I remembering that right?

[trevorade]

I'm guessing you're thinking of #13

In that one, I renamed it to ExpectTypeOf<Actual, ExpectedResult extends boolean>

I could incorporate parts of that PR to just the generic renaming.

[papb]

yet still has potential holes (i.e. readonly properties in parameters/returns of overloaded functions simply can't be handled correctly)

I didn't know this, can someone show an example please?

[papb]

Updated to keep around DeepBrand. Removed Simplified. Added a .branded chain.

This has the same ergonomic issues as the .simplified chain.

Main benefit here is that all the other exported types beyond expectTypeOf are now unchanged with this commit. expectTypeOf is a breaking change though in the off chance that people are checking one of the few cases where this new version fails.

If I understood correctly, from the point of view of expectTypeOf, what changed since my last comment is:

• What previously was expectTypeOf().toEqualTypeOf() is now expectTypeOf().branded.toEqualTypeOf()
• What previously was expectTypeOf().simplified.toEqualTypeOf() is now expectTypeOf().toEqualTypeOf()

Did I understand correctly? If so, unfortunately I have to say that all my objections still stand, since only the way of calling the function changed, not the behavior per se.

However, considering that...

• The DeepBrand method already has this known flaw
• Apparently there is another flaw that I don't know of

...then I may concede that none of these two solutions are exactly correct, and that we might

• document this feature as slightly imprecise (and justify its existence as the best effort available for now, and invite users to show interest in the appropriate TS issue)
• provide both options to the users
• provide a thorough documentation on how to choose and what are the risks
  • Showing false expectation-passes with the DeepBrand method
  • Showing false expectation-failures with the DeepBrand method
  • Showing false expectation-failures with the "generic-extends-trick" method

[papb]

I have very high confidence based on the tests, and not being (properly) breaking makes it much easier to say "yes" to.

@mmkal The reason I don't agree with this sentence is that the test for expectTypeOf<{ a: 1 } & { b: 2 }>().toEqualTypeOf({ a: 1, b: 2 }) should exist in the first place. Since it doesn't, which I think is a mistake, this PR gives the impression of not being a breaking change.

[trevorade]

@papb Note that all of the tests I add in this PR fail before this change. These tests also passed with my previous PR 304a54b which improved DeepBrand but the complexity of DeepBrand started hitting the ceiling of how much work tsc can do in type-land. I reverted it in a6d1f99 because it was causing tests to fail for type-fest with some of their types (we use expect-type instead of tsd to run the type-fest tests in our codebase).

DeepBrand is just very unwieldily and slow.

We're in a situation with: pick 2:

1. Fast
2. Covers most cases
3. Covers {a: 1} & {b: 2} equals {a: 1, b: 2}

This PR gets you 1. and 2.

We could un-revert 304a54b and we'd presumably get 2. and 3. but at the cost of compile speed and not working with more complex types.

---

Lastly, note that even my previous PR doesn't work around microsoft/TypeScript#50670 whereas this PR does.

It's pretty crappy that TS just doesn't have an equality check for types. At the moment, there is just no perfect equality check that covers all cases. I find that not covering 3. is acceptable for this library.

[shicks]

@papb wrote:

yet still has potential holes (i.e. readonly properties in parameters/returns of overloaded functions simply can't be handled correctly)

I didn't know this, can someone show an example please?

{
  // Overloads indistinguishable from non-overloads.
  interface A {
    foo(a: string): string;
    foo(a: number): number;
  }
  interface B {
    foo(a: number): number;
  }
  expectTypeOf<A>().toEqualTypeOf<B>(); // should fail, but passes
}
{
  // Generics indistinguishable from non-generics.
  interface A {
    foo<T>(a: T): T;
  }
  interface B {
    foo(a: unknown): unknown;
  }
  expectTypeOf<A>().toEqualTypeOf<B>(); // should fail, but passes
}
{
  // Readonly properties in parameters indistinguishable.
  interface A {
    foo(a: {readonly a: number}): number;
  }
  interface B {
    foo(a: {a: number}): number;
  }
  expectTypeOf<A>().toEqualTypeOf<B>(); // should fail, but passes
}
{
  // These are actually the same type, but DeepBrand can't figure it out, either.
  type Digit = 0|1|2|3|4|5|6|7|8|9;
  type A = `${number}${string}`;
  type B = `${Digit}${string}`;
  expectTypeOf<A>().toEqualTypeOfBranded<B>(); // should pass, but fails
}

Here are some examples of how the current DeepBrand-based implementation doesn't work as you might hope/expect (playground link).

I have very high confidence based on the tests, and not being (properly) breaking makes it much easier to say "yes" to.

@mmkal The reason I don't agree with this sentence is that the test for expectTypeOf<{ a: 1 } & { b: 2 }>().toEqualTypeOf({ a: 1, b: 2 }) should exist in the first place. Since it doesn't, which I think is a mistake, this PR gives the impression of not being a breaking change.

Ultimately this comes down to "what does equal mean?" It's true that I'm not aware of any other way to distinguish {a:1}&{b:2} from {a:1,b:2}, aside from the one here. But clearly TypeScript represents them differently internally, or else we wouldn't have this problem. In most cases, TypeScript treats optional/readonly as irrelevant, so there's a slippery slope of which reductions you want to allow in "equals" versus which you want to preserve. As we're all aware, there is no good way to ask the type checker if these two types will be treated exactly identically. It's probably not even a decidable question, given that the type system is apparently Turing complete. So I think it makes a lot of sense to expose the lowest-level check we possibly can, and then if you want to force it to apply certain transformations (i.e. look at it on a per-field level, etc) then you can opt into that explicitly.

[trevorade]

@mmkal Another proposal.

We remove .branded and DeepBrand and add the following:

export type MutuallyAssignable<Actual, Expected> =
    [Actual, Expected] extends [Expected, Actual] ? true : false;
...
export interface ExpectTypeOf<Actual, B extends boolean> {
...
  toAssignMutuallyWithTypeOf: {
    <Expected>(...MISMATCH: MismatchArgs<MutuallyAssignable<Actual, Expected>, B>): true
    <Expected>(expected: Expected, ...MISMATCH: MismatchArgs<MutuallyAssignable<Actual, Expected>, B>): true
  }

This doesn't have protections against any or never because it doesn't need to. let a: any and let b: number are mutually assignable to each other.

In the off-case that you run into something like {a: 1} & {b: 2} not being equal to {a:1, b:2}, you can use toAssignMutuallyWithTypeOf. It's a similar check but not as strong as toEqualTypeOf.

This simplifies expect-type quite a bit by removing DeepBrand and removes the confusion of the .branded chain method. If we do want an alternative to toEqualTypeOf, I think it would be best were it as simple and non-magic as possible. I think MutuallyAssignable fits that bill.

[mmkal]

I have very high confidence based on the tests, and not being (properly) breaking makes it much easier to say "yes" to.

@mmkal The reason I don't agree with this sentence is that the test for expectTypeOf<{ a: 1 } & { b: 2 }>().toEqualTypeOf({ a: 1, b: 2 }) should exist in the first place. Since it doesn't, which I think is a mistake, this PR gives the impression of not being a breaking change.

@papb good point, and sorry to have forgotten this. I'm currently on paternity leave so git history will be much more reliable than my memory.

@trevorade in the tradeoff between fast and correct I'd prefer correct, but it does seem that "slow" also exposes tsc limitations which also means "not correct".

To make clear the tradeoff, @papb would you be open to a simple PR adding a test for the {a: 1} & {b: 2} case? That would force this PR to at least disable that test, explicitly, so we could see what we're losing.

A couple more thoughts that come to mind:

1. We could support both modes, and use some kind of pseudo-higher-kinded type via module augmentation to allow advanced users to opt in to a speed boost if they are prepared to sacrifice & working properly. I did something similar here: https://github.com/mmkal/handy-redis/blob/39f4f823c7d169491f93aa087fca7d122fe9d016/src/node_redis/multi.ts#L7-L22 and fp-ts has a more advanced implementation.
2. In either case we should probably have DeepBrand or its replacement force an error when we know it's going "too deep" for tsc to handle so we don't get bad types being marked as ok.

For either one, we should aim to have it "just work" for all the simple use cases in usage.test.ts if we possibly can. We might also want a limitations section of the documentation (maybe also generated from tests) so we can be honest and clear about what problems you might hit once you start trying to test big, complex types.

[mmkal]

@trevorade I finally had a chance to dig into this (and experiment with an alternative in #27). Everything I was able to come up with that technically covers all edge cases had serious performance problems, so because this implementation is so dramatically much simpler, I'm inclined to agree that we just have the & problem as a documented limitation. I also think that in most cases there are simpler workarounds than branded though. For example this workaround is available in simple cases:

type A = {a: 1}
type B = {b: 1}
type Intersection = A & B
expectTypeOf<Intersection>().toEqualTypeOf<{a: 1; b: 1}>()
expectTypeOf<{[K in keyof Intersection]: Intersection[K]}>().toEqualTypeOf<{a: 1; b: 1}>()

Is that what the original simplified was doing, more or less? I can't find the original commit.

[trevorade]

It's been a while so I can't remember what .simplified was doing exactly. It's annoying that GitHub does not preserve older PR versions.

This current state of this PR has .branded. I think it would be better to clean up this file as much as possible and just remove the brand-related code.

Unfortunately, I don't really have capacity these days to help out with changes here. Feel free to create a new commit based on this or take over this one (if you can?)

[mmkal]

Sounds good - will see if I can simplify, and merge when it's ready

[gomain]

When will this be released?

[unional]

Covers {a: 1} & {b: 2} equals {a: 1, b: 2}

Just for sharing, I have it working in type-plus

[mmkal]

When will this be released?

@gomain in the next few days. Have been out!

[mmkal]

Covers {a: 1} & {b: 2} equals {a: 1, b: 2}

Just for sharing, I have it working in type-plus

@unional while that does cover the specific test that was added to this PR to document the limitation, I don't think I would like a workaround like that to be included in expect-type. Really, it's just kicking the can down the road where it's less visible and harder to debug, since it's just explicitly looking for top-level intersections and handling them manually. Nested properties with intersection types will meet the same problem. That may be ok for your use case - I'm not familiar with type-pluts - but I think it'd be confusing here. I opened a PR to demonstrate/document this: unional/type-plus#302

[mmkal]

@gomain / others on this thread - this was released in v0.16.0