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

Conditional Types - Checking extends never only works sometimes #23182

Closed
dsherret opened this issue Apr 5, 2018 · 10 comments
Closed

Conditional Types - Checking extends never only works sometimes #23182

dsherret opened this issue Apr 5, 2018 · 10 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@dsherret
Copy link
Contributor

dsherret commented Apr 5, 2018

TypeScript Version: 2.9.0-dev.20180405

Search Terms: "extends never"

Code

I am writing a function for testing conditional types and I'd like to write it like this:

function assert<T extends true>(expectTrue: T extends never ? false : true) {
}

// or even
function assert<T extends true>(expectTrue: T extends true ? true : false) {
}

That way I can write:

type IsNullableType<T> = T extends null | undefined ? true : never;

const result = functionUnderTest(someParam);

assert<IsNullableType<typeof result>>(true);
// or
assert<IsNullableType<typeof result>>(false);

So given this:

assert<IsNullableType<string>>(false);

Expected behavior:

  • The type of the parameter resolves to false.
  • No error.

Actual behavior:

  • The type of the parameter resolves to never.
  • false is not assignable to never.

Example where extends never works:

type IsNonNullableType<T> = IsNullableType<T> extends never ? true : never;
assert<IsNonNullableType<string>>(true); // no compile error, works fine

// this doesn't cause a compile error in playground, but it correctly throws a compile error in the latest version
assert<IsNonNullableType<string | undefined>>(true); // compile error, as expected

Playground Link: Link.

Basically, an IsNeverType check does not work: type IsNeverType<T> = T extends never ? true : never;


Edit: By the way, this is probably a better way to do my check:

export type IsNullableType<T> = Extract<T, null | undefined> extends never ? false : true;
@dsherret
Copy link
Contributor Author

dsherret commented Apr 5, 2018

I'm just thinking this might be expected behaviour. So when a type is passed never (as T) and there's an expression like T extends ... ? ... : ..., then it will resolve that entire T extends ... ? ... : ... to never?

Anyway, I resolved my issue by making everything work with true or false, which is what I probably should have been doing from the start anyway... just couldn't figure out the right way to do it, but Extract helped.

@jack-williams
Copy link
Collaborator

jack-williams commented Apr 5, 2018

IsNeverType needs to be implemented like:

type IsNeverType<T> = [T] extends [never] ? true : never;

to stop it being distributive.

@RyanCavanaugh
Copy link
Member

Seems like a bug to me?

// Actual instantiation of 'T' shouldn't matter - conditional returns 'number' either way
function assert<T>(expectTrue: T extends never ? number : number) { }
// OK
assert<boolean>(0);
assert<string>(0);
assert<true>(0);
// Error; argument of type '0' not assignable to 'never'
assert<never>(0);

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Apr 5, 2018
@jack-williams
Copy link
Collaborator

jack-williams commented Apr 5, 2018

As far as my understanding goes the conditional doesn't always return number, it can also return never because it may distribute over nothing.

// Ensure conditional is always `number`.
function assert<T>(expectTrue: [T] extends [never] ? number : number) { }
// OK
assert<boolean>(0);
assert<string>(0);
assert<true>(0);
assert<never>(0);

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Apr 5, 2018
@RyanCavanaugh
Copy link
Member

After reading the code I agree with @jack-williams 's assessment. In this case never behaves as the empty union, so it distributes over the conditional and produces another empty union (which is just never again)

@dsherret
Copy link
Contributor Author

dsherret commented Apr 5, 2018

@RyanCavanaugh good to know! Thanks for looking into this and explaining the behaviour.

@jack-williams thanks a lot for that trick! Very helpful! 😄

@dsherret dsherret closed this as completed Apr 5, 2018
@jcalz
Copy link
Contributor

jcalz commented Jun 6, 2018

Holy cow, this just bit me. The empty union!

@pie6k
Copy link

pie6k commented Feb 7, 2020

I've created simple helper type

