add types for set methods proposal #57230
Conversation
typescript-bot
added
the
For Uncommitted Bug
I think this concept of "overlaps with" or its opposite "is disjoint with" is actually useful quite often. For example, it would be the ideal type of the parameter in export const includes = <array extends readonly unknown[], element>(
array: array,
element: element & array[number] extends never ? array[number] : element
): element is element & array[number] => array.includes(element)
declare const array: string[]
declare const overlapping: string | number
declare const nonOverlapping: boolean | number
// okay
includes(array, overlapping)
// error
includes(array, nonOverlapping)I wonder if the team would consider adding a builtin |
I'm not sure there's a good solution for this right now - it's more or less the same problem as #48247/#26255 and the fix is generally considered to be #14520 (lower-bounded type parameters)--though FWIW I think an "overlaps with" constraint would be more useful for this than "supertype of" |
|
@fatcerberus I definitely agree declare const a: { a: true }
declare const b: { b: true }
// '{ a: true; }' and '{ b: true; }' have no overlap
if (a === b) {
} |
|
Well, in reality any |
|
@fatcerberus Maybe it would be an opportunity to revisit that logic if it were to be published as its own utility 😅 |
src/lib/esnext.collection.d.ts
Outdated
| @@ -9,3 +9,49 @@ interface MapConstructor { | |||
| keySelector: (item: T, index: number) => K, | |||
| ): Map<K, T[]>; | |||
| } | |||
|
|
|||
| interface SetLike<T> { | |||
There was a problem hiding this comment.
At first glance, this looks a lot like ReadonlySet. I know that these methods only need a subset of functionality, but it might be worth using that instead.
There was a problem hiding this comment.
You need the keys method, so I don't think that works unless that gets added ReadonlySet as well. (And users shouldn't have to implement forEach for their own set-likes, though that isn't as big a deal since that probably doesn't come up as much.)
There was a problem hiding this comment.
Wait, no, keys is already there, just in a different file, whoops.
Still, I think it would be kind of annoying not to be able to do set.union({ keys(){ return array.keys() }, size: array.size, has: () => {} }) or similar. Not the end of the world, but I'd prefer to keep SetLike unless there's a reason to switch to ReadonlySet.
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at d9efe96. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at d9efe96. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at d9efe96. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at d9efe96. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the regular perf test suite on this PR at d9efe96. You can monitor the build here. Update: The results are in! |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
The "overlaps" concept is what we internally call "comparability". The problem with expressing it as an intersection that produces In other words, you can't say that |
|
@DanielRosenwasser Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Hey @DanielRosenwasser, the results of running the DT tests are ready. |
|
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
|
@DanielRosenwasser Yes, that is exactly the behavior I'd expect in a structural type system, with a result of I'm often frustrated by TS overreaching based on a comparability check (e.g. #44645), especially when the word "overlap" is used in the error message which has a specific meaning that does not match the criteria for the error. |
|
I brought this up in the design meeting, specifically focusing on
The feedback for them was:
@bakkot, in this PR I'd suggest to just address the first two points, do whatever you feel is right for point 3, and we will bikeshed by the time we can bring this in for TypeScript 5.5. |
|
WFM, thanks for the feedback. I'll get that pushed up later today. Any opinions on |
I think the right thing for those is to take a |
This is generally what people expect of e.g. |
|
@bakkot I'm not from TS team and just curious: wouldn't it be better if type ReadonlySetLike<T> = Pick<Set<T>, 'size' | 'has' | 'keys'>In any case of probable future changes of |
|
Sure, that would work too. I leave it up to the TS to decide which approach they'd prefer. |
|
Doesn't TypeScript/src/lib/esnext.collection.d.ts Line 17 in 6ceb395 In current Chrome: const mySetLike = {
size: 2,
has(a) { return a === 'foo' || a === 'bar' },
keys() { return ['foo','bar'] }
};
// new Set(['baz']).union(mySetLike);
// Uncaught TypeError: string "next" is not a function
mySetLike.keys = () => {
let i = 0;
return {
next() {
i++;
if (i === 1) return { value: 'foo', done: false };
if (i === 2) return { value: 'bar', done: false };
return { done: true };
}
};
};
new Set(['baz']).union(mySetLike);
// Set(3) {'baz', 'foo', 'bar'}Piggybacking off |
|
@noinkling Nice catch, that's an embarrassing typo given that I designed the feature 😅. Fixed. Possibly should update the description of |
|
@bakkot One more question about return type for difference<U>(otherSet: ReadonlySetLike<U>): Set<Exclude<T, U>>
symmetricDifference<U>(otherSet: ReadonlySetLike<U>): Set<Exclude<T, U> & Exclude<U, T>> |
|
|
|
Any blockers* for this PR? My PR into Mobx mobxjs/mobx#3853 state manager (which mostly used for React.js) is depending on it * except failed tests |
Those definitions would be too strict since a given instantiation of const set1 = new Set<"A" | "B">(["A", "B"]);
const set2 = new Set<"B" | "C">(["C"]);
const set3 = set1.difference(set2);
set.has("A"); // true
set.has("B"); // trueEven though |
IMO, an interface is a better option as it allows for interface merging if a polyfill is loaded that also introduces these types. |
@rbuckton so maybe wrap it in |
| isSubsetOf(other: ReadonlySetLike<unknown>): boolean; | ||
| /** | ||
| * @returns a boolean indicating whether all the elements in the argument are also in this Set. | ||
| */ | ||
| isSupersetOf(other: ReadonlySetLike<unknown>): boolean; |
There was a problem hiding this comment.
@DanielRosenwasser would overloads like the following be worthwhile?
interface Set<T> {
isSubsetOf<U>(other: ReadonlySetLike<U>): this is Set<T & U>;
isSupersetOf<U>(other: ReadonlySetLike<U>): other is ReadonlySetLike<T & U>;
}There was a problem hiding this comment.
We can always change these in a follow-up PR.
const set1 = new Set<"A" | "B">(["A", "B"]);
const set2 = set1.difference(["B", "C"]); // infer the type `["B", "C"]` hereHowever, |
Hm. The way I've been doing that ( |
|
CI always tests main + the PR; have you merged main locally? |
|
Aha, thanks, that's fixed it. |
| isSubsetOf(other: ReadonlySetLike<unknown>): boolean; | ||
| /** | ||
| * @returns a boolean indicating whether all the elements in the argument are also in this Set. | ||
| */ | ||
| isSupersetOf(other: ReadonlySetLike<unknown>): boolean; |
There was a problem hiding this comment.
We can always change these in a follow-up PR.
ghost
mentioned this pull request
Fixes #57228.
Set methods are stage 3, and I expect to ask for stage 4 in April.
I've used the simplest possible types here, but in some sense these are too restrictive. It's perfectly reasonable to ask if a Set of
0 | 1is a subset of a Set ofnumber, for example, or even of a Set of1 | 2- anything where the intersection of the types is nonempty (I guess that would be, any typeSsuch thatT & S extends neveris false).Unfortunately I don't know of a good way to express that constraint, at least not without making the types way more complex. So I've stuck with the simple thing here. But if someone has a suggestion for better types I'd happily take it.
One possibility would be to have the Set-producing types take
SetLike<T>but the predicates takeSetLike<unknown>. I don't know if that would be better.Do note that this this PR introduces the
SetLikeinterface.