Flow analysis doesn't work with es6 collections 'has' method

[MastroLindus]

TypeScript Version: 2.1.1

Code

const x = new Map<string, string>();
x.set("key", "value");
if (x.has("key")) {
  const y : string = x.get("key");  // error: y is string | undefined, not string
}

Expected behavior:
y is narrowed down to string
Actual behavior:
y is still string | undefined even after checking if the map has that key

[arusakov]

@MastroLindus
You can workaround this by using:

const y = x.get('key');
if (y != null) {
  // y is string here
}

[DanielRosenwasser]

I wrote up a fun little hack that uses this types, this type guards, and literal types to get the basic functionality going:

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

interface Map<K, V> {
    has<CheckedString extends string>(this: Map<string, V>, key: CheckedString): this is MapWith<K, V, CheckedString>
}

interface MapWith<K, V, DefiniteKey extends K> extends Map<K, V> {
    get(k: DefiniteKey): V;
    get(k: K): V | undefined;
}

x.set("key", "value");
if (x.has("key")) {
  const a: string = x.get("key"); // works!
}

Unfortunately these don't stack perfectly - you can't write x.has("key") && x.has("otherKey") and have it work for both because of the way that the overloads are reconciled with intersection types. 😞

[DanielRosenwasser]

Actually, you can get this to work better with another overload.

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

interface Map<K, V> {
    // Works if there are other known strings.
    has<KnownKeys extends string, CheckedString extends string>(this: MapWith<string, V, KnownKeys>, key: CheckedString): this is MapWith<K, V, CheckedString | KnownKeys>

    has<CheckedString extends string>(this: Map<string, V>, key: CheckedString): this is MapWith<K, V, CheckedString>
}

interface MapWith<K, V, DefiniteKey extends K> extends Map<K, V> {
    get(k: DefiniteKey): V;
    get(k: K): V | undefined;
}

x.set("key", "value");
if (x.has("key") && x.has("otherKey")) {
  const a: string = x.get("key"); // works!
  const b: string = x.get("otherKey"); // also works!
}

[ivstas]

@DanielRosenwasser I think you're a bit overcomplicating the stuff.

const map = new Map<string, string>();
const value = <string>(map.has('key') ? map.get('key') : 'default value');

Anyway, these all are workarounds.
What's need to be fixed if flow analysis.

[herecydev]

@DanielRosenwasser is on the cards for being fixed via flow analysis? It feels like there are some workarounds but no acknowledgment whether this is intended or needs fixing.

[jeysal]

Just stumbled upon this issue, wanted to note that this is quite hard to get right. For example the workarounds do not take into account that the entry might be deleted from the map between the has and the get.

[ivstas]

@jeysal that's true, but I believe 90% of use cases are just has check and get right after the check.

[emilio-martinez]

This is such common use case—I was surprised it's not covered! Fortunately, as others have mentioned, it's easy to work around the gap.

To the argument of complexity, would the static analysis here not be much simpler given that data access in ES2015 collections is much more straightforward compared to, for example, arrays?

array.indexOf(key) >= 0 // O(n)
set.has(key) // O(1)

[RyanCavanaugh]

This much, much, much, much more tricky than it looks because you also have to define how long the has check should last.

[chapterjason]

I want to note that this should also apply if the map is a property in a class.
Annoying to create workarounds everywhere... 😕

[herecydev]

@chapterjason you could do

const factory = this.factories.get(name)

if(factory)
return factory;

throw new ArgumentException("...");

[benwiley4000]

Wouldn't you have the exact same problem with an object index lookup when the members on the object are optional? Does TypeScript handle that case correctly?

[benwiley4000]

Ok well I found my answer which is that TypeScript does not handle this correctly with objects:

[bbugh]

Will the awesome new control flow analysis in TypeScript beta 4.4 make this possible/easier?

[avin-kavish]

It's possible to implement this way, but it's as verbose as loading the value and checking if it's defined.

class OutBox<V> {
  public value!: V

  constructor() {}

  static empty<V>() {
    return new OutBox<V>()
  }
}

class PowerfulMap<K, V> extends Map<K, V> {
  getOrThrow(key: K): V {
    return this.get(key) ?? throw new Error('where throw expressions???')
  }

  getIfHas(
    key: K,
    outBox: OutBox<V | undefined>,
  ): outBox is OutBox<NonNullable<V>> {
    outBox.value = this.get(key)
    return this.has(key)
  }
}

class Foo {
  someProp = 'a'
}

const map = new PowerfulMap<string, Foo>()
const box = OutBox.empty<Foo | undefined>()

if (map.getIfHas('hello', box)) {
  console.log(box.value.someProp) // guarded
} else {
  console.log(box.value.someProp) // no guard
}

[somebody1234]

or... just this (Playground):

class XMap<K, V> extends Map<K, V> {
    getThen<T>(key: K, map: (value: V) => T): T | undefined {
        if (this.has(key)) {
            return map(this.get(key)!);
        }
        return;
    }
}

const m = new XMap([['a', 1]]);
// (of course you can do all computation in the callback)
console.log('mapped v', m.getThen('a', v => { console.log('v', v); return v + 1; }));
console.log('mapped v (2)', m.getThen('b', v => { console.log('v (2)', v); return v + 1; }));

[jonlepage]

it's frustrating to see the mediocrity of some api in js
the Set and Map should have implemented natively .get() .find()

Map.prototype.get should return strictly V or throw error and Map.prototype.find should return V|undefined
Idk who approved this es2015 API feature, but they coded lazily.

[icecream17]

map#find I think will be added in the iterators protocol
https://github.com/tc39/proposal-iterator-helpers

[fabiospampinato]

By the way this capability could be pretty useful in Solid.js codebases, where you could write code like signal() ? foo(signal()) : undefined), and the second signal() call could get narrowed correctly.

Though obviously it's unclear how to invalidate this perfectly.

[paulius-valiunas]

By the way this capability could be pretty useful in Solid.js codebases, where you could write code like signal() ? foo(signal()) : undefined), and the second signal() call could get narrowed correctly.

Though obviously it's unclear how to invalidate this perfectly.

this is impossible, Typescript doesn't know what signal() does, it might have side effects, it might return a random value every time etc.

[paulius-valiunas]

or... just this (Playground):

class XMap<K, V> extends Map<K, V> {
    getThen<T>(key: K, map: (value: V) => T): T | undefined {
        if (this.has(key)) {
            return map(this.get(key)!);
        }
        return;
    }
}

const m = new XMap([['a', 1]]);
// (of course you can do all computation in the callback)
console.log('mapped v', m.getThen('a', v => { console.log('v', v); return v + 1; }));
console.log('mapped v (2)', m.getThen('b', v => { console.log('v (2)', v); return v + 1; }));

do you really want to go back to callback hell?

[Batleram]

Is there a reason why has() can't use the same "check lifetime" that the is keyword is providing?

I feel like their applications are very similar

[ConnorBrennan]

It has been seven years and the thing clearly designed to be used as a type guard, still isn't treated as a type guard. Is there a reason that this isn't built in or has it just never been a priority?

[avin-kavish]

On Wed, 16 Aug 2023 at 00:37, Connor Brennan ***@***.***> wrote: It has been seven years and the thing clearly designed to be used as a type guard, still isn't treated as a type guard. Is there a reason that this isn't built in or has it just never been a priority? — Reply to this email directly, view it on GitHub <#13086 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALRQ7U2DED3TJ5NOOW2W7QLXVPCFJANCNFSM4C2KVZUQ> . You are receiving this because you commented.Message ID: ***@***.***>

It is not clearly designed to be a type guard. It’s an existence check for the key. The existence of the key doesn’t necessarily imply that the value is the type you expect it to be. You want that? Write a type guard directly for the value.

[ConnorBrennan]

It is not clearly designed to be a type guard. It’s an existence check for
the key.

I phrased that poorly. What I intended was that it is designed to work as an undefined check, and should be treated as a type guard with a generic type, so that after its use the compiler treats the value as definitively not undefined.

if(mapOne.has('foo') {
    let bar = mapOne.get('foo')
    ...
}

Should not have bar throwing "possibly undefined" issues, since we should know for certain it is not undefined.

[avin-kavish]

The value stored at the key could be undefined. mapOne.set('foo', undefined) -> mapOne.has('foo') // true

import invariant from 'ts-invariant'

if(mapOne.has('foo')) {
    let bar = mapOne.get('foo')
    invariant(bar) // no-op or remove in prod build
}

let bar = mapOne.get('foo')
if (bar) {
  ...
}

What's wrong with either of these approaches?

[GabenGar]

mapOne.set('foo', undefined) -> mapOne.has('foo') // true

That's a pretty terrible example because Map has generics for key and value types.
So something like this won't even pass the compilation:

const mapExample = new Map<"foo"| "bar", number>()
mapExample.set("foo", 1)
mapExample.set("bar", undefined)

But typescript is still unsure of the value:

if (mapExample.has("foo")) {
  const val = mapExample.get("foo")
} else {
  const val = mapExample.get("foo")
}

In both branches it is number | undefined.

[avin-kavish]

Why bother with compile-time data assumptions when you can... wait for it.... use a runtime type guard with the same LOC.

[GabenGar]

By the same logic you have to use type guards for any collection property access because you never know if one returns undefined at runtime. And also validate that the runtime typeguard function wasn't tampered with before each call.

[paulius-valiunas]

if I may add, by the very same logic you might as well write type guards for every variable no matter the type, because you never know if someone decided to shoehorn an undefined into your string variable.

Typescript is 100% compile-time. If you don't trust compile-time type checks, why use Typescript at all?

[avin-kavish]

No, the thing comes into play here because a check for the truthiness of the value is necessary. I'm saying might as well go with the existing type guards at that point as opposed to trying to modify the language in a way that I'm not sure even is possible.

if (mapExample.has("foo")) {
  const val = mapExample.get("foo")

  mapExample.delete("foo")

  mapExample.get("foo") // whats the type here?
}

So you are saying the compiler has to add special case to detect that these two method calls add and delete values from the values that can be returned from the get call? TypeScript has no sense of internal state when it comes to types.

If you have reason to believe string variables can be polluted due to I/O, yes by all do a runtime check on them.

Regarding de-reffing array variables, yes, you have to turn on the noUncheckedIndexAccess flag to get that feature, once you do - you have to check for the existence of the value when you do indexed access.

[jonlepage]

i find alternative hack for my usecase but your eyes will bleeding
tsplayground

[somebody1234]

@djmisterjon that only works if your keys are distinct types, if your key is in a string variable it's not possible to do this

[GabenGar]

So you are saying the compiler has to add special case to detect that these two method calls add and delete values from the values that can be returned from the get call? TypeScript has no sense of internal state when it comes to types.

Yet it will infer the type of variables declared with let (for all branches) without explicit casts just fine. So it absolutely tracks the state of values in order to function. Do note I am talking about Maps with generics set (derived from as const objects and arrays for example), not the freeform dictionary use, so there is no ambiguity for values. The call mapExample.delete("foo") should be a compile-time error, because typescript knows the result of Map.delete(key) makes the value effectively undefined, which is not of type number.
But so far not even ReadonlyMap can derive its type from an as const object. And as dictionaries they are already pretty annoying to use due to not being serializable, typescript making to write extra boilerplate code for convenience methods doesn't help it either. Instead it nudges to write in the old way of using objects as dictionaries, which has all problems of key access as Map plus extra more but without compile-time errors.

Regarding de-reffing array variables, yes, you have to turn on the noUncheckedIndexAccess flag to get that feature, once you do - you have to check for the existence of the value when you do indexed access.

I wasn't talking only about arrays, objects are collections too. You have to validate the value of every single dot/index notation access (do note the key might be a getter, so you have to call with Object.getOwnPropertyDescriptor() instead). And in case of global objects, which are frequent targets for implicit changes, you have to write a wasm module to validate those and run it before each call (without implicit changes ofc, as the symbol will be invalid on the second validator call).

[paulius-valiunas]

No, the thing comes into play here because a check for the truthiness of the value is necessary.

What? What does truthiness have to do with any of this? As for your snippet:

if (mapExample.has("foo")) { // should narrow the type
  const val = mapExample.get("foo"); // the type here is NOT undefined

  mapExample.delete("foo"); // should narrow the type again

  mapExample.get("foo"); // the type here is undefined
}

TypeScript has no sense of internal state when it comes to types.

I beg to differ:

interface Cat {
  name: string;
  breed?: string;
}

const cat = {} as Cat;

doStuff(cat.breed); // why is there an error here
if(cat.breed !== undefined) {
  doStuff(cat.breed); // but not here?
}

function doStuff(breed: string): void {}

I don't see how a map should be different from an object. Internally, they're both hash maps, they just have different APIs for accessing their members (and Map also maintains an ordered index of entries, but that's irrelevant). In both cases someone can technically use type casting or simply vanilla Javascript to inject values that Typescript does not expect. Nevertheless, that doesn't stop Typescript in my above example from correctly assuming that the internal state of object/hashmap cat will have an entry with the key breed defined.

[alecgibson]

Yet another workaround helper, which leverages type predicates:

function has<K, T extends K>(set: ReadonlySet<T> | ReadonlyMap<T, any>, key: K): key is T {
  return set.has(key as T);
}

Some tests:

describe('a const Set', () => {
  const set = new Set(['foo', 'bar'] as const);

  it('checks a key outside the set', () => {
    expect(has(set, 'lorem')).to.be.false;
  });

  it('checks a key inside the set', () => {
    expect(has(set, 'foo')).to.be.true;
  });

  it('narrows a superset type', () => {
    function test(key: 'foo' | 'bar' | 'baz'): void {
      if (has(set, key)) {
        // @ts-expect-error :: never true
        if (key === 'baz') return;
      } else {
        // @ts-expect-error :: never true
        if (key === 'foo') return;
      }
    }

    test('baz');
  });

  it('has type errors if the types do not overlap', () => {
    // @ts-expect-error :: key can never be valid
    expect(has(set, 123)).to.be.false;
  });
});

Would love for this to be built-in.