microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.94k stars 12.48k forks source link

tsc allows incorrect Set constructor call #60412

Open maksnester opened 1 day ago

maksnester commented 1 day ago

🔎 Search Terms

Set, merge sets

🕗 Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/play/?#code/PTAEDsFMHdQZUgFwBQDp0G0CMAaUAmPAZgF0BKUEUAJwEMBLAZ0kdEmuoHtq8BzayLUSgAUAGNO4RsMZZQAXggx4SZAHIsasgG5xk6aEb4FS2AhRr8W3XqkyiJqGdXpUsvK6M7KYesIC2tADWLBCchpBSkKAARgCuwgAqcKC0ADZpnNCsfqIiVIgAFkygJUXREgAm0QCSoNC04IiQlQBcohJ2hgAsjsrmyBieuKCe+OS6QA

💻 Code

// new Set(...[1, 2, 3]) // raises error, great 
const s1 = new Set('1');
const s2 = new Set('2');

const s3 = new Set(...s1, ...s2); // it makes no sense but TS allows it 

// this is the code I actually wanted to write
const s4 = new Set([...s1, ...s2]);

🙁 Actual behavior

TS allows me to call the Set constructor in a way that makes no sense

🙂 Expected behavior

I'd like some error for the code that makes no sense

Additional information about the issue

No response

Andarist commented 1 day ago

Self-isolated repro~:

const s1 = new Set('1');
const s2 = new Set('2');

function test1<T>(iterable?: Iterable<T>) {}
const s3 = test1(...s1, ...s2); // no error

function test2<T>(iterable: Iterable<T>) {}
const s4 = test2(...s1, ...s2); // A spread argument must either have a tuple type or be passed to a rest parameter.(2556)
jcalz commented 1 day ago

Isn't it generally okay that you can spread an Iterable<X> into a function expecting a single optional parameter of type X?

function foo(x?: string) { }
const arr = ["a", "b"];
foo(...arr); // okay

That's reasonable because at worst you're just passing too many arguments into the function.

I feel like the weirdness here is the apparently intentional spreading of strings (that is, treating a string like an Iterable<string>) so that new Set("abc") would produce a 3-element set. (Otherwise what is the intent of new Set("1")?) Maybe in the case of Set<string> this is too confusing to allow, but this doesn't look like a bug anywhere to me, just off-brand use of the set constructor.

RyanCavanaugh commented 1 day ago

I agree; there's unfortunately nothing statically wrong about this code, it just has a valid intent other than the one the author wanted.

Andarist commented 1 day ago

@RyanCavanaugh what do you think about the difference shown in https://github.com/microsoft/TypeScript/issues/60412#issuecomment-2455401217 ?

RyanCavanaugh commented 1 day ago

@Andarist seems correct, or at least correct enough?

Andarist commented 1 day ago

The error message could be seen as slightly misleading there as the call without an error doesn't satisfy any of the requirements mentioned by it. And the message mentions that they must be met.

EDIT:// Ah, I see now how specific even this one is to strings. Still pretty surprising difference in the error message but I think it's OK.

maksnester commented 23 hours ago

Otherwise what is the intent of new Set("1")

it has nothing to do with the strings, this is just an example, maybe not the greatest one, but you can imagine 2 arbitrary sets with any types inside.

What I wanted is to merge 2 sets, but unfortunately, it is impossible to do it this way:

const s3 = new Set(...s1, ...s2); // it makes no sense but TS allows it 

which I had to learn by debugging irrelevant code for a few hours instead of having an error for this line.

nmain commented 21 hours ago

you can imagine 2 arbitrary sets with any types inside.

Typescript rejects this. See this example with both number and unconstrained T:

Playground

The problem is really about just strings, or more generally making sets where the element type is itself an iterable.