Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to narrow union to Record in TS 4.8 #50671

Closed
AlCalzone opened this issue Sep 7, 2022 · 19 comments
Closed

Unable to narrow union to Record in TS 4.8 #50671

AlCalzone opened this issue Sep 7, 2022 · 19 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@AlCalzone
Copy link
Contributor

AlCalzone commented Sep 7, 2022

Bug Report

πŸ”Ž Search Terms

union record narrow type guard

πŸ•— Version & Regression Information

  • This changed between versions 4.7.4 and 4.8.2

⏯ Playground Link

Playground Link

πŸ’» Code

function isObject<T>(it: T): it is T & Record<string, unknown> {
    return Object.prototype.toString.call(it) === "[object Object]";
}

declare function test(arg: Record<string, any>): void

declare const arg: (string | number | { a: "b" });

if (isObject(arg)) {
    // arg is:
    // - TS 4.7: {a: "b"}
    // - TS 4.8: (string | number | { a: "b" }) & Record<string, unknown>
    test({ ...arg }) // ERROR: Spread types may only be created from object types.(2698)
}

πŸ™ Actual behavior

arg stays as some intersection type, which TS does not recognize as an object type.

πŸ™‚ Expected behavior

arg gets narrowed to {a: "b"}

@AlCalzone
Copy link
Contributor Author

Bonus attempt using conditional types, not much better though:

type ExtractObject<T> =
	T extends readonly unknown[] ? never
	: {} extends T ? Record<string | number | symbol, unknown>
	: T extends Record<string | number | symbol, unknown> ? T
	: never;

function isObject<T>(it: T): it is T & ExtractObject<T> {
	return Object.prototype.toString.call(it) === "[object Object]";
}

declare function test(arg: Record<string, any>): void

declare const arg: (string | number | { a: "b" });

if (isObject(arg)) {
    // arg is:
    // - TS 4.7: {a: "b"}
    // - TS 4.8: (string | number | { a: "b" }) & { a: "b" }
    test({ ...arg }) 
}

Playground Link

@AlCalzone
Copy link
Contributor Author

Note that narrowing arrays broke in a similar fashion, but here I was able to work around it with a conditional type.

Works in 4.8:

type ExtractArray<T> =
	T extends readonly unknown[] ? T
	: {} extends T ? (T & unknown[])
	: never;

export function isArray<T>(it: T): it is T & ExtractArray<T> {
	if (Array.isArray != null) return Array.isArray(it);
	return Object.prototype.toString.call(it) === "[object Array]";
}

Used to work until 4.7:

export function isArray<T>(it: T): it is T & unknown[] {
	if (Array.isArray != null) return Array.isArray(it);
	return Object.prototype.toString.call(it) === "[object Array]";
}

@whzx5byb
Copy link

whzx5byb commented Sep 7, 2022

function isObject<T>(it: T): it is T & object & Record<string, unknown>

or just

function isObject<T>(it: T): it is T & object

Does this work for you?

@AlCalzone
Copy link
Contributor Author

AlCalzone commented Sep 7, 2022

