#49625 breaks node type expectations

Piotr Błażejewicz (Peter Blazejewicz) (@peterblazejewicz)

#49625 breaks node's type tests on DT.
Note that this causes node's util to return a union where it didn't before:

function isMap<T>(object: T | {}): object is T extends ReadonlyMap<any, any> ? (unknown extends T ? never : ReadonlyMap<any, any>) : Map<unknown, unknown>;
declare const romor: ReadonlyMap<any, any> | Record<any, any>
if (types.isMap(romor)) {
    romor; // $ExpectType ReadonlyMap<any, any>
}

But it's ReadonlyMap<any, any> | Map<unknown, unknown> now.

I think this is correct, because the conditional type distributes and ReadonlyMap extends ReadonlyMap, while Record<any, any> doesn't. So the result is the union of the conditional's branches. However, this might turn out to be too big a breaking change for node, so I want to track it here.

Piotr Błażejewicz (Peter Blazejewicz) (@peterblazejewicz) also noticed discussion at DefinitelyTyped/DefinitelyTyped#61353 on this change.

Hm, seems frustrating. #49625 came in after the beta - did we not see changes in DT from it though?

Update node types.isMap tests

Starting Saturday morning

Also breaks is-object:

declare function isObject(value: unknown): value is Record<string | symbol | number, unknown>;
const obj = {};
if (isObject(obj)) obj['attr'];

I'm not sure why, but now obj: {} inside the if. Previously, it was Record<string | symbol | number, unknown>.

Maybe I'm missing something, but isn't {} equivalent to Record<string | symbol | number>?

e.g. Playground Link

(But, they don't seem to be identical enough to reduce in unions...)

The visible difference in is-object's tests is that obj['attr'] is legal for Record, illegal for {}.

underscore's _.isFunction is also broken in a way I don't understand:

    declare var _: UnderscoreStatic;
    interface UnderscoreStatic {
        /**
         * Returns true if `object` is a Function.
         * @param object The object to check.
         * @returns True if `object` is a Function, otherwise false.
         **/
        isFunction(object: any): object is Function;
    }
    const anyFunctionIteratee: _.Iteratee<any, string> = (element, key, collection) => {
        element; // $ExpectType any
        key; // $ExpectType any
        collection; // $ExpectType any
        return element.a;
    };
    type Iteratee<V, R, T extends TypeOfCollection<V, any> = TypeOfCollection<V>> =
        CollectionIterator<T, R, V> |
        string | number |
        (string | number)[] |
        Partial<T> |
        null |
        undefined;
    interface CollectionIterator<T extends TypeOfList<V> | TypeOfDictionary<V, any>, TResult, V = Collection<T>> {
        (element: T, key: CollectionKey<V>, collection: V): TResult;
    }

    if (_.isFunction(anyFunctionIteratee)) {
        anyFunctionIteratee(recordDictionary['a'], 'a', recordDictionary); // $ExpectType string
    }

After #49625,

anyFunctionIteratee: _.CollectionIterator<any, string, any> | Partial<any> outside the if,
(elt: any, key: any, collection: any) => any inside the if.
Before,
anyFunctionIteratee: _.CollectionIterator<any, string, any> | Partial<any> outside the if,
(elt: any, key: any, collection: any) => string inside the if. The only difference is that the function returns a string in the older commit.

I don't understand what's happening here because CollectionIterator<any, string, any> explicitly has TResult=string, which is the return type. Do we have special handling for Function that copies the parameters but not the return type when narrowing?

One more from weak-napi:

declare class WeakRef<T> {}
declare function weak<T extends object>(object: T, callback?: () => void): WeakRef<T>;
declare namespace weak {
    /**
     * Checks to see if ref is a dead reference. Returns true if the original Object has already been GC'd, false otherwise
     * @param ref weak reference object
     */
    function isDead(ref: WeakRef<any>): ref is WeakRef<undefined>;
}

const weakReference = weak({ a: 123 }, () => { });
if (weak.isDead(weakReference)) {
    const a = weakReference; // $ExpectType WeakRef<undefined>
    const value = weak.get(weakReference); // undefined only possible
}

after, outside the if weakReference: WeakRef<{a : number }>, and inside weakReference: WeakRef<{ a: number }>
before, outside the if weakReference: WeakRef<{a : number }>, and inside weakReference: WeakRef<undefined> as expected.

Regarding the isMap change, the new behavior is definitely correct. In fact, I struggle to explain our previous behavior when type predicate functions with union type results were applied to union type arguments. The fx1 example in the description of #49625 shows the weirdness, and the isMap example above is but another one (through shrouded by the strange conditional type that doesn't actually affect narrowing).

The isObject break is unfortunate and I'll think about how to address it. The issue is that {} and Record<string, any> are both subtypes of each other. With #49625 we consistently prefer leaving types alone when they're subtypes of the narrowing target type. Before #49625 we'd leave them alone in union types, but narrow them as singletons. For example, isObject used to narrow {} to Record<string, any>, but narrow {} | undefined to {}. It now always narrows to {}, but should probably consistently narrow to Record<string, any> as that is a more specific type.

The isFunction example it subtle, but the new behavior is correct. The union type being narrowed includes a Partial<any> which could be a function. We therefore include Function in the narrowed type as part of the union result. Then, because invoking a Function returns an any, the result becomes an any.

In the weak-napi example, the WeakRef<T> target type doesn't witness T, so all instantiations are subtypes of each other. Therefore, we leave the types alone (where previously we'd sometimes change them, based on singleton vs. union inputs.

Update node types.isMap tests (#61389)

Actions from the design meeting:

Try to fix node isMap/isSet
Update underscore's tests to include new type.
Improve handling of {} exposed in is-object
Update weak-napis' tests to include new type.

Let's continue to use this bug to track the improved handling of {}.

Navigation Menu

Repository navigation

#49625 breaks node type expectations #49988

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

#49625 breaks node type expectations #49988

Description

Activity

DanielRosenwasser commented on Jul 21, 2022

sandersn commented on Jul 21, 2022

sandersn commented on Jul 21, 2022

jakebailey commented on Jul 21, 2022

sandersn commented on Jul 21, 2022

sandersn commented on Jul 21, 2022

sandersn commented on Jul 21, 2022

ahejlsberg commented on Jul 22, 2022

sandersn commented on Jul 22, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions