fix: add generic info for methods with thisArg of built-in classes #12784

e-cloud · 2016-12-09T03:47:41Z

Here's a checklist you might find useful.

There is an associated issue that is labelled
'Bug' or 'Accepting PRs' or is in the Community milestone
Code is up-to-date with the master branch
You've successfully run jake runtests locally
You've signed the CLA
There are new or updated unit tests validating the change

Note: need guide to write tests

msftclas · 2016-12-09T03:47:46Z

Hi @e-cloud, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

e-cloud · 2016-12-09T03:48:57Z

@DanielRosenwasser @mhegazy can you have a review and give some guide on how to write corresponding tests?

msftgits · 2016-12-09T17:14:54Z

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

msftclas · 2016-12-09T17:14:59Z

Hi @e-cloud, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

e-cloud · 2016-12-15T02:06:51Z

@mhegazy can you take a review?

sandersn · 2016-12-16T18:14:01Z

The change looks good. To write tests, add a file named something like thisTypeInLib.ts in the directory tests/cases/compiler. In the file, put code from the bug you reported in #12548.

Run gulp runtests-parallel then look in tests/baselines/local to make sure that no .errors.txt file exists and also that the .types file is correct. Then run gulp baseline-accept to put the baselines in the reference directory, ready to commit.

After you get that workflow going, you should add tests for all the functions you improved too.
These instructions are explained better in CONTRIBUTING.md; I'm just summarising from memory.

e-cloud · 2016-12-17T11:23:51Z

@sandersn, new test is added. And i find out the new version of tslint breaks down the linting task.

sandersn · 2016-12-21T23:42:07Z

tests/cases/compiler/thisTypeInNativeThisAssignableMethods.ts

+        options.every(function (val, index) {
+            return val === this.options[index];
+        }, this);
+    }


could you also add tests for ReadonlyArray and the other U?.*Array types?

sandersn · 2016-12-21T23:43:39Z

tests/cases/compiler/thisTypeInNativeThisAssignableMethods.ts

+    }
+
+    test(options: string[]) {
+        options.some(function (val, index) {


can you also add tests for find, findIndex, forEach, map?

I'm afraid that it would be problem. As the default compile target seems to be es5. Then it prompts that find not exist in xxx[]. Should we create a test project for it?

You can change the target with a line at the beginning: // @target: es6

your guidance is helpful

sandersn

Can you add tests for the other array methods and array types?

mhegazy · 2016-12-22T00:50:55Z

should not this be two overloads:

find(predicate: (this: undefined, value: T, index: number, obj: Array<T>) => boolean): T | undefined;
find<U>(predicate: (this: U, value: T, index: number, obj: Array<T>) => boolean, thisArg : U): T | undefined;

the spec seems to indicate that, here is what MDN has to say: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

mhegazy · 2016-12-22T00:51:22Z

Also there are other find functions for TypedArrays that should be updated as well.. e.g. Int32Array

e-cloud · 2016-12-22T01:29:25Z

@mhegazy what do you mean by:

should not this be two overloads:
find(predicate: (this: undefined, value: T, index: number, obj: Array<T>) => boolean): T | undefined;
find<U>(predicate: (this: U, value: T, index: number, obj: Array<T>) => boolean, thisArg : U): T | >undefined; 
the spec seems to indicate that, here is what MDN has to say: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

Can you describe it more clearly?

mhegazy · 2016-12-22T18:15:18Z

From MDN:

If a thisArg parameter is provided to find, it will be used as the this for each invocation of the callback. If it is not provided, then undefined is used.

if this is not defined on a function it is inferred to be any. but the behavior at runtime is that if thisArg is not specified as parameter, the this for the callback is undefined.

e-cloud · 2016-12-24T10:03:59Z

@mhegazy I see what you mean now.

I have some tries and find out some problems.

TL;DR

Use the forEach of Array<T> in the testfile as example.
with the test code:

class A {
    options: string[];

    addOptions(options: string[]) {
        if (!this.options) {
            this.options = [];
        }
        options.forEach(function (item) {
            this.options.push(item);
        }, this);
        return this;
    }

    testUndefined(options: string[]) {
        options.forEach(function (item) {
            this.options.push(item);
        }); // case1
        options.forEach(function (item) {
            this.options.push(item);
        }, undefined); // case2
        options.forEach(function (item) {
            this.options.push(item);
        }, null); // case3
    }
}

in lib/es5.d.ts

version 1

forEach(callbackfn: (this: undefined, value: T, index: number, array: T[]) => void): void;
forEach<Z>(callbackfn: (this: Z, value: T, index: number, array: T[]) => void, thisArg: Z): void;

erros from thisTypeInNativeThisAssignableMethods.errors.txt:

        testUndefined(options: string[]) {
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'undefined'.
            }); // case1
            options.forEach(function (item) {
                this.options.push(item);
            }, undefined); // case2
            options.forEach(function (item) {
                this.options.push(item);
            }, null); // case3
        }

only case1 will be recognized as error, others are legal rather than error.

version 2

    forEach(callbackfn: (this: undefined, value: T, index: number, array: T[]) => void): void;
    forEach(callbackfn: (this: undefined, value: T, index: number, array: T[]) => void, thisArg: undefined): void;
    forEach<Z>(callbackfn: (this: Z, value: T, index: number, array: T[]) => void, thisArg: Z): void;

erros from thisTypeInNativeThisAssignableMethods.errors.txt:

        testUndefined(options: string[]) {
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'undefined'.
            }); // case1
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'undefined'.
            }, undefined); // case2
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'undefined'.
            }, null); // case3
        }

now all are recognized as undefined, even case3's this should be null.

version 3

    forEach(callbackfn: (this: undefined, value: T, index: number, array: T[]) => void): void;
    forEach(callbackfn: (this: null, value: T, index: number, array: T[]) => void, thisArg: null): void;
    forEach(callbackfn: (this: undefined, value: T, index: number, array: T[]) => void, thisArg: undefined): void;
    forEach<Z>(callbackfn: (this: Z, value: T, index: number, array: T[]) => void, thisArg: Z): void;

erros from thisTypeInNativeThisAssignableMethods.errors.txt:

        testUndefined(options: string[]) {
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'undefined'.
            }); // case1
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'null'.
            }, undefined); // case2
            options.forEach(function (item) {
                this.options.push(item);
                     ~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'null'.
            }, null); // case3
        }

It seems the nulled-this interface takes precedence. And if the undefined version is placed before null version, the errors are same as version 2

In short

undefined and null is recognized as same. And when undefined is assigned as generic type, it's inferred as any.

Personally, I'm ok with version 2 for temporary solution. @mhegazy do you agree to make three interfaces for all involved methods?

e-cloud · 2016-12-24T12:43:44Z

Another thing worths mentioning is that the from static method of all array constructor. They should support iterable parameter and the generic info of Array like is not proper. I just find it accidentally just because from's mapfn also supports thisArg.

e-cloud · 2017-01-05T03:15:14Z

@mhegazy @sandersn I've rewrite the PR to accomplish the solution commented above.

Note: I use several regexps to globally update the interfaces. Manually updating per interface would be nightmare.

sandersn · 2017-01-05T18:00:34Z

Note that option 1 works correctly with // strictNullChecks: true. However, option 2 is OK with me since it provides better errors with strict null checks off. @mhegazy, are you OK with this solution as well?

e-cloud · 2017-01-06T14:55:50Z

@sandersn It seems to fail many tests. Should I update the other baseline files? It would be a lot of files.

sandersn · 2017-01-10T17:09:11Z

Yes, the baseline changes are expected: when you add overloads, it changes the symbols and types to reflect those additional overloads, even if the types resulting from the call doesn't change.

e-cloud · 2017-01-11T02:12:56Z

@mhegazy @sandersn hey, guys. All checks have passed now. How about have a review again?

e-cloud · 2017-01-24T08:00:07Z

@mhegazy request for review

alitaheri · 2017-01-24T11:16:51Z

You have a yarn.lock file checked in. Should it be there?

e-cloud · 2017-01-24T11:27:33Z

Yes, accident. 😭

when enabling `noImplicitThis`, if assing this argument for methods like `array.forEach` will cause compilation error. This commit fixes it. fix microsoft#12548

e-cloud · 2017-03-09T08:39:47Z

@sandersn ，I've rebased on master just now. Can you guys give some review and feedback? Coz it's been a while since starting the PR.

The tests failed because of the latest problematic @types/node module. The tests passed locally when i switch @types/node to v6.x.

mhegazy · 2017-03-09T17:24:25Z

I have a fix in master now for the build break. can you give it another try.

e-cloud · 2017-03-10T01:31:09Z

tests pass now.

sandersn · 2017-03-10T17:17:36Z

Thanks @e-cloud!

msftclas added the cla-required label Dec 9, 2016

msftgits closed this Dec 9, 2016

msftgits reopened this Dec 9, 2016

msftclas added the cla-already-signed label Dec 9, 2016

msftgits removed the cla-required label Dec 9, 2016

e-cloud force-pushed the fix/generic-this-arg branch 2 times, most recently from 4339c9a to c42a029 Compare December 17, 2016 11:22

sandersn reviewed Dec 21, 2016

View reviewed changes

sandersn requested changes Dec 21, 2016

View reviewed changes

e-cloud force-pushed the fix/generic-this-arg branch from c42a029 to 6872883 Compare January 5, 2017 03:10

e-cloud force-pushed the fix/generic-this-arg branch from b046380 to b6998a4 Compare January 11, 2017 01:24

e-cloud force-pushed the fix/generic-this-arg branch from b6998a4 to 4756b6a Compare January 22, 2017 12:00

e-cloud force-pushed the fix/generic-this-arg branch from 4756b6a to 222b828 Compare January 24, 2017 12:05

e-cloud force-pushed the fix/generic-this-arg branch from 222b828 to 5c26e62 Compare February 8, 2017 12:17

e-cloud added 3 commits March 9, 2017 11:52

fix: add generic info for methods with thisArg of built-in classes

cedad97

when enabling `noImplicitThis`, if assing this argument for methods like `array.forEach` will cause compilation error. This commit fixes it. fix microsoft#12548

test: add tests for this type in native-this-assignable methods

18154fe

test: adjust test case completionEntryForUnionMethod

5f5aa20

e-cloud force-pushed the fix/generic-this-arg branch from 5c26e62 to 1b7c11a Compare March 9, 2017 05:52

mhegazy approved these changes Mar 9, 2017

View reviewed changes

sandersn approved these changes Mar 9, 2017

View reviewed changes

test(baseline): update references affected

09fcf5b

e-cloud force-pushed the fix/generic-this-arg branch from 1b7c11a to 09fcf5b Compare March 10, 2017 01:16

sandersn merged commit bdb6a8a into microsoft:master Mar 10, 2017

mhegazy mentioned this pull request May 23, 2017

Fix Array interface (#10886) #11294

Closed

mhegazy mentioned this pull request Jun 4, 2017

Array cleanup #16223

Merged

microsoft locked and limited conversation to collaborators Jun 19, 2018

fix: add generic info for methods with thisArg of built-in classes #12784

fix: add generic info for methods with thisArg of built-in classes #12784

Uh oh!

Conversation

e-cloud commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Dec 9, 2016

Uh oh!

e-cloud commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftgits commented Dec 9, 2016

Uh oh!

msftclas commented Dec 9, 2016

Uh oh!

e-cloud commented Dec 15, 2016

Uh oh!

sandersn commented Dec 16, 2016

Uh oh!

e-cloud commented Dec 17, 2016

Uh oh!

sandersn Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

e-cloud Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

e-cloud Dec 23, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Dec 22, 2016

Uh oh!

mhegazy commented Dec 22, 2016

Uh oh!

e-cloud commented Dec 22, 2016

Uh oh!

mhegazy commented Dec 22, 2016

Uh oh!

e-cloud commented Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

In short

Uh oh!

e-cloud commented Dec 24, 2016

Uh oh!

e-cloud commented Jan 5, 2017

Uh oh!

sandersn commented Jan 5, 2017

Uh oh!

e-cloud commented Jan 6, 2017

Uh oh!

sandersn commented Jan 10, 2017

Uh oh!

e-cloud commented Jan 11, 2017

Uh oh!

e-cloud commented Jan 24, 2017

Uh oh!

alitaheri commented Jan 24, 2017

Uh oh!

e-cloud commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

e-cloud commented Mar 9, 2017

Uh oh!

mhegazy commented Mar 9, 2017

Uh oh!

e-cloud commented Mar 10, 2017

Uh oh!

sandersn commented Mar 10, 2017

Uh oh!

e-cloud commented Dec 9, 2016 •

edited

Loading

e-cloud commented Dec 9, 2016 •

edited

Loading

e-cloud commented Dec 24, 2016 •

edited

Loading

e-cloud commented Jan 24, 2017 •

edited

Loading