type NeverAlternative<T, P, N> = [T] extends [never] ? P : N;

// and then
type VariableLengthArray<A, B = never, C = never> =
  //
  NeverAlternative<
    C,
    NeverAlternative<B, [A], [A, B]>,
    NeverAlternative<
      B,
      [A, C],
      [A, B, C]
    >
  >;

type Foo = VariableLengthArray<1,2> // type [1, 2]
type Bar = VariableLengthArray<1,2,3> // type [1, 2, 3]

@loynoir
Copy link

loynoir commented Nov 11, 2021

@jack-williams WOW! Awesome! Thanks a lot!

Is it safe to change return never to return false?

// type IsNeverType<T> = [T] extends [never] ? true : never;
type IsNeverType<T> = [T] extends [never] ? true : false;

@SyMind
Copy link

SyMind commented Sep 13, 2022

After reading the code I agree with @jack-williams 's assessment. In this case never behaves as the empty union, so it distributes over the conditional and produces another empty union (which is just never again)

Where is the code to do these things? The TypeScript checker code is very complex. I can't find the specific code.

juanluispaz added a commit to juanluispaz/ts-sql-query that referenced this issue Apr 8, 2023
Offroaders123 added a commit to Offroaders123/NBTify that referenced this issue Aug 7, 2023
Wow. Wow wow wow! Went down the rabbit hole with discussions in the MM discord, it was great! Thanks @HoldYourWaffle!

I was gonna include more of our conversations, but there were so many I don't know if it would make sense here haha. Super productive, really fun.

I tried asserting the return type to reflect whether the input generic is valid/unset or not, but it's not reliable to work with, because it widens the types too much.

Essentially:
```ts
type SafeNever<T> = [T] extends [never] ? unknown : T;

export declare function parse<T extends RootTag = never>(data: string): SafeNever<T>;
```

But this doesn't work well/breaks when working with `NBTData`, which doesn't allow `unknown` as an input generic type (which is correct).

```ts
// @ts-expect-error
export declare function read<T extends RootTag = never>(data: Uint8Array): Promise<NBTData<<SafeNever>>>;
```

So, the next step was to try the other option available, move the generic type check out of the generic and into the return type. But this isn't realistic/reliable, and also hard to work with like this first one.

```ts
export declare function parse<T>(data: string): T extends RootTag ? T : unknown;
```

This worked semi-nicely with incorrect values, because if they weren't `RootTag`-compatible, the return value would always be `unknown`. Perfect! But a great point that HoldYourWaffle pointed out was that it doesn't correctly represent the control flow of the function path, because `unknown` is not the same as `never` (not talking types, but actions.)

```ts
const gg: unknown = parse<5>("5");
```

While the type is 'correct' because it isn't a shape, it's not really correct because it should show an error here, mirroring what it would do in run time. Because '5' isn't a valid SNBT string, it would 'never' complete, meaning that `unknown` isn't correct because that means it did run as expected, you just don't know the value.

So that's not ideal, and not accurate. It also didn't help that the same `NBTData`-and-the-like parameter type issues were happening here, because the return type has a possible `unknown` in it, which isn't accepted by the other parameters for the API (again, which is correct).

So then we came back to the setup to how this commit looks right now, where the generics don't have default values. This means that if unsupplied, the return types for the functions are `RootTag`, which didn't *seem* too bad. But `CompoundTag` is build with an index-signature, and it's paired with `ListTag`, which is built from the `Array<Tag>` type. Sounds good and dandy, but turns out that an index-signature can validate alongside that one in a union type, meaning that a plain `RootTag` declaration allows you to (type-wise) access methods and values on the value which might not exist! What the heck!

```ts
// hallucinate.ts

export type RootTag = CompoundTag | ListTag<Tag>;

export type Tag = string | number | boolean | ListTag<Tag> | CompoundTag;

export interface ListTag<T extends Tag> extends Array<T> {}

export interface CompoundTag {
  // Try commenting out this index signature line
  [name: string]: Tag;
}

declare const gg: RootTag;

gg.concat;
gg.length;

// @ts-expect-error
gg.concat();
```

