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

Type Guard Issue with Array.prototype.fill, Array Constructor #31785

Open
waaaaRapy opened this issue Jun 6, 2019 · 10 comments
Open

Type Guard Issue with Array.prototype.fill, Array Constructor #31785

waaaaRapy opened this issue Jun 6, 2019 · 10 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@waaaaRapy
Copy link

Typegurd Issue with Array.prototype.fill, Array Constructor

TypeScript Version: 3.5.1

Search Terms:

  • Array.prototype.fill
  • fill
  • ArrayConstructor
  • array implicit any

Code

const foo: number[] = new Array(3).fill("foo"); // accepted

Actual behavior:
This code above is accepted because new Array(3) returns any[], and so fill("foo") returns any[].

I know giving type explicitly to Array like

const foo: number[] = new Array<number>(3).fill("foo"); // error

would work, but I believe the compiler should reject the first one. (This is TypeScript.)

Expected behavior:

Option A. Array.prototype.fill returns narrow type
replace declaration of Array.fill in lib.es2015.core.d.ts like fill<S extends T>(value: S, start?: number, end?: number): S[];, then

const foo: number[] = new Array(3).fill("foo");
// error: "Type 'string[]' is not assignable to type 'number[]'."
// because `fill` is resolved as `fill<string>(value: string, .....): string[]`

const bar = new Array(3).fill(0); // `bar` is resolved as `number[]`
bar.fill("bar"); // error: "Type '"bar"' is not assignable to type 'number'."

const baz: (number|string)[] = new Array(3).fill(0); // accepted.
baz.fill("baz"); // accepted.

Option B. Array Constructor never return implicit any
The problem is that Array constructor returns any[] implicitly. The first code is accepted even with --strict or any other options. It means we always have to care about "Array constructor returns any[]".

Something like #26188 could solve this issue.

Playground Link:
https://www.typescriptlang.org/play/#src=const%20foo%3A%20number%5B%5D%20%3D%20new%20Array(3).fill(%22foo%22)%3B

Related Issues:
#29604

@fatcerberus
Copy link

Hmm, this seems challenging to fix. In this case:

let a: number[] = new Array(3);

TS could in theory use contextual typing to fill in for Array<T>, but then how would this case work:

let a: number[] = new Array(3).fill("foo");

// which is basically equivalent to:
let tmp = new Array(3);
let a: number[] = tmp.fill("foo");

How do we know what type tmp should be? We can't use contextual typing to figure it out anymore. We could make it unknown[] but my fear is that would break a lot of existing code in the wild.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Jun 13, 2019
@RyanCavanaugh
Copy link
Member

We could potentially expand the evolving array type logic to track values from new Array(), but this pattern doesn't seem super common.

@legraphista
Copy link

Even though this is an uncommon pattern, I believe that there should have some way to warn that the array will be implicitly any when noImplicitAny is on.
Since you don't use it often, you tend to forget about that quirk.

I got stuck for at least 15 minutes trying to figure out what's wrong with a similar snippet of code

if (someCondition) {
  // set actual data
  setObjDataToBin(getObjectDataWithGoodTypes());
} 
else {
  // set placeholders
  setObjDataToBin(new Array(count).fill({x: 0.5, y: 0.5, zoom: 1}));
}

// somewhere else in the codebase

function setObjDataToBin(obj: {x: number, y: number: z: number}[]): Float32Array {
  // some code
}

@markrian
Copy link

markrian commented May 7, 2020

I may misunderstand the nuance/purpose of noImplicitAny, but to me, it means that for a value to have any in its type, I must have written any explicitly somewhere in the type definition.

That means that, in my mind, the following should throw a warning/error when noImplicitAny is enabled:

const foo = new Array(3);

since its inferred (i.e., implicit) type is any[].

Would it be possible to add a reallyNoImplicitAny flag (or similar 😄) to do this? Or is there some complexity/nuance I'm not seeing that makes that infeasible?

@legraphista
Copy link

@markrian The compiler and its options aren't the problem here, it's this line:

new(arrayLength?: number): any[];

form the array constructor definitions

TypeScript/lib/lib.es5.d.ts

Lines 1374 to 1383 in 05d59a1

interface ArrayConstructor {
new(arrayLength?: number): any[];
new <T>(arrayLength: number): T[];
new <T>(...items: T[]): T[];
(arrayLength?: number): any[];
<T>(arrayLength: number): T[];
<T>(...items: T[]): T[];
isArray(arg: any): arg is any[];
readonly prototype: any[];
}

if you don't specify a type, it implicitly uses the constructor which has any explicitly used.

I fear that it is too late for this to change as it will introduce many breaking changes in existing codebases. Maybe in typescript v4? We'll see...

@MicahZoltu
Copy link
Contributor

What is the reason for the Array constructor not returning an unknown[]? I'm a big fan of purging any from all of the TypeScript standard libraries, perhaps if it is too big of a change it can be added to the 4.0 todo list?

@Nokel81
Copy link

Nokel81 commented Oct 20, 2021

Actually I think that the typings of https://github.com/microsoft/TypeScript/blob/main/lib/lib.es2015.core.d.ts#L45-L53 are not correct. I think that they should be

fill<U>(value: U): Array<U>;
fill<U>(value: U, start?: number, end?: number): Array<T | U>;

@beauxq
Copy link

beauxq commented Feb 3, 2022

this pattern doesn't seem super common

I just ran into it today.
I think the only reason it's not super common is that people aren't aware of fill, so they make a for loop pushing values to the array, which I hope we can agree is not a good way to do it.

@timschwab
Copy link

Agreed with the above comment. The issue is Array<T>.fill(value: U) should return U[] rather than T[]. I'm not very familiar with the TypeScript codebase but it may be coming from this line now: https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.core.d.ts#L33

Pinging @RyanCavanaugh, as you seem to maybe be able to resolve this. Would a quick PR would be accepted?

@timschwab
Copy link

Oh also, I wrote this simple function to resolve this for my codebase:

function arrayFill<T, U extends T>(arr: T[], val: U): U[] {
	const res = arr.fill(val) as U[];
	return res;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants