search

microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.08k stars 12.5k forks source link

[NewErrors] 4.8.0-dev.20220609 vs 4.7.3 #49461

Closed typescript-bot closed 2 years ago

typescript-bot commented 2 years ago

The following errors were reported by 4.8.0-dev.20220609, but not by 4.7.3 Pipeline that generated this bug File that generated the pipeline

kamranahmedse/developer-roadmap

tsconfig.json

RyanCavanaugh commented 2 years ago

kamranahmedse/developer-roadmap - Ryan coder/code-server - Daniel microsoft/playwright - Andrew vercel/hyper - Ryan react-hook-form/react-hook-form - Daniel typeorm/typeorm - Andrew mobxjs/mobx - Ryan palantir/blueprint - Daniel apollographql/apollo-client - Andrew

collate

kamranahmedse/developer-roadmap - Ryan mobxjs/mobx - Ryan vercel/hyper - Ryan react-hook-form/react-hook-form - Daniel coder/code-server - Daniel palantir/blueprint - Daniel microsoft/playwright - Andrew typeorm/typeorm - Andrew apollographql/apollo-client - Andrew

RyanCavanaugh commented 2 years ago

kamranahmedse/developer-roadmap

const descendants = useRef(new DescendantsManager<T, K>())

Here, K is defaulted to { } but unconstrained. DescendantsManager's second type argument is constrained: K extends Record<string, any> = {}. There are two instances in this file. Adding extends Record<string, any> to both instances fixes the errors with no follow-on errors.

ahejlsberg commented 2 years ago

Note that I would expect a significant number of these to result from the fact that an unconstrained type parameter is no longer assignable to {} or Record<string, any> in --strictNullChecks mode (introduced in #49119, which re-incorporated the change from #48366). Those are entirely appropriate new errors since an unconstrained type parameter could be null or undefined.

RyanCavanaugh commented 2 years ago

mobxjs/mobx

This method called object has an unconstrained type parameter defaulted to any:

    object<T extends object = any>(
        props: T,
        decorators?: AnnotationsMap<T, never>,
        options?: CreateObservableOptions
    ): T {
        return extendObservable(
            globalState.useProxies === false || options?.proxy === false
                ? asObservableObject({}, options)
                : asDynamicObservableObject({}, options),
            props,
            decorators
        )
    },

it calls extendsObservable which has a constraint of Object (yes, Object) on both of its type parameters:

export function extendObservable<A extends Object, B extends Object>(

Adding extends object to T fixes the error with no follow-on errors.

RyanCavanaugh commented 2 years ago

vercel/hyper

All these breaks were in external .d.ts files.

conf had

export declare type Migrations<T> = Record<string, (store: Conf<T>) => void>;

where Conf<T> was constrained

declare class Conf<T extends Record<string, any> = Record<string, unknown>>

This was fixable by upstreaming the constraint

type-fest had two instances

Key extends keyof WithStringKeys<BaseType>
 WithStringKeys<BaseType>[Key]

This required two layers of upstreaming the constraint, which in turn broke another package (electron-store) because we surfaced the constraint on Except's first type argument:

type Options<T> = Except<ConfOptions<T>, ...
sandersn commented 2 years ago

I believe conf and type-fest have fixes in their newest versions. Edit: I double-checked, and they do. Edit 2: electron-store also has a fix, but it hasn't shipped yet. I pinged Sindre about it. Edit 3: electron-store shipped its fix about 10 hours ago.

andrewbranch commented 2 years ago

playwright

Ostensibly a missing constraint on the type parameter of a subclass whose superclass required a constraint. However, the type of said type parameter is never meaningfully witnessed in the subclass. Its only usage is like this:

class DummyChannelOwner<T> extends ChannelOwner<T> {} // unconstrained `T` not assignable to `{}`
// ...
let result: ChannelOwner<any> = new ChannelOwner(/* only inference source typed `any` here */)

and then result is returned in a method whose return type is any, and the one caller of that method does not use the return value. In other words, this error did not catch a bug in practice, because the offending type parameter should never have been declared in the first place.

typeorm

Every issue was resolvable by adding a missing constraint. The appropriate constraint, for internal consistency, was an interface called ObjectLiteral defined as just a string-to-any index signature. It seems like extends object or even extends {} would have squelched the errors due to the very loose assignability of that index signature, but it seemed like the correct thing was to propagate ObjectLiteral through, so the codefix wasn’t usable where offered. I also noticed that there were places where the codefix wasn’t offered due to the failing assignment being of the array form T[] is not assignable to ObjectLiteral[]. I also noticed one instance where the codefix would have been good, and the related diagnostic “...probably needs an extends object constraint” was attached, but there was no corresponding codefix.

Did this catch a real bug: I think so? It’s really hard to tell. The unconstrained Entity type parameters are treated like opaque values throughout most of the codebase. I finally traced it down to where it does some object operations on those values, and some of those functions actually guard against null and undefined even though the existing types refute that possibility. Others guard only against undefined, and others do not check for either. So, I’m pretty sure in 4.7 it would have been possible to invoke something with null and/or undefined and find something that crashes like 20 stack frames later.

apollo-client

First one I looked at showed the codefix / related diagnostic as buggy again; duplicated related info and no codefix:

image

Second one, related diagnostic is correct but again no codefix. Starting to wonder if the codefix ever works?

A couple more and it appears that this codebase has probably mistaken type parameter defaults for constraints through and through. Virtually every type parameter has a default and no constraint. Many of the defaults are any. I tried to see if I could trace one down to where it would crash, but the values of these unconstrained type parameters almost always get assigned to a property that was optional, so down the line, the code tends to guard against falsy values. It seems 100% clear that an object constraint is the intent, but I can’t easily find a place where a null or undefined would not be gracefully handled. (This is basically what I expected to find in all of these: yes, the types are clearly wrong; yes, because the types are wrong, it would be easy to make a code change that type checks but crashes at runtime; no, I cannot find existing instances of code like that.)

DanielRosenwasser commented 2 years ago

react-hook-form

I sent the fixes upstream over at https://github.com/react-hook-form/react-hook-form/pull/8484.

code-server

This repo feels kind of like a less-than-useful benchmark because it's just letting us report over an old copy of VS Code; but I guess it gave us a chance to rerun on VS Code from before they dealt with this break over two months ago (https://github.com/microsoft/vscode/commit/46abb2bd3c200113a1ed08c72f1b0bbdcf3471cc). In theory this double type assertion should no longer be necessary @mjbvz.

Otherwise, @mjbvz already upgraded to a recent nightly and fixed other breaks due to narrowing, this one involving how functions are no longer narrowed (https://github.com/microsoft/vscode/commit/640898db17d39b80723a0bf77fc9574dab67420f).

Since there was no work for me to do, I will have to defer to the VS Code team's feedback.

blueprint

I spent a bit of time last week trying to figure this one out with @ahejlsberg. It seems like we have regressed in some capacity with respect to destructuring defaults. @andrewbranch might know more about this since he played around with destructuring recently, but I don't recall the exact change or whether it's even relevant.

export interface BarProps {
    barProp?: string;
}

export interface FooProps {
    fooProps?: BarProps & object;
}

declare const foo: FooProps;
const { fooProps = {} } = foo;

fooProps.barProp;
//       ~~~~~~~
// error! Property 'barProp' does not exist on type '{}'.ts(2339)

It seems like every single break here comes from this. There's not a clear fix, and it seems like a regression, so I didn't try fixing it (though I spent a lot of time getting a minimal repro).

DanielRosenwasser commented 2 years ago

I opened up https://github.com/microsoft/TypeScript/issues/49480 for the {} default issue, which was introduced by the intersection reduction PR at https://github.com/microsoft/TypeScript/pull/49119 @ahejlsberg.

DanielRosenwasser commented 2 years ago

Have others sent PRs to these projects? I sent out

RyanCavanaugh commented 2 years ago

type-fest seems to have been updated already

I sent a PR to the other project

andrewbranch commented 2 years ago