isObject() asserts untrue types

[tsmaeder]

Bug Description:

in core/common/types.ts, we have the following code:

export function isObject<T = Record<string | number | symbol, unknown>>(value: unknown): value is T {
    // eslint-disable-next-line no-null/no-null
    return typeof value === 'object' && value !== null;
}

Let's say we have an interface like so:

interface MyInterface {
   title: string;
}

Now if we do this:

function foo(someObject: unknown) {
   if (isObject<MyInterface>.is(someObject) {
      console.log(someObject.title.toString());
   }
}

foo({});

Note that we don't get a compile problem, since inside the if, someObject is typed as a MyInterface, but we get a runtime error, since the VM frowns upon calling toString() on undefined.

[tsmaeder]

@msujew since you brought in this change in #11831, I'd be interested in your thinking.

[tsmaeder]

This issue is following up on #12250

[msujew]

It's imo obvious that the function cannot assert that the input type is actually of type used for the generic parameter. The parameter is there to keep the code more concise and help devs with code completion compared to just object. There might be a comment missing about this though.

[tsmaeder]

Hmh...that's sacrificing type safety for convenience. Not something I'm in favor of, in general. Could we assert Partial<T> or something like that? That way, we might get the help from the tooling without asserting non-thruths.

[msujew]

Yeah, returning Partial<T> probably does make sense.

[paul-marechal]

Partial<T> is not really better as it's still possible to get in a situation such as:

interface MyType {
    foo: string
}

const unknownValue: unknown = { foo: 2 };

if (isObject<MyType>(unknownValue)) {
    // We made it to this branch, and `unknownValue` now looks like:
    // `{ foo?: string }` but `unknownValue.foo` is currently a number.
}

I opened a PR with the most sane type we might be able to use instead.