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.

Sign up for GitHub

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

lib Fix Part 2/6 – Array-related methods #50450

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

lib Fix Part 2/6 – Array-related methods #50450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1385bb7
lib Types and Documentations Fix
graphemecluster Aug 25, 2022
4bcd472
Merge main branch: `Array` line number changes
graphemecluster Sep 18, 2022
9481bab
Merge main branch: `Array` line number changes
graphemecluster Oct 11, 2022
06c6e5d
Merge remote-tracking branch 'upstream/main' into implement49733-part2
graphemecluster Nov 8, 2022
42c170b
Merge remote-tracking branch 'upstream/main' into implement49733-part2
graphemecluster Jan 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 40 additions & 20 deletions src/lib/es2015.core.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,18 @@ interface Array<T> {
* @param thisArg If provided, it will be used as the this value for each invocation of
* predicate. If it is not provided, undefined is used instead.
*/
find<S extends T>(predicate: (value: T, index: number, obj: T[]) => value is S, thisArg?: any): S | undefined;
find(predicate: (value: T, index: number, obj: T[]) => unknown, thisArg?: any): T | undefined;
find<S extends T>(predicate: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S | undefined;

/**
* Returns the value of the first element in the array where predicate is true, and undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the change to unknown in es2020.bigint reminds me that technically 'true' here is technically 'truthy'. I don't know if it's worth calling out in the text, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, but this is another issue: whether there should be a syntax to allow returning anything in type predicates.

* otherwise.
* @param predicate find calls predicate once for each element of the array, in ascending
* order, until it finds one where predicate returns true. If such an element is found, find
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* order, until it finds one where predicate returns true. If such an element is found, find
* order, until predicate returns true for some element. If such an element is found, find

I would personally drop the second sentence and change the last one to start "If no element is found,". The second sentence is redundant with the first, though, and redundancy can be useful for understanding, so it's fine if you want to keep it.

* immediately returns that element value. Otherwise, find returns undefined.
* @param thisArg If provided, it will be used as the this value for each invocation of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param thisArg If provided, it will be used as the this value for each invocation of
* @param thisArg If provided, thisArg will be used as the this value for each invocation of

* predicate. If it is not provided, undefined is used instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the repl, this === global when not provided. I'd either leave off the last sentence, or reference the rules for this.

*/
find(predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T | undefined;

/**
* Returns the index of the first element in the array where predicate is true, and -1
Expand All @@ -20,50 +30,50 @@ interface Array<T> {
* @param thisArg If provided, it will be used as the this value for each invocation of
* predicate. If it is not provided, undefined is used instead.
*/
findIndex(predicate: (value: T, index: number, obj: T[]) => unknown, thisArg?: any): number;
findIndex(predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): number;

/**
* Changes all array elements from `start` to `end` index to a static `value` and returns the modified array
* @param value value to fill array section with
* @param start index to start filling the array at. If start is negative, it is treated as
* length+start where length is the length of the array.
* length + start where length is the length of the array.
* @param end index to stop filling the array at. If end is negative, it is treated as
* length+end.
* length + end.
*/
fill(value: T, start?: number, end?: number): this;
fill(value: T, start?: number, end?: number): T[];

/**
* Returns the this object after copying a section of the array identified by start and end
* to the same array starting at position target
* @param target If target is negative, it is treated as length+target where length is the
* @param target If target is negative, it is treated as length + target where length is the
* length of the array.
* @param start If start is negative, it is treated as length+start. If end is negative, it
* is treated as length+end.
* @param start If start is negative, it is treated as length + start. If end is negative, it
* is treated as length + end.
* @param end If not specified, length of the this object is used as its default value.
*/
copyWithin(target: number, start: number, end?: number): this;
copyWithin(target: number, start: number, end?: number): T[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible break: subtypes of Array will now just be Array, missing any of the subtype methods.

Is the only reason for this change consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to be long.

Returning this in {Array, %TypedArray%}.prototype.{fill, copyWithin, sort} seems to be correct, but TypeScript's type system is purely type-of-value-based, not instance-based. And because a subtype such as a tuple, or the generic parameters of a type, can contain the information of the values, returning this, which means an object with a same type, is technically incorrect for these methods which mutate the original object in a disruptive way. For example:

declare const foo: [42, 1, 2, 3];
const bar = foo.fill(42); // !!?

interface ArraySubtype extends Array<string> {
	0: "foo";
}
const bar: ArraySubtype = ["foo", "bar", "baz"];
const bar1 = bar.sort(); // !!!

interface Int8ArraySubtype extends Int8Array {
	0: 42;
}
const baz = new Int8Array([42, 1, 2, 3]) as Int8ArraySubtype;
const baz1 = baz.sort(); // ?!!

In fact, I'd done the some thing on RegExp (it's a deprecated method though):

TypeScript/src/lib/es5.d.ts

Line 988 in facef94

compile(pattern: string, flags?: string): RegExp;

I understand these are all rare cases, but since they return the same instance, people who find the original type desirable could just use the original thing. It just makes sense, and just because these are all rare cases it has to be corrected.

In contrast, the following is better typed this. That's why the this keyword in type level is misleading:

class Foo<T> {
	constructor(public foo: T) {}
	clone(): this {
		return Object.create(Object.getPrototypeOf(this), Object.getOwnPropertyDescriptors(this));
	}
}
class Bar<T> extends Foo<T> {
	bar = true;
}
new Bar(42).clone(); // still a Bar<number>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A use case of the above:

interface Array<T> {
	slice(): this;
}

(But then for ReadonlyArray, should the same method overload be typed this & T[]?)

(idea from #36554 (comment))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug for this? Like the flatMap change in es2019, this isn't worth fixing unless it's breaking somebody's actual code, because any fix will break other code. Backward compatibility like this is the reason, for example, that we haven't been able to make overriding methods inherit parameter types.

}

interface ArrayConstructor {
/**
* Creates an array from an array-like object.
* @param arrayLike An array-like object to convert to an array.
* Creates an array from an array-like or iterable object.
* @param source An array-like or iterable object to convert to an array.
*/
from<T>(arrayLike: ArrayLike<T>): T[];
from<T>(source: ArrayLike<T>): T[];

/**
* Creates an array from an iterable object.
* @param arrayLike An array-like object to convert to an array.
* Creates an array from an array-like or iterable object.
* @param source An array-like or iterable object to convert to an array.
* @param mapfn A mapping function to call on every element of the array.
* @param thisArg Value of 'this' used to invoke the mapfn.
*/
from<T, U>(arrayLike: ArrayLike<T>, mapfn: (v: T, k: number) => U, thisArg?: any): U[];
from<T, U>(source: ArrayLike<T>, mapfn: (v: T, k: number) => U, thisArg?: any): U[];

/**
* Returns a new array from a set of elements.
* @param items A set of elements to include in the new array object.
*/
of<T>(...items: T[]): T[];
of<T extends any[]>(...items: T): T;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible break: better typing of tuples, meaning more precise downstream errors.

}

interface DateConstructor {
Expand Down Expand Up @@ -329,8 +339,18 @@ interface ReadonlyArray<T> {
* @param thisArg If provided, it will be used as the this value for each invocation of
* predicate. If it is not provided, undefined is used instead.
*/
find<S extends T>(predicate: (value: T, index: number, obj: readonly T[]) => value is S, thisArg?: any): S | undefined;
find(predicate: (value: T, index: number, obj: readonly T[]) => unknown, thisArg?: any): T | undefined;
find<S extends T>(predicate: (value: T, index: number, array: readonly T[]) => value is S, thisArg?: any): S | undefined;

/**
* Returns the value of the first element in the array where predicate is true, and undefined
* otherwise.
* @param predicate find calls predicate once for each element of the array, in ascending
* order, until it finds one where predicate returns true. If such an element is found, find
* immediately returns that element value. Otherwise, find returns undefined.
* @param thisArg If provided, it will be used as the this value for each invocation of
* predicate. If it is not provided, undefined is used instead.
*/
find(predicate: (value: T, index: number, array: readonly T[]) => unknown, thisArg?: any): T | undefined;

/**
* Returns the index of the first element in the array where predicate is true, and -1
Expand All @@ -341,7 +361,7 @@ interface ReadonlyArray<T> {
* @param thisArg If provided, it will be used as the this value for each invocation of
* predicate. If it is not provided, undefined is used instead.
*/
findIndex(predicate: (value: T, index: number, obj: readonly T[]) => unknown, thisArg?: any): number;
findIndex(predicate: (value: T, index: number, array: readonly T[]) => unknown, thisArg?: any): number;
}

interface RegExp {
Expand Down
Loading