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

Change the default type parameter constraints and defaults to unknown from {} #30637

Merged
merged 7 commits into from
Apr 4, 2019

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Mar 28, 2019

We've oft talked about wanting to try this to see how "bad" the breaks this could cause may be. Well, here's me, seeing how bad the breaks are.

Incidentally, this exposes #30634, since swapping the type parameter constraint from {} to unknown makes unconstrained type parameters no longer assignable to every partial type. (Type parameters which explicitly extend {} still will be, though.) This means I already know this PR breaks react-redux.

Looking over the test baseline changes, most are procedural (just swapping from {} to unknown) and a few of the error changes from what I can understand simply fall out from inferring from an unknown constraint instead of a {} (and, IMO, for the most part act in a more expected way).

Take for example the tsx error deletion and change - they both stem from those tests not defining some JSX namespace elements. I now default those to unknown instead of emptyObjectType, which means attempts to pass properties to them no longer result in excess property errors. I think this is better (not that I think anyone should be using JSX without a well-defined JSX namespace), so I've kept it (however if there's disagreement here, this part is easy to undo separately from the type parameter default change).

@weswigham
Copy link
Member Author

It is time - @typescript-bot test this & @typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 28, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 5224a6a. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 28, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at 5224a6a. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham weswigham changed the title Change the default type parameter constraint and default to unknown from {} Change the default type parameter constraints and defaults to unknown from {} Mar 28, 2019
@weswigham weswigham changed the title Change the default type parameter constraints and defaults to unknown from {} Change the default type parameter constraints and defaults to unknown from {} Mar 28, 2019
@weswigham weswigham changed the title Change the default type parameter constraints and defaults to unknown from {} Change the default type parameter constraints and defaults to unknown from {} Mar 28, 2019
@weswigham
Copy link
Member Author

At first glance, I really like most of the RWC breaks. (Once beyond all the {} -> unknown surface-level message changes.) Many of the new errors are things that point out that "hey, unknown isn't assignable to Object or object or really anything...". These are places where the code was assuming it was using an object type, but the types didn't back it up, and are real bugs that we should be catching. Interestingly, this actually fixes an error in fp-ts, where HKT<unknown> really behaves meaningfully better on inference failure than HKT<{}>. In another project, instanceof Object on an unconstrained type parameter no longer incorrectly narrows to never in the false branch - that's great.

There's definitely a lot of baseline churn here, but this does seem to do good things.

@weswigham
Copy link
Member Author

weswigham commented Mar 29, 2019

The DT changes are super simple to summarize:

  1. {} -> unknown in some inference failure expected types. (It's usually easy enough to rewrite the tests to not rely on the exact printout of the type and still assert something useful, so those are easy to fix)
  2. Object is of type 'unknown'. - This error arises because we warn on accesses of unknown like it was null or undefined even outside of strictNullChecks (meaning you can't say unknownThing.toString() like you could for emptyObjectThing.toString() without an error). We could adjust this, or we could decide the added check is good. (This happens once in our tests and a bit in RWC, too)
  3. Erroneous code relying on weakly-typed objects to typecheck. Things like (answers: Answers) => void being assigned to (value: unknown) => void | PromiseLike<void> - the unknown used to be {}, and we'd check if {} was assignable to Answers, which was a weak type, and say "ah, sure". Now that we use unknown, we're like "this might not be an object... this won't fly". It's a good thing to find, IMO.
  4. Lodash. Lodash is... ungh. I think some of its overloads rely on inference failing to {} to order correctly. It's so strange. It just needs more investigation.
  5. react-redux and everything that depends on it is broken 😄 (per the issue mentioned in the OP) I should probably look into a way to write their Shared type that doesn't rely on an confluence of an unsound assignability relationship and an incorrect inference failure fallback to function.

And that seems to be it.

I don't think this is actually as breaking as we thought it could be.

@weswigham
Copy link
Member Author

@typescript-bot test this & @typescript-bot run dt - Between the changes to this PR and the small modifications I've made to DT, I expect this PR to be pretty clean now. We very well may be able to merge this on Monday and ship it out in a 3.5 nightly unflagged ❤️

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 29, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 66049c3. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 29, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at 66049c3. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member Author

RWC test results now consist of:

  1. Errors/declarations reporting unknown instead of {} for inference failure positons.
  2. A handful of new errors along the lines of
!!! error TS2345: Argument of type '() => Promise<unknown>' is not assignable to parameter of type '() => Promise<string | number | boolean | void | object>'.
!!! error TS2345:   Type 'Promise<unknown>' is not assignable to type 'Promise<string | number | boolean | void | object>'.
!!! error TS2345:     Type 'unknown' is not assignable to type 'string | number | boolean | void | object'.
!!! error TS2345:       Type 'unknown' is not assignable to type 'object'.

which is exactly the kind of thing we wanted to catch here - that {} we infered could have easily been null or undefined which shouldn't have been assignable. ❤️
3. A removal of one

!!! error TS2339: Property 'toString' does not exist on type 'never'.

which was issued in the false branch of a condition that was checking if an unconstrained type parameter was instanceof Object. The false branch now correctly no longer narrows to never, so this is also a nice improvement.
4. An addition of a couple

!!! error TS2339: Property 'hasOwnProperty' does not exist on type 'T'.

as an unconstrained type parameter cannot be treated as an object (as mentioned in the design meeting notes). Catching this was also one of our motivating examples. ❤️
5. Errors like

!!! error TS2339: Property 'finish' does not exist on type '{}'.

become

!!! error TS2571: Object is of type 'unknown'.
  • purely a cosmetic change; we simply have a different error/span for accesses on unknown than on "missing property" accesses.

...and that's it. All in all, all positive or neutral changes. Nice~ Now to wait for the DT results.

@weswigham
Copy link
Member Author

I'll need to run DT again; I missed updating an old version of react-redux and have opened a PR to fix it and clear out 3-4 of the last 4 DT failures on this PR.

In any case, I have super high confidence in this change now. I'm going to take this out of draft state. ❤️

@weswigham
Copy link
Member Author

@typescript-bot run dt should now just have two react-dnd failures (from them using the same bad circular constraint as react-redux). I already have a PR on react-dnd to fix it, so DT should be effectively clean now.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 30, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 66049c3. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member Author

Ping @ahejlsberg for review so we can merge this ❤️

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor suggested fix, otherwise looks good.

src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
@xaviergonz
Copy link

Ahh, I just wish there was some way to declare an actual empty object

@ahejlsberg
Copy link
Member

Ahh, I just wish there was some way to declare an actual empty object

Any reason object doesn't work?

@DanielRosenwasser
Copy link
Member

@ahejlsberg {} and object have the apparent type of Object IIRC. There's an issue from a while back asking for this so we can model Object.create(null) better.

@xaviergonz
Copy link

xaviergonz commented May 1, 2019

object doesn't model an empty object as well AFAIK

const a1: object = {} // (no-error) good
const b1: object = { y: 5 } // (no-error) weird, nobody says y should be there
const c1: object = [5] // (no-error) weird, I guess an array is an "object", but still...
const d1: object = 5 // (error) good, not an object

The issue I have really is that I use something like this to turn props that can take an undefined value into optional props:

type IsOptionalValue<C> = undefined extends C ? true: never

type DefinablePropsNames<T> = { [K in keyof T]: IsOptionalValue<T[K]> extends never ? K : never }[keyof T]

type MakeUndefinedPropsOptional<PC> = { [P in DefinablePropsNames<PC>]: PC[P] } & Partial<PC>

But it seems to break when the first object generated by MakeUndefinedPropsOptional ends up being an empty object because all properties can be turned into optionals

const a: MakeUndefinedPropsOptional<{}> = [] // is ok :-(
const b: MakeUndefinedPropsOptional<{ a: number | undefined }> = [] // is ok :-(
const c: MakeUndefinedPropsOptional<{ a: number }> = [] // it is not ok :D
const d: MakeUndefinedPropsOptional<{ a: number }> = {a: 5} // it is ok :D

Maybe another option to fix it would be if one could modify the "?" modifier based on some extends clause, like:

type MakeUndefinedPropsOptional<PC> = { [P in keyof PC](undefined extends PC[P] ? +? : -?): PC[P] }

but that's not there

@fatcerberus
Copy link

Overall 👍 on this change, but I am concerned about one thing: the Promise constructor. tsc cannot currently infer the type parameter for it (because it’s in the resolve() call and function calls in lambdas are not an inference source), so it will default to Promise<unknown>, unless a type argument is given explicitly. This might be okay in practice, but I’m not 100% convinced.

@weswigham weswigham deleted the noConstraint-is-unknown branch May 10, 2019 21:05
@AnyhowStep AnyhowStep mentioned this pull request Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants