Warn if StrictObject could be used

[lukasrad02]

PR Checklist

Please make sure to fulfil the following conditions before marking this PR ready for review:

• If this PR adds or changes features or fixes bugs, this has been added to the changelog
• If this PR adds new actions or other ways to alter the state, test scenarios have been added.
• I have the right to license the code submitted with this Pull Request under the mentioned license in the file LICENSE-README.md (i.e., this is my
  own code or code licensed under a license compatible to AGPL v3.0 or later, for exceptions look into LICENSE-README.md) and
  hereby license the code in this Pull Request under it.
  I certify that by signing off my commits (see In case of using third party code, I have given appropriate credit.
  We are using DCO for that, see here for more information.
• If I have used third party code and I mentioned it in the code, I also updated the inspired-by-or-copied-from-list.html list to include the links.

[ClFeSc]

I like the idea! However, you might need to allow non-string keys (i.e. number | string | symbol) in StrictObject to allow usage of its method in more places. Currently, you only allow string which seems to be lost, however, at some point, leading to errors like keyof T cannot be assigned to string, although we have the constraint T extends { [key: string]: any } (e.g. in shared/src/utils/sort-object.ts).

[fekoch]

After looking at StrictObject, I am unsure if it is desirable to use StrictObject.* as much as possible. It seems to me, as if it only serves as a (hidden) as directive?

For example:

import {StrictObject} from "./strict-object";

interface Foo {
    a: number;
}

interface Bar extends Foo {
    b: number;
}

const c: Bar = {
    a: 1,
    b: 2,
};

function doSomthingToFoo(bar: Bar): Foo {
    return bar;
}

const keys = StrictObject.keys(doSomthingToFoo(c)) satisfies (keyof Foo)[]; // <- incorrect type

console.log(keys) // [ 'a', 'b' ]

This compiles without issues but results in unexpected behaviour.

[ClFeSc]

Yes. As described in the doc comment of the StrictObject namespace, TypeScript's type system is not capable of and not designed to ensure only the typed properties are present. If no-one uses as any or similar to abuse, it will guarantee that a property that the typings say should be there is in fact there, the other direction, however, cannot hold as TS only checks whether the expected interface is present. StrictObject is a helper introduced to simplify some code where we know exactly which properties are present and want to transfer this knowledge into the type system.

[ClFeSc]

An idea might be to introduce also a WeakObject or whatever that is the same as Object but makes you think about what you actually need as currently I could imagine some usages of Object being happily compatible with StrictObject but are not using it because it was either copied or the programmer simply did not think about it.

[lukasrad02]

I am unsure if it is desirable to use StrictObject as much as possible

At least this is the current recommendation of the styleguide: https://github.com/hpi-sam/digital-fuesim-manv/blob/f77dae94c7711fc3908de66bebf7df83d3b32660/README.md?plain=1#L154

My initial intention with this PR was not to introduce new rules but only enforce the existing styleguide with a linter rule, since I noticed many cases where Object was used, although StrictObject could also be used. As Clemens said:

I could imagine some usages of Object being happily compatible with StrictObject but are not using it because it was either copied or the programmer simply did not think about it

I would guess that in most cases, it makes sense to use StrictObject, since we mostly use keys()/values()/entries() to iterate through UUIDMaps, ResourceDescriptions, or similarly structured objects where we don't have any inheritance or other cases of re-typing.

If StrictObject is actually "wrong" (i.e., there are possible keys/values that don't match the type definition), one can still disable the rule for that line and hopefully use that as a reminder to (a) re-think the type definitions for that piece of code (why is StrictObject providing wrong typings`) or (b) provide an explanation of the object's structure in a comment.

I think that overall, the benefit of stricter typings outweighs the potential confusion due to type definitions that are to narrow. Without StrictObject, people might just use assertions or type casts in their code since they "know" that the type is right, which can hide type errors if the input object changes its structure during a refactoring.

[lukasrad02]

However, you might need to allow non-string keys (i.e. number | string | symbol) in StrictObject to allow usage of its method in more places. Currently, you only allow string which seems to be lost, however, at some point, leading to errors like keyof T cannot be assigned to string, although we have the constraint T extends { [key: string]: any } (e.g. in shared/src/utils/sort-object.ts).

I'll look into this

[fekoch]

StrictObject is a helper introduced to simplify some code where we know exactly which properties are present and want to transfer this knowledge into the type system

Do you have an example where this is actually necessary? The examples I tested all infer the types without issues while just using Object (or in some cases, there are even cleaner ways to do it, without even needing Object. at all)

[lukasrad02]

StrictObject is a helper introduced to simplify some code where we know exactly which properties are present and want to transfer this knowledge into the type system

Do you have an example where this is actually necessary? The examples I tested all infer the types without issues while just using Object

Object.values() typically works well, but Object.keys() (or .entries()) suffers from the fact that TypeScript by default does not assume that no other keys than specified by the type definition are present (see Clemens' comment). So searching for StrictObject.keys should reveal some examples where we actually benefit from the more strict typings.

The first example I found would be https://github.com/hpi-sam/digital-fuesim-manv/blob/f77dae94c7711fc3908de66bebf7df83d3b32660/shared/src/simulation/activities/count-patients.ts#L65-L69

If we were using Object.keys here, patientStatus would be typed as string and thus, indexing patientCount with patientStatus would not work (Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '[…]'.).

In general, StrictObject is helpful as soon as we deal with literal union types.

or in some cases, there are even cleaner ways to do it, without even needing Object. at all

Obviously, there could be a pre-defined array of all possible PatientStatus values here so that we don't have to collect the keys from patientStatusAllowedValues. But with TypeScript's type system, we can't ensure the array contains all possible values, while we can ensure that an object has all possible values as keys.

[lukasrad02]

(Summary of or in-person discussion from today)

• StrictObject is misleading, as it can result in wrong type definitions (see Felix' comment). We want to rename it so that it reflect its actual behavior (something like "AssertPropertiesObject", a good name is still to be found)
• We don't want to add the linter rules proposed in this PR. Instead, the recommendation to always use StrictObject if possible should be dropped
• We want to reduce the overall number of (Strict)Object.{keys, values, entries}()-calls in favor of approaches that use less meta-programming. Removing some hardcoded types of domain objects (e.g. Generalize materials and personnel #1124) is already in progress, an example refactoring for the code discussed in this comment is shown below. It is yet to be discussed whether this should only be a guideline for future PRs or whether existing code should also be refactored (maybe even with a linter warning for any use of (Strict)Object.{keys, values, entries}()), so that legacy code does not serve as a bad example for newly implemented features

---

As a refactoring of https://github.com/hpi-sam/digital-fuesim-manv/blob/f77dae94c7711fc3908de66bebf7df83d3b32660/shared/src/simulation/activities/count-patients.ts#L65-L69, one could (as suggested by Felix)

1. Zero-initialize patientCount (TypeScript will catch missing status colors, so this is type-safe)
2. Loop over all patients and increment the respective entry patientCount

This approach neither requires Object.keys() metaprogramming, nor is the final as ResourceDescription<PatientStatus> cast required