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

Partial inconsistent behavior with undefined types #18823

Closed
deregtd opened this issue Sep 28, 2017 · 5 comments
Closed

Partial inconsistent behavior with undefined types #18823

deregtd opened this issue Sep 28, 2017 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@deregtd
Copy link

deregtd commented Sep 28, 2017

We're trying to figure out the right way to deal with potential react issues with typings. We've recently migrated our enormous codebase over to using TS 2.6 and enabled strictNullChecks (after working through over 7 thousand compile errors!) and found that we still have some bugs due to typescript allowing nulls to make their way into state through what feels to me like a bug. Someone complained about a similar problem with #12793 but it sounds like the behavior they were talking about could be argued about. However, I have what feels more like a contradiction in the compiler.

interface a { 
    x: number;
    y: number|undefined;
}

let z1: a = { x: 4, y: undefined }; // OK -- Makes sense
let z2: a = { x: undefined, y: undefined }; // Error -- x can't be undefined, sure
let z3: a = { x: 4, y: undefined, z: 4 }; // Error -- z isn't on the interface
let z4: Partial<a> = { z: 4, x: undefined };  // Error -- Makes sense, extra z parameter not on interface
let z5: Partial<a> = { x: undefined };  // OK -- **Whoa, what?**

In this example (z5), x is on the interface, but explicitly doesn't allow undefined. Why is it okay to have this "missing parameter" that's outside the interface, but I can't add z:4 to the same Partial? If you switched to allowing z: 4 on the Partial interface, that'd make it a lot less useful, but that would at least be consistent; but the very correct behavior here (and what the common usage of Partial tends to be) feels like banning x: undefined.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 28, 2017

The fundemental challenge is that TypeScript's type system sees a property with an undefined value as the same as an absent property. There is only a very subtle run-time distinction between the two. The Partial is a bit of a red herring in that all of these are equivalent types to TypeScript under strict null checks:

interface a {
  x?: number;
}

interface b {
  x: number | undefined;
}

type c = Partial<{ x: number }>;

The subtle runtime behaviour is that:

const a = { x: undefined };
const b = { };

console.log(a.x); // undefined
console.log(b.x); // undefined
console.log(typeof a.x === 'undefined'); // true
console.log(typeof b.x === 'undefined'); // true
console.log('x' in a); // true
console.log('x' in b); // false
console.log(a.hasOwnProperty('x')); // true
console.log(b.hasOwnProperty('x')); // false
console.log(Object.create(a).hasOwnProperty('x')); // false

I believe the opinion has been that to actually implement the presence of a property in an object as part of the object's structure is really complicated to implement for what is limited value from identifying issues in the code.

What sort of issues was this causing in the run-time code/behaviour?

@deregtd
Copy link
Author

deregtd commented Sep 28, 2017

We switched to strictNullChecks recently, and as such had areas all over the codebase where devs had previously just willy-nilly been using undefined that we locked down to be reliably set. However, we switched to using Partial for setState ages ago, so we have lots of cases of people setting undefined on fields that can't be undefined, which the compiler is not catching.

I'm not sure I consider the runtime behavior "subtle" -- when you do something like _.merge (basically what setState does), where you iterate over keys for an object and merge any value associated with it into the master object, there's a huge difference between not having the key present in the type and having it present and set to undefined. The non-partial behavior agrees with this and is a super-helpful compiler validation, we just really need this with the Partial level as well to catch the next series of bugs that we're finding by hand right now.

@deregtd
Copy link
Author

deregtd commented Sep 28, 2017

And, to be fair, if you type-coerce two different things that overlap, that works too:

let z6 = { x: 4, y: undefined, z: 4 } as a; // OK -- x/y are on a, and it doesn't care that z is not

I'm only particularly worried about the specific pattern of defining a partial and assigning to it, rather than type-coercing something INTO a Partial. With the existing pattern of type coersion, we accept the ability to "downcast" { x: 4, y: 4, z: 4 } down to { x: number, y: number }.

let z7 = { x: 4, y: 4, z: 4 } as { x: number, y: number}; // Works

However, I shouldn't be able to do:

let z7: { x: number, y: number } = { x: 4, y: 4, z: 4 }; // Properly errors

.. which I correctly can't.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 25, 2017

Relevant discussion in #13195

The type system treats a missing (or optional) property as undefined. This approximation mostly works in the js world. React state depends on this distinction.

We have talked about it in the past and the main problem is higher order relationships has assumptions that can not be satisfied in this scheme, namely that T["p"] = T["p"] is an unsafe operation, since the type of an optional property of read (undefined) is not the same as its type for write ( U | missing).

closing in favor of #13195

@mhegazy mhegazy added the Duplicate An existing issue was already created label Oct 25, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Nov 9, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Nov 9, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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