Strict null checks for Map members

[sanex3339]

TypeScript Version: 2.0.0-beta
Code

export interface INode {
    type: string;
    parentNode?: INode;
}

export interface IIdentifierNode extends INode {
    name: string;
}

public static isIdentifierNode (node: INode): node is IIdentifierNode {
    return node.type === NodeType.Identifier;
}

var namesMap = new Map<string, string>();

// main part
if (Nodes.isIdentifierNode(node) && namesMap.has(node.name)) {
    node.name = namesMap.get(node.name);  //`Type 'string | undefined' is not assignable to type 'string'.`
}

Expected behavior:
No errors

Actual behavior:
Type 'string | undefined' is not assignable to type 'string'.

Looks like here no TypeGuards for Maps?

[mhegazy]

the issue is not type guards, since the type of the map does not change between he has and the get calls. the issue is relating two calls. you want to tell the compiler the output of get is known to be undefined, because a call to has was successful. i am not sure i see how this can be done given the current system.

[use-strict]

This effectively forces users to add the postfix ! operator for every Map#get. It doesn't look like something that can be statically analyzed by the compiler. There are two consequences, which to be honest, make me want to give up using strict null checks.

A. Type inference for return values is no longer reliable. Given that null or undefined can't be automatically stripped in certain cases, like above, one can get an incorrect return type. Because the behavior is unpredictable, this means one has to always write the return type annotations and not use type inference. This is very inconvenient.

Example:

function getNumber(map: Map<string, number>, key: string, defaultValue: number) {
    if (map.has(key)) {
        return map.get(key);
    }
    return defaultValue;
}

The inferred return type is number|undefined, even though it can never be undefined.

B. The explicit ! acts just as a type assertion so it suffers from the same limitations. It's an unconditional bail from the compiler checks. Since there is no way to relate it to the condition (Map#has in this case), once the condition is changed, the user also has to revisit the !. This is just as error-prone as not having strict null checks and remembering to check against null values.

[mhegazy]

the alternative is to change the definition of Map, to be less strict, and assume that users will always call has appropriately.

[RyanCavanaugh]

People can already augment the Map interface themselves with a non-null-returning signature for get if they want to assume they're doing the right thing at all times (though if we did modify the signatures, the reverse would also be true for people who want soundness over convenience).

[use-strict]

@RyanCavanaugh , changing the Map interface to be less strict goes in the opposite direction of having the 'strictNullChecks' flag in the first place. The get method can and will return undefined in some cases. So it's a matter of compromise. One can either accept this behavior or not use 'strictNullChecks' at all. Either is fine. It would also be nice to have these issues documented in brief on the official docs page, so people know to expect.

Going a bit off-topic, in my personal project, from 100+ migration errors from 'strictNullChecks', only 2-3 were actually relevant and only appeared in error cases anyway. Others were just things the compiler didn't figure out by itself (I use Maps heavily), including the forEach closures problem. So, in light of my previous comment, I'm yet undecided if this feature would help me or not.

For the work project, it would make more sense to add this. However, most developers will struggle at first with the issues regarding lambda functions and they will also tend to overuse the ! operator as they do with any type assertions. I feel that the learning curve is already a bit steep because of the typing complexities and these subtleties introduced by 'strictNullChecks' don't really help the situation. Luckily, I don't have to make the decision alone in this case :)

[kitsonk]

As we (@dojo) have converted more of our code over, we are finding when using things like Map and Set and other higher order constructs, the ! escape hatch is getting a bit annoying (and might even become unsafe, as developer get desensitised if they have properly guarded for the value.

I wonder how difficult it would be to allow an expressing, like a custom type guard that would allow you to expressing something that CFA would be able to track... like maybe something like:

interface Map<K, V> {
    has<L extends K>(key: L): L is V;
    get<L extends K>(key: L): V | undefined;
}

Where where CFA could be fairly narrow in its type inference and if it see that same literal type, in the same block, it assumes the type on the right (which would eliminate undefined).

[zheeeng]

Hope the sound improvement to avoid manually casting its non-null.

[sod]

I like that it just assumes by default that it may be undefined. But in a case like:

type Key = 'foo' | 'bar';

const map = new Map<Key, number>([
    ['foo', 1],
    ['bar', 2],
]);

map.get('foo').toExponential();
^^^^^^^^^^^^^^ TS: Object is possibly 'undefined'

Could at least take the keys from the constructor for granted.

[hueyhe]

@sod I also prefer using typescript with strict null check. But here is a strange thing, I tried code below in typescript playground.

const map = new Map<string, number>();
const a: number = map.get('foo'); // map.get might return `number | undefined`

I expect a type check error, but it seems passed typescript type check. Is this because strict null check is not enabled in typescript playground?

[kitsonk]

@hueyhe

[hueyhe]

@kitsonk Got it. Didn't notice, my bad...

[tadhgmister]

One solution is to use a construct similar to swift's optional binding where you assign a variable in the if statement, then the block of if statement is type guarded that the variable is not undefined or null.

const map = new Map<string, number>();

let val: ReturnType<typeof map.get>;
if ( val = map.get('foo')) {
    // here val is type guarded as number since if statement
    // only enters if it's not undefined.
    val.toExponential();
}

link to playground

[tadhgmister]

As for a solution that makes has a type guard, something like this may serve some cases:

interface GuardedMap<K, T, KnownKeys extends K = never> extends Map<K, T> {
    get(k: KnownKeys): T;
    get(k: K): T | undefined;

    has<S extends K>(k: S): this is GuardedMap<K, T, S>;
}

This works great for string literals and sometimes with enums but has enough odd behaviour that definitely should not be implemented as main Map type:

let map: GuardedMap<string, number> = new Map();

// Works for string literals!
if (map.has('foo')) {
    // works for string literals!
    map.get('foo').toExponential();
    // this is identified as maybe being undefined!!
    map.get('bar').toExponential();
}

let x = 'foo'; // typeof x === string
if (map.has(x)) {
    // in here all strings are considered known keys.
    map.get(x).toExponential();
    // no error D:
    map.get('bar').toExponential();
}
// lets say we end up with a variable with type never by mistake
let n = 0 as never; 
// this is now considered type guarded??
map.get(n).toExponential(); 

[dragomirtitian]

@tadhgmister I was thinking along the same line when reading this issue. You can forbid dynamic access altogether with a conditional type:

type GetKnownKeys<G extends GuardedMap<any, any, any>> = G extends GuardedMap<any, any, infer KnownKeys>  ? KnownKeys: never;
interface GuardedMap<K, T, KnownKeys extends K = never> extends Map<K, T> {
    get(k: KnownKeys): T;
    get(k: K): T | undefined;

    has<S extends K>(k: S): this is GuardedMap<K, T, (K extends S ? never : S)>;
}

let map: GuardedMap<string, number> = new Map();

// Works for string literals!
if (map.has('foo')) {
    map.get('foo').toExponential();
        
    map.get('bar').toExponential();
}

let x = 'foo'; // typeof x === string
if (map.has(x)) {
    // error
    map.get(x).toExponential();
    // error
    map.get('bar').toExponential();
} 

I tried to get it to work for nested if statements, but I think I hit a compiler but I'm still investigating

[dragomirtitian]

@tadhgmister

Or an even simpler version that works for nested ifs as well:

interface GuardedMap<K, V> extends Map<K, V> {
    has<S extends K>(k: S): this is (K extends S ? {} : { get(k: S): V }) & this;
}

let map: GuardedMap<string, number> = new Map();

// Works for string literals!
if (map.has('foo')) {
    if(map.has('bar')) 
    {
        // works for string literals!
        map.get('foo').toExponential();
        map.get("bar") // ok 
    }
        
    map.get('bar').toExponential(); ///  error
}

declare var x: string
if (map.has(x)) {
    map.get(x).toExponential() // error
    map.get("x").toExponential()// error
}

[louiidev]

one solution i found for this was storing the map value in a variable then checking if that's undefined for before using it.

const namesMap = new Map<string, string>();

// main part
const name = namesMap.get(node.name)
if (Nodes.isIdentifierNode(node) && name) {
    node.name = name;
    // no error
}

not perfect but prevents error being thrown

[ElianCordoba]

Would be possible to do something like shown here?

[pelssersconsultancy]

What about adding a getOrElse(f: () => V): V method that

• if has(key) ==> returns value of type V
• if !has(key) ==> insert f() for key and return f()

[MarcSharma]

Even with the ! postfix operator on get, typescript still throw an undefined error.

MWE:

let m = new Map<string, string>()
if (m.get('a') === undefined) {
    m.set('a', 'foo')
}
let value = m!.get('a') // value still possibly undefined according to ts
m.set('a', value) // throw an error

export default m

I feel this should be changed ?

[sod]

Wrong placing of the !. If you want to prune the undefined, you have to ! the return value: const value = m.get('a')!

[cmdruid]

Type inference shouldn't fail for native type-guards on primitive, period. This isn't a situation with some hokey 'flexible' API. It is literally a boolean check whether a value exists or not, for an object that is part of the core language.

Typescript should support language primitives defined by the ECMA standard, full stop. That should take precedence over a 'popular library'. There's no point in continuing this discussion if the answer is to use the '!' escape hatch, which by typescript's own linting standards declares that your code is no longer 'type-safe'.

If we were having this conversation in any other language it would be considered a joke. Typescript has its flaws, but not supporting core language features is very, very bad. It means that we now have two competing standards: ECMA standard and the Typescript standard. If that's the game then maybe it is time to switch to another language.

[tadhgmister]

If we were having this conversation in any other language it would be considered a joke.

well no, flow does the exact same thing and they didn't even give it a second thought so at least there it was not a joke.

What is the point of .has() for maps and sets if they are fundamentally unsupported by typescript?

1. they can serve other purposes.
2. Maps can have undefined as values so it'd be necessary in that case to be able to differentiate if the value is undefined or if the key was not present.

the answer so far is to abandon ECMA standards in favor of typescript's way of doing things.

we now have two competing standards: ECMA standard and the Typescript standard.

Could you please point me to the place where ECMA standards explicitly endorses the paradigm to call Map.has() as a predicate to every call to Map.get()? I was under the impression ECMA standards were for implementations to support core features, not to encourage specific ways of writing code.

---

The way I understand it, either your map doesn't have undefined values in which case you can just call get then check if it is undefined so there is no issue, or you can have undefined as values in your map in which case the return value after checking the key is inside the map can still be undefined so it is still not an issue.

[xgqfrms]

some solutions

1. using ?? 👍

function majorityElement(nums: number[]): number[] {
  const base = ~~(nums.length / 3);
  const map = new Map<number, number>();
  const set = new Set<number>();
  for(const num of nums) {
    if(!map.has(num)) {
      map.set(num, 1);
    } else {
      map.set(num, (map.get(num) ?? 0) + 1);
    }
    if((map.get(num) ?? 0) > base) {
      set.add(num);
    }
  }
  return [...set];
};

2. override Map.get, remove undefined 👎

interface Map<K, V> {
  // get(key: K): V | undefined;
  get(key: K): V | typeof key;
  get(key: K): V | K;
}

function majorityElement2(nums: number[]): number[] {
  const base = ~~(nums.length / 3);
  const map = new Map<number, number>();
  const set = new Set<number>();
  for(const num of nums) {
    if(!map.has(num)) {
      map.set(num, 1);
    } else {
      map.set(num, map.get(num) + 1);
    }
    if(map.get(num) > base) {
      set.add(num);
    }
  }
  return [...set];
};

[TomokiMiyauci]

A practical workaround would be TC39 proposal-upsert.
I have created a type-safe ponyfill.

[eladchen]

Hope I'm not missing anything.

Why does the get method adds "undefined" to its return signature? Why not use the generic that was passed in as is?

const safe: Map<string, string>() = new Map();

const unsafe: Map<string, string | undefined> = new Map();

[minecrawler]

@eladchen

Why does the get method adds "undefined" to its return signature? Why not use the generic that was passed in as is?

Because get() may be undefined, as in:

const map = new Map<string, string>();

map.set('foo', 'bar');
console.log(map.get('this_key_does_NOT_exist_in_the_map'));// undefined ;)

TypeScript doesn't and cannot track what you set inside your Map (esp. for dynamic content), so for practicality it just reminds you to check yourself. However, as explained in the OP, TypeScript is missing type guarding support for has().

[eladchen]

@minecrawler undefined is always a possibility even for object literals or any other typed object I may come up with.

If I specify Map<string, string> its my way of controlling the get method, In case I need to, I can explicitly pass in undefined as well.

Don't see the point in TS making that decision for me.

[minecrawler]

@eladchen

undefined is always a possibility even for object literals or any other typed object I may come up with.

In the context of a Map.prototype.get(), yes. Outside that context, it depends. Let's use an example of a map without undefined:

const obj: Record<string, number> = { a: 17 };

console.log(obj.a); // always 17
console.log(obj.b); // always undefined... OOOPS, user didn't expect undefined because you forgot to type it!!!

function sendAPI(data: number): Promise<Response> {
    return fetch('http://example.com/' + data);
}

// more practical example:
sendAPI(obj[document.querySelector<HTMLInputElement>('input[type="text"].key-input')!.value]);
// might show an error on the frontend or backend side, since it's undefined instead of a number, but at a hard to debug point...

In this example, TypeScript can statically verify a few things, but not everything. And with this dangerous kind of typing, which disregards undefined, you get all kinds of errors and the code cannot statically be tested. You'll see the contract-offending behavior only at runtime.

A better type for obj would be { a: number }. You can try this yourself in the TS playground. TS will magically tell you inside your editor that the code has bugs. With { a: number }, there won't be any (hidden) undefineds and you can code with confidence!

Don't see the point in TS making that decision for me.

TypeScript doesn't make that decision for you, it simply implements the standard to which you need to adhere: ECMA-262.

The point is that TypeScript is typing out all possibilities to make your code statically verifiable according to the standard. Since undefined is an option, it must be part of get()'s type. If undefined weren't part of the type, you couldn't statically verify if the code is correct. Hence, in TypeScript, the type includes the correct typing of T | undefined, as documented in the ECMA-262 standard.

[dexx086]

@eladchen Then it's somewhat controversial why obj.b access doesn't add then undefined as well to its type, but Map.get does.
But then undefined should be added for simple array accesses as well, right? Like:

const arr: string[] = [];
const badValue: string = arr[0]; // Bad, casues runtime error...

Don't know what's the sweet pot here because adding undefined to simple indexed array accesses could be very cumbersome to handle as well. But it's not unified handling if Map.get() adds undefined, while a dynamic object's property access, or even just a simple array indexed access doesn't...

[eladchen]

I'm not sure my message is clear enough. I'm saying typescripy should let me define whether a map has undefined value or not.

…

On Fri, Sep 22, 2023, 12:26 dexx086 ***@***.***> wrote: @eladchen <https://github.com/eladchen> Then it's somewhat controversial why obj.b access doesn't add then undefined as well to its type, but Map.get does. But then undefined should be added for simple array accesses as well, right? Like: const arr: string[] = []; const badValue: string = arr[0]; // Bad, casues runtime error... Don't know what's the sweet pot here because adding undefined to simple indexed array accesses could be very cumbersome to handle as well. But it's *not unified* handling if Map.get() adds undefined, while a dynamic object's property access, or even just a simple array indexed access doesn't... — Reply to this email directly, view it on GitHub <#9619 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3V64MVH3HCP6SJMINZIJ3X3VKSTANCNFSM4CJGZCVQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[dexx086]

Sry, I wanted to mention @minecrawler in my post, not you.

With you basically I agree, it bothers me too that we cannot control Map's behaviour.

But viewing a bigger picture, it's controversial that Map is not handled the same way regarding undefined type possiblity as the mentioned other cases (Records, array indexed access).

But still, don't know what the optimal solution could be (handling Map without undefined would align to the other cases at least, but the other cases already introduce unhandled undefined cases...).

[minecrawler]

Ok, I understand what you mean, however there's a config option for that: noUncheckedIndexedAccess.

I didn't want to start a discussion about basics in the language, though, since they are not in scope for this request. Let's stay on topic. TypeScript isn't perfect, and if you feel like it should be improved, you can open up a new proposal to remove the undefined from get() or add more parameters to trigger your desired behavior :)

[dexx086]

@minecrawler You're right, with this option we can control the mentioned Record<> property and array indexed accesses, however this is still a non-unified handling (regarding Map.get) which I tried to point out.

Maybe the solution would be then to extend noUncheckedIndexedAccess option to control Map.get as well, so they could be handled in a unified way. Does it seem to be a valid proposal?

[essenmitsosse]

I can understand and agree with the handling of Map<string, number>, as it is aligned with Record<string, number> (given noUncheckedIndexedAccess). What confuses me, that unlike Record<'a' | 'b', number>, Map<'a' | 'b', number> doesn't behave the same way when given a string union. Record not only expects all members to be there (which is great), but also guarantees them to exist.

const map = new Map<'a' | 'b', number>([
  ['a', 1],
  ['b', 2], // leaving this out would do nothing
])

const resultMap = map.get('a')
//      ^? const resultMap: number | undefined

const record: Record<'a' | 'b', number> = {
  a: 1,
  b: 2, // leaving this out would give a ts error:
        // Property 'b' is missing in type '{ a: number; }' but required 
        // in type 'Record<"a" | "b", number>'.ts(2741)
}

const resultRecord = record.a
//      ^? const resultRecord: number

While there is probably some low-level language reason for it to be this way, it certainly is confusing. Also, it makes the use of Map pretty unattractive for a lot of cases.

[cdskill]

Can we consider to have this naive approach like this one:

const mop = new Map<string, string>([
    ['coco', 'coucou']
]);

function check(id?: string | null): boolean {
    return mop.has(id!);
}

check(undefined) // Valid => false
check(null) // Valid => false
check(1) // Invalid cause of typing.

Taking advantage to use the non-null assertion operator in the body of function should be conscientious and considering to know what we're doing. Is there any edge cases that I didnt thought about it ?

[dexx086]

@cdskill I don't see what check(...) would solve here. It's not even a type-guard (cannot be that complex that it could validate that a given key exists in a map...), we would still need to ensure a received value is not undefined:

const value = mop.get('test');
if (value !== undefined) {
    // ... value is `string` from here
}

Or else, it would still have string | undefined type.

[cdskill]

Actually my comment is the redundant with this one above: #9619 (comment) from use-strict