Edit: ok, object put me on the right track. Together with this explanation, I managed to whip up this god-awful conditional type that does the same as `T & Record<...> in TS 4.7:

function isObject<T>(it: T): it is {} extends T
	? // Narrow the `{}` type to an unspecified object
	  T & Record<string | number | symbol, unknown>
	: unknown extends T
	? // treat unknown like `{}`
	  T & Record<string | number | symbol, unknown>
	: T extends object // function, array or actual object
	? T extends readonly unknown[]
		? never // not an array
		: T extends (...args: any[]) => any
		? never // not a function
		: T // no, an actual object
	: never {

	// This is necessary because:
	// typeof null === 'object'
	// typeof [] === 'object'
	// [] instanceof Object === true
	return Object.prototype.toString.call(it) === "[object Object]";
}

Note that this MUST be inlined or TS complains about the type possibly not being related to T.

@lll000111

This comment was marked as off-topic.

@andrewbranch
Copy link
Member

@AlCalzone can you provide some test cases that show why @whzx5byb’s solution of T & object isn’t sufficient? It works fine for the only case in your repro.

@AlCalzone
Copy link
Contributor Author

AlCalzone commented Sep 9, 2022

@andrewbranch Sure thing! Note the type in the first if-branch, and IntelliSense isn't working there either.

Playground Link

function isObject<T>(it: T): it is T & object {
    return Object.prototype.toString.call(it) === "[object Object]";
}

declare function test(arg: Record<string, any>): void

declare const arg: (() => {}) | { a: "b" } | [] | number | undefined;

if (isObject(arg)) {
    // spreading works:
    test({ ...arg });
	arg.a; // error, shouldn't be
	// type of arg1 is ([] & object) | ({a: "b"} & object) | ((() => {}) & object)
}

// ↑ broken
//====================================================
// ↓ works

function isObjectFixed<T>(it: T): it is {} extends T
	? // Narrow the `{}` type to an unspecified object
	  T & Record<string | number | symbol, unknown>
	: unknown extends T
	? // treat unknown like `{}`
	  T & Record<string | number | symbol, unknown>
	: T extends object // function, array or actual object
	? T extends readonly unknown[]
		? never // not an array
		: T extends (...args: any[]) => any
		? never // not a function
		: T // no, an actual object
	: never {

	// This is necessary because:
	// typeof null === 'object'
	// typeof [] === 'object'
	// [] instanceof Object === true
	return Object.prototype.toString.call(it) === "[object Object]";
}

if (isObjectFixed(arg)) {
    // spreading works:
    test({ ...arg });
	arg.a;
    // this is now an error:
    arg.concat; // concat does not exist on type {a: "b"}
}

@AlCalzone
Copy link
Contributor Author

Sorry, had to edit this a few times, I messed up in between.

@andrewbranch
Copy link
Member

Ah. Yeah, since functions and arrays are assignable to our object, I’m inclined to say this is working as intended, and what you’re checking for is indeed more complex, hence the several conditions you need in the predicate. Perhaps object’s name is a bit unfortunate. What it really is is the non-primitive type. (In fact that’s what we name it in the checker.) But for what it is, not what it’s named, I think the behavior is expected.

@andrewbranch andrewbranch added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 9, 2022
@AlCalzone
Copy link
Contributor Author

Strangely T & Record<...> did work just fine in 4.7 and narrowed T to its Record-compatible subset.
Just like T & unknown[] narrowed T to its array subset and no longer does now.

Is it really supposed to be this complex to make sure one is dealing with an actual Record (or an actual array)?

@andrewbranch
Copy link
Member

What’s an β€œactual Record”? Lots of things that are not made out of object literals have named properties that can be accessed, which is what the {} and interface syntaxes mean in TypeScript. () => {}, [], and "hello" are all examples of a Record<"length", number>. Such is the nature of a structural type system.

@AlCalzone
Copy link
Contributor Author

AlCalzone commented Sep 9, 2022

I don't disagree in an academical sense, but I also didn't see anything in this regard mentioned in the release notes, only for unknown and the {} type. And the fact that this has worked like this for years felt like it might be an unintended side effect.

What’s an β€œactual Record”?

Anything for which typeof returns object, except null and arrays. There isn't really a type for this, but Record comes (or rather used to come) close. In practice, many things/types serve a specific and distinct purpose, despite them being compatible. In the most primitive sense, arrays are a numbered list of things; objects are a named (and strangely ordered) list of things and feel most natural when used in a way that's close in spirit to the proposed Record data structure - with some extra features like being able to put functions in there.

I guess what the functions above are supposed to do is distinguish those from other (structurally compatible) things without having to repeat the somewhat cumbersome check Object.prototype.toString.call(it) === "[object Object]". If TS had negative types, the desired type would probably be along the lines of

function isObject<T>(arg: unknown): arg is T & not nullish & not primitive & not array & not function;

(some shortcuts taken for brevity).

@andrewbranch
Copy link
Member

I’m honestly shocked that the definition of isObject worked before. It’s totally inconsistent. Even in 4.7:

type T1 = (string | number | { a: "b" }) & Record<string, unknown>; // (string | number | { a: "b" }) & Record<string, unknown>
declare let arg2: T1;
test({ ...arg2 }); // error

I just inlined the intersection you’re performing in the type predicate. If you expect to get { a: "b" } out of that intersection, it means you think string & Record<string, unknown> is never, which has never been true. I’m actually totally baffled that the type guard acted like it was.

distinguish those from other (structurally compatible) things

Philosophically, this is just going to be a heavy lift, if not impossible, in a structural type system. TypeScript reasons much more about what you can do with a value than what it is. What’s returned by a value’s toString() is very nearly outside the scope of the language.

@AlCalzone
Copy link
Contributor Author

If you expect to get { a: "b" } out of that intersection, it means you think string & Record<string, unknown> is never, which has never been true. I’m actually totally baffled that the type guard acted like it was.

Heh, I guess I just went along with what TS gave me when I wrote this function that way. That behavior was pretty stable since 3.3 (or earlier, dunno).

@fatcerberus
Copy link

@andrewbranch Technically I guess it’s not accurate to call object the non-primitive type either since I recently realized primitives can be assigned to { toString(): string } which in turn is assignable to object. I don’t really know what to call this odd duck of a type anymore, heh. Maybe just β€œ{} with extra steps” πŸ˜†

@andrewbranch
Copy link
Member

Even {} is assignable to object which is just transparently wrong. I prefer to think of the types as well-defined and the relationships as sloppy. Potato potato.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@alebrozzo
Copy link

Regardless of if this expected behaviour, it is a breaking change. Perhaps it should be rolled back and re introduced in a v5?

in our case we are comparing primitives and getting this same error:

type ValueType = (string | number | bigint | Date) | undefined | null;

const isEmpty = <T extends ValueType>(val: T) => {
    return val === undefined || val === null || val === '';
};

export const customComparer = <T extends ValueType>(a: T, b: T, locale: string) => {
    if (isEmpty(a) || isEmpty(b)) {
        return compareWithEmptyValues(a, b);
    }

    if (typeof a === 'string' && typeof b === 'string') {
        return a.localeCompare(b, locale, { sensitivity: 'base' });
    }

    return a < b ? -1 : a > b ? 1 : 0;
};

on v4.7 it worked fine but on v4.8.2 TS complains that a and b could be null on the last line.

@Apollon77
Copy link

@alebrozzo The TypeScript project here do not use semantic versioning, so you need to consider each minor version as most likely "breaking" and not just major ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

8 participants