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

2.1 Regression: (A | undefined) & B is possibly undefined #12354

Closed
jods4 opened this issue Nov 18, 2016 · 10 comments
Closed

2.1 Regression: (A | undefined) & B is possibly undefined #12354

jods4 opened this issue Nov 18, 2016 · 10 comments
Labels
Duplicate An existing issue was already created

Comments

@jods4
Copy link

jods4 commented Nov 18, 2016

TypeScript Version: 2.1.? / as shipped with VS 2017 RC

Code

let x: { id: number } | undefined;
let y: { id: number };
x = Object.assign(y, x);
x.id;

Expected behavior: x.id was fine in TS 2.0, and should be as it is guaranteed to be x === y.

Actual behavior: x.id triggers x is possibly undefined.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 18, 2016

The current declaration of Object.assign is:

assign<T, U>(target: T, source: U): T & U;

so the result is ({ id: number } | undefined) & ({ id: number }) which is { id: number } | undefined

The language does not have the constructs today to accurately model the behavior of Object.assing. Adding a spread like operator on types and using it to define Object.assign is tracked by #11100

@mhegazy mhegazy added the Duplicate An existing issue was already created label Nov 18, 2016
@jods4
Copy link
Author

jods4 commented Nov 18, 2016

Did something change in this regard since 2.0? Because it worked.
(I think there was a change related to distributivity of | and & in typing, I swear I saw an issue about that somewhere)

so the result is ({ id: number } | undefined) & ({ id: number }) which is { id: number } | undefined.

Shouldn't the result of (T | undefined) & T be (T & T) | (T & undefined) which is just T.

@blakeembrey
Copy link
Contributor

@mhegazy FWIW, I did https://github.com/types/npm-xtend/blob/master/immutable.d.ts#L1 to allow extending undefined/null as they have no effect on the final type.

@jods4
Copy link
Author

jods4 commented Nov 18, 2016

@blakeembrey interesting but is that required? The result of (A | undefined) & B is not possibly undefined because it must include all members of B.

This is the behavior of 2.0 and it's a regression, which is not a dupe (I'm not asking for perfect typing, which will come with spread types, I know).

@jods4 jods4 changed the title 2.1 Regression: Object.assign and undefined 2.1 Regression: (A | undefined) & B is possibly undefined Nov 18, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Nov 18, 2016

@jods4 this behavior is introduced in #11717. the change normalizes union types, so that (A | undefined) & B is treated the same way as (A & B) | (undefined & B).
the fact that it used to work in the past is just a bug that was uncovered by this change.

the correct fix for the issue in the OP is by correctly modeling the behavior or Object.assign.

@jods4
Copy link
Author

jods4 commented Nov 18, 2016

@mhegazy not sure I understand here.
How is (A & B) | (undefined & B) potentially undefined?
The left side A & B is obviously not.
The right side undefined & B is not undefined and has to provide all properties of B.
For as much as I can think of, undefined & B is equivalent to just B.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 18, 2016

undefined & B is undefined and is B. undefined has a value not like never for instance.

@jods4
Copy link
Author

jods4 commented Nov 18, 2016

As an illustration, assume type B = { n: number } and some b: undefined & B.

From your own words, undefined & B is undefined and is B so there is no reason that I should not be able to do b.n.

n has to exist on b, otherwise it would not be undefined & B.

Currently the compiler is complaining that b.n is not valid and that is incorrect.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 18, 2016

the compiler says if the type is undefined then it is not safe to use. and the type is undefined.

the problem is not which rule to invoke. the type undefined & B is as invalid as number & string. it represents an empty set with no values, and it does not exist at runtime. I do not think there is a right/wrong way to deal with them.

You would not write a type annotation on something to be undefined & B; the real issue here is you got one of these meaningless types from calling Object.assign.

@jods4
Copy link
Author

jods4 commented Nov 18, 2016

OK I see what you mean.
As Anders pointed out in the initial PR for intersection types, intersection of primitive types don't really work but they can't be prevented in generics instanciations.

It just happened that in 2.0 undefined & B allowed property access because it is a B and now in 2.1 it is forbidden because it is undefined. The statements are contradictory but they are both true, so I agree that none is better than the other.

Surely when the spread types PR lands, Object.assign will return A ... B and the situation will be great. Will it land in 2.1 RTM though?

Otherwise, since this is an annoying breaking change, maybe you should do a little mitigation in the meantime. You could change the assign definition to Object.assign<T, U>(a: T, b: U | null | undefined): T & U as suggested by @blakeembrey. That would prevent some breakage until we get spread types.

BTW, I tried to think of other cases where T & null might happen and it seems to me that for many (or even all?) use cases, A ... B replaces A & B. Right now I can't think of one situation in JS where you actually implement A & B, what you do is A ... B.

@jods4 jods4 closed this as completed Nov 18, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants