Add nullability annotations to core.d.ts and es6.d.ts #7650

Arnavion · 2016-03-23T02:04:27Z

| null and | undefined where appropriate.
- As expressed in the original PR, indexers do not have | undefined. I have added it to the ES6 collection accessors like Map.get however. I think that's better.
- I haven't added | null or | undefined to functions that stringify their parameter and thus accept null and undefined, eg Symbol(null) is the same as Symbol("null"). It doesn't seem right to allow null just for that.

~~1. ES6's String.normalize's form parameter was typed as string - changed to union of string literals.~~

~~1. Map and Set methods that are specced to return this were not annotated as : this, changed accordingly.~~

~~I can split the changes into separate PRs if you want. I just fixed them as I came across them while doing the first one.~~

The other changes became too big and will be in another PR.

jake runtests passes.

Arnavion · 2016-03-23T02:22:03Z

src/lib/es6.d.ts

      * @param source The source object from which to copy properties.
      */
-    assign<T, U>(target: T, source: U): T & U;
+    assign<T, U>(target: T, source: U | null | undefined): T & U;


This (and similar for the overloads below) is needed because null and undefined are allowed and ignored by Object.assign, but the compiler should not infer U as null if null is passed in, as the result T & null is meaningless.

Should I add a comment to that effect?

Actually, on closer look it seems U is still specialized as null instead of {}. But also the resulting type T & null doesn't seem to have any issues either.

Should I remove the | null | undefined from this function?

T & null and T & undefined should not be a problem, because null and undefined add no new member to the type? In fact, I think the type system could (should?) simplify T & null & undefined to just T.

If it were T | null that would be a real problem.

BTW I don't think Object.assign is "meant" to explicitely accept undefined. It reflects the fact the the parameter is optional and that should probably be modelled with assign<T>(target: T): T, so that you can call assign(x); or maybe assign<T, U>(target: T, source?: U).

Note that removing null and undefined from being explicit in the signature doesn't change the fact that assign(x, null) is still valid.

Of course the right thing to do here would be variadics (2.1) since the real signature is assign<T, ...U>(target: T, ...source: ...U): T & ...U.

T & null and T & undefined should not be a problem, because null and undefined add no new member to the type?

Yes, it appears so, but...

In fact, I think the type system could (should?) simplify T & null & undefined to just T.

It doesn't, hence my initial concern that this would be problematic. But it seems to be fine regardless, in that the instances of T & null appear to be usable as T everywhere. I'm waiting for the TS team to confirm / deny if this is expected behavior or a bug.

BTW I don't think Object.assign is "meant" to explicitely accept undefined.

Yes, this is what I thought as well. I'm not familiar with all of assign's uses, so if it's not expected that users will want to use it with null / undefined sources then it would be nice to have a way to specify that U can't be null. Perhaps U extends {} ? I'm not in front of tsc to try at the moment.

Since this is brand new stuff, maybe @ahejlsberg should add a reduction rule from T | null | undefined to T... that may simplify a few other places where this might come up?

do not think this is the correct change here. the current signature declaration is better. i think what you want is a type operator like the expression operator ! (which does not exist) on the return type to indicate that null and undefined are ignored. e.g. assign<T, U>(target: T, source: U): T! & U!;

jods4 · 2016-03-23T10:36:04Z

src/lib/core.d.ts

      * predicate. If it is not provided, undefined is used instead.
      */
-    findIndex(predicate: (value: number) => boolean, thisArg?: any): number;
+    findIndex(predicate: (value: number) => boolean, thisArg?: any): number | undefined;


predicate is incorrect, it should be the same as for find().
See https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/TypedArray/findIndex

@Arnavion feel free to send a separate PR for the predicate changes.

@mhegazy And should I move the other commits after the first one into a separate PR as well?

On a second thought, the commit messages look clear enough, so feel free to include the findIndex chances in this PR if you like or have it in a separate one. up to you.

I've removed them from this PR. Will make another one.

mhegazy · 2016-03-23T23:39:45Z

thanks!

Arnavion · 2016-03-24T00:28:55Z

The other changes are in #7664

mihailik · 2016-03-24T12:23:22Z

src/lib/core.d.ts


    // Non-standard extensions
-    compile(): RegExp;
+    compile(): this;


I don't think there is a guarantee that compile returns your subclass if you subclass RegExp, so it should return plain RegExp instead.

According to ES specs it should return the this value of the compile() call.
So using this type here seems like the correct thing to do.

BTW: this method is deprecated so hopefully it won't matter much.

Concur 👍

Add nullability annotations to core.d.ts and es6.d.ts

7bb0ce9

msftclas added the cla-required label Mar 23, 2016

Arnavion reviewed Mar 23, 2016
View reviewed changes

RegExp.compile() actually returns this, per ES6 annex B.

d2d02c7

Arnavion mentioned this pull request Mar 23, 2016

Non-nullable types #7140

Merged

jods4 reviewed Mar 23, 2016
View reviewed changes

Revert change to Object.assign

bb5fa16

Arnavion changed the title ~~Add nullability annotations to core.d.ts and es6.d.ts (and other changes)~~ Add nullability annotations to core.d.ts and es6.d.ts Mar 23, 2016

mhegazy merged commit 560ab23 into microsoft:master Mar 23, 2016

Arnavion deleted the lib-d-ts-fixes branch March 23, 2016 23:48

mihailik reviewed Mar 24, 2016
View reviewed changes

Arnavion mentioned this pull request Apr 4, 2016

Modularization reverted adding nullability annotations to es6.d.ts #7810

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Add nullability annotations to core.d.ts and es6.d.ts #7650

Add nullability annotations to core.d.ts and es6.d.ts #7650

Uh oh!

Conversation

Arnavion commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Mar 23, 2016

Uh oh!

Arnavion commented Mar 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants