Change HashMap to export mapped types

[gustavo-shigueo]

Goal

Allow enums to be used as HashMap keys
Closes #338

Changes

Change HashMap to export { [key in K]?: V } instead of { [key: K]: V }

Checklist

• I have followed the steps listed in the Contributing guide.
• If necessary, I have added documentation related to the changes made.
• I have added or updated the tests related to the changes made.

[NyxCode]

Interesting! How sure are we that

• this is fully compatible with what we had before
• there are no edge-cases with this?

There definitely are some semantic differences. Here are a couple I have found:

type A = { [key: string]: number };
let a: A = { "x": 42 };
let x: number = a["x"]; // ok

type B = { [key in string]?: number };
let b: B = { "y": 42 };
let y: number = b["y"]; // error

type A = { [key: string]: number };
let a: A = { "x": undefined }; // error

type B = { [key in string]?: number };
let b: B = { "y": undefined }; // ok

For just string keys, the first one (what we had before) gives a number for every string key. That might not be what one usually wants, though, and getting back a string | undefined does seem more sensible.

That does mean that this change is breaking, correct? Do we want to treat it as such, or is this more of a bug fix?

[gustavo-shigueo]

There definitely are some semantic differences. Here are a couple I have found:

type A = { [key: string]: number };
let a: A = { "x": 42 };
let x: number = a["x"]; // ok

type B = { [key in string]?: number };
let b: B = { "y": 42 };
let y: number = b["y"]; // error

I think this behavior is better because a HashMap tells you what type the value is, if the key exists, so this kinda translates HashMap::get to TS

type A = { [key: string]: number };
let a: A = { "x": undefined }; // error

type B = { [key in string]?: number };
let b: B = { "y": undefined }; // ok

This is annoying, and I don't know if there's a way to implement the feature without this behavior.
It is one of those TS quirks that can only be resolved by having the tsconfig flag for exactOptionalPropertyTypes
It should be harmless as far as (de)serialization is concerned though.

That does mean that this change is breaking, correct? Do we want to treat it as such, or is this more of a bug fix?

Yeah, this does look like a breaking change, it'll make TypeScript demand a whole lot of null checks when dealing with HashMap, but honestly, that's probably a good thing

[SpecialMike]

This breaks iterating over object values:

//before
type A = {[key: string]: number};
let a: A = {"a": 1, "b": 2, "c": 3};
//el is a `number` below
let sum = Object.values(a).reduce((acc, el) => acc + el, 0);

//after
type A = {[key: string]?: number};
let a: A = {"a": 1, "b": 2, "c": 3};
//el is a `number|undefined` below
let sum = Object.values(a).reduce((acc, el) => acc + el, 0);

This does make sense that record types will return undefined on keys that aren't in them, but I think it's better handled by setting noUncheckedIndexedAccess in tsconfig, which doesn't cause this problem.

[NyxCode]

Interesting!
We do still need the new representation for #349, but we could revert it back for other cases.

I do still think that the new representation is better overall, though I'd love to be convinced otherwise.
If you have a strong opinion on this, I'd appreciate if you could lay out the tradeoffs of the two representations for your usecase.

For one, I think we should chose the most sensible bindings with the default tsc config. I'd also err on the side of having false-positive tsc errors instead of false-negatives.

[gustavo-shigueo]

You can give el a default value to make TS understand it's not undefined, also, if you're getting acc as number | undefined (you didn't say anything about it, but just in case), use <number> on the reduce call

type A = { [key in string]?: number }
const a: A = { a: 1, b: 2, c: 3 }

Object.values(a).reduce<number>((acc, el = 0) => acc + el, 0)

[SpecialMike]

You can give el a default value to make TS understand it's not undefined, also, if you're getting acc as number | undefined (you didn't say anything about it, but just in case), use <number> on the reduce call

Yeah, this trivial case is somewhat easy to work around, but if T is a non-primitive type there is not always an obvious default.

Is it possible to detect that the key type is enumerable and only output the ? in { [key in string]?: number } in that case? All other use cases seem to work well with just { [key in string]: number } - the value type is not mapped to undefined. The only downside with this is that if the key type is enumerable, then the value types are going to have | undefined, whereas other value types will not. If that's a problem, then maps with enumerable key types could be output as anonymous objects with the fields enumerated:

enum Keys {
  A,
  B,
  C,
}

struct A<T> {
  foo: HashMap<Keys, T>
}

type A<T> = {
  foo: {
    "A"?: T,
    "B"?: T,
    "C"?: T,
  }
}

This is essentially doing the type mapping by hand. I think this TS bug is related, and if fixed setting exactOptionalPropertyTypes would allow the optional mapped type to work: microsoft/TypeScript#46969

[42triangles]

I honestly don't think that emitting {[key: string]?: number} is more correct than {[key: string]: number}, since there ARE ways to check if a given object has a given key, making {"foo": undefined} and {} semantically still different.

Since

let foo: {[key: string]: number} = {};
let bar: number = foo["bar"];

works, I believe this to be an issue much more with how TypeScript doesn't handle the possibility of missing keys (which is only one of many type safety issues in TypeScript), instead of an issue with the type that ts-rs emitted previously.

I understand the rationale (as much as I personally don't agree with trying to fix safety issues in TypeScripts type system in this crate), but I do believe that especially where Rust is expecting inputs from TypeScript (as serde_json would error out deserializing {"foo": undefined} into a HashMap<String, i32>), there should be a way to get {[key: string]: number} instead via an attribute, or reverting the change entirely could also make sense.
Though reverting it would also be another breaking change.

As is, it is impossible (without implementing the TS trait manually) to get the old behaviour (including with #[ts(type = "...")]) if it requires importing other types, unless there's a way of importing those types which I just haven't been able to find.

[NyxCode]

Thanks, this is valuable feedback.
One hope to fix this is #334. I hope I'll find some time soon to finish that.

Right now, the most ergonomic way to alter the representation of HashMap in user code would be:

#[derive(TS)]
#[ts(export)]
struct A {
    #[ts(as = "TsHashMap<_>")]
    a: HashMap<String, B>,
}

That works, even with imports, but is not great if a codebase is littered with HashMaps. That's where I hope #334 might help.

The `TsHashMap` helper

struct TsHashMap<M>(M);

impl<K: TS, V: TS, H> TS for TsHashMap<HashMap<K, V, H>> {
  type WithoutGenerics = TsHashMap<HashMap<ts_rs::Dummy, ts_rs::Dummy, H>>;

  fn ident() -> String { panic!() }
  fn name() -> String { format!("{{ [key: {}]: {} }}", K::name(), V::name()) }
  fn inline() -> String { format!("{{ [key: {}]: {} }}", K::inline(), V::inline()) }
  fn inline_flattened() -> String { panic!("{} cannot be flattened", Self::name()) }
  fn decl() -> String { panic!("{} cannot be declared", Self::name()) }
  fn decl_concrete() -> String { panic!("{} cannot be declared", Self::name()) }
  fn visit_dependencies(v: &mut impl ts_rs::TypeVisitor) where Self: 'static {
      <HashMap<K, V, H>>::visit_dependencies(v);
  }
  fn visit_generics(v: &mut impl ts_rs::TypeVisitor) where Self: 'static {
      <HashMap<K, V, H>>::visit_generics(v);
  }
}

[gustavo-shigueo]

The main reason to have the optional key is for cases where the key is an enum (aka a union of strings), which would make every entry mandatory in TS, causing a lot of problems. I also personally think this nicely reflects Rust's HashMap::get API, which returns an Option, implying that the keys are optional within the dictionary

since there ARE ways to check if a given object has a given key, making { "foo": undefined } and {} semantically still different.

You are correct in that they are different, so to prevent code that has { "foo": undefined } for a type { [key in string]?: T } I would suggest you enable a TS flag specifically made to pick up on that semantic difference, called exactOptionalPropertyTypes

serde_json would error out deserializing {"foo": undefined} into a HashMap<String, i32>

This error occurs because this is not valid JSON. JSON doesn't support undefined, which you will see if you try to JSON.stringify({ foo: undefined }), which will give you the string "{}". You can also try to JSON.parse('{ "foo": undefined }') and JS will throw an error due to invalid JSON