microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.15k stars 12.51k forks source link

Type 'X<false>' is not assignable to type 'X<boolean>'. #48150

Closed tomhanax closed 2 years ago

tomhanax commented 2 years ago

Bug Report

Since 4.6 TS is complaining about that false is not assignable to boolean, when it is "wrapped by interface".

🔎 Search Terms

4.6 type

🕗 Version & Regression Information

⏯ Playground Link

Playground Link

💻 Code

interface Observable<T>
{
  (): T;
  (value: T): any;
}

function observable<T>(value: T): Observable<T>
{
  return undefined as any;  // the implementation is not important
}

const x: Observable<boolean> = observable(false);

🙁 Actual behavior

Type 'Observable<false>' is not assignable to type 'Observable<boolean>'.
  Types of parameters 'value' and 'value' are incompatible.
    Type 'boolean' is not assignable to type 'false'.

🙂 Expected behavior

No compiler problem as in previous versions.

whzx5byb commented 2 years ago

I believe this is a correct error which was not handled properly in old versions. The generic parameter T is invariant as it is used in both a covariant position () => T and a contravariant position (value: T) => any.

tomhanax commented 2 years ago

@whzx5byb Suppose this is correct behavior, can you suggest how to continue to use the sketched concept? The interface in fact represents the core concept of Knockout.js library.

To set value to x - use x(val). To read value from x - use x().

If x is defined to operate on boolean, It is very illogical that I can not set true or false as val. Of course I can cast to boolean, but is it the correct way? And is it the only way?

whzx5byb commented 2 years ago

If x is defined to operate on boolean, It is very illogical that I can not set true or false as val. Of course I can cast to boolean, but is it the correct way? And is it the only way?

A way to make your example work:

function observable<T>(value: T extends infer U ? U: never): Observable<T>
{
  return undefined as any;  // the implementation is not important
}

const x: Observable<boolean> = observable(false); // no error
tomhanax commented 2 years ago

@whzx5byb Just perfect, thanks! I do not know if it is appropriate here, but if you want, you could post this as answer on https://stackoverflow.com/questions/71319489/type-observablefalse-is-not-assignable-to-type-observableboolean (there is a small bounty if you care). Or I could do it using the code you proposed if you have no interest. Please give me the info, I will close this bug then.

bennieswart commented 2 years ago

@tomhanax perhaps this should be referred to the knockout project so that the necessary changes can be made there.

tomhanax commented 2 years ago

@tomhanax perhaps this should be referred to the knockout project so that the necessary changes can be made there. Done: https://github.com/knockout/knockout/issues/2589, hope that somebody there will pick that up, because the project seem to go to winter sleep for good.

whzx5byb commented 2 years ago

@tomhanax I don't care, you could post it yourself.

fatcerberus commented 2 years ago

For the record, the reason for the type error:

const oFalse: Observable<false> = observable(false);
const oBool: Observable<boolean> = oFalse;
oBool(true);
// now Observable<false> has value true
whzx5byb commented 2 years ago

@tomhanax

The answer in https://github.com/microsoft/TypeScript/issues/48150#issuecomment-1060539236 is not perfect, as it will produce unknown without a type annotation.

const bad = observable(false);
//     ^ unknown, which should be Observable<boolean>

The issue here is the inference T extends infer U ? U : never infers U from the LHS of assignment. But when there is none, the default constraint type unknown will be used.

Here is an alternative workaround which will produce the same behavior without a type annotation, but a bit more verbose. I have to introduce two type parameter. T is for the refered type which is from LHS. U is for the "raw" widening type, which will be used only when T is unknown.

declare function observable<T, U>(value: T extends infer R ? R : U): unknown extends T ? Observable<U> : Observable<T>;

I'm not sure if there is a better solution. If you find some please let me know.

maskmaster commented 2 years ago

Cross-posting here, just for the record. Make sure that also both these lines are valid with any new proposal.

const x = observable<false | undefined>(false);
const y = observable(false);
mbest commented 2 years ago

@tomhanax I think this should be fixed in TypeScript. The "fixes" for the Knockout code don't make much sense.

Consider that this code has the correct result:

const a = observable(false);    // Observable<boolean>

But this one errors:

const a: Observable<boolean> = observable(false);

Also that this works:

const a: Observable<string> = observable('cash'); 

But not this:

const a: Observable<'cash' | 'card'> = observable('cash'); 

Playground Link

tomhanax commented 2 years ago

@mbest, the "most updated" playground (see also https://github.com/knockout/tko/issues/168#issuecomment-1061759617) contains many examples and seems OK, of course anybody can check and correct if needed, I am no expert in this.

Playground Link

I am not saying anything about what "should" be fixed, I am no expert at all. Simply trying to help to solve existing issue, because now we are forced to remain on TS 4.5 until this is solved.

maskmaster commented 2 years ago

@tomhanax why was the issue closed? Isn't this a valid bug in Typescript? As @mbest pointed out:

const w = observable(false); // Typescript identifies 'w' as Observable<boolean>
const x: Observable<boolean> = observable(false); // Type being Observable<false> now

Typescript seems to change it's mind of what type observable(false) actually is.

tomhanax commented 2 years ago

@maskmaster to be honest I am not sure. I only know that what worked previously does not work now. As @whzx5byb pointed out, the whole thing may be "reversed" - it was working because of less perfect previous TS version that allowed this and now it is not working because simply it should not work in the first place. I am not good at type theory at all, so anybody else have to say funded words. Anyway, I am reopening this in hope for clarification.

whzx5byb commented 2 years ago

Typescript seems to change it's mind of what type observable(false) actually is.

This should be related to the "type widening" mechiasm. For more details: https://github.com/microsoft/TypeScript/pull/10676, https://github.com/microsoft/TypeScript/pull/11126, https://github.com/microsoft/TypeScript/pull/24310.

And here boolean behaves a little bit different because it is actually implemented as a union equivalent to true | false.

declare function fn<T>(x: T): T[];

const b1 = fn(true); // T is boolean (widening)
const n1 = fn(1);  // T is number (widening)

const b2: boolean[] = fn(true); // T is true (non-widening)
const n2: number[] = fn(1); // T is number (widening)

const b3: (true | false)[] = fn(true); // T is true (non-widening)
const n3: (1 | 0)[] = fn(1); // T is 1 (non-widening)
maskmaster commented 2 years ago

The following is valid

const w = observable(false);
const v: Observable<boolean> = w;

However, this is not valid

const v: Observable<boolean> = observable(false);

To me, it makes no sense.

RyanCavanaugh commented 2 years ago

@maskmaster any type with a contravariant position has this problem

RyanCavanaugh commented 2 years ago

Bisected to #47341. @ahejlsberg this seems like an unintended consequence?

ahejlsberg commented 2 years ago

Before 4.6 we'd measure variance of Observable<T> wrong:

declare let xb: Observable<boolean>;
declare let xf: Observable<false>;

xb = xf;  // Error, but wasn't before 4.6
xf = xb;  // Error

The type parameter T is used in both input and output positions within Observable<T>, so clearly Observable<T> is invariant and our current behavior is correct. We got it wrong before because we'd cross-relate the overloaded signatures, which sometimes makes sense for different source and target types, but never makes sense when source and target are instantiations of the same type (which they are when we're measuring variance). Not only was it wrong to cross-relate them, it also generated a lot more work, which was the problem fixed in #47341.

maskmaster commented 2 years ago

@ahejlsberg I totally agree with the correctness of your example and that the new behavior as you describe is correct.

I think we are talking about two different things. In this simple example

const w = observable(false);

According to Typescript 4.6 'w' is an Observable<boolean>. Still, it is not possible to do (exactly what typescript suggests)

const w: Observable<boolean> = observable(false);

According to you, what is the correct type of 'w' in const w = observable(false)?

ahejlsberg commented 2 years ago

@maskmaster Hmm, yeah, that is a known design limitation, and I agree it's unfortunate. See my comment here. The workaround is to write:

const w: Observable<boolean> = observable(false as boolean);
ahejlsberg commented 2 years ago

@maskmaster Regarding the type of

const w = observable(false);

it is producing the intended Observable<boolean>. There is no contextual type for the false argument and therefore we widen the type to boolean. Just like we would widen 42 to number.

tomhanax commented 2 years ago
const w: Observable<boolean> = observable(false as boolean);

Bluntly said this is absolutely silly. In year 2022 you can not possibly tell this to the "average programmer". Anyway, what is the problem with "more verbose" typings illustrated in the beforementioned playground? I am only asking because that "simply works", but to be true, I really do not understand the magic there.

typescript-bot commented 2 years ago

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

ahejlsberg commented 2 years ago

Bluntly said this is absolutely silly

Yeah, as I said, it's unfortunate. I'll open an issue on this specifically and we'll see if we can do better.

maskmaster commented 2 years ago

Thank you @ahejlsberg for the explanation. It makes sense if it a design limitation. I'll try to wrap my head around this later on so I understand exaxtly what happens. Thanks for linking the other issue. Edit: it took too long for me to write on the phone. It was already linked.