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

4.3.2 regression involving branded types #32

Closed
m-rutter opened this issue May 29, 2021 · 9 comments
Closed

4.3.2 regression involving branded types #32

m-rutter opened this issue May 29, 2021 · 9 comments

Comments

@m-rutter
Copy link
Contributor

m-rutter commented May 29, 2021

Describe the bug
I'm not sure if this is a typescript or a ts-pattern bug, but I thought you might be best placed to triage.

I tried upgrading to typescript 4.3 the other day, and I hit some odd type errors in ts-pattern usages. I've managed to narrow and simplify it down to this example which involves branded types:

import { match, when } from "ts-pattern";
export type BrandedId = string & { __brand: "brandId" };
type FooBar = { type: "foo"; value: string } | { type: "bar" };

type State = {
  fooBar: FooBar;
  fooBarId: BrandedId;
};

declare const state: State;

match(state).with(
  { fooBar: { type: "foo" }, fooBarId: when((id) => id === "") },
  ({ fooBar: tempApp }) => { // 4.2 narrowed to the "foo" variant, 4.3 is still `FooBar`
    console.log(tempApp.value); // error in 4.3.x but not 4.2.x,
  }
);

4.4-dev playground (4.3-beta does not reproduce, and 4.3 isn't an option yet)

4.2 playground example
4.3-beta playground example

Versions

  • TypeScript version: 4.3.2 and 4.4 nightly
  • ts-pattern version: 3.1.6
  • environment: not relevant
@m-rutter m-rutter changed the title 4.3 regression involving branded types 4.3.2+ regression involving branded types May 29, 2021
@m-rutter m-rutter changed the title 4.3.2+ regression involving branded types 4.3.2 regression involving branded types May 29, 2021
@m-rutter
Copy link
Contributor Author

If you change the type of id from BrandedId to string, it works in both 4.2 and 4.3 and 4.4-nightly

@gvergnaud
Copy link
Owner

Thanks for the report! I had a quick look and it looks like ExtractPreciseValue doesn't return the correct type in this specific case anymore:

type inverted = InvertPattern<{
  fooBar: { type: 'foo' };
  fooBarId: GuardPattern<BrandedId, BrandedId>;
}>;
// inverted: { fooBar: { type: 'foo' }; fooBarId: BrandedId }. This is correct

type t = ExtractPreciseValue<State, inverted>;
// t: never :(

// Here is the test case that should pass:
type cases = [
  Expect<
    Equal<
      t,
      {
        fooBar: {
          type: 'foo';
          id: BrandedId;
          value: string;
        };
        fooBarId: BrandedId;
      }
    >
  >
];

Would you be interested in having a look at why this is a case and opening a PR if you find a way to fix it? It would be amazing to have more contributors than just myself on this project :) I would be happy to walk you through the code if you are interested and if you need some help

@gvergnaud
Copy link
Owner

gvergnaud commented May 30, 2021

I investigated a bit more on this issue and it looks like IsPlainObject<BrandedId> returns different results in 4.2 and in 4.3.

type BrandedId = string & { __brand: 'brandId' };

type BuiltInObjects =
  | Function
  | Error
  | Date
  | RegExp
  | Generator
  | { readonly [Symbol.toStringTag]: string };

export type IsPlainObject<o> = o extends object
  ? o extends BuiltInObjects
    ? false
    : true
  : false;

type x = IsPlainObject<BrandedId> // true in 4.4 and 4.3, false in 4.2 and 4.3-betas

4.2 Playground

4.4 beta Playground

That's really odd, because the result of BrandedId extends object and BrandedId extends BuiltInObjects are consistent between 4.2 and 4.3. We should maybe fill a bug report on the latest typescript version?

@gvergnaud
Copy link
Owner

@m-rutter I made a quick fix and released it in v3.1.7. Please, tell me if that works as expected on your project

@m-rutter
Copy link
Contributor Author

m-rutter commented May 31, 2021

@gvergnaud

Would you be interested in having a look at why this is a case and opening a PR if you find a way to fix it? It would be amazing to have more contributors than just myself on this project :) I would be happy to walk you through the code if you are interested and if you need some help

I think you might have already beat me to it, but thanks for the offer! I do want to look into the internals of ts-pattern if only to improve my type level programming, but also maybe to contribute a bit.

That's really odd, because the result of BrandedId extends object and BrandedId extends BuiltInObjects are consistent between 4.2 and 4.3. We should maybe fill a bug report on the latest typescript version?

Yeah, if I had to guess from your investigation findings this is a typescript regression. We should probably open an issue or search for an existing issue on the typescript issue tracker. The fact that this was introduced sometime between 4.3-beta and 4.3.2 and its not reported as a breaking change makes me think this wasn't an intentional breaking change. Branded types/new type patterns do kinda feel like a second class citizen in typescript and a bit hacky, so its not too surprising that they might have missed it.

@m-rutter I made a quick fix and released it in v3.1.7. Please, tell me if that works as expected on your project

Yep, that fixes it, but I still think its best to raise this upstream. If you want I can try searching for a relevant issue to link here, or create a new one.

@m-rutter
Copy link
Contributor Author

@gvergnaud reported it here: microsoft/TypeScript#44353

@m-rutter
Copy link
Contributor Author

@gvergnaud

I've been staring at the examples you provided, and I cannot help to conclude that it should be evaluating to true, and that false was a bug in 4.3-beta and earlier. Am I going crazy?

If BrandedId extends BuiltInObjects is false in all versions, then that means the inner ternary of IsPlainObject should go to the else branch, and thus evaluate to true

Am I going insane?

@gvergnaud
Copy link
Owner

gvergnaud commented May 31, 2021

No you are not! I think you are right, that was my conclusion too ;) That's why I ended up changing the code of ts-pattern rather than opening an issue on the TS repo. I think it's still a good thing that you opened this issue because it's better to let them know about this weird case and that it was broken in the previous versions.

@m-rutter
Copy link
Contributor Author

Maybe as a proper fix there should be some kind of guard for branded primitives.

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

No branches or pull requests

2 participants