So you can see the methods aren't callable, but it does let you access properties that don't exist. This is the case even with `noPropertyAccessFromIndexSignature` and `noUncheckedIndexAccess` (I think that one has more to do with arrays, but yeah).

So the realisticness I decided out of the different options are that there are a few different ways to take this decision:
(I think I mentioned it in #32, but yeah)

If the generic was optional, the catch for the implicit nature of the generic is when you use it, it will be `unknown`
But making the generic non-optional means you have to decide ahead of time whether it's strongly-typed, or turtle-brained as `any`
So it moves the first step to the beginning, when the variable is defined, not when it's accessed
So it removes `unknown` from it altogether
Optional Generic state: either `unknown`, `interface ThisIsMyShape`, or `any`
Non-Optional Generic state: `ERROR: ts(missing generic parameter)`, `interface ThisIsMyShape`, or `any`

Now that we know that `unknown` and `never` aren't possible/not feasable/realistic, the three options are either: `interface ThisIsMyShape`, or defaulting back to `any`. If function generics could be made mantatory, then I could essentially enable the 'unknown' state, where it would show as a missing generic when used without a supplied generic, like `NBTData` does, because classes *can* have required generics.

```ts
// @ts-i-want-this-to-error - If you could make this generic mandatory, the return type could be `unknown` if you didn't supply a generic value, rather than being `RootTag`, which is an option we've narrowed out.
export declare function parse<T extends RootTag>(data: string): T;
```

So, if:
- `never` can't be returned, nor used reliably
- `unknown` can't be returned, nor used reliably
- The return type can't be narrowed, nor used reliably
- `RootTag` can't be checked outside of the generic/used reliably

That leaves it back to either defaulting the generic to `RootTag`, or `any`. And for me, I think making it consistent is the most important part. `RootTag` can make up values altogether, which could be misleading. But if you plain just see an `any` there, you know for a fact that anything goes, so you *know* it will hallucinate values, because that's what it does. It doesn't mislead you to values that are/might not be there.

So while `any` is unsafe because it allows you to traverse it as anything, I think that's better than traversing it as what you *think* it is. That's not the purpose of typing it out; If you want to type it out just for the sake of making it not `any`, that seems more dangerous to me, because that's saying that you know that `RootTag` is. But `any` just plain says, "hey, all rules are gone here, beware".

`unknown` would be the best scenario here for the return type, because it would warn you if you are trying to use the return type as if it were `any` implicitly. It would be the best to have the user specify that they intend to use `any` on purpose.

Wow, learned a lot from this! Thanks again to @HoldYourWaffle for helping discern all of this in the last day or so. Can't believe how much our group of 'turtle-brains' over at Minecraft Manipulator are good at getting stuff done. We might be slow, and or quiet sometimes at things, but when we get movin', oh man!

https://stackoverflow.com/questions/71193522/typescript-inferred-never-is-not-never
microsoft/TypeScript#23182 (comment)
https://www.typescriptlang.org/play#example/unknown-and-never
https://stackoverflow.com/questions/55610260/how-to-make-generics-mandatory
https://stackoverflow.com/questions/69763621/check-if-object-is-empty-using-typescript-generic

https://www.youtube.com/watch?v=J1f5b4vcxCQ

The next thing we started talking about after this commit-and-related-to-it, was whether NBT data should be represented using primitives, or wrapper objects with methods and such. I think it boiled down to that I like primitives for declaring new data structures, but primitive wrappers are probably the better choice for working with the data itself. Think of things like `Array.prototype.push()`. You wouldn't call it as a standalone function, or as a static method, it doesn't really make sense to do it that way.

```ts
const thingo: number[] = [];
thingo.push(5);

Array.push(thingo,5);

pushArray(thingo,5);
```

HoldYourWaffle:
> Yes, composing NBT from scratch is much nicer with the primitive variant, no argument there.
> The thing is: you're not doing that regularly, at least not as much as manipulating the data, especially for a project called Minecraft *Manipulator* :)
>
> Secondly: the manipulation part of any program is much more intricate, bug-prone and maintenance-heavy than composing some static-ish data. When debugging shenanigans or implementing new features I need to be able to read, interpret, understand and write manipulation code quickly
> Finally: while the "primitive composing" is certainly nicer, the alternative is not that bad. The "convenience method manipulation" variant has a lot of advantages compared to the "functional" alternative though.

Me:
> That's a great argument, and I think I have to convince myself now that it makes more sense

HoldYourWaffle:
> 🔥

I think it's a great argument, I think my holdup is to whether NBT is something that needs an 'abstract' representation. I have felt like that using methods around the primitives allows for more composability, and you can build things together better using plain functions to manipulate the structures, rather than instance methods on the data abstractions themselves.

Me:
> I was gonna say, I want to look into classes for these again, if we do it right
> Cause before I don't like how it turned out *(about NBTify's primitive wrappers from last year about)*
> So once we start to see where having classes could be helpful, then we can move things over properly to fill in the gaps

About where I think classes should be used/concerns about instance methods:

```ts
export declare class Thingo {
  constructor(value: string);
  noice(): number;
}

const noice: number = new Thingo("heya").noice();

// vs - this example is so cursed I'm sorry

export declare function toNoice(value: string): number;

const noice: number = noice("heya");
```

Ok, the example doesn't show it well, but I think I know what it is
So my concern with method chaining is that you can only use the method by having an instance in the first place, meaning if you only need the class instance just for that one call, you have to make a class just to use that method
But if the method is just a function, you can use it by itself without having to build a new class just to use it
*^^this is where it's at, not the derpy demo lol^^*
Oooh
I think I personally draw the line between when to use a class, vs just a function, if there is state within the 'thing' that might have to me maintained or mutated during it's life of using it
So for a connection to your server, you'd want a class because it has methods that talk to that one server, and it will manage the details about that connection for you, internally
But if you just wanted to zip up a file using ZLIB, you might just want to do it in one go. Doing `new ZLIB("gzip").compress("myString")` isn't as ideal as just doing `gzip("myString")`

*back to normal me again*
I think that's good for now. I got a lot of the good stuff into this commit.

Wait, went back to the Discord, had some more rants! Here's the convo; signing off this commit with this one :P

Me:
> Working on the commit message for this discussion over the last day or so, it's going pretty good actually

HoldMyTurtleBrain:
> The commit message or the discussion? 👀

Me:
> I usually summarize my learnings/findings as part of my commits
> Almost like a coding diary, it really helps keep track of references for learned things along the way, especially since it's right next to the code related to it that you just committed

HoldMyTurtleBrain:
> Huh that's an interesting thing to do
> (ref below) Damn now I kinda wish I would've done that over the years, I would've had some great 4am rants to look back on ahahaha

> How often do you find yourself looking back at these?
> I do have a habit of including thought processes of non-obvious reasoning or design decisions in commit messages, but I end up barely using them, and the people reviewing PR's seem to always miss them....

Me:
> (ref above) That's exactly what they are at times haha
> I gotta find one of my more ridiculous ones

> I don't usually look back too often, but I think it's completely worth it because it's worse to have found a solution, and to just have the fix there. There's no learning in that, so if anyone wonders why I did something a certain way, they just see what I did, not why I did it
> So if we found a new bug with TypeScript earlier, now we can go back to this commit to find out what went wrong
> It's also helpful to keep track of the things I've already tried
> Sometimes I will commit code that isn't right/won't work, just so I can explain it as a failed experiment
> That's been a very big thing that has helped a lot
> It's not about perfect code, it's about getting towards 'perfect' code lol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

8 participants