Strict null checks for Map members

[sanex3339]

trafficstars

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')!

[MoussaAdam]

any progress?

[sharedrory]

Is there any news on this?

[rushi7997]

Is there any progress on this?

[wmertens]

If you're always going to .get() the value if .has() returns true, is there even a benefit to calling .has() first? Is it faster somehow, better memory use?

If not, then the already proposed solution of storing .get() and checking that seems fine?

[nullhook]

If you're always going to .get() the value if .has() returns true, is there even a benefit to calling .has() first? Is it faster somehow, better memory use?

i'm also curious on this. i believe there's a difference in access times but also cache can take place here.

[o-az]

Should I give up?

[chocolateboy]

@dragomirtitian

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

Unfortunately, as @tadhgmister mentions, there are limitations to this approach, e.g. map.get(uncheckedKey) has the wrong type and map ends up being defined as never in the else branch:

if (map.has('foo')) {
    const foo = map.get('foo') // number ✔
    const bar = map.get('bar') // number x
} else {
    const temp = map // never x
}

That can be worked around with a bit of indirection, e.g.:

interface MapWithSafeGet<K, V, KnownKey extends K> extends GuardedMap<K, V, KnownKey> {
    get (k: KnownKey): V;
    get (k: K): V | undefined;
}

interface GuardedMap<K, V, K1 extends K = never> extends Map<K, V> {
    has <K2 extends K>(key: K2): this is MapWithSafeGet<K, V, K1 | K2>;
}

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

if (map.has('foo')) {
    const temp = map           // MapWithSafeGet<string, number, "foo">
    const foo = map.get('foo') // number
    const bar = map.get('bar') // number | undefined
} else {
    const temp = map           // GuardedMap<string, number, never>
    const foo = map.get('foo') // number | undefined
    const bar = map.get('bar') // number | undefined
}

if (map.has('foo') && map.has('bar')) {
    const temp = map           // MapWithSafeGet<string, number, "foo" | "bar">
    const foo = map.get('foo') // number
    const bar = map.get('bar') // number
    const baz = map.get('baz') // number | undefined
} else {
    const temp = map           // GuardedMap<string, number, never>
    const foo = map.get('foo') // number | undefined
}

EDIT: though this can still result in map reverting to never:

if (map.has('foo')) {
    const temp = map // MapWithSafeGet<string, number, "foo">

    if (map.has('bar')) {
        const temp = map           // MapWithSafeGet<string, number, "foo" | "bar">
        const foo = map.get('foo') // number
        const bar = map.get('bar') // number
        const baz = map.get('baz') // number | undefined
    } else {
        const temp = map // never x
    }
}

[relu91]

If you're always going to .get() the value if .has() returns true, is there even a benefit to calling .has() first? Is it faster somehow, better memory use?

If not, then the already proposed solution of storing .get() and checking that seems fine?

Since it is a Hash map I don't think it changes too much for trivial cases. I did a fast check here and both solutions perform quite well with 10k items.

[papagunit]

I abhor this issue's existence 6 years later without resolution.

Insane to consider that this is one of the most loved languages and yet we have core issues like this and the language spec/ref has been sacked.

Please let me know if I'm out of line, but this is frustrating.

[cmdruid]

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

I feel it is a bit embarrassing that core parts of ECMA script meant for type guarding are not supported by typescript, and that the answer so far is to abandon ECMA standards in favor of typescript's way of doing things. Which then begs the question who is steering the ship?