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

Make 'pop' the first method that uses 'T' in 'Array' #25565

Merged
merged 4 commits into from
Jul 16, 2018
Merged

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jul 10, 2018

Chatted with @mhegazy about this, but I'd like to get your thoughts on this @ahejlsberg since you sent out #436.

When elaborating errors on an Array<Foo> to an Array<Bar>, diving into a structural comparison is often dizzying for the user since the first elaboration occurs on push.

Since push uses T contravariantly, we'll end up flipping the direction which is much more difficult for users to parse. Furthermore, in cases where the covariant check would provide better error messages and error message spans (such as excess property checking spans, and picking better branches in unions), we would end up doing significantly worse.

As an example from @cliffkoh and @dzearing, the following

type IStyle = IStyleBase | IStyleArray;
interface IStyleArray extends Array<IStyle> {}
interface IStyleBase { 
    foo: string;
}

const blah: IStyle = [
    [[{
        foo: 'asdf',
        jj: 1 // intentional error
    }]]
];

Provides an error on blah:

const blah: IStyle = [
      ~~~~

[ts] Type '{ foo: string; jj: number; }[][][]' is not assignable to type 'IStyle'.
       Type '{ foo: string; jj: number; }[][][]' is not assignable to type 'IStyleArray'.
         Types of property 'push' are incompatible.
           Type '(...items: { foo: string; jj: number; }[][][]) => number' is not assignable to type '(...items: IStyle[]) => number'.
             Types of parameters 'items' and 'items' are incompatible.
               Type 'IStyle' is not assignable to type '{ foo: string; jj: number; }[][]'.
                 Type 'IStyleBase' is not assignable to type '{ foo: string; jj: number; }[][]'.
                   Property 'length' is missing in type 'IStyleBase'.

Whereas with this change, we provide an error on jj (the excess property):

        jj: 1 // intentional error
        ~~
 
[ts] Type '{ foo: string; jj: number; }[][][]' is not assignable to type 'IStyle'.
       Type '{ foo: string; jj: number; }[][][]' is not assignable to type 'IStyleArray'.
         Types of property 'pop' are incompatible.
           Type '() => { foo: string; jj: number; }[][]' is not assignable to type '() => IStyle'.
             Type '{ foo: string; jj: number; }[][]' is not assignable to type 'IStyle'.
               Type '{ foo: string; jj: number; }[][]' is not assignable to type 'IStyleArray'.
                 Types of property 'pop' are incompatible.
                   Type '() => { foo: string; jj: number; }[]' is not assignable to type '() => IStyle'.
                     Type '{ foo: string; jj: number; }[]' is not assignable to type 'IStyle'.
                       Type '{ foo: string; jj: number; }[]' is not assignable to type 'IStyleArray'.
                         Types of property 'pop' are incompatible.
                           Type '() => { foo: string; jj: number; }' is not assignable to type '() => IStyle'.
                             Type '{ foo: string; jj: number; }' is not assignable to type 'IStyle'.
                               Object literal may only specify known properties, and 'jj' does not exist in type 'IStyle'.

This is horrifyingly longer, but it gives the right span and gets to the root cause at the end.

@DanielRosenwasser DanielRosenwasser changed the title Move 'pop' to to the first method using 'T' in 'Array' Move 'pop' to the first method using 'T' in 'Array' Jul 12, 2018
@DanielRosenwasser DanielRosenwasser changed the title Move 'pop' to the first method using 'T' in 'Array' Make 'pop' the first method that uses 'T' in 'Array' Jul 12, 2018
@DanielRosenwasser
Copy link
Member Author

Really the RWC baseline changes are minimal, and aren't impacted at all. I'm just seeing reports of missing pop instead of push:

  Argument of type 'Uint8ClampedArray' is not assignable to parameter of type 'any[]'.
+   Property 'pop' is missing in type 'Uint8ClampedArray'.
-   Property 'push' is missing in type 'Uint8ClampedArray'.

@DanielRosenwasser DanielRosenwasser merged commit 93ab352 into master Jul 16, 2018
@DanielRosenwasser DanielRosenwasser deleted the popFirst branch July 16, 2018 18:41
@mohsen1
Copy link
Contributor

mohsen1 commented Nov 13, 2018

Can you improve those errors for Arrays even further. I got confused with code like this:

interface Values {
    textValues: {
        values: string[]
    }
}

const items = [{ name: 'a' }, { name: 'b' }]

var values: Values = {
    textValues: items.map(i => i.name)
}
Type 'string[]' is not assignable to type '{ values: string[]; }'.
  Types of property 'values' are incompatible.
    Type '() => IterableIterator<string>' is not assignable to type 'string[]'.
      Property 'pop' is missing in type '() => IterableIterator<string>'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants