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.

Sign up for GitHub

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

Jump to bottom

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

Closed
typescript-bot opened this issue Jun 9, 2022 · 12 comments
Closed

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

typescript-bot opened this issue Jun 9, 2022 · 12 comments
Labels
Discussion Issues which may not have code impact

Comments

@typescript-bot
Copy link
Collaborator

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

  • error TS2344: Type 'K' does not satisfy the constraint 'Record<string, any>'.
    • file:///mnt/ts_downloads/developer-roadmap/node_modules/@chakra-ui/descendant/src/use-descendant.ts#L11
    • file:///mnt/ts_downloads/developer-roadmap/node_modules/@chakra-ui/descendant/src/use-descendant.ts#L87
    • file:///mnt/ts_downloads/developer-roadmap/node_modules/@chakra-ui/descendant/src/use-descendant.ts#L91

coder/code-server

7 of 57 projects failed to build with the old tsc

src/tsconfig.monaco.json

src/tsconfig.tsec.json

microsoft/playwright

4 of 9 projects failed to build with the old tsc

packages/html-reporter/tsconfig.json

vercel/hyper

2 of 3 projects failed to build with the old tsc

tsconfig.json

  • error TS2344: Type 'BaseType' does not satisfy the constraint 'Record<string | number, any>'.
    • file:///mnt/ts_downloads/hyper/app/node_modules/type-fest/ts41/get.d.ts#L93 in app/tsconfig.json
    • file:///mnt/ts_downloads/hyper/app/node_modules/type-fest/ts41/get.d.ts#L94 in app/tsconfig.json
  • error TS2344: Type 'T' does not satisfy the constraint 'Record<string, any>'.
    • file:///mnt/ts_downloads/hyper/app/node_modules/conf/dist/source/types.d.ts#L201 in app/tsconfig.json

react-hook-form/react-hook-form

2 of 3 projects failed to build with the old tsc

tsconfig.json

typeorm/typeorm

tsconfig.json

mobxjs/mobx

7 of 9 projects failed to build with the old tsc

packages/mobx/tsconfig.json

palantir/blueprint

13 of 27 projects failed to build with the old tsc

packages/datetime/src/tsconfig.json

packages/select/src/tsconfig.json

apollographql/apollo-client

tsconfig.json
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jun 9, 2022 •
edited
Loading

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
Copy link
Member

RyanCavanaugh commented Jun 9, 2022 •
edited
Loading

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
Copy link
Member

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
Copy link
Member

RyanCavanaugh commented Jun 9, 2022 •
edited
Loading

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
Copy link
Member

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
Copy link
Member

sandersn commented Jun 9, 2022 •
edited
Loading

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
Copy link
Member

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 DanielRosenwasser mentioned this issue Jun 10, 2022
Merged
@DanielRosenwasser
Copy link
Member

react-hook-form

  • I don't think I got the quick fix at all.
  • Lots of places just needed an constraint.
    • Some of those constraints were clearly intentional and more specific than - for example,
      • In createFormControl, there's an unconstrained U which is used against an all-optional object type called SetValueConfig. U should have had a meaningful constraint that extended that type.
      • In form.ts, getProxyFormState.ts, getResolverOptions.ts, fieldArray.ts, resolvers.ts, and others a TFieldValues probably should always extends FieldValues, though it's just an alias for Record<string, any>. So there is readability intent, but this is the same as just adding object for most cases.
  • There's a few cases that does look questionable - like probable bugs - in getFieldAsValue.ts and validateField.ts - but it's not from the stronger check on generics, it's from the improved narrowing. At the end of the day, there's 100% no reason that these functions had to be generic though.
  • I don't think it was hard to fix these, but I don't think these would've been solvable on the publicly-produced .d.ts file

I sent the fixes upstream over at react-hook-form/react-hook-form#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 (microsoft/vscode@46abb2b). 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 (microsoft/vscode@640898d).

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 DanielRosenwasser mentioned this issue Jun 10, 2022
Closed
@DanielRosenwasser
Copy link
Member

I opened up #49480 for the {} default issue, which was introduced by the intersection reduction PR at #49119 @ahejlsberg.

@DanielRosenwasser DanielRosenwasser mentioned this issue Jun 13, 2022
Closed
@DanielRosenwasser DanielRosenwasser added the Discussion Issues which may not have code impact label Jun 13, 2022
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 13, 2022 •
edited
Loading

Have others sent PRs to these projects? I sent out

@RyanCavanaugh
Copy link
Member

type-fest seems to have been updated already

I sent a PR to the other project

@andrewbranch andrewbranch mentioned this issue Jun 13, 2022
Merged
andrewbranch added a commit to andrewbranch/typeorm that referenced this issue Jun 13, 2022
@andrewbranch
build: Fix build errors in TypeScript 4.8
cbbe3ff 
See microsoft/TypeScript#49461 (comment)
@andrewbranch andrewbranch mentioned this issue Jun 13, 2022
Merged
7 tasks
@andrewbranch
Copy link
Member

pleerock added a commit to typeorm/typeorm that referenced this issue Jun 28, 2022
@andrewbranch @pleerock
fix: support for TypeScript 4.8 (#9106)
d924b70 
* build: Fix build errors in TypeScript 4.8

See microsoft/TypeScript#49461 (comment)

* style: Format

* removed `.js` extension

* removed `.js` extension

* removed `.js` extension

* fixing compiler errors

* fixing compiler errors

Co-authored-by: Umed Khudoiberdiev <[email protected]>
@renovate renovate bot mentioned this issue Jul 4, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Discussion Issues which may not have code impact
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@sandersn @DanielRosenwasser @andrewbranch @ahejlsberg @RyanCavanaugh @typescript-bot