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

Change Map default generic parameters from "any" to "unknown" #52552

Closed
Zamiell opened this issue Feb 1, 2023 · 7 comments
Closed

Change Map default generic parameters from "any" to "unknown" #52552

Zamiell opened this issue Feb 1, 2023 · 7 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@Zamiell
Copy link
Contributor

Zamiell commented Feb 1, 2023

Suggestion

Description

const foo = new Map(); // The type is: Map<any, any>
const bar = new Set(); // The type is: Set<unknown>

Here, the behavior of the types is inconsistent. Naively, I would expect both of them to be unknown, or both of them to be any, depending on what Microsoft wants to do. (Would it result in safer code for both of them to default to unknown?)

More Info

I asked about this inconsistency in the TypeScript discord:

Raziel — Today at 11:34 AM
Probably because nobody bothered updating the Map constructor type definition
226dd0b#diff-79b0ebcbd15b54ae283f4edf9b35237aed7957275439a4aeff66dde3a5258f48R160

mkantor — Today at 11:40 AM
i think it was actually this: #43396

mkantor — Today at 11:42 AM
tl;dr MapConstructor has this overload but SetConstructor doesn't have a similar one (not sure if there's a good reason why Map has that since its other constructor signature's parameter is optional anyway):

new(): Map<any, any>;

jakub — Today at 1:59 PM
Is that overload even necessary?
it seems useless

🕗 Version & Regression Information

I tested on the playground, and:

  • In v3.3.3, the default type of a Set is any.
  • In v3.5.1 and later, the default type of a Set is unknown, causing the inconsistency.
@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Feb 1, 2023
@RyanCavanaugh
Copy link
Member

Two type definitions for two different types being different is not a per se bug. They have different use cases, etc..

Please make a positive case for one or the other to change.

@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 1, 2023

Sets are used in the specific subset of situations where you need a Map<T, true>. At a high-level, we can think of a Set<T> as syntactic sugar to express Map<T, true> more succinctly and in a more type-safe way.

We can visualize the two types like this:

image

Based on this, we can establish a type equality of:
Set<T> === Map<T, true>

Because of this "equality", we would expect Set to inherit the specific first default parameter of Map.
Written in "math-like" form:

Map<K, V>
-->
Map<any, any> (instantiating the generic types with the default generic parameters)
-->
Map<any, true> (constraining the type to the Set subset case)
-->
Set<any> (converting the type to a set using the equality established above)

Thus, because of this transitivity, we would expect that the default parameter of Set would be equal to the first default parameter of Map.

By breaking this transitivity in an arbitrary(?) way, TypeScript is introducing a subtle pitfall that interferes with end-users ability to properly reason about their code.

As a short example, imagine that a lint rule exists that forbids the any type on maps and sets. It would force code to explicitly declare maps like this:

const myMap1 = new Map(); // Fails the lint rule.
const myMap2 = new Map<string, true>(); // Passes the lint rule.

But such a rule would not work properly with sets:

const mySet1 = new Set(); // Passes the lint rule. WTF?
const mySet2 = new Set<string>(); // Passes the lint rule.

Of course, other pitfalls exist as well. The main difference between any and unknown is the ability to type narrow the latter. Thus, we could imagine a function that handles sets in a generic way, properly narrowing the generic types to handle each case. Say that the programmer needed to refactor the function to also handle maps. "Oh, no problem, I'll just change Set to Set | Map, and I'll be done!" But the function would subtly break, because any cannot be narrowed. With maps and sets having such close parity, this behavior would probably be unexpected by the author.

@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 1, 2023

I see two courses of action:

  1. Change Map to default to Map<unknown, unknown>.
  2. Change Set to default to Set<any>.

Either of these changes would restore the parity and alleviate the issues discussed in my previous comment.
I am mostly agnostic as to which of these two would be better, and defer to the TypeScript team.

In my mind, option 1 (changing Map to unknown) might be better, as it guides the ecosystem towards writing safer code, but on the other hand, it would potentially cause existing code to fail complication. (Although to be fair, a similar break already occurred when Set<any> got changed to Set<unknown> in TypeScript version 3.5.1, so this would just be more of the same.)

@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 1, 2023

Retsam from Discord argues that parity between Map and Set isn't really relevant, and that all we should be concerned with is that TypeScript provides safe defaults for its users.

For example, consider the following code:

interface Person {
  id: string;
  name: string;
}

class PersonStore {
    private people = new Map();

    storePerson(person: Person) {
        this.people.set(person.id, person)
    }

    getName(personId: string): string {
      // Oops, this should have been name, not username
      return this.people.get(personId)?.username ?? "???";
    }
}

If TypeScript changed the default of Map from any to unknown, then this bug would have been successfully caught by the compiler! That's a big win in my book.

So, if you don't really care about Map and Set having parity, then focus on this part instead. =p

@RyanCavanaugh
Copy link
Member

Right, what I'm saying is literally log that suggestion. Two definitions being different isn't an actionable bug; "Change the type signature of Map" is an actionable suggestion.

@mrclay
Copy link

mrclay commented Feb 7, 2023

Map<unknown, unknown> would be preferable because this is currently valid with no warnings at all:

const cache = new Map();

const item: string = cache.get('foo');

@Zamiell Zamiell changed the title Inconsistent behavior between Map and Set default generic parameters Change Map default generic parameters from "any" to "unknown" Feb 21, 2023
@github-actions
Copy link

github-actions bot commented Jun 8, 2023

This issue has been marked as 'Not a Defect' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

3